[Impeller] fix render pass depth descriptor.#51031
[Impeller] fix render pass depth descriptor.#51031auto-submit[bot] merged 6 commits intoflutter:mainfrom
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. |
bdero
left a comment
bdero
left a comment
There was a problem hiding this comment.
LGTM (once StC is flipped back off)!
|
Tested: is covered by the tests in #50856 not failing? |
|
Golden file changes are available for triage from new commit, Click here to view. |
|
hmm, what did I do wrong. I changed more stuff that I perhaps shouldn't have |
| } | ||
|
|
||
| if (desc.HasStencilAttachmentDescriptors()) { | ||
| } else if (desc.HasStencilAttachmentDescriptors()) { |
There was a problem hiding this comment.
Wait, won't this just turn off the stencil attachment and make legacy clips + StC not work anywhere?
I think the depth and stencil are separately configurable attachments even if they share a texture. Were you getting validation errors before this?
There was a problem hiding this comment.
IIRC Depth+Stencil are the same attachment in Vulkan. Calling SetStencilAttachment blows away the value of SetDepthStencilAttachment at least in our code
RenderPassBuilderVK& RenderPassBuilderVK::SetDepthStencilAttachment(
PixelFormat format,
SampleCount sample_count,
LoadAction load_action,
StoreAction store_action) {
vk::AttachmentDescription desc;
desc.format = ToVKImageFormat(format);
desc.samples = ToVKSampleCount(sample_count);
desc.loadOp = ToVKAttachmentLoadOp(load_action);
desc.storeOp = ToVKAttachmentStoreOp(store_action);
desc.stencilLoadOp = desc.loadOp; // Not separable in Impeller.
desc.stencilStoreOp = desc.storeOp; // Not separable in Impeller.
desc.initialLayout = vk::ImageLayout::eGeneral;
desc.finalLayout = vk::ImageLayout::eGeneral;
depth_stencil_ = desc;
return *this;
}
RenderPassBuilderVK& RenderPassBuilderVK::SetStencilAttachment(
PixelFormat format,
SampleCount sample_count,
LoadAction load_action,
StoreAction store_action) {
vk::AttachmentDescription desc;
desc.format = ToVKImageFormat(format);
desc.samples = ToVKSampleCount(sample_count);
desc.loadOp = vk::AttachmentLoadOp::eDontCare;
desc.storeOp = vk::AttachmentStoreOp::eDontCare;
desc.stencilLoadOp = ToVKAttachmentLoadOp(load_action);
desc.stencilStoreOp = ToVKAttachmentStoreOp(store_action);
desc.initialLayout = vk::ImageLayout::eGeneral;
desc.finalLayout = vk::ImageLayout::eGeneral;
depth_stencil_ = desc;
return *this;
}
So as long as depth/stencil have the same load/store it should work?
There was a problem hiding this comment.
Oh, we should fix this builder so that it doesn't force us to wipe away depth-specific options when the stencil is set. We don't need to do it right at this moment, but it's an easy fix and will probably bite us later if we don't fix it.
|
@bdero PTAL! |
|
|
||
| stencil->load_action = LoadAction::kClear; | ||
| stencil->store_action = StoreAction::kDontCare; | ||
| depth->load_action = LoadAction::kClear; |
There was a problem hiding this comment.
I think this was missing.
| stencil->store_action = StoreAction::kDontCare; | ||
| depth->load_action = LoadAction::kClear; | ||
| depth->store_action = StoreAction::kDontCare; | ||
| pass_target_.target_.SetDepthAttachment(depth); |
There was a problem hiding this comment.
I can come back around later and try to clean some of the stuff up with the VK pass builder, but I think this is good to unblock.
The depth attachment is already set above, but this might be order sensitive because RenderPassBuilderVK::SetStencilAttachment sets the depth load/store to eDontCare, whereas the depth call doesn't.
|
Oh wait a second... All the goldens just resolved on #50856 without this patch. |
|
I think this patch fixes the tofu issue though. With the sleep added I can consistently repro the failure before this patch. |
|
Actually, the golden test isn't getting marked resolved in #50856, so I'm pretty sure I'm just running into the force push issue and there are invisible golden differences which are just not showing up. So I'd say merge away. |
jonahwilliams
added
the
autosubmit
flutter/engine@455c814...10331db 2024-02-28 [email protected] Roll Fuchsia Linux SDK from T1xAi_ww_mWEiDkVN... to ujOkbeYbrC8loPbfR... (flutter/engine#51066) 2024-02-28 [email protected] Roll abseil-cpp to 1db3bdd4eb208bef55c77f22aa94991e52225230 (flutter/engine#51062) 2024-02-28 [email protected] Roll Dart SDK from 7896a944fe67 to fdf4a62bd07b (1 revision) (flutter/engine#51064) 2024-02-28 [email protected] [Impeller] Mark subpass framebuffer fetch tests as unsupported on GLES (flutter/engine#50982) 2024-02-28 [email protected] Roll Skia from 93f245da0097 to 27171d6a9205 (1 revision) (flutter/engine#51063) 2024-02-28 [email protected] [Impeller] fix render pass depth descriptor. (flutter/engine#51031) 2024-02-28 [email protected] Make Skia object ostream operators work with unit tests (flutter/engine#51041) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from T1xAi_ww_mWE to ujOkbeYbrC8l 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
2 participants
Depth+stencil should be treated as the same attachment. Adding more than one d/s attachment in the render pass descriptor seems to be confusing swiftshader.
@bdero
This fixes things for me locally, lets let CI take a spin