Repository navigation
[Impeller] Fix hardcoded Vulkan validation state in C API - #186767
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request updates the Vulkan context creation in Impeller to propagate the enable_validation flag from the C API settings to the internal renderer. Feedback indicates that this logic change requires a corresponding regression test to comply with the repository style guide's requirement that all code changes be tested.
| impeller_settings.shader_libraries_data = CreateShaderLibraryMappings(); | ||
| impeller_settings.cache_directory = fml::paths::GetCachesDirectory(); | ||
| impeller_settings.enable_validation = true; | ||
| impeller_settings.enable_validation = settings.enable_validation; |
There was a problem hiding this comment.
This bug fix lacks a corresponding regression test. Although the pull request description mentions a test exemption, forwarding a setting from the C API to the internal renderer is a logic change that should be tested to ensure the flag is correctly propagated and functional. Per the Repository Style Guide (line 11), all code changes should be tested.
References
- Code should be tested and follow the guidance described in the writing effective tests guide. (link)
|
Tested on Linux with Built the Impeller unit test binary: ./bin/et build -c host_debug_unopt //flutter/impeller:impeller_unittestsOutput: Ran the interop object tests: ../out/host_debug_unopt/impeller_unittests --gtest_filter="InteropObjectTest.*"Output: Ran the OpenGLES interop playground tests: ../out/host_debug_unopt/impeller_unittests --gtest_filter="Play/InteropPlaygroundTest.*OpenGLES*"Output: Note: I did not include the local Vulkan playground run here because it aborts in my local Vulkan environment while initializing debug utils. Relying on CI for Vulkan coverage. |
|
autosubmit label was removed for flutter/flutter/186767, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
Previously we were hardcoding This only affects the PlaygroundTest because If you agree please update your PR with these changes |
Thanks for catching this! I’ll take a look and update the PR accordingly. |
…ng pointer issue with UserData by moving it from stack to heap
|
Done, I’ve cherry-picked both commits. Please take another look. |
|
I've synced the branch with the latest master. @andywolff, could you approve them and re-add the label when you get a chance? Thanks! |
|
Thanks. It seems all impacted tests are passing, but there's a github outage right now which is affecting some of the other checks. I'll try it again when that outage is resolved |
jtmcdole
left a comment
There was a problem hiding this comment.
The LGTM exchange rates are currently trading at:
- 1 LGTM = golden images match closely enough if I squint.
- 2 LGTMs = I checked that this won't break the devicelab benchmarks
- 3 LGTMs = I trust that this C++ pointer won't leak memory every frame.
- 4 LGTMs = CI is green on the first try without a single retry or shard failure
Make the standalone Impeller Vulkan interop layer respect
ImpellerContextVulkanSettings.enable_vulkan_validation.The C API already exposes this setting and
ContextVK::Settingsreads it, butContextVK::Createcurrently forcesimpeller_settings.enable_validation = true. This means callers cannot disable Vulkan validation through the public standalone SDK API.This PR changes the interop layer to pass through
settings.enable_validation.Fixes #186764
Tested
See PR comment for the full local test output.
Locally verified from
engine/src/flutter:./bin/et build -c host_debug_unopt //flutter/impeller:impeller_unittests../out/host_debug_unopt/impeller_unittests --gtest_filter="InteropObjectTest.*"passed, 3 tests.../out/host_debug_unopt/impeller_unittests --gtest_filter="Play/InteropPlaygroundTest.*OpenGLES*"passed, 18 tests.Vulkan coverage is left to CI because the local Vulkan playground run aborts in my Vulkan environment while initializing debug utils.
Pre-launch Checklist
///).