[Impeller] force render pass construction on all backends to ensure image layout is transitioned/render pass state setup. #50539
Conversation
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
| import tempfile | ||
| import time | ||
| import typing | ||
| import xvfb |
There was a problem hiding this comment.
The formatter told me to do this.
| import tempfile | ||
| import time | ||
| import typing | ||
| import xvfb |
|
Ahh shoot, I lost my big comment. I hit submit before hitting the "Add comment" button. |
impeller/entity/entity_pass.cc
Outdated
| if (!collapsed_parent_pass && (GetClearColor(clear_color_size).has_value() || | ||
| renderer.GetContext()->GetBackendType() == | ||
| Context::BackendType::kVulkan)) { |
There was a problem hiding this comment.
What my big comment here said was that I'm wary of adding branches in the crossplatform code since I think it can lead to an unclear abstraction and code that's hard to follow. I think we should turn this to a different abstraction pass_context.GetRenderPass(pass_depth, GetClearColor(clear_color_size)). Inside that function it can check to see if the color has value and also call onto the renderpass something like pass_->SetImageLayout() which is only implemented for RenderPassVK. That way there will be no branch, but just SetImageLayout() that does nothing except on vulkan. Maybe we could even use more generic name. PrepareRenderPass?
There was a problem hiding this comment.
I understand the concern about branches but adding a virtual method that is only implemented in one backend is roughly equivalent to a branch, no?.
I could definitely make this clearer by pulling the condition out to a helper method though
There was a problem hiding this comment.
(If you don't like this one, you're gonna hate the next PR I send you)
There was a problem hiding this comment.
I understand the concern about branches but adding a virtual method that is only implemented in one backend is roughly equivalent to a branch, no?.
There is a subtle difference that when you make a virtual method. You are contributing to an interface which collectively defines the abstraction. If you add branching statements there is no single place that defines or enforces the interface.
Imagine we wanted to add a directx backend, if we had the branch we wouldn't know that we need to make a decision about wether we need to setup the renderpass regardless, like we do in vulkan.
Also, when we expand the abstraction, people can read through that interface and understand the system hierarchically. With branches the interface exists but it is implicit and spread around multiple locations.
There was a problem hiding this comment.
I don't think exposing ImageLayout is the right way to do that though, that requires tons of Vulkan only internal state to be exposed to entity pass.
Imagine we wanted to add a directx backend,
I mean, we can handle that if/when we do DirectX.
There was a problem hiding this comment.
I don't think exposing ImageLayout is the right way to do that though, that requires tons of Vulkan only internal state to be exposed to entity pass.
It won't be tons of state, it will just be a hook like PrepareRenderPass(). You understand how branching on the backend type compromises the abstraction, right?
Would you like me to take a stab at proposing an alternative that expands the interface? If this comes up in a later PR it could potentially serve as a template there too.
There was a problem hiding this comment.
I think the abstraction being intentionally leaky (as its private API) is valuable.
There was a problem hiding this comment.
If you wanna take a crack at it, have at it, I'm not in a rush :)
…ngine into force_create_render_pass
|
FYI I'm seeing a magenta square (error color) on the metal invocation of the new test. It looks fine in the golden image test. |
|
oh interesting. i wonder if this is actually a problem on Metal too? |
|
What if we just made this unconditional? |
That fixes the error for me and avoids the discussion about the branching. |
|
Golden file changes are available for triage from new commit, Click here to view. |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM, can you file an issue against the golden test runner for the fact that it succeeded here where local testing would fail?
jonahwilliams
changed the title
|
Done: flutter/flutter#143324 |
|
Thanks for being thorough as always @gaaclarke 😄 |
jonahwilliams
added
the
autosubmit
2 participants
Fixes flutter/flutter#142358
We rely on the render pass encoding to transition image textures from undefined to shader read layout. If an empty pass is created with no clear color, we still need to create the pass on the Vulkan backend.