[Impeller] add mechanism for sharing bdf inputs. #55701

jonahwilliams · 2024-10-07T16:35:34Z

Introduces a mechanism to allow backdrop filters to 1) share backdrop inputs and 2) fuse filter applications for faster blurs.

This is a proposed solution to flutter/flutter#131568

Implemented:

Developer can specify a "backdrop id" which indicates that a backdrop layer should share the input texture and potentially cached filter for a layer.
Removes second save layer for each backdrop filter
Removes save layer trace event for backdrop filter
Can fuse backdrop filters if there is more than one identical filter

TBD:

Adjust heruristic to avoid applying bdf filter to entire screen

Suggestions: applying a bdf should be a distinct operation from a save layer in the DL builder/dispatcher. The saveLayer implmenentation in the impeller dispatcher is super convoluted because it needs to handle both.

Video

Video starts with normal bdf then I hot reload to specify that the bdfs share inputs/filters. This is running on a pixel 8 pro

Change to the macrobenchmark app is just:

  Widget build(BuildContext context) {
    Widget addBlur(Widget child, bool shouldBlur) {
      if (shouldBlur) {
        return ClipRect(
          child: BackdropFilter(
            filter: ImageFilter.blur(sigmaX: 5, sigmaY: 5),
            backdropId: 1, // Added ID
            child: child,
          ),
        );
      } else {
        return child;
      }
    }

demo.mp4

Requires framework changes in https://github.com/jonahwilliams/flutter/pull/new/backdrop_id

jonahwilliams · 2024-10-07T16:53:53Z

This code isn't ready for review yet, but adding @flar and @gaaclarke for high level input. This is the doc that I said I was going to write that I dragged my feet on...

jonahwilliams · 2024-10-07T16:58:56Z

Also I notice in the video that there is a slight pixel shift when switching modes...

jonahwilliams · 2024-10-11T16:47:25Z

Currently the transforms are incorrect if the same bdf id is used in different save layers.

flar

Just reviewed the first half - the front end - on this pass.

One thought - why 64-bit signed integer for ID? Do we anticipate having this many BDFs? An optional value feels more appropriate at the Dart level than a magic value (or declaring that valid IDs must be positive). Making the parameter nullable at the Dart level seems simple. std::optional seems like overkill at the C++ level, but these don't get used all that much compared to all of the other work going on.

flar · 2024-10-11T20:41:57Z

display_list/display_list_unittests.cc

    FML_UNREACHABLE();
  }

  virtual void saveLayer(const DlRect& bounds,
                         const SaveLayerOptions& options,
                         uint32_t total_content_depth,
                         DlBlendMode max_content_blend_mode,
-                         const DlImageFilter* backdrop = nullptr) {
+                         const DlImageFilter* backdrop = nullptr,


These are implementations, aren't they? Do they need the default values?

(Or all of these virtual tags?)

These are not overrides so I think they need the default values. overrides of a virtual method don't need to re-specify them.

flar · 2024-10-11T20:43:20Z

display_list/dl_builder.h

@@ -51,7 +51,8 @@ class DisplayListBuilder final : public virtual DlCanvas,
  // |DlCanvas|
  void SaveLayer(const SkRect* bounds,
                 const DlPaint* paint = nullptr,
-                 const DlImageFilter* backdrop = nullptr) override;
+                 const DlImageFilter* backdrop = nullptr,
+                 int64_t backdrop_id = -1) override;


Ditto - default values probably unnecessary.

Actually, in this case there is a lot of code calling directly to a DlBuilder not through the DlCanvas interface so perhaps it needs them for ease of use. That need would disappear if we did something more like Flutter and Skia and have the Builder simply represent the recording process and provide an actual DlCanvas implementation via a getter rather than via direct implementation...?

flar · 2024-10-11T20:45:15Z

display_list/dl_op_receiver.h

@@ -143,15 +141,17 @@ class DlOpReceiver {
  // layer before further rendering happens.
  virtual void saveLayer(const DlRect& bounds,
                         const SaveLayerOptions options,
-                         const DlImageFilter* backdrop = nullptr) = 0;
+                         const DlImageFilter* backdrop = nullptr,
+                         int64_t backdrop_id = -1) = 0;


