[Impeller]: new blur - adds mips for backdrop filters #49607

gaaclarke · 2024-01-08T21:47:18Z

If the new blur flag is on and a blur is used as a backdrop filter, the rendered texture will now have mipmaps which will make the downscaling step of the blur have more signal (and thus be less shimmery).

issue: flutter/flutter#140193
fixes: flutter/flutter#140949

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

impeller/entity/contents/filters/gaussian_blur_filter_contents.cc

bdero · 2024-01-08T22:42:11Z

Although this won't generate a mipmap for the backdrop filter case yet, this should already be enough to fix the problem for ui.Images. If you wanna try it out in the playground, you can:

use the enable_mipmapping param when calling CreateTextureForFixture, and then
generate the mipmap using the GenerateMipmap() utility in aiks_unittests.cc.

impeller/entity/entity_pass.cc

impeller/entity/contents/filters/gaussian_blur_filter_contents.cc

gaaclarke · 2024-01-09T18:23:00Z

Okay, after started invalidating mipmaps I've been able to test the sample code from flutter/flutter#140193

This greatly reduces the jitters but it doesn't 100% eliminate it.

I can't easily test against the old blur right now because there is a discrepancy between blur radii between the old and new blur. I wonder if the remainder of the shimmers is from the pixel alignment issue I was looking into friday.

before

before.mov

after

after.mov

gaaclarke · 2024-01-09T18:49:04Z

I did performance testing with the backdrop blur test and gpu time is around 21ms. So I think we are good there.

gaaclarke · 2024-01-09T23:48:44Z

I've rebased this PR onto the fix for the blur radius scaling and I got the same exact result. I'm guessing maybe that benchmark wasn't using an effect_transform? That or the downsampling is counterbalancing the scaled blur radius. Although suspiciously the result was the exact same number (maybe its a from a discrete set?)

impeller/renderer/texture_mipmap.cc

bdero · 2024-01-11T01:38:31Z

impeller/renderer/texture_mipmap.cc

+                                const std::shared_ptr<Texture>& texture) {
+  if (texture->NeedsMipmapGeneration()) {
+    std::shared_ptr<CommandBuffer> mip_cmd_buffer =
+        context->CreateCommandBuffer();


Rather than creating a new command buffer, we could actually append this blit pass to the existing command buffer before submitting it in InlinePassContext::EndPass()

Do you think we should do it? I can add a todo, I probably don't want to fiddle with what works right now.

gaaclarke · 2024-01-12T17:27:44Z

@bdero I went through the draft comments and resolved them. Think everything was addressed or noted in cases where it was guidance. Feel free to reopen or raise similar points.

Right now the mip count is just hardcoded to 4 as we agreed upon and it is also only triggered for the new blur.

flutter-dashboard · 2024-01-12T17:55:48Z

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.

Changes reported for pull request #49607 at sha 7cba038

jonahwilliams · 2024-01-12T18:52:19Z

impeller/entity/entity_pass.cc

+  int32_t required_mip_count = 1;
+  IterateAllElements(
+      [&required_mip_count, lazy_glyph_atlas = renderer.GetLazyGlyphAtlas()](
+          const Element& element) {
+        if (auto entity = std::get_if<Entity>(&element)) {
+          if (const auto& contents = entity->GetContents()) {
+            contents->PopulateGlyphAtlas(lazy_glyph_atlas,
+                                         entity->DeriveTextScale());
+          }
+        }
+        if (auto subpass = std::get_if<std::unique_ptr<EntityPass>>(&element)) {
+          const EntityPass* entity_pass = subpass->get();
+          required_mip_count =
+              std::max(required_mip_count, entity_pass->GetRequiredMipCount());
+        }
+        return true;
+      });



I don't like that we're adding on to this callback - this is a fairly expensive traversal that we're doing in addition to the regular traversal. Can we compute this during normal traversal?

For example, here:

https://github.com/flutter/engine/blob/main/impeller/entity/entity_pass.cc#L531-L551

Or here:

https://github.com/flutter/engine/blob/main/impeller/entity/entity_pass.cc#L856-L874

This lookahead traversal already exists, this just adds one extra branch for looking at mip counts. So the addition won't be expensive. The problem is that the resource is created before we actually render it, so there has to be some form of look ahead afaik. I didn't see an alternative, but maybe brandon has a suggestion.

Yes but I want to remove this lookahead, and this makes it harder :)

I mean, you have to either look ahead, or be able to switch mid render. Since mip map counts have to be specified at creation I think that would mean assuming mip count 1, then blitting to a mipmap texture if we ever encounter something that needs mipmaps.

The other option is to unconditionally add mipmaps which we said we don't want.

So, if I have nested save layers the render target won't have mip maps if it can't be collapsed? Something like this?

Canvas canvas; canvas.Save({.blend_mode = kMultiply}); canvas.DrawCircle(); canvas.Save({}, ImageFilter::MakeBlur());

Yeah. Inverting this to remove the nested SaveLayer would also have a similarly mip-less result:

Canvas canvas; canvas.SaveLayer({.image_filter = ImageFilter::MakeBlur()}); canvas.DrawCircle();

The following works with the proper mipmaps, is this what you had in mind?

TEST_P(AiksTest, GaussianBlurFoo) { Canvas canvas; canvas.DrawCircle({100, 100}, 50, {.color = Color::CornflowerBlue()}); canvas.SaveLayer({}, std::nullopt, ImageFilter::MakeBlur(Sigma(30), Sigma(30), FilterContents::BlurStyle::kNormal, Entity::TileMode::kClamp)); canvas.DrawCircle({200, 200}, 50, {.color = Color::Chartreuse()}); ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); }

