[Impeller] Correct attachment description for offscreen MSAA resolve. #42753
Conversation
|
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. |
|
lol now we get a validation error. I think I just need to correct the layout info. |
|
hilariously I misread the tutorial, but visually things are fixed for me 😮💨 |
|
So I suspect that rather than fixing this, the error has introduced some pessimization which is causing the synchronization issue to resolve itself. |
| resolve_texture ? vk::ImageLayout::ePresentSrcKHR | ||
| : vk::ImageLayout::eColorAttachmentOptimal; |
There was a problem hiding this comment.
Is this the right switch value?
Spec says that eColorAttachmentOptimal "must only" be used as a color ro resolve in a VkFramebuffer.
Perhaps the argument should instead be bool is_framebuffer or something? Or perhaps an enum where the enumerated values are something like kFramebuffer and kNonFramebuffer.
There was a problem hiding this comment.
I'm not sure I really understand what ePresentSrcKHR is really for. Did you already try eGeneral?
There was a problem hiding this comment.
I don't really understand either, but it was the value used in https://vulkan-tutorial.com/Multisampling so I just went with it. I'll try general
There was a problem hiding this comment.
eGeneral works! lets see if it validates
| @@ -95,7 +98,7 @@ SharedHandleVK<vk::RenderPass> RenderPassVK::CreateVKRenderPass( | |||
| if (color.resolve_texture) { | |||
| resolve_refs[bind_point] = | |||
| vk::AttachmentReference{static_cast<uint32_t>(attachments.size()), | |||
| vk::ImageLayout::eColorAttachmentOptimal}; | |||
| vk::ImageLayout::ePresentSrcKHR}; | |||
There was a problem hiding this comment.
(Ditto here: maybe eGeneral instead?)
| @@ -49,14 +49,17 @@ static vk::AttachmentDescription CreateAttachmentDescription( | |||
|
|
|||
| if (desc.storage_mode == StorageMode::kDeviceTransient) { | |||
There was a problem hiding this comment.
Is the real problem that we're setting the storage mode to device transient?
I'm not quite clear on when we should set dontcare or not though.
There was a problem hiding this comment.
no that doesn't seem to be it, at least things are already working for me
| @@ -466,7 +467,9 @@ constexpr vk::AttachmentDescription CreateAttachmentDescription( | |||
| switch (kind) { | |||
| case AttachmentKind::kColor: | |||
| vk_attachment.initialLayout = current_layout; | |||
| vk_attachment.finalLayout = vk::ImageLayout::eColorAttachmentOptimal; | |||
| vk_attachment.finalLayout = | |||
| resolve_texture ? vk::ImageLayout::eGeneral | |||
There was a problem hiding this comment.
The nitpick here is using "resolve_texture" as the reason to do this.
The not nitpick here is that I think we should go with eGeneral unless we know for sure that this is for the framebuffer. eGeneral should always be safe if I'm understanding the spec, whereas the eColorAttachmentOptimal is only safe if we know we're using this for framebuffer.
jonahwilliams
added
the
autosubmit
…128721) flutter/engine@33e0693...de68fba 2023-06-12 [email protected] Fix crash with CJK keyboard with emoji at end of text field (flutter/engine#42540) 2023-06-12 [email protected] Roll Skia from 658b1d366758 to 6bdb0ef30cb6 (2 revisions) (flutter/engine#42778) 2023-06-12 [email protected] [Impeller] Correct attachment description for offscreen MSAA resolve. (flutter/engine#42753) 2023-06-12 [email protected] Remove dependency on memfs (flutter/engine#42773) 2023-06-12 [email protected] Roll Skia from 0f974a0f8c10 to 658b1d366758 (1 revision) (flutter/engine#42776) 2023-06-12 [email protected] Roll ANGLE from 3abbc4f99970 to 43ef50f389e9 (1 revision) (flutter/engine#42775) 2023-06-12 [email protected] Remove unnecessary #include of SkPromiseImageTexture (flutter/engine#42770) 2023-06-12 [email protected] Roll Skia from 951123096e55 to 0f974a0f8c10 (5 revisions) (flutter/engine#42771) 2023-06-12 [email protected] [Impeller] opt all vertex shader position/uvs into highp (flutter/engine#42746) 2023-06-12 [email protected] [Impeller] added debug info to frame debuggers like AGI (flutter/engine#42717) 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
Two issues:
The resolve texture is created from the same impeller attachment as the msaa attachment, so the store mode was getting set to dont care instead of store.
the image layout for the resolve attachment should be ePresentSrcKHR, at least from following the guide at https://vulkan-tutorial.com/Multisampling
Fixing both of these locally fixes all of the weird offscreen cursed rendering.
Fixes flutter/flutter#128600