[Impeller] Respect resolving store actions in the Vulkan backend. #50374
Conversation
Earlier, the Vulkan backend would perform a resolve as long as store the main texture had a non-unit sample count. This completely ignored StoreAction::kMultisampleResolve and StoreAction::kStoreAndMultisampleResolve which the Metal backend correctly expects. Fixes flutter/flutter#142947
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). 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. |
|
I cannot explain how this worked earlier. The store action on the resolve texture used to be a eDontCare. |
There was a problem hiding this comment.
Maybe it wasn't working, and we've gotten lucky on the implementations we've tested on?
This can be tested tough. A unit test in impeller/renderer/backend/vulkan should be able to initialize a context via playground, construct MSAA and non-MSAA render targets and then assert the store actions by calling this method, or one of the builder methods, et cetera?
|
We were setting up Vulkan incorrectly for sure. But the weird bit is that Vulkan was doing the right thing in spite of it. That is, treating the resolve texture as a eStore instead of eDontCare. I suppose conflating DontCare with Store is still within the bounds of undefined behavior. |
|
Let me check this on actual devices to see the difference. Will add a test. |
|
I don't have cycles to spend on this this week. Closing and removing my assignment. Will re-open when I make progress. AFAICT, we haven't observed bad behavior due to this... yet. |
#51740) Added tests for #50374 This makes RenderDoc replays work a whole lot better since all of the offscreen textures aren't cleared with DONT CARE. Fixes flutter/flutter#142947
Earlier, the Vulkan backend would perform a resolve as long as store the main texture had a non-unit sample count. This completely ignored StoreAction::kMultisampleResolve and StoreAction::kStoreAndMultisampleResolve which the Metal backend correctly expects.
Fixes flutter/flutter#142947