[Impeller] blur - cropped the downsample pass for backdrop filters #53562

gaaclarke · 2024-06-25T16:39:46Z

This should be no functional change, just makes the case of clipped backdrop filters take up less memory.

fixes: flutter/flutter#150713

test coverage:

the framework's "backdrop filter" test
GaussianBlurAnimatedBackdrop

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.

gaaclarke · 2024-06-25T18:03:16Z

This doesn't have a huge impact on FPS, but it does on memory usage.

before

after

gaaclarke · 2024-06-25T18:10:15Z

Also worth noting: this isn't good to land yet because it introduces shimmering in animation. I think we may be able to eliminate that by adding padding to make sure that when downsampling we end up with integer size. That would require us to modify the source_expanded_coverage_hint in CalculateDownsamplePassArgs.

jonahwilliams · 2024-06-25T20:20:46Z

This is using about a quarter of the memory as ToT

ToT

Patched

gaaclarke · 2024-06-25T21:36:21Z

This breaks image filters currently. I gotta go back and see what happened there.

gaaclarke · 2024-06-26T17:52:37Z

I fixed the image filter by adding a branch for handling backdrop image filters differently, keeping the old math for image filters since they are already cropped.

There are quite a few blur changes from the pixel alignment changes which are fine. Here are the ones that look broken. I think this has to do with matrix not being correct yet for the backdrop filter flow and the blur styles not taking the new math into account:

…side of the snapshot

jonahwilliams · 2024-06-26T20:13:47Z

impeller/entity/contents/filters/gaussian_blur_filter_contents.cc

 #include "impeller/renderer/render_pass.h"
 #include "impeller/renderer/vertex_buffer_builder.h"

+#pragma GCC diagnostic ignored "-Wunused-function"


nit: remove

jonahwilliams · 2024-06-26T20:14:46Z

impeller/entity/contents/filters/gaussian_blur_filter_contents.cc

+    ISize source_size = ISize(aligned_coverage_hint.GetSize().width,
+                              aligned_coverage_hint.GetSize().height);
+    Vector2 downsampled_size = source_size * downsample_scalar;
+    Scalar int_part;


You can also add [[maybe_unused]]

I think I did that in the past and the compiler for fuchsia or windows didn't care =T

jonahwilliams

LGTM!!!!!!!!

flutter-dashboard · 2024-06-26T20:18:03Z

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 #53562 at sha e6d3ffe

bdero

The restrictions on applying the coverage hint LGTM!

bdero · 2024-06-26T20:19:37Z

impeller/entity/contents/filters/gaussian_blur_filter_contents.cc

+    // coverage hint that fits inside of the snapshots coverage that means the
+    // coverage hint was ignored so we will trim out the area we are interested
+    // in the down-sample pass. This usually means we have a backdrop image
+    // filter.


Ah, the described behavior happens whenever there's a Texture filter input, right? Makes sense that we need to do some extra work to apply the coverage hint for backdrop filters here.

Ah, the described behavior happens whenever there's a Texture filter input, right?

Yep, exactly.

