Adds opengles to engine dart tests#180704
Conversation
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for running engine Dart tests with an OpenGL ES backend via Impeller. This is achieved by introducing a TesterContextGLESFactory that sets up an EGL context using ANGLE on top of SwiftShader (Vulkan). The changes touch the build system to include necessary dependencies and source files, the test runner to add "opengles" as a backend option, and the main test executable to wire up the new context factory. The author notes that creating a display currently fails but is not a blocker for the tests. My review includes suggestions to improve code clarity and simplify some C++ constructs in the new factory implementation.
|
|
||
| // |GPUSurfaceGLDelegate| | ||
| bool GLContextPresent(const GLPresentInfo& present_info) override { | ||
| return true; // PBuffer doesn't present? Or maybe eglSwapBuffers? |
There was a problem hiding this comment.
The comment here and the commented-out code below suggest some uncertainty about the correct behavior for presenting a PBuffer. For an offscreen p-buffer, eglSwapBuffers is often a no-op. If returning true is the intended behavior for this testing context, it would be clearer to state that explicitly rather than posing it as a question. For example: // PBuffers are not presented to a screen, so this is a no-op.
|
Initial results of investigating the tests failures: Running the tests on MacThese currently pass, but this is misleading. There's an issue that makes the
Then immediately after this, the Vulkan tester_context code runs and creates a Vulkan tester_context. The vulkan code runs because it doesn't have an explicit settings.requested_rendering_backend == "vulkan" check. It runs as long as So tests with --impeller-backend=opengles misleadingly end up using the vulkan backend. The tests were never actually running with an opengles backend. I think we should use an explicit If I add this check so that it doesn't unexpectly use Vulkan, tester_context remains null (because of the issue with extensions missing above), and the tester exits here without running any tests. Running the tests on LinuxWith
|
…irectory for `vk_swiftshader_icd.json`
…les and vulkan TesterContext.
|
I figured out the issue causing ✦ The issue is caused by a symbol conflict between ANGLE's volk dependency and SwiftShader's Vulkan implementation. Here is the detailed breakdown:
This fixes the broken tests when using the Vulkan impeller backend. I also modified The opengles backend issues described in my above comment remain. I will continue to investigate. |
…s VULKAN_SO_PATH for proc_address_callback rather than using vkGetInstanceProcAddr
fyi that fix is also in #181503 which is merging soon, it also adds a dimension for metal. one will have to be added for opengles too. |
…ls/gn `angle_enable_metal` is True, which is the default The other two angle_enable values need to be True (default) to enable opengles testing on mac
- Fill in shader_mappings - Add a reactor worker to ContextGLES. The worker sets the current egl context when requested, and properly handles releasing the current context by using TesterGLContext
… in the arguments
…d by the most recent merge
|
Did some more work to make most of the opengl tests pass:
With these changes, the majority of tests run and pass on opengles. However, there are still a significant number of tests that crash on opengles. I disabled all these tests for now in run_tests.py. We can dig into these as a follow-up. I also merged with HEAD, and changed |
|
Still seeing a bunch of test failures. I will look into it. |
One of them is just a pylint: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8691383561670995345/+/u/test:_pylint/stdout |
…/non-vm-service loops in gather_dart_tests
…d AddTaskObserver() if the context is already current. Otherwise, the added observer replaces the existing one, and the current context gets inadvertently cleared.
…is set but requested_rendering_backend is empty. I think this case happens when running "flutter test --impeller-enabled", so I couldn't get rid of it like before. This uses an empty() check, so unlike the original "!tester_context" check, this won't silently fall back to vulkan when trying and failing with a different backend
…testing, this takes longer than the default 30 min.
…secs is set for the mac_unopt CI test
Updates since my last comment:
Disabled opengles testsIn an earlier commit, I disable a bunch of the opengles tests that weren't passing: cd5f614. From testing locally, after the most recent commits I think many (maybe all?) of these are now passing. So we should try re-enabling them. However, before trying this out, there's one blocking issue that I need to figure out how to fix first: What's still broken:A bunch of mac tests are failing. All fail with a similar error: e.g. https://github.com/flutter/flutter/pull/180704/checks?check_run_id=62063259937 I tried adding libvulkan.dylib to the engine artifacts: 75d6aef. The generated engine artifacts after this can be seen here: https://storage.googleapis.com/flutter_archives_v2/flutter_infra_release/flutter/6fd5a1f13f0b36317b8b23162d92de11b3dc267a/darwin-x64/artifacts.zip. This is what's downloaded to the flutter/bin/cache/artifacts/engine/darwin-x64 directory and contains the flutter_tester executable which is run for these tests. With my commit, libvulkan.dylib is copied to this directory. But the flutter_tester runs don't seem to be able to find it. Even if libvulkan.dylib can be found, would these tests pass? tester_context_vk_factory.cc loads vulkan by looking for libvk_swiftshader.dylib in the executable directory. And tester_context_gles_factory.cc looks for vk_swiftshader_icd.json in the executable directory (and that icd json file points to "./libvk_swiftshader.dylib"). So would we also have to copy libvk_swiftshader.dylib and vk_swiftshader_icd.json to the artifacts output (or to whatever location flutter_tester can find them)? Looking more into how swiftshader is linked into flutter_tester, I found this PR: flutter/engine#48708. That PR seems to be doing the reverse of what this current PR does. It changes swiftshader loading from a dynamic library to a static library. But this current PR makes the vk and gles tester context factories try to dynamically load the swiftshader library. Would that previous PR be a signal that what we're doing here isn't the right way to do things, and we should instead be trying to statically link these vk/swiftshader libraries? Is trying to statically link those libraries compatible with using ANGLE? |
On macos you can check where an executable is getting its linked dynamic libraries with the tool: If one of the dependencies is something like "./libvulkan.dylib" it's going to expect to find that library relative to the CWD when the executable is executed. The options are to change the install names in the macho headers for that executable to look somewhere else or to change the CWD when the tests are executed. |
…s executable specifically for the opengles tests. Revert the changes to tester_context_vk_factory.cc. These changes are no longer needed because the main flutter_tester executable no longer has ANGLE dependencies. Change run_tests.py to use the flutter_tester_opengles executable for the opengles dart tests.
|
After discussing with gaaclarke, decided to go with a different approach. Instead of trying to make the existing flutter_tester executable support both vulkan and opengles, split the opengles testing into a different executable. This way, we avoid all the problems with one executable having both a static swiftshader library (used by the vulkan test context) and a dynamic swiftshader library (used by opengles through angle). The existing flutter_tester executable is mostly unchanged, and will not support --impeller-backend=opengles. A new flutter_tester_opengles executable is used for the opengles tests. This also means we don't have to copy various vulkan/swiftshader dylib files to the artifacts directory. These libraries are built and output to the engine output directory along with the flutter_tester and flutter_tester_opengles executables. So flutter_tester_opengles is able to find and use them. The engine artifacts are used for "flutter test" shell commands. These commands will continue to use flutter_tester and will not support testing with an opengles impeller backend. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for running engine Dart tests with an OpenGL ES backend, leveraging ANGLE and SwiftShader. The changes include refactoring the flutter_tester build configuration into a template to support a new flutter_tester_opengles executable, adding a TesterContextGLESFactory for creating an Impeller GLES context, and updating the test runner script to include the 'opengles' backend. My feedback focuses on improving the clarity and maintainability of comments in the new tester_context_gles_factory.cc file to remove ambiguity and better document known issues.
engine/src/flutter/shell/testing/tester_context_gles_factory.cc
Outdated
Show resolved
Hide resolved
|
|
||
| // |GPUSurfaceGLDelegate| | ||
| bool GLContextPresent(const GLPresentInfo& present_info) override { | ||
| return true; // PBuffer doesn't present? Or maybe eglSwapBuffers? |
There was a problem hiding this comment.
This comment indicates some uncertainty. For offscreen rendering to a PBuffer, eglSwapBuffers is often a no-op. Since these are offscreen tests where the result is not displayed, simply returning true is likely correct. It would be good to update the comment to reflect this reasoning for better maintainability.
| return true; // PBuffer doesn't present? Or maybe eglSwapBuffers? | |
| return true; // For offscreen rendering to a PBuffer, presentation is a no-op. |
References
- Documentation should be useful and explain the why and the how. A more confident and explanatory comment improves readability and maintainability. (link)
| // Render to surface is true? | ||
| // For offscreen testing, we might still want to say "true" to create | ||
| // ON-SCREEN surface abstraction but backed by the delegate's FBO/Surface. |
There was a problem hiding this comment.
The question in the comment at line 394 suggests uncertainty. The following lines explain the reasoning well. It would be clearer to have a single, more direct comment explaining the intent.
| // Render to surface is true? | |
| // For offscreen testing, we might still want to say "true" to create | |
| // ON-SCREEN surface abstraction but backed by the delegate's FBO/Surface. | |
| // For offscreen testing, we create a surface that behaves like an on-screen | |
| // one but is backed by the delegate's FBO/Surface. This is controlled by | |
| // the `render_to_surface` parameter being set to `true`. |
References
- Optimize for readability. A clear, assertive comment is more readable than a question followed by an explanation. (link)
|
This is passing all the tests now (except for the opengles_skipped_tests specified in run_tests.py). So I think it's ready for you to take a look at @gaaclarke |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Looking good. I have some questions about some additions and requests to remove duplication. Do you want to fork this and make it your own PR? Then I can approve it. I won't be able to approve it while it's still under my branch.
|
|
||
| namespace { | ||
|
|
||
| static void FindSwiftShaderICDAtKnownPaths() { |
There was a problem hiding this comment.
This function exists already in impeller/playground/backend/vulkan/swiftshader_utilities.cc, can we share it?
| } | ||
| } | ||
|
|
||
| void SetupSwiftshaderOnce(bool use_swiftshader) { |
There was a problem hiding this comment.
Same for this other stuff it looks like.
| // Impeller should only be enabled if the Metal backend is enabled. | ||
| #define TESTER_ENABLE_METAL \ | ||
| (IMPELLER_SUPPORTS_RENDERING && IMPELLER_ENABLE_METAL) | ||
| (IMPELLER_SUPPORTS_RENDERING && IMPELLER_ENABLE_METAL && SHELL_ENABLE_METAL) |
There was a problem hiding this comment.
Aren't IMPELLER_ENABLE_METAL and SHELL_ENABLE_METAL synonymous? I'm not sure when you'd have one and not the other.
| dart_tests = glob.glob(f'{dart_tests_dir}/*_test.dart') | ||
|
|
||
| impeller_backends = ['', 'vulkan'] | ||
| opengles_skipped_tests = [ |
There was a problem hiding this comment.
Are these every test that actually uses opengles though? I'm seeing other tests that pass are things like platform channels tests, which never touch graphics. What are some of the failures we have here?
As discussed, forked this to #181933 |
Forked from #180704. See comments there for more context. Enables most of the dart tests for opengles. There are some which aren't passing, which are filtered out in run_tests.py. Fixes an issue in snapshot_controller_impeller.cc where a returned GLContextResult is ignored. The returned GLContextResult is potentially an RAII object, so it must stay in scope for subsequent GL calls. Fixes #180601 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Aaron Clarke <[email protected]> Co-authored-by: gaaclarke <[email protected]>
Forked from flutter#180704. See comments there for more context. Enables most of the dart tests for opengles. There are some which aren't passing, which are filtered out in run_tests.py. Fixes an issue in snapshot_controller_impeller.cc where a returned GLContextResult is ignored. The returned GLContextResult is potentially an RAII object, so it must stay in scope for subsequent GL calls. Fixes flutter#180601 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Aaron Clarke <[email protected]> Co-authored-by: gaaclarke <[email protected]>
Forked from flutter#180704. See comments there for more context. Enables most of the dart tests for opengles. There are some which aren't passing, which are filtered out in run_tests.py. Fixes an issue in snapshot_controller_impeller.cc where a returned GLContextResult is ignored. The returned GLContextResult is potentially an RAII object, so it must stay in scope for subsequent GL calls. Fixes flutter#180601 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Aaron Clarke <[email protected]> Co-authored-by: gaaclarke <[email protected]>
2 participants
These fail to make a display. I tried to get it working but it appears that it isn't actually required for these tests to pass so I gave up.
fixes #180601
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.