Reland: [Impeller] Turned on new blur. (#48472) #49642

gaaclarke · 2024-01-09T19:13:24Z

This is a reland of the new gaussian blur.

Changes since revert:

Textures are now recycled with ping ponging to reduce memory usage
Mipmaps are generated to diminish the shimmering that happens with animated blurs (in metal)

This new blur should perform faster since it scales down the image before blurring it. Jonah did early testing of it and found it to be faster. Scrolling around with the blur perf bug it seems faster. It also has a wider test bed and is hopefully easier to maintain since it contains all of its logic for both directions.

testing: There are existing blur tests and we've backfilled more as we've added features to this blur.

fixes flutter/flutter#131580 fixes flutter/flutter#138259

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].

flutter-dashboard · 2024-01-09T19:13:27Z

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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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.

gaaclarke · 2024-01-13T00:17:45Z

This looks like no one is ever setting TextureVK::mipmap_generated_. I tried to make that set and there was still a validation error. There seems to be some problems with vulkan mipmap generation that need to get sorted out.

gaaclarke · 2024-01-13T00:54:28Z

Looks like this is failing on vulkan still. Jonah suggested just turning it on for iOS.

Error log:

error: XDG_RUNTIME_DIR not set in the environment.
error: XDG_RUNTIME_DIR not set in the environment.
../../flutter/third_party/swiftshader/src/Device/Blitter.cpp:257 ABORT: ASSERT(subresourceRange.levelCount == 1)

gaaclarke · 2024-01-17T18:55:53Z

After #49836 this should be good to go.

gaaclarke · 2024-01-17T19:38:19Z

This depends on #49836, but that is approved and will be landing shortly so I'm putting this into review now.

gaaclarke · 2024-01-17T19:39:30Z

I'm going to hit the rebase button to retrigger tests even though everything is green. It looks like golden tests ci step isn't smart enough to trigger when switching from a draft pr.

jonahwilliams

LGTM

flutter-dashboard · 2024-01-17T20:29:35Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #49642 at sha 8f13cdf

gaaclarke · 2024-01-17T22:43:40Z

Golden images are looking good, no need to approve them since this will just get rebased after the vulkan goldens tests pr lands.

bdero

LET'S GO!

This new blur should perform faster since it scales down the image before blurring it. Jonah did early testing of it and found it to be faster. Scrolling around with the blur perf bug it seems faster. It also has a wider test bed and is hopefully easier to maintain since it contains all of its logic for both directions. testing: There are existing blur tests and we've backfilled more as we've added features to this blur. fixes flutter/flutter#131580 fixes flutter/flutter#138259 - [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

This reverts commit cae5c50.

This reverts commit 590a245.

This reverts commit 3d8682e.

flutter-dashboard · 2024-01-18T00:43:48Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #49642 at sha 0d8dfd5

…tter/engine#49642)

