[Impeller] add mechanism for sharing bdf inputs.

[jonahwilliams]

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]

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]

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

[jonahwilliams]

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]

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

[flar]

(Or all of these virtual tags?)

[jonahwilliams]

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]

Ditto - default values probably unnecessary.

[flar]

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]

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

[flar]

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

[flar]

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

[flar]

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]

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.

[gaaclarke]

Suggested change

	helper.saveLayer(bounds, SaveLayerOptions::kWithAttributes, nullptr, -1);
	helper.saveLayer(bounds, SaveLayerOptions::kWithAttributes, nullptr, /*backdrop_id=*/-1);

[jonahwilliams]

Done

[gaaclarke]

Suggested change

	helper.saveLayer(bounds, SaveLayerOptions::kWithAttributes, nullptr, -1);
	helper.saveLayer(bounds, SaveLayerOptions::kWithAttributes, nullptr, /*backdrop_id=*/-1);

[jonahwilliams]

Done

[gaaclarke]

Instead of making this a separate argument, does it make more sense to have this be a property of the backdrop itself?

[jonahwilliams]

The backdrop filter argument is a DlImageFilter, which is a mirror of the dart:ui ImageFilter object. It doesn't make sense for the backdrop id to be a property of the image filter as 1) a single filter object can be used in multiple places in the same scene and 2) can be used in contexts for which the backdropId propety doesn't make sense like image filters.

I think instead it would make more sense to refactor saveLayer to group both the backdrop image filter and the backdrop id into one BackdropData struct.

[gaaclarke]

Suggested change

	true //
	/*reset_host_buffer=*/true //

[jonahwilliams]

Done

[gaaclarke]

Add docstring.

[jonahwilliams]

Done

[gaaclarke]

Suggested change

	const auto& backdrop_data = backdrop_keys_.find(backdrop_id.value());
	const auto& backdrop_data_it = backdrop_keys_.find(backdrop_id.value());

[jonahwilliams]

Done

[gaaclarke]

Data is potentially nullptr here no? It's hard to read this code in github with comments.

[jonahwilliams]

will_cache_texture is only true if backdrop_data wasn't nullptr, so we'd only take the first part of the OR branch here.

[gaaclarke]

will_cache_texture is so generic, but it specifically works with BackdropData, can't it be named more appropriate for what it's used for, backdrop filters?

[jonahwilliams]

will_cache_backdrop_texture?

[gaaclarke]

Remove auto please.

[jonahwilliams]

done

[gaaclarke]

Holy moley, maybe we can break this chain up. It's quite a bit.

[jonahwilliams]

Yeah its bad

[jonahwilliams]

Added a helper

[gaaclarke]

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]

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

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

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]

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.