[Impeller] generate mipmaps for imagefilters #49794
Conversation
impeller/aiks/canvas.cc
Outdated
| paint.image_filter->Visit(mip_count_visitor); | ||
| new_layer_pass.SetRequiredMipCount( | ||
| std::max(mip_count_visitor.GetRequiredMipCount(), | ||
| new_layer_pass.GetRequiredMipCount())); |
There was a problem hiding this comment.
The new layer will always have a default required mip count. The place where the maxing needs to happen is when a layer is being appended that has a backdrop filter.
There was a problem hiding this comment.
The place where the maxing needs to happen is when a layer is being appended that has a backdrop filter.
That still exists from the last PR. This covers both cases, image filter and backdrop filter. Is there a case this doesn't cover, or are you suggesting there is a singular place we can put this code instead of having it in 2 locations?
There was a problem hiding this comment.
Wait, you are saying the max needs to happen there as well? I'm not understanding the order of operations that is problematic. Can you lay it out for me?
There was a problem hiding this comment.
The mip count we need to use when allocating the texture is the maximum we detect for all reads. A backdrop texture is read by both subpass backdrop filters as well as the image filter that happened to be set at the time the SaveLayer was made.
Inside of Save(), the RequiredMipCount of the new layer is being set to the backdrop filter's mip count. But the backdrop filter will read from the parent layer's backdrop, not the new layer's backdrop.
There was a problem hiding this comment.
So when we come across a backdrop filter, we need to max the mip count with the parent pass mip count. And when an image filter is being applied via the paint state, no maxing is necessary when setting the mip count because nothing should have set the mip count yet.
There was a problem hiding this comment.
Ah yea I see, thanks. This started making the previous tests print out the "no mips" message when rendering the blur. I would like to have a way to assert that in the tests. We still have some tests that use the filter without mips (assuming they will have them at runtime) so I was a bit squishy with that. I'll see if I can find a way to assert it.
There was a problem hiding this comment.
Okay fixed. I added a logs check to make sure this doesn't regress. We don't have a better way to assert this currently. I can file an issue to make it a hard assert and migrate all the tests over.
impeller/aiks/canvas.cc
Outdated
| @@ -190,6 +190,9 @@ void Canvas::Save(bool create_subpass, | |||
| MipCountVisitor mip_count_visitor; | |||
| backdrop_filter->Visit(mip_count_visitor); | |||
| subpass->SetRequiredMipCount(mip_count_visitor.GetRequiredMipCount()); | |||
There was a problem hiding this comment.
| subpass->SetRequiredMipCount(mip_count_visitor.GetRequiredMipCount()); |
gaaclarke
added
the
autosubmit
…141664) flutter/engine@d4b6b7e...1382ff7 2024-01-16 magder@google.com Remove iOS 12 availability checks (flutter/engine#49771) 2024-01-16 30870216+gaaclarke@users.noreply.github.com [Impeller] generate mipmaps for imagefilters (flutter/engine#49794) 2024-01-16 ditman@gmail.com [web] Leave blob URLs untouched in TT policy. (flutter/engine#49782) 2024-01-16 jason-simmons@users.noreply.github.com [Impeller] Fix a race between SwapchainImplVK::Present and WaitForFence (flutter/engine#49777) 2024-01-16 737941+loic-sharma@users.noreply.github.com Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49726) 2024-01-16 jason-simmons@users.noreply.github.com [Impeller] Add an API for sampling strictly within the bounds of the source rect in DrawImageRect (flutter/engine#49696) 2024-01-16 jonahwilliams@google.com [Impeller] Encode directly to command buffer for Vulkan. (flutter/engine#49780) 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 jonahwilliams@google.com,rmistry@google.com,zra@google.com 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
This resolves Jonah's feedback on #49607 and Brandons feedback on #49607 and #49778.
This avoids the need to do a lookahead by storing the proper mip count value while building the drawing on the canvas.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.