…141744) flutter/engine@73a2de5...98c16b4 2024-01-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Manual roll Dart SDK from d6c08fa9bb54 to 6ff69d6b7f59 (15 revisions)" (flutter/engine#49852) 2024-01-18 [email protected] Roll Skia from 5abf9717ea92 to 31f275e8fcc7 (8 revisions) (flutter/engine#49850) 2024-01-18 [email protected] Reland: [Impeller] Turned on new blur. (#48472) (flutter/engine#49642) 2024-01-17 [email protected] [Windows] Reduce Visual Studio build errors caused by keyboard unit tests (flutter/engine#49814) 2024-01-17 [email protected] [Impeller] disabled misleading vulkan golden image tests (flutter/engine#49836) 2024-01-17 [email protected] [Windows] Remove unnecessary statics in keyboard (flutter/engine#49834) 2024-01-17 [email protected] Roll Skia from 5c9e3474cf13 to 5abf9717ea92 (1 revision) (flutter/engine#49831) 2024-01-17 [email protected] Move mac cache builder to bringup. (flutter/engine#49843) 2024-01-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Switch from transient stencil-only to depth+stencil buffer." (flutter/engine#49832) 2024-01-17 [email protected] Roll Skia from d8da1ee69767 to 5c9e3474cf13 (2 revisions) (flutter/engine#49827) 2024-01-17 [email protected] Manual roll Dart SDK from d6c08fa9bb54 to 6ff69d6b7f59 (15 revisions) (flutter/engine#49825) 2024-01-17 [email protected] Roll Skia from 02e94b3b4d29 to d8da1ee69767 (1 revision) (flutter/engine#49824) 2024-01-17 [email protected] Flutter GPU: Add GpuContext.createHostBuffer (flutter/engine#49822) 2024-01-17 [email protected] Roll Fuchsia Linux SDK from Klxww53tA4-TG5pA9... to GuU0e5WxJCi92Scz8... (flutter/engine#49823) 2024-01-17 [email protected] [Impeller] Switch from transient stencil-only to depth+stencil buffer. (flutter/engine#47987) 2024-01-17 [email protected] Roll Skia from 31309ff09537 to 02e94b3b4d29 (43 revisions) (flutter/engine#49819) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from Klxww53tA4-T to GuU0e5WxJCi9 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

gaaclarke · 2024-01-18T17:07:39Z

Benchmarks for the new blur (about 2x faster): https://flutter-flutter-perf.skia.org/e/?keys=X01fc3d52ebd6fbf38afef91d82ab8d2b&xbaroffset=38815

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

This comment was marked as outdated.

Sign in to view

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

gaaclarke mentioned this pull request Jan 9, 2024

[Impeller] new blur doesn't handle effect transform properly flutter/flutter#141204

Closed

gaaclarke force-pushed the reland-new-blur branch 4 times, most recently from 0effece to cadf048 Compare January 12, 2024 23:48

gaaclarke marked this pull request as draft January 13, 2024 00:29

gaaclarke mentioned this pull request Jan 13, 2024

[Impeller] Vulkan mipmap generation does not work flutter/flutter#141495

Closed

gaaclarke force-pushed the reland-new-blur branch 5 times, most recently from af4bb63 to e1eb078 Compare January 16, 2024 22:54

gaaclarke force-pushed the reland-new-blur branch from 806e837 to 2869bf8 Compare January 17, 2024 19:04

gaaclarke marked this pull request as ready for review January 17, 2024 19:37

gaaclarke requested review from bdero and jonahwilliams January 17, 2024 19:37

gaaclarke force-pushed the reland-new-blur branch from 2869bf8 to 8f13cdf Compare January 17, 2024 19:39

jonahwilliams approved these changes Jan 17, 2024

View reviewed changes

bdero approved these changes Jan 17, 2024

View reviewed changes

gaaclarke and others added 10 commits January 17, 2024 15:33

chinmays vulkan patch

b3c96e1

started setting TextureVK::mipmaps_generated_

330e1ac

made tests only want metal mips

5107cd6

turned on for macos ios only

c92e348

Revert "started setting TextureVK::mipmaps_generated_"

7a095b3

This reverts commit cae5c50.

Revert "chinmays vulkan patch"

ee976a4

This reverts commit 590a245.

Revert "turned on for macos ios only"

e57fc5b

This reverts commit 3d8682e.

disabled mipmap generation for vulkan

a37ff1b

fixed vulkan/opengles test

733b7e3

gaaclarke force-pushed the reland-new-blur branch from 8f13cdf to 733b7e3 Compare January 17, 2024 23:33

updated comment

0d8dfd5

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

auto-submit bot merged commit aede5b0 into flutter:main Jan 18, 2024

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

Roll Flutter Engine from 73a2de5da53f to aede5b0b2df0 (14 revisions) flutter/flutter#141741

Closed

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

aede5b0b2 Reland: [Impeller] Turned on new blur. (flutter#48472) (flu…

594715e

…tter/engine#49642)

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

Roll Flutter Engine from 73a2de5da53f to 8f393335d571 (15 revisions) flutter/flutter#141742

Closed

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

aede5b0b2 Reland: [Impeller] Turned on new blur. (flutter#48472) (flu…

a7e9d28

…tter/engine#49642)

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

Roll Flutter Engine from 73a2de5da53f to 98c16b430e6b (16 revisions) flutter/flutter#141744

Merged

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

aede5b0b2 Reland: [Impeller] Turned on new blur. (flutter#48472) (flu…

494e5fd

…tter/engine#49642)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reland: [Impeller] Turned on new blur. (#48472) #49642

Reland: [Impeller] Turned on new blur. (#48472) #49642

Uh oh!

gaaclarke commented Jan 9, 2024 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Jan 9, 2024

Uh oh!

This comment was marked as outdated.

gaaclarke commented Jan 13, 2024

Uh oh!

gaaclarke commented Jan 13, 2024 •

edited

Loading

Uh oh!

gaaclarke commented Jan 17, 2024

Uh oh!

gaaclarke commented Jan 17, 2024

Uh oh!

gaaclarke commented Jan 17, 2024

Uh oh!

jonahwilliams left a comment

Uh oh!

flutter-dashboard bot commented Jan 17, 2024

Uh oh!

gaaclarke commented Jan 17, 2024

Uh oh!

bdero left a comment

Uh oh!

flutter-dashboard bot commented Jan 18, 2024

Uh oh!

gaaclarke commented Jan 18, 2024

Uh oh!

Uh oh!

Reland: [Impeller] Turned on new blur. (#48472) #49642

Reland: [Impeller] Turned on new blur. (#48472) #49642

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jan 9, 2024

Uh oh!

This comment was marked as outdated.

gaaclarke commented Jan 13, 2024

Uh oh!

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

Uh oh!

Uh oh!

gaaclarke commented Jan 17, 2024

Uh oh!

gaaclarke commented Jan 17, 2024

Uh oh!

gaaclarke commented Jan 17, 2024

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Jan 17, 2024

Uh oh!

gaaclarke commented Jan 17, 2024

Uh oh!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Jan 18, 2024

Uh oh!

gaaclarke commented Jan 18, 2024

Uh oh!

Uh oh!

gaaclarke commented Jan 9, 2024 •

edited

Loading

gaaclarke commented Jan 13, 2024 •

edited

Loading