…filters (flutter/engine#53562)

…150888) flutter/engine@1d5e3cc...a9194f0 2024-06-26 [email protected] Roll Skia from 173ee0af82c9 to 55ada83438cd (3 revisions) (flutter/engine#53596) 2024-06-26 [email protected] Return a null image from ImageExternalTextureGL::CreateEGLImage if an EGL display is not available (flutter/engine#53594) 2024-06-26 [email protected] [Impeller] blur - cropped the downsample pass for backdrop filters (flutter/engine#53562) 2024-06-26 [email protected] Roll Skia from 0a979d9f3606 to 173ee0af82c9 (2 revisions) (flutter/engine#53593) 2024-06-26 [email protected] Remove otherwise unused third_party/web_dependencies. (flutter/engine#53588) 2024-06-26 [email protected] Copy `flutter/flutter/docs/engine` to `flutter/engine/docs` as-is (no changes) (flutter/engine#53595) 2024-06-26 [email protected] Roll Dart SDK from 38bb74f63829 to c01f907d34d8 (5 revisions) (flutter/engine#53585) 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-06-27T15:36:30Z

Hmm, there was no change in our benchmark: https://flutter-flutter-perf.skia.org/e/?keys=X6db42c30be541ffea6a232432d0e198b

That's odd since we see something completely different in local testing.

jonahwilliams · 2024-06-27T15:50:26Z

That metric does not measure GPU memory FYI

gaaclarke · 2024-06-27T15:55:20Z

That metric does not measure GPU memory FYI

My local testing is showing 1/2 (500MB->250MB) memory size and 1/5 gpu (250MB->50MB) memory above. So I would expect some change.

jonahwilliams · 2024-06-27T15:57:40Z

Its only capturing some portion of the overall heal though, Is your first number Host + GPU memory? I wouldn't but too much stock in it, we have the local numbers that are easily reproducible.

jonahwilliams · 2024-06-27T16:00:09Z

*heap

jonahwilliams · 2024-06-27T16:00:20Z

*but -> put

Man I can't spell today

gaaclarke · 2024-06-27T16:20:54Z

Ugh, yea it's just frustrating. I just verified it locally and I'll file an issue. Those probes seem bogus.

gaaclarke · 2024-06-27T16:28:14Z

filed issue here: flutter/flutter#150936

jonahwilliams · 2024-06-27T16:44:30Z

We can add our own tracker in allocator_mtl, for example on Vulkan: https://github.com/flutter/engine/blob/main/impeller/renderer/backend/vulkan/allocator_vk.cc#L512

…lutter#150888) flutter/engine@1d5e3cc...a9194f0 2024-06-26 [email protected] Roll Skia from 173ee0af82c9 to 55ada83438cd (3 revisions) (flutter/engine#53596) 2024-06-26 [email protected] Return a null image from ImageExternalTextureGL::CreateEGLImage if an EGL display is not available (flutter/engine#53594) 2024-06-26 [email protected] [Impeller] blur - cropped the downsample pass for backdrop filters (flutter/engine#53562) 2024-06-26 [email protected] Roll Skia from 0a979d9f3606 to 173ee0af82c9 (2 revisions) (flutter/engine#53593) 2024-06-26 [email protected] Remove otherwise unused third_party/web_dependencies. (flutter/engine#53588) 2024-06-26 [email protected] Copy `flutter/flutter/docs/engine` to `flutter/engine/docs` as-is (no changes) (flutter/engine#53595) 2024-06-26 [email protected] Roll Dart SDK from 38bb74f63829 to c01f907d34d8 (5 revisions) (flutter/engine#53585) 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

[Impeller] blur - cropped the downsample pass for backdrop filters

2234d77

github-actions bot added the e: impeller label Jun 25, 2024

gaaclarke added 4 commits June 25, 2024 14:10

got rid of the shimmering

b2b314c

cleanup

203062b

fix tidy

8beff0c

renamed function

b963253

gaaclarke added 3 commits June 26, 2024 10:13

fixed imagefilter

815ac5b

added comments

45f3f28

update comment

85619de

gaaclarke added 2 commits June 26, 2024 11:28

started trimming in the downsample only if the expanded range fits in…

3e9ca5e

…side of the snapshot

tidy

e6d3ffe

gaaclarke marked this pull request as ready for review June 26, 2024 19:55

gaaclarke requested review from bdero and jonahwilliams June 26, 2024 20:07

jonahwilliams reviewed Jun 26, 2024

View reviewed changes

jonahwilliams approved these changes Jun 26, 2024

View reviewed changes

flutter-dashboard bot added the will affect goldens label Jun 26, 2024

removed stray pragma

8d32b77

bdero approved these changes Jun 26, 2024

View reviewed changes

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

auto-submit bot merged commit 7f81e71 into flutter:main Jun 26, 2024

engine-flutter-autoroll mentioned this pull request Jun 26, 2024

Roll Flutter Engine from 1d5e3cc55a75 to a9194f0f01f4 (7 revisions) flutter/flutter#150888

Merged

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

7f81e71ec [Impeller] blur - cropped the downsample pass for backdrop …

de03d19

…filters (flutter/engine#53562)

[Impeller] blur - cropped the downsample pass for backdrop filters #53562

[Impeller] blur - cropped the downsample pass for backdrop filters #53562

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

gaaclarke commented Jun 25, 2024

before

after

Uh oh!

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

Uh oh!

Uh oh!

jonahwilliams commented Jun 25, 2024

ToT

Patched

Uh oh!

gaaclarke commented Jun 25, 2024

Uh oh!

gaaclarke commented Jun 26, 2024

Uh oh!

jonahwilliams Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

gaaclarke Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

gaaclarke Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Jun 26, 2024

Uh oh!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

bdero Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

gaaclarke Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Jun 27, 2024

Uh oh!

jonahwilliams commented Jun 27, 2024

Uh oh!

gaaclarke commented Jun 27, 2024

Uh oh!

jonahwilliams commented Jun 27, 2024

Uh oh!

jonahwilliams commented Jun 27, 2024

Uh oh!

jonahwilliams commented Jun 27, 2024

Uh oh!

gaaclarke commented Jun 27, 2024

Uh oh!

gaaclarke commented Jun 27, 2024

Uh oh!

jonahwilliams commented Jun 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gaaclarke commented Jun 25, 2024 •

edited

Loading

gaaclarke commented Jun 25, 2024 •

edited

Loading