This gets us in a place without mipmaps:

TEST_P(AiksTest, GaussianBlurFoo) { Canvas canvas; canvas.DrawPaint({.color = Color::Wheat()}); canvas.SaveLayer({.blend_mode = BlendMode::kMultiply}); canvas.DrawCircle({100, 100}, 50, {.color = Color::CornflowerBlue()}); canvas.SaveLayer({}, std::nullopt, ImageFilter::MakeBlur(Sigma(30), Sigma(30), FilterContents::BlurStyle::kNormal, Entity::TileMode::kClamp)); canvas.DrawCircle({200, 200}, 50, {.color = Color::Chartreuse()}); ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); }

is this what you had in mind?

The example I gave sets an image filter on the paint state handed to the SaveLayer -- no backdrop filter involved. So something like:

TEST_P(AiksTest, GaussianBlurFoo) { Canvas canvas; canvas.SaveLayer({.image_filter = ImageFilter::MakeBlur(...)}); canvas.DrawCircle({200, 200}, 50, {.color = Color::Chartreuse()}); ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); }

bdero · 2024-01-12T20:51:04Z

impeller/aiks/canvas.cc

@@ -156,6 +187,9 @@ void Canvas::Save(bool create_subpass,
            return filter;
          };
      subpass->SetBackdropFilter(backdrop_filter_proc);
+      MipCountVisitor mip_count_visitor;
+      backdrop_filter->Visit(mip_count_visitor);
+      subpass->SetRequiredMipCount(mip_count_visitor.GetRequiredMipCount());


We can work out the required mip level for the EntityPass texture in Canvas::Save without look-aheads later on.

When creating a new subpass:

If the new subpass has backdrop filter, assign the current pass (not the child pass being created) a new max mip level: GetCurrentPass().SetRequiredMipCount(std::max(GetCurrentPass().GetRequiredMipCount(), backdrop_filter_mip_count))

If the Canvas paint state has an image filter, then initialize the max mip level of the subpass with the image filter's max mip level: subpass->SetRequiredMipCount(paint_image_filter_mip_count)

Then, we just use subpass->required_mip_count_ in the two places where we provision the child pass texture in EntityPass.

jonahwilliams

LGTM

bdero · 2024-01-12T22:40:26Z

impeller/entity/entity_pass.cc

