[Impeller] support for VK_EXT_blend_operation_advanced (coherent only). #164852
Conversation
What's the upside to the change? Only being able to manually test something is a pretty big downside. I'm not sure we want to adopt something like that unless it has a big upside. |
|
I chatted with Jonah offline. This is to address compatibility with some devices that seem to be having problems with the advanced blend approach in vulkan. We also decided to double check if we can test this with swiftshader and/or non-coherent memory. |
|
Swiftshader supports the extension but not on mac (or linux?) looks like just some windows variants. Not sure why. |
|
actually, no, that makes sense "SwiftShader device" on GPUinfo.org may be the android emulator, but on desktop machines it may use passthrough - so it ends up using the host card. Swiftshader the software backend has no advanced blend support. |
|
@gaaclarke / @jtmcdole PTAL and let me know if you want us to support this device. If so you must review/approve this PR. No alternatives at this point. |
There was a problem hiding this comment.
This PR seems to set a preference for the advanced blend operations, which has no automated tested. I'd prefer to see us use framebuffer fetch unless we know we need advanced blend operations, since that is tested. That might take the form of a device deny list though.
This code sprinkles if (renderer_.GetDeviceCapabilities().SupportsAdvancedBlendOperations()) throughout the codebase. It would be preferable to create an object that represents how blends will happen, then this could be added without modifying those places in the code. It also unifies the decision about what will be done to one place.
class Blend
class AdvancedBlend
Blend <|-- AdvancedBlend
class FramebufferBlend
Blend <|-- FramebufferBlend
class BackdropBlend
Blend <|-- BackdropBlend
| if (renderer_.GetDeviceCapabilities().SupportsAdvancedBlendOperations()) { | ||
| // Let entity render with advanced blend mode, the backend supports | ||
| // rasterization of it without additional shaders. | ||
| } else if (renderer_.GetDeviceCapabilities().SupportsFramebufferFetch()) { |
There was a problem hiding this comment.
Can we swap this so that we are preferring framebuffer fetch over the advanced blend operations? We want to send them down the tested path if supported, right? We'd probably need a way to single out devices that need to invert that priority though.
There was a problem hiding this comment.
You're asking that I base the check on reported driver info rather than the reported device capabilities? That seems less safe as driver info is not scrutinized the way reported extension support is, and devices are free to change it in an update
There was a problem hiding this comment.
We want to send them down the tested path if supported, right?
I think you need to do some soul searching on what you expect from testing on this project. We have ~3 devices in the lab. There are thousands of android devices.
There was a problem hiding this comment.
While acknowledging the number of test devices we have in the lab is small; this is the current path in the field and deployed to user phones. The default-to-safest path would be to keep the exist one. Do we have any comparable devices in the 'library' we can do a sniff test with?
There was a problem hiding this comment.
You're asking that I base the check on reported driver info rather than the reported device capabilities?
I believe the problem device does report it supports framebuffer fetch, but doesn't, right? So trusting the reported device capabilities isn't really an option. Changing behavior based on a device is already happening elsewhere.
I think you need to do some soul searching on what you expect from testing on this project. We have ~3 devices in the lab.
I need to do a lot of soul searching, not just on android testing =)
It's not just device lab, we've shipped vulkan and we sent all the phones down the framebuffer fetch path so there is testing there. Sending users down the path that has automated tests will always be safer.
Here's what I have in mind:
if (deviceNeedsAdvancedBlends()) {
doAdvancedBlend();
} else if (supportsFramebufferFetch()) {
doFramebufferFetch();
} else if (supportsAdvancedBlends()) {
doAdvancedBlend();
} else {
doBackdropBlend();
}There was a problem hiding this comment.
What criteria do you want to use for the device check?
we're not expanding the devicelab as far as I know.
There was a problem hiding this comment.
What criteria do you want to use for the device check?
I'd use vkGetPhysicalDeviceProperties, probably the device id.
There was a problem hiding this comment.
I don't really like that. seems like we'd actually lose test coverage via our developers. Not to mention if a variant device ships we'll be broken again
There was a problem hiding this comment.
I don't really like that. seems like we'd actually lose test coverage via our developers.
If we are limiting the feature to one phone, we can manage that manually. I think we were already planning on manual testing.
Not to mention if a variant device ships we'll be broken again
In order for that to happen the following would have to be true:
- The phone ships with the bug.
- A subsequent variant of the phone ships with the same bug before we are able to get automated test coverage and remove the allow list.
I think we laid out some good ideas to consider and it was important we had this discussion. I'd be interested to hear if @jtmcdole has any thoughts. I think opting everyone into advanced blends, a feature without automated testing, is more risky but I'll support whatever you guys decide.
There was a problem hiding this comment.
I think opting everyone into advanced blends, a feature without automated testing, is more risky but I'll support whatever you guys decide.
It is tested... manually, but tested. And its not everyone, its only devices that support this feature.
engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc
Outdated
Show resolved
Hide resolved
engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk.h
Show resolved
Hide resolved
engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc
Outdated
Show resolved
Hide resolved
| blend_state.setAttachments(attachment_blend_state); | ||
|
|
||
| if (caps->SupportsAdvancedBlendOperations()) { |
There was a problem hiding this comment.
Can we have a unit test for this? I'm thinking of mocking out SupportsAdvancedBlendOperations() and checking the calls to vulkan.
As an alternative to a subpass self dependency and advanced blends implemented via shaders, add support for the extension https://registry.khronos.org/vulkan/specs/latest/man/html/VK_EXT_blend_operation_advanced.html which provides build in support for the advanced blend operations via the normal blend enums.
When using this mode, construct render passes without the self dependency and instead of GENERAL use COLOR_ATTACHMENT_OPTIMAL (theoretically faster). This is now configured via the "Topology" parameter of the RenderPassBuilder constructor.
There are still some todos, namely:
This requires "coherent" advanced blends. To support "incoherent" blends, we'd need to detect them and insert a memory barrier according to the instructions in the extension documentation. The particular device I am testing on supports coherent operations so this is fine (for now). Though we should follow up with incoherent support to get broader usage.
We're still using the programmable advanced blend filters, but we could actually use the blend modes too - which will be faster and result in fewer loaded shaders.
While I did update some unit tests, AFAIK we have no physical devices capable of coherent advanced blend operations, so this is entirely manually tested at this point.