Reland: [Impeller] Use downsample shader for blur instead of mip levels. #54149

gaaclarke · 2024-07-26T19:56:42Z

relands #53760
reverted in #54148

The fix for this was found with the help of Jonah.

Part of a series of gaussian blur changes:

Revert "[Impeller] Use downsample shader for blur instead of mip levels. (#53760)" #54148
[impeller] adds test for catching shimmer in gaussian blur #54116
[Impeller] makes the gaussian down sample scalar fixed by adjusting the downsample padding #54150
Reland: [Impeller] Use downsample shader for blur instead of mip levels. #54149

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.

jonahwilliams · 2024-07-26T20:48:01Z

impeller/entity/contents/filters/gaussian_blur_filter_contents.cc

-    if (pass_args.effective_scalar.x <= 0.125f) {
+    if (pass_args.effective_scalar.x <= 0.0625f) {
+      edge = 7.0;
+      ratio = 1.0f / 64.0f;


probably a better way of doing it...

jonahwilliams

LGTM

…ls. (#53760)" (#54148) This is reverted since it introduces shimmering when the downsample scalar goes to 1/16th. It was reverted instead of fixed to allow the test that should have blocked this to land in the meantime. reverts #53760 fixed and relanded in #54149 Part of a series of gaussian blur changes: 1) #54148 1) #54116 1) #54150 1) #54149 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] 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. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

fixes flutter/flutter#152195 Part of a series of gaussian blur changes: 1) #54148 1) #54116 1) #54150 1) #54149 Note: this test won't pass until #54148 lands. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] 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. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

auto-submit · 2024-07-26T22:31:31Z

auto label is removed for flutter/engine/54149, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

gaaclarke · 2024-07-26T22:47:36Z

This PR is counting on #54150 to get it below the 1.0 threshold. That PR however creates some artifacts at the edges of 1D blurs. I'll look to see if we can fix that otherwise we can look into other options.

jonahwilliams · 2024-07-29T22:19:16Z

Does this still visually look wrong even if the threshold is above 1?

gaaclarke · 2024-07-30T15:23:01Z

Does this still visually look wrong even if the threshold is above 1?

Looking into it. The test passes for me locally and only should have affected sigma ranges that are not in the test. So something weird is going on.

gaaclarke · 2024-07-30T15:27:18Z

I just forgot to push my fix for the 1d blur and rebase. I think we are good.

…he downsample padding (#54150) This removes jitter in the gaussian blur. Previous version of the shimmer test show this drops the average RMSE 30%. testing: - positive change shown in average RMSE in ShimmerTest - existing golden tests Part of a series of gaussian blur changes: 1) #54148 1) #54116 1) #54150 1) #54149 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

gaaclarke · 2024-07-30T15:35:33Z

Or more accurately, once #54150 lands this will pass.

…tter#53760) Fixes flutter/flutter#150973 On a Pixel 7 pro, this speeds up the repeated blur macrobenchmark by 20ms (~65 -> ~45). The mip computation for backdrop filters is especially wasteful, as most of the contents are unused. RIght now, we also need to continually re-generate mips every time we do a texture flip, even if there is only a single blur. This is because the mip level is a property of the texture, which gets reused in the entity pass backdrop.

flutter-dashboard · 2024-07-30T16:29:40Z

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 #54149 at sha 014c577

gaaclarke · 2024-07-30T16:49:08Z

This PR coops old tests but maintains an old branch in the code for when mipmaps are used. I verified that the following tests still exercise that old branch. We might want something more apropos though to make sure that we don't get rid of testing that branch.

Play/AiksTest.BlurredRectangleWithShader/Metal
Play/AiksTest.MaskBlurVariantTestSolidTranslucentWithFilters/Metal
Play/AiksTest.MaskBlurVariantTestInnerTranslucentWithBlurImageFilter/Metal
Play/AiksTest.MaskBlurVariantTestOuterOpaqueWithBlurImageFilter/Metal
Play/AiksTest.GaussianBlurSolidColorTinyMipMap/Metal
Play/AiksTest.GaussianBlurBackdropTinyMipMap/Metal
Play/AiksTest.CanRenderEmojiTextFrameWithBlur/Metal
Play/DlGoldenTest.TextBlurMaskFilterDisrespectCTM/Metal
Play/DlGoldenTest.GaussianVsRRectBlurScaled/Metal
Play/DlGoldenTest.GaussianVsRRectBlurScaledRotated/Metal
Play/DlGoldenTest.FastVsGeneralGaussianMaskBlur/Metal

…f mip levels. (flutter/engine#54149)

…152573) flutter/engine@31bb9f9...a6c5ff2 2024-07-30 [email protected] Roll Skia from a4d59988bde0 to c1fcf8e5a55c (1 revision) (flutter/engine#54220) 2024-07-30 [email protected] Reland: [Impeller] Use downsample shader for blur instead of mip levels. (flutter/engine#54149) 2024-07-30 [email protected] Roll Skia from 2cb7f5443a48 to a4d59988bde0 (1 revision) (flutter/engine#54218) 2024-07-30 [email protected] [Impeller] makes the gaussian down sample scalar fixed by adjusting the downsample padding (flutter/engine#54150) 2024-07-30 [email protected] Roll Dart SDK from ad4d2e8b2c65 to 14b51d32e3a6 (1 revision) (flutter/engine#54216) 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

…lutter#152573) flutter/engine@31bb9f9...a6c5ff2 2024-07-30 [email protected] Roll Skia from a4d59988bde0 to c1fcf8e5a55c (1 revision) (flutter/engine#54220) 2024-07-30 [email protected] Reland: [Impeller] Use downsample shader for blur instead of mip levels. (flutter/engine#54149) 2024-07-30 [email protected] Roll Skia from 2cb7f5443a48 to a4d59988bde0 (1 revision) (flutter/engine#54218) 2024-07-30 [email protected] [Impeller] makes the gaussian down sample scalar fixed by adjusting the downsample padding (flutter/engine#54150) 2024-07-30 [email protected] Roll Dart SDK from ad4d2e8b2c65 to 14b51d32e3a6 (1 revision) (flutter/engine#54216) 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

github-actions bot added the e: impeller label Jul 26, 2024

gaaclarke force-pushed the reland-downsample-sahder branch from 929c607 to 47404e1 Compare July 26, 2024 20:25

gaaclarke requested a review from jonahwilliams July 26, 2024 20:42

gaaclarke marked this pull request as ready for review July 26, 2024 20:42

gaaclarke requested a review from bdero July 26, 2024 20:42

jonahwilliams reviewed Jul 26, 2024

View reviewed changes

jonahwilliams approved these changes Jul 26, 2024

View reviewed changes

gaaclarke force-pushed the reland-downsample-sahder branch from 47404e1 to 96cae3b Compare July 26, 2024 21:55

gaaclarke force-pushed the reland-downsample-sahder branch from 96cae3b to ee9b90d Compare July 26, 2024 22:09

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2024

gaaclarke force-pushed the reland-downsample-sahder branch 2 times, most recently from d0403fb to 633a4d9 Compare July 30, 2024 15:29

jonahwilliams and others added 2 commits July 30, 2024 08:43

fixed 1/16th downsample

014c577

gaaclarke force-pushed the reland-downsample-sahder branch from 633a4d9 to 014c577 Compare July 30, 2024 15:44

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 30, 2024

flutter-dashboard bot added the will affect goldens label Jul 30, 2024

auto-submit bot merged commit 41e4b39 into flutter:main Jul 30, 2024
34 checks passed

engine-flutter-autoroll mentioned this pull request Jul 30, 2024

Roll Flutter Engine from 31bb9f98472a to 41e4b3952b6b (4 revisions) flutter/flutter#152570

Closed

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

41e4b3952 Reland: [Impeller] Use downsample shader for blur instead o…

75f937f

…f mip levels. (flutter/engine#54149)

engine-flutter-autoroll mentioned this pull request Jul 30, 2024

Roll Flutter Engine from 31bb9f98472a to a6c5ff26c266 (5 revisions) flutter/flutter#152573

Merged

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

41e4b3952 Reland: [Impeller] Use downsample shader for blur instead o…

1c748c2

…f mip levels. (flutter/engine#54149)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reland: [Impeller] Use downsample shader for blur instead of mip levels. #54149

Reland: [Impeller] Use downsample shader for blur instead of mip levels. #54149

Uh oh!

gaaclarke commented Jul 26, 2024 •

edited

Loading

Uh oh!

jonahwilliams Jul 26, 2024

Uh oh!

jonahwilliams left a comment

Uh oh!

auto-submit bot commented Jul 26, 2024

Uh oh!

gaaclarke commented Jul 26, 2024

Uh oh!

jonahwilliams commented Jul 29, 2024

Uh oh!

gaaclarke commented Jul 30, 2024

Uh oh!

gaaclarke commented Jul 30, 2024

Uh oh!

gaaclarke commented Jul 30, 2024

Uh oh!

flutter-dashboard bot commented Jul 30, 2024

Uh oh!

Uh oh!

gaaclarke commented Jul 30, 2024

Uh oh!

Uh oh!

Reland: [Impeller] Use downsample shader for blur instead of mip levels. #54149

Reland: [Impeller] Use downsample shader for blur instead of mip levels. #54149

Uh oh!

Conversation

gaaclarke commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

jonahwilliams Jul 26, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Jul 26, 2024

Uh oh!

gaaclarke commented Jul 26, 2024

Uh oh!

jonahwilliams commented Jul 29, 2024

Uh oh!

gaaclarke commented Jul 30, 2024

Uh oh!

gaaclarke commented Jul 30, 2024

Uh oh!

gaaclarke commented Jul 30, 2024

Uh oh!

flutter-dashboard bot commented Jul 30, 2024

Uh oh!

Uh oh!

gaaclarke commented Jul 30, 2024

Uh oh!

Uh oh!

gaaclarke commented Jul 26, 2024 •

edited

Loading