-    auto offscreen_target = CreateRenderTarget(
-        renderer, root_render_target.GetRenderTargetSize(),
+    EntityPassTarget offscreen_target = CreateRenderTarget(
+        renderer, root_render_target.GetRenderTargetSize(), required_mip_count,


I thought we were going to protect this behind a flag for the new blur to keep the benchmark comparison sane?

It is behind the new blur flag. Notice how the tests make sure the mip count is 1. When you turn the flag ~~off~~ on those bump up to 4.

So it's doing all the new calculations have test coverage, but the effect on performance should be negligible.

Ah, I missed the FilterContents::kBlurFilterRequiredMipCount. LGTM

…ter/engine#49607)

…141492) flutter/engine@b8e5d47...205ed68 2024-01-12 30870216+gaaclarke@users.noreply.github.com [Impeller]: new blur - adds mips for backdrop filters (flutter/engine#49607) 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 bdero@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

resolves #49607 (comment) [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

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. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions bot added the e: impeller label Jan 8, 2024

bdero reviewed Jan 8, 2024

View reviewed changes

impeller/entity/contents/filters/gaussian_blur_filter_contents.cc Outdated Show resolved Hide resolved

bdero reviewed Jan 8, 2024

View reviewed changes

impeller/entity/contents/filters/gaussian_blur_filter_contents.cc Outdated Show resolved Hide resolved

bdero reviewed Jan 9, 2024

View reviewed changes

impeller/entity/entity_pass.cc Outdated Show resolved Hide resolved

bdero reviewed Jan 9, 2024

View reviewed changes

impeller/entity/contents/filters/gaussian_blur_filter_contents.cc Outdated Show resolved Hide resolved

gaaclarke force-pushed the blur-mips branch from 3a0d21c to e3dc026 Compare January 9, 2024 23:46

gaaclarke force-pushed the blur-mips branch from ecfd10a to 8baeed3 Compare January 10, 2024 23:05

bdero reviewed Jan 11, 2024

View reviewed changes

impeller/renderer/texture_mipmap.cc Show resolved Hide resolved

bdero reviewed Jan 11, 2024

View reviewed changes

impeller/renderer/texture_mipmap.cc Outdated Show resolved Hide resolved

bdero reviewed Jan 11, 2024

View reviewed changes

gaaclarke added 16 commits January 11, 2024 18:59

[Impeller] new blur: added mipmap generation

b803359

added lod parameter

6f20569

started sending the correct lod

92859a2

textureLod

48522bb

hack to set mip level to 4

7b248f7

started invalidating mipmaps

cd3136d

Added texture_mipmap

8bf8bb9

moved mipmap generation to the spots identified by brandon

97f5799

moved mipmap generation to the inline pass context

80813fe

format

2b5790e

removed logs

7204e7c

Removed invalidation and updated the docstring on NeedsMipmapGeneration

afdc277

brought over the mip_count parameter to non msaa paths

6aa39c3

added todo

6896e32

license

e59dc8c

added decal lod support

3f52295

added test

f13d449

gaaclarke force-pushed the blur-mips branch from 5ffa8eb to f13d449 Compare January 12, 2024 03:00

gaaclarke added 6 commits January 11, 2024 19:06

turned off hack

345cb30

fixed linux usage of mip_count

b04debf

added another test for setting mip count levels

12a1d30

added another test

4bea7dc

removed todo

9bf4355

updated comment

7cba038

gaaclarke changed the title ~~[Impeller]: new blur - adds mips~~ [Impeller]: new blur - adds mips for backdrop filters Jan 12, 2024

gaaclarke marked this pull request as ready for review January 12, 2024 17:25

gaaclarke requested review from jonahwilliams and bdero January 12, 2024 17:25

flutter-dashboard bot added the will affect goldens label Jan 12, 2024

jonahwilliams reviewed Jan 12, 2024

View reviewed changes

bdero reviewed Jan 12, 2024

View reviewed changes

gaaclarke requested review from bdero and jonahwilliams January 12, 2024 21:31

jonahwilliams approved these changes Jan 12, 2024

View reviewed changes

gaaclarke mentioned this pull request Jan 12, 2024

[Impeller] remove mipmap count lookahead flutter/flutter#141483

Closed

gaaclarke merged commit 205ed68 into flutter:main Jan 12, 2024
29 checks passed

bdero reviewed Jan 12, 2024

View reviewed changes

gaaclarke mentioned this pull request Jan 12, 2024

[Impeller] fixed nested save layer mipmap counts #49778

Merged

8 tasks

engine-flutter-autoroll mentioned this pull request Jan 12, 2024

Roll Flutter Engine from b8e5d4776908 to 205ed6882a8f (1 revision) flutter/flutter#141492

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 12, 2024

205ed6882 [Impeller]: new blur - adds mips for backdrop filters (flut…

9dfb86a

…ter/engine#49607)

auto-submit bot pushed a commit that referenced this pull request Jan 13, 2024

[Impeller] fixed nested save layer mipmap counts (#49778)

a0ac66e

resolves #49607 (comment) [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

gaaclarke mentioned this pull request Jan 16, 2024

[Impeller] generate mipmaps for imagefilters #49794

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller]: new blur - adds mips for backdrop filters #49607

[Impeller]: new blur - adds mips for backdrop filters #49607

gaaclarke commented Jan 8, 2024 •

edited

bdero commented Jan 8, 2024 •

edited

gaaclarke commented Jan 9, 2024

gaaclarke commented Jan 9, 2024

gaaclarke commented Jan 9, 2024

bdero Jan 11, 2024

gaaclarke Jan 12, 2024

gaaclarke commented Jan 12, 2024

flutter-dashboard bot commented Jan 12, 2024

jonahwilliams Jan 12, 2024

jonahwilliams Jan 12, 2024

gaaclarke Jan 12, 2024

jonahwilliams Jan 12, 2024

gaaclarke Jan 12, 2024

gaaclarke Jan 12, 2024

bdero Jan 12, 2024

gaaclarke Jan 12, 2024 •

edited

gaaclarke Jan 12, 2024

bdero Jan 12, 2024

bdero Jan 12, 2024

bdero Jan 12, 2024

jonahwilliams left a comment

bdero Jan 12, 2024

gaaclarke Jan 12, 2024 •

edited

gaaclarke Jan 12, 2024

bdero Jan 12, 2024

[Impeller]: new blur - adds mips for backdrop filters #49607

[Impeller]: new blur - adds mips for backdrop filters #49607

Conversation

gaaclarke commented Jan 8, 2024 • edited

Pre-launch Checklist

bdero commented Jan 8, 2024 • edited

gaaclarke commented Jan 9, 2024

before

after

gaaclarke commented Jan 9, 2024

gaaclarke commented Jan 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke commented Jan 12, 2024

flutter-dashboard bot commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke Jan 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke Jan 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke commented Jan 8, 2024 •

edited

bdero commented Jan 8, 2024 •

edited

gaaclarke Jan 12, 2024 •

edited

gaaclarke Jan 12, 2024 •

edited