[Impeller] implements incorporating the gaussian blur snapshot transform #48426

gaaclarke · 2023-11-27T22:40:03Z

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.

…sforms

gaaclarke · 2023-11-27T23:38:26Z

impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc

+
+  std::shared_ptr<Texture> texture = MakeTexture(desc);
+  auto texture_contents = std::make_shared<TextureContents>();
+  texture_contents->SetSourceRect(Rect::MakeSize(texture->GetSize()));


@bdero is this something I have to worry about? I don't think your tester has a way of modifying this.

Having a smaller source rect will result in the snapshot performing a subpass instead of just passing through the texture, but the snapshot will look otherwise very similar as far as the blur is concerned.

gaaclarke · 2023-11-27T23:39:05Z

impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc

+      EXPECT_TRUE(RectNear(result_coverage.value(),
+                           Rect::MakeLTRB(49.f, 39.f, 151.f, 141.f)));
+      EXPECT_TRUE(RectNear(contents_coverage.value(),
+                           Rect::MakeLTRB(98.f, 78.f, 302.f, 282.f)));


I'm not sure why this is correct, this matches our existing behavior. I'm guessing because one is relative to the subpass @bdero ?

(This turned out to be an ordering issue in the test)

jonahwilliams · 2023-11-27T23:42:41Z

Woot! Taking a look.

jonahwilliams · 2023-11-28T00:27:08Z

I can confirm that the text shadow backgrounds are showing up correctly now!

I don't know that these next issues are specific to this PR so feel free to land and move on:

I am noticing two specific artifacts: when resizing the blur the shakes are back. This can be seen in wonderous on the timeline view. I pulled up the downsampled texture and it looks pretty chunk in a way that might explain the banding, I could be wrong but with linear sampling I'd expect smoother edges. maybe this is due to missing mip maps?

Second issue is that the blur doesn't go all the way to the edge in flutter/flutter#132735 (comment) . Maybe the halo stuff is incorrect, or the returned snapshot transform needs to align top left to the start of the blur and not the halo? (I'm not sure what the halo is).

Skia

Impeller

jonahwilliams

LGTM

jonahwilliams · 2023-11-28T00:35:30Z

Or maybe the issue isn't mip levels, its just that we need to clamp the blur to be less agressive at downscaling (sigma 4 minimum like Skia)

gaaclarke · 2023-11-28T00:39:45Z

I could be wrong but with linear sampling I'd expect smoother edges. maybe this is due to missing mip maps?

I think that may just be our aggressive downsampling. We might want to ease up on that.

I'll start playing around with wonderous now that it seems like we got the easy stuff done.

jonahwilliams · 2023-11-28T00:47:56Z

I think you're right. It looks like we scale down our blur radius so its about 4. something. Skia will scale down the sigma to 4 something, but that corresponds to a radius of roughly ~6ish I think

jonahwilliams · 2023-11-28T00:51:20Z

another possibility is that we need to use linear everywhere

…hot transform (flutter/engine#48426)

…139163) flutter/engine@d375d5b...fd3a33f 2023-11-28 skia-flutter-autoroll@skia.org Roll Skia from 8752700a2565 to f07025afd712 (1 revision) (flutter/engine#48462) 2023-11-28 30870216+gaaclarke@users.noreply.github.com [Impeller] implements incorporating the gaussian blur snapshot transform (flutter/engine#48426) 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 matanl@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

…lutter#139163) flutter/engine@d375d5b...fd3a33f 2023-11-28 skia-flutter-autoroll@skia.org Roll Skia from 8752700a2565 to f07025afd712 (1 revision) (flutter/engine#48462) 2023-11-28 30870216+gaaclarke@users.noreply.github.com [Impeller] implements incorporating the gaussian blur snapshot transform (flutter/engine#48426) 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 matanl@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

gaaclarke added 5 commits November 27, 2023 13:32

[Impeller] made the new gaussian blur take into account snapshot tran…

2d0d9d4

…sforms

got a simple test passing

5ca913b

backfilled test

5838366

matched existing behavior

47735f0

format

7d45243

github-actions bot added the e: impeller label Nov 27, 2023

gaaclarke added 2 commits November 27, 2023 15:00

switched to assuming the directional filter is correct

7a83e84

added comment

c630976

gaaclarke marked this pull request as ready for review November 27, 2023 23:37

gaaclarke requested review from bdero and jonahwilliams November 27, 2023 23:37

gaaclarke commented Nov 27, 2023

View reviewed changes

made sure to set transform before getting the result

e08072b

jonahwilliams approved these changes Nov 28, 2023

View reviewed changes

gaaclarke merged commit 4168cf7 into flutter:main Nov 28, 2023

engine-flutter-autoroll mentioned this pull request Nov 28, 2023

Roll Flutter Engine from d375d5b95d59 to 4168cf737fd7 (1 revision) flutter/flutter#139160

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 28, 2023

4168cf737 [Impeller] implements incorporating the gaussian blur snaps…

73c189a

…hot transform (flutter/engine#48426)

engine-flutter-autoroll mentioned this pull request Nov 28, 2023

Roll Flutter Engine from d375d5b95d59 to fd3a33f8b239 (2 revisions) flutter/flutter#139163

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 28, 2023

4168cf737 [Impeller] implements incorporating the gaussian blur snaps…

e615193

…hot transform (flutter/engine#48426)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] implements incorporating the gaussian blur snapshot transform #48426

[Impeller] implements incorporating the gaussian blur snapshot transform #48426

Uh oh!

gaaclarke commented Nov 27, 2023 •

edited

Loading

Uh oh!

gaaclarke Nov 27, 2023

Uh oh!

bdero Nov 28, 2023

Uh oh!

gaaclarke Nov 27, 2023

Uh oh!

bdero Nov 28, 2023

Uh oh!

jonahwilliams commented Nov 27, 2023

Uh oh!

jonahwilliams commented Nov 28, 2023

Uh oh!

jonahwilliams left a comment

Uh oh!

jonahwilliams commented Nov 28, 2023

Uh oh!

gaaclarke commented Nov 28, 2023

Uh oh!

jonahwilliams commented Nov 28, 2023

Uh oh!

jonahwilliams commented Nov 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Impeller] implements incorporating the gaussian blur snapshot transform #48426

[Impeller] implements incorporating the gaussian blur snapshot transform #48426

Uh oh!

Conversation

gaaclarke commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

gaaclarke Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

bdero Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

gaaclarke Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

bdero Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Nov 27, 2023

Uh oh!

jonahwilliams commented Nov 28, 2023

Skia

Impeller

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Nov 28, 2023

Uh oh!

gaaclarke commented Nov 28, 2023

Uh oh!

jonahwilliams commented Nov 28, 2023

Uh oh!

jonahwilliams commented Nov 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gaaclarke commented Nov 27, 2023 •

edited

Loading