This used to be the front end and back end for DLs. Now that it is just the backend I think all default values throughout this interface are obsolete. But, maybe there are a couple of unit tests that still call these directly? I'll watch for this as I de-skiafy...

(The DlOp records used during dispatch shouldn't need any defaults for the calls they make.)

I filed flutter/flutter#156617 to remind me to clean this up.

flar · 2024-10-11T21:00:02Z

display_list/skia/dl_sk_canvas.h

@@ -32,7 +32,8 @@ class DlSkCanvasAdapter final : public virtual DlCanvas {
  void Save() override;
  void SaveLayer(const SkRect* bounds,
                 const DlPaint* paint = nullptr,
-                 const DlImageFilter* backdrop = nullptr) override;
+                 const DlImageFilter* backdrop = nullptr,
+                 int64_t backdrop_id = -1) override;


Do we use this directly (as in on an instance of this class as opposed to casting it to a DlCanvas where the defaults already exist)? Actually we might in the embedders or other platform code.

jonahwilliams · 2024-10-11T21:18:14Z

Re: the 64 bit integer. Dart ints are 64 bits and signed, so using a smaller int means we'd need to do a runtime check or throw an error (folks could reasonably use Dart max int has an input). We could remove the -1 and make it nullable instead, I need to look into how to pass that through dart::ffi.

display_list/benchmarking/dl_complexity_metal.cc

display_list/benchmarking/dl_complexity_gl.cc

display_list/dl_canvas.h

impeller/display_list/aiks_playground.cc

impeller/display_list/canvas.cc

gaaclarke · 2024-10-14T17:54:46Z

Since we didn't end up writing a design doc, I know we talked about it that one day over gvc but can you write down (or link to someplace where it is already written) why we needed to have users specify this explicitly and we couldn't come up with a heuristic to do this automatically?

jonahwilliams · 2024-10-14T18:11:09Z

Since we didn't end up writing a design doc..

The invalidation heuristics for backdrop filters would be expensive to track. We'd need to measure dirty regions between any two backdrops and that means recording/intersecting the bounds for all draws.
The huerstics fail if filters like blurs are too close as the sample region will go far out of the output region (and this cannot be controlled).

gaaclarke · 2024-10-14T18:50:50Z

Did you consider making the Dart API look like this instead of specifying an integer? This captures a scope more succinctly and doesn't require managing ids. The downside is that it is impossible to mix and match the blur layer across the code (though that makes the code easier to follow). Under the covers you could keep the implementation the same if you wanted.

Widget build(BuildContext context) {
  return BackdropGroup(
    children:[
      BackdropFilter(...),
      BackdropFilter(...),
    ]
  );
}

jonahwilliams · 2024-10-14T19:20:17Z

I discussed this a bit with @flar and we both like the idea of separating the Backdrop widget from a backdrop-reading widget. So something like:

Widget build(BuildContext context) {
  return BackdropGroup(
    children:[
      BackdropFilter(parent: BackdropGroup.of(context)),
      BackdropFilter(...),
    ]
  );
}

But that is all Dart API that would be constructed at the framework layer. The implementation should look almost identical.

gaaclarke

This was mentioned above but there is actually 2 places we need to make this fix.

impeller/display_list/dl_dispatcher.cc

impeller/display_list/canvas.cc

jonahwilliams · 2024-10-14T23:54:41Z

@gaaclarke I think the one major unadressed point so far is the manipulation of the backdrop filter raw ptrs. I think the display list stores these as shared_ptrs anyway, so I'm going to lookinto changing the dispatcher. If we do that, I'll do that separately.

gaaclarke · 2024-10-15T03:50:47Z

impeller/display_list/canvas.cc

+          !backdrop_data->shared_filter_snapshot.has_value()) {
+        // TODO(jonahwilliams): compute minimum input hint.
+        backdrop_data->shared_filter_snapshot =
+            backdrop_filter_contents->RenderToSnapshot(renderer_, {});


I'm realizing that this can result in worse performance even after we've computed the minimum input hint. Imagine 2 blurred rectangles whose corners touch.

Should we maybe just stick to sharing the input? That will always be an improvement.

sharing the input is a good GPU perfomrnce improvement but removing the filter sharing would discard the vast majority of the CPU overhead for many small filters. I'll show you some numbers tomorrow but even with extra blurring this is still generally faster.

We could also detect how much of the snapshot is used and add a heuristic based on covered %. i do think it would be a bit of an edge case, like someone that adds a single pixel blur to both the top left and bottom right corners, and then opts into (remember this is opt in) backdrop id.

Maybe we should add a benchmark for this case then so we can quantify the worst case scenario. Something like 2 squares on opposite sides of the screen animating sigma, group versus not group. Maybe we could leave some room outside the union so the minimum input hint change can show up when we implement that.

That sounds like a good idea! will do.

flutter-dashboard · 2024-10-15T05:49:06Z

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 #55701 at sha 11ab30b

jonahwilliams · 2024-10-17T16:46:24Z

I modified the backdrop filter blur benchmark application to place a single blur in the top left and in the bottom right corner. This should in theory be the worst possible case for backdrop id. This test was on an iPhone and I made sure to sync after the BDF fix from @gaaclarke and with partial repaint disabled. GPU numbers are performance state medium

No backdrop Id:

2.13 ms GPU Time / ~2.5 ms Raster time.

Backdrop Id w/ shared filter (current design)

1.55 ms GPU Time / ~2.5 ms Raster time

Backdrop id w/ shared input but not shared filter*:

2.0 ms GPU Time / ~2.5 ms Raster time

can be tested by setting the sigmas to be slightly different

So for the two small filter case, there really isn't any hit to CPU time. But computing the blur once is still faster than doing it twice, even over a larger area. These numbers will likely be diffrent across devices though.

flutter-dashboard · 2024-10-17T17:36:54Z

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

Changes reported for pull request #55701 at sha 95abc95

gaaclarke

LGTM!

gaaclarke · 2024-10-17T22:39:35Z

impeller/display_list/canvas.cc

+      // layer once.
+      if (backdrop_data->all_filters_equal &&
+          !backdrop_data->shared_filter_snapshot.has_value()) {
+        // TODO(jonahwilliams): compute minimum input hint.


This needs an issue.

ahh forgot to update it, I had filled flutter/flutter#157110

…ngine#55701)

[Impeller] add mechanism for sharing bdf inputs.

1dad72a

github-actions bot added the e: impeller label Oct 7, 2024

jonahwilliams mentioned this pull request Oct 7, 2024

[Impeller] Improve performance when rendering contiguous backdrop filter layers. flutter/flutter#131568

Open

Merge branch 'main' of github.com:flutter/engine into dirty_bdf

d1461b7

jonahwilliams mentioned this pull request Oct 9, 2024

When multiple blur effects are present on the screen, Impeller performs worse than Skia. flutter/flutter#156455

Open

1 task

jonahwilliams added 6 commits October 10, 2024 10:15

Cleanup.

14174b7

testing.

cc08f60

++

fb94ae6

++

63344a3

++

1c050fd

update op sizes.

34b7d3f

jonahwilliams marked this pull request as ready for review October 10, 2024 22:22

jonahwilliams added 2 commits October 10, 2024 19:34

remove copies, fix depth.

8bfe0e8

use sampler desc.

6bc5363

flar reviewed Oct 11, 2024

View reviewed changes

jonahwilliams added 5 commits October 11, 2024 15:13

use std::optional and make dart value nullable instead of using magic -1

5c71eeb

Merge branch 'main' of github.com:flutter/engine into dirty_bdf

60df9f7

++

90bbfbe

do dl rect convrsion.

68b5f6b

++

fdb55a0

gaaclarke reviewed Oct 14, 2024

View reviewed changes

aaron review.

7480309

gaaclarke reviewed Oct 14, 2024

View reviewed changes

impeller/display_list/dl_dispatcher.cc Outdated Show resolved Hide resolved

impeller/display_list/canvas.cc Outdated Show resolved Hide resolved

gaaclarke reviewed Oct 14, 2024

View reviewed changes

impeller/display_list/canvas.cc Outdated Show resolved Hide resolved

jonahwilliams added 2 commits October 14, 2024 16:03

more aaron review.

a423906

fixes camel case naming and some iterator issues.

f9b16af

web ui changes.

959efc8

github-actions bot added the platform-web Code specifically for the web engine label Oct 15, 2024

fix param order.

05b24db

gaaclarke reviewed Oct 15, 2024

View reviewed changes

++

11ab30b

flutter-dashboard bot added the will affect goldens label Oct 15, 2024

Merge branch 'main' into dirty_bdf

95abc95

jonahwilliams requested a review from gaaclarke October 17, 2024 20:37

gaaclarke approved these changes Oct 17, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

gaaclarke reviewed Oct 17, 2024

View reviewed changes

jonahwilliams added 2 commits October 17, 2024 15:42

Update canvas.cc

9d16bb5

Merge branch 'main' into dirty_bdf

d87a6c6

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

auto-submit bot merged commit ad9e4fe into flutter:main Oct 18, 2024
35 checks passed

jonahwilliams deleted the dirty_bdf branch October 18, 2024 15:38

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

Roll Flutter Engine from 76d310e42c06 to ad9e4fef451a (1 revision) flutter/flutter#157178

Open

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

ad9e4fef4 [Impeller] add mechanism for sharing bdf inputs. (flutter/e…

55b99e9

…ngine#55701)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] add mechanism for sharing bdf inputs. #55701

[Impeller] add mechanism for sharing bdf inputs. #55701

jonahwilliams commented Oct 7, 2024 •

edited by gaaclarke

Loading

jonahwilliams commented Oct 7, 2024

jonahwilliams commented Oct 7, 2024

jonahwilliams commented Oct 11, 2024

flar left a comment

flar Oct 11, 2024

flar Oct 11, 2024

jonahwilliams Oct 11, 2024

flar Oct 11, 2024

flar Oct 11, 2024

flar Oct 11, 2024

flar Oct 11, 2024

flar Oct 11, 2024

flar Oct 11, 2024

jonahwilliams commented Oct 11, 2024

gaaclarke commented Oct 14, 2024

jonahwilliams commented Oct 14, 2024

gaaclarke commented Oct 14, 2024

jonahwilliams commented Oct 14, 2024

gaaclarke left a comment

jonahwilliams commented Oct 14, 2024

gaaclarke Oct 15, 2024

jonahwilliams Oct 15, 2024

gaaclarke Oct 15, 2024

jonahwilliams Oct 15, 2024

flutter-dashboard bot commented Oct 15, 2024

jonahwilliams commented Oct 17, 2024

flutter-dashboard bot commented Oct 17, 2024

gaaclarke left a comment

This comment was marked as outdated.

gaaclarke Oct 17, 2024

jonahwilliams Oct 17, 2024

jonahwilliams Oct 17, 2024

[Impeller] add mechanism for sharing bdf inputs. #55701

[Impeller] add mechanism for sharing bdf inputs. #55701

Conversation

jonahwilliams commented Oct 7, 2024 • edited by gaaclarke Loading

Video

jonahwilliams commented Oct 7, 2024

jonahwilliams commented Oct 7, 2024

jonahwilliams commented Oct 11, 2024

flar left a 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

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 commented Oct 11, 2024

gaaclarke commented Oct 14, 2024

jonahwilliams commented Oct 14, 2024

gaaclarke commented Oct 14, 2024

jonahwilliams commented Oct 14, 2024

gaaclarke left a comment

Choose a reason for hiding this comment

jonahwilliams commented Oct 14, 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

flutter-dashboard bot commented Oct 15, 2024

jonahwilliams commented Oct 17, 2024

flutter-dashboard bot commented Oct 17, 2024

gaaclarke left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Oct 7, 2024 •

edited by gaaclarke

Loading