[Impeller] cache MSAA texture in swapchain for devices that do not support memoryless. #43349
Conversation
| for (auto i = 0u; i < memory_properties.memoryTypeCount; i++) { | ||
| if (memory_properties.memoryTypes[i].propertyFlags & | ||
| vk::MemoryPropertyFlagBits::eLazilyAllocated) { | ||
| supports_memoryless_textures_ = true; |
There was a problem hiding this comment.
I sound like a broken record, but I don't think one can make the leap. I'm reading through https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#memory-device-lazy_allocation and it doesn't talk about this.
Maybe there is an extension that can give us an exact query for this support?
There was a problem hiding this comment.
I understand that you are not convinced this is sufficient. However, lacking evidence of a phone where this is an issue I think we should focus on the existing pile of concrete problems.
There was a problem hiding this comment.
Yea, fair point. The Android ecosystem is so diverse, it's difficult to know if there is a device that would pass this test but have better performance caching the textures instead. As a tools developer too we don't have analytics or anything that will tip us off if there is a problem. I think it's worth at least looking, I was hoping there some documentation somewhere about this.
There was a problem hiding this comment.
Here is the documentation I could find. The name vulkan gives this is "Transient Attachments": https://github.com/KhronosGroup/Vulkan-Samples/tree/main/samples/performance/subpasses#transient-attachments
We should use that nomenclature instead of memoryless textures. I couldn't find a more apropos capabilities check than this.
There was a problem hiding this comment.
memoryless is consistent with the nomenclature of the Metal backend and is what @chinmaygarde asked me to use here.
|
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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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. |
jonahwilliams
changed the title
| private: | ||
| vk::Image image_ = VK_NULL_HANDLE; | ||
| vk::UniqueImageView image_view_ = {}; | ||
| std::shared_ptr<Texture> msaa_tex_; |
There was a problem hiding this comment.
This is rough as these textures are huge. We should measure memory usage but I doubt we can use the Vulkan backend in this instance. This is just the the swapchain images. These will add up for intermediates as well.
If this is to stabilize the benchmarks, its fine I suppose. But we should then find a device to put in the lab that does support lazily allocated images.
There was a problem hiding this comment.
Yeah, I'd like to punt on whether or not we decided to ship Vulkan on these devices. We'll likely learn even more once we actually attempt the Vulkan to GL texture interop.
Though for the short term, we must keep something on CI using Vulkan and I don't think we have any other devices.
There was a problem hiding this comment.
I doubt we can use the Vulkan backend in this instance
Couldn't we use a different AA than MSAA, like fxaa?
|
I don't think memory usage vs memoryless is the right comparsion to think about for this. Clearly our current strategy is using dramatically more memory than OpenGL due to the spikey performance. And if OpenGL has access to the same capabilities as Vulkan does on this phone (probably) then I don't think we'll be worse off. Shipping OpenGL backend to more devices is absolutely an option, but we should try to figure out if we're actually using more memory than an equivalent OpenGLES application. If not, we may just want to take the L and deal with the overhead. As a secondary concern, the current OpenGLES backend has no offscreen MSAA. Which, seem appropriate given the low target. But if we need to dramatically expand the number of devices that opt into OpenGL then I don't think we can accept the visual fidelity regression. |
So, instead of switching Vulkan to OpenGLES if we just turn off MSAA in Vulkan we'll be no worse off? Seems like keeping the largest number of devices on the Vulkan backend is the most economical solution since it will absorb the most resources. The pixel 4a isn't passing the The ideal layout would be 90% of phones on Vulkan and increasing, or 100% of phones on OpenGLES. Being in a situation where it's a 50/50 split is going to be costly since it will divide our resources. |
|
I'm absolutely not suggesting we turn of MSAA, on the contrary suggesting that we can't ship with no offscreen MSAA for a large % of Android devices. |
|
and in this case, we're talking about onscreen msaa - which we need to pay for vulkan or opengl |
|
yolo |
jonahwilliams
added
the
autosubmit
… do not support memoryless. (flutter/engine#43349)
…129852) flutter/engine@54b573e...e6b8292 2023-06-30 [email protected] [Impeller] cache MSAA texture in swapchain for devices that do not support memoryless. (flutter/engine#43349) 2023-06-30 [email protected] [Impeller] Fix validation error in position color shader. (flutter/engine#43356) 2023-06-30 [email protected] Fix inverted boolean in Windows document selection changed a11y event (flutter/engine#43281) 2023-06-30 [email protected] Revert "Reland "add non-rendering operation culling to DisplayListBuilder" (#41463)" (flutter/engine#43358) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…pport memoryless. (flutter#43349) For devices that don't support memoryless textures, the fullscreen MSAA texture adds a substantial amount of memory thrashing when it is allocated and deallocated. For these devices, lets cache the MSAA texture in the swapchain image. flutter/flutter#129737
3 participants
For devices that don't support memoryless textures, the fullscreen MSAA texture adds a substantial amount of memory thrashing when it is allocated and deallocated. For these devices, lets cache the MSAA texture in the swapchain image.
flutter/flutter#129737