[Impeller] Defer applying opacity when saving layer

[ColdPaleLight]

fix issue: flutter/flutter#112175

1. Snapshot adds an opacity parameter
2. ImageFilter (Include ImageFilterColorFilter) will pass opacity, and ColorFilter will absorb opacity.
3. The opacity needs to be applied lazily only in save layer scenarios.

Test: SaveLayerWithColorMatrixFiltersAndAlphaDrawCorrectly

20220927-181145.mp4

Test: SaveLayerWithBlendFiltersAndAlphaDrawCorrectly

20220927-181246.mp4

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

[flutter-dashboard]

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

[bdero]

Is opacity not currently working correctly for some draw operations? I'm not sure if filters themselves should be dealing with info from the paint state.

[ColdPaleLight]

Is opacity not currently working correctly for some draw operations? I'm not sure if filters themselves should be dealing with info from the paint state.

In the save layer scenario, the opacity needs to affect the output of the image filter.

For example, the following color matrix is ​​used as the color filter image filter of paint, and then the alpha value of paint is set to 0.5, and the final result should be translucent green.

0,0,0,0,0
0,0,0,0,1
0,0,0,0,0
0,0,0,0,1

And the same color matrix if used as a color filter instead of an image filter, the result will be an opaque green

c.f.

engine/impeller/aiks/paint_pass_delegate.cc

Line 43 in 0a6360d

return paint_.WithFilters(std::move(contents), false, effect_transform);

[bdero]

I haven't looked at this in detail yet so I'm not sure if it's the same issue, but FYI: I came across an alpha bug earlier today that got introduced by a pass elimination optimization I landed a few weeks ago: #36337

The bug causes paint alpha to get dropped from save layers pretty much any time a filter is applied.

[ColdPaleLight]

I haven't looked at this in detail yet so I'm not sure if it's the same issue, but FYI: I came across an alpha bug earlier today that got introduced by a pass elimination optimization I landed a few weeks ago: #36337

The bug causes paint alpha to get dropped from save layers pretty much any time a filter is applied.

I've also noticed the issue you mentioned recently, and it's nice to see it fixed. However, it is not the same issue that this PR wants to fix.

Here is an example, the same color matrix, when used as an image filter, it will be affected by alpha, and when used as a color filter, it will not be affected by alpha.

https://fiddle.skia.org/c/80d76540ad488d31a086022ac06a9f3b

In order to fix this issue, I propose to add a color matrix related to the alpha value between the image filter and the color filter. The code is as follows (Of course, we also need to delete the line of code that sets the opacity of TextureContents in PaintPassDelegate::CreateContentsForSubpassTarget). Do you have any ideas?

std::shared_ptr<Contents> Paint::WithFilters(std::shared_ptr<Contents> input,
                                             std::optional<bool> is_solid_color,
                                             const Matrix& effect_transform,
                                             bool with_opacity) const {
  bool is_solid_color_val = is_solid_color.value_or(!color_source);
  if (mask_blur_descriptor.has_value()) {
    input = mask_blur_descriptor->CreateMaskBlur(
        FilterInput::Make(input), is_solid_color_val, effect_transform);
  }

  if (image_filter.has_value()) {
    const ImageFilterProc& filter = image_filter.value();
    input = filter(FilterInput::Make(input), effect_transform);
  }

+  if (with_opacity && color.alpha < 1) {
+    FilterContents::ColorMatrix matrix = {
+       1, 0, 0, 0, 0,  //
+       0, 1, 0, 0, 0,  //
+        0, 0, 1, 0, 0,  //
+        0, 0, 0, color.alpha, 0,  //
+    };
+    input = ColorMatrixFilterContents::MakeColorMatrix(FilterInput::Make(input), matrix);
+ }

  if (color_filter.has_value()) {
    const ColorFilterProc& filter = color_filter.value();
    input = filter(FilterInput::Make(input));
  }

  return input;
}

[ColdPaleLight]

@bdero @chinmaygarde I updated the implementation and added the issue and video. Please tell me if you guys have any ideas. thanks!

[bdero]

Thanks for the explanation! This behavior of applying the save layer alpha after the image filter seems quite unintuitive and I'm wondering if this is one of those rare behavioral quirks we should document as a difference and intentionally not support.

It would also be worth checking if this issue only applies to layers. Does it behave the same when drawing images in general? What about when drawing paths -- does the opacity of the path get applied after the image filter but before the color filter as well?

If we need to defer resolving the opacity of source images, there are less expensive tricks we can try that don't involve appending new filters. For example, tracking the opacity in a new Snapshot field seems feasible depending on the consistency of this behavior in Skia. The color matrix filter would get a new flag that can be optionally set to absorb and apply the opacity as part of the operation -- this flag can get flipped on in the dispatcher whenever a color matrix filter is set as the color filter. If the opacity is not absorbed by a filter (the common case), it'll just get handled by the final TextureContents blit.

[ColdPaleLight]

Thanks for the explanation! This behavior of applying the save layer alpha after the image filter seems quite unintuitive and I'm wondering if this is one of those rare behavioral quirks we should document as a difference and intentionally not support.

I think we need to implement this feature, and there are such scenarios in the real world. For example, when an OpacityLayer wraps an ImageFilterLayer, OpacityLayer may defer applying opacity in ImageFilterLayer.
c.f.
https://github.com/flutter/engine/blob/main/flow/layers/image_filter_layer.cc#L120-L121

It would also be worth checking if this issue only applies to layers. Does it behave the same when drawing images in general? What about when drawing paths -- does the opacity of the path get applied after the image filter but before the color filter as well?

This issue is only appear when saving layers. Usually, opacity will not be delayed to apply.

But I found another issue, in the usual case, it seems to apply the color filter first, and then apply the image filter.
See this example for details
https://fiddle.skia.org/c/5e62809886613c144f50fa99f1954850

If we need to defer resolving the opacity of source images, there are less expensive tricks we can try that don't involve appending new filters. For example, tracking the opacity in a new Snapshot field seems feasible depending on the consistency of this behavior in Skia. The color matrix filter would get a new flag that can be optionally set to absorb and apply the opacity as part of the operation -- this flag can get flipped on in the dispatcher whenever a color matrix filter is set as the color filter. If the opacity is not absorbed by a filter (the common case), it'll just get handled by the final TextureContents blit.

Good idea, thanks! I'll try that.

[ColdPaleLight]

@bdero I refactored this patch as you suggested, please take a look when you have a chance. thanks!

[zanderso]

From PR triage: @bdero It looks like this PR is ready for another look.

cc @flar

[bdero]

I think we need to implement this feature, and there are such scenarios in the real world. For example, when an OpacityLayer wraps an ImageFilterLayer, OpacityLayer may defer applying opacity in ImageFilterLayer.
c.f.
https://github.com/flutter/engine/blob/main/flow/layers/image_filter_layer.cc#L120-L121

What are the implications of this? Would this cause behavioral inconsistencies that are difficult to reconcile in the framework, or perhaps cause issues with existing optimizations in the layer tree? In general, it's OK for Impeller usage to cause real apps to behave a bit differently, especially in corner cases if they're easy to work around.

I guess I'm trying to gauge the severity of this fidelity issue -- this Skia behavior feels somewhat like a consistency bug or a circumstantial oversight; would we be doing everyone a favor in the long run if we didn't replicate it in Impeller?

[bdero]

Since this is only a thing for color filters and we resolve this in the dispatcher anyhow, can we just add this setter to each of the filters that need it rather than all FilterContents?

[ColdPaleLight]

Done.

[bdero]

Here and everywhere else, maybe just absorbs_opacity?

[ColdPaleLight]

Done.

[ColdPaleLight]

I think we need to implement this feature, and there are such scenarios in the real world. For example, when an OpacityLayer wraps an ImageFilterLayer, OpacityLayer may defer applying opacity in ImageFilterLayer.
c.f.
https://github.com/flutter/engine/blob/main/flow/layers/image_filter_layer.cc#L120-L121

What are the implications of this? Would this cause behavioral inconsistencies that are difficult to reconcile in the framework, or perhaps cause issues with existing optimizations in the layer tree? In general, it's OK for Impeller usage to cause real apps to behave a bit differently, especially in corner cases if they're easy to work around.

I guess I'm trying to gauge the severity of this fidelity issue -- this Skia behavior feels somewhat like a consistency bug or a circumstantial oversight; would we be doing everyone a favor in the long run if we didn't replicate it in Impeller?

I see. I will share an example to illustrate the importance of the order in which these attributes (alpha, color_filter, image_filter) are applied in the save layer scenario.

Suppose user's widget is the following structure, an Opacity wraps an ImageFiltered

Opacity(
  opacity: 0.5,
  child: ImageFiltered(
    imageFilter: ui.ImageFilter.dilate(radiusX: 3, radiusY: 3),
    child: Image.network('http://xxxx'),
  ),
)

Then the layer structure of its corresponding will be like this

- OpacityLayer
  - ImageFliterLayer
    - DisplayListLayer

Will eventually generate code similar to this (note that the following code will be slightly different from the actual code corresponding to the widget above, but this does not affect our discussion).

    DisplayListBuilder builder;
    DlPaint opacity_paint;
    opacity_paint.setAlpha(alpha);
    builder.saveLayer(&opacity_save_bounds, &opacity_paint);
    DlPaint image_filter_paint;
    image_filter_paint.setImageFilter(image_filter);
    builder.saveLayer(&filter_save_bounds, &image_filter_paint);
    builder.drawImageRect(...)
    builder.restore();
    builder.restore();

Here we see that there are two saveLayer. And an optimization mechanism (opacity peephole) and a new optimization mechanism (#36458) that is being implemented now will merge them.

    DisplayListBuilder builder;
    DlPaint paint;
    paint.setAlpha(alpha);
    paint.setImageFilter(image_filter);
    builder.saveLayer(&save_bounds, &paint);
    builder.drawImageRect(...);
    builder.restore();

And this merging mechanism depends on the order in which these attributes are applied, that is, image_filter -> alipa -> color_filter. So I suggest that the Impeller needs to be aligned with Skia here, otherwise this optimization mechanism will cause the display result to be wrong on the Impeller.

[bdero]

Ah, that's interesting... thanks for the detailed explanations. I understand now why we'd end up relying on this behavior in optimizations that flatten out savelayers.

I have another idea to help make this behavior a little more explicit/maintainable by pulling it out of the dispatcher while simultaneously preparing us to solve the other ordering problem you uncovered while investigating (it sounds like in the non-savelayer case, the order is Alpha -> Color Filter -> Image Filter, but in the savelayer case, it's Image Filter -> Alpha -> Color Filter) -- let me know what you think!

[bdero]

Since we'll have to diverge with the ordering of WithFilters at some point anyhow, maybe we should just do the WithFilters logic inline here and build the filter chain exactly as it needs to be. Then, we can set the alpha absorption flag on the color filter here where it's needed instead of having to do it in the dispatcher or color filter procs.

To facilitate this, we could also add a ColorFilterContents class that extends FilterContents which is inherited by all of the color filters -- this would hold the setter and remove the repeated code without having to place it on filters that will never use it.

[ColdPaleLight]

Good idea! I'll make the changes as you suggested after the holidays (aka next week).

[ColdPaleLight]

Done. I adjusted the PR according to your suggestion, the one difference is that I added a method Paint::WithFiltersForSubpassTarget, which can reuse some logic. Let me know what you think!

[flar]

Just a note from discussions with Skia.

Their "natural order" appears to be IF then alpha then CF (consistent with a CF layer wrapping an Opacity layer wrapping an IF layer).

But, CF can be expressed as an IF via the DlColorFilterImageFilter and its associated Skia equivalent.
And, technically opacity/alpha can be expressed as a matrix CF.

So, we could technically take all of these in any order by expressing all as CF then IF and then the order is defined by the compose-ing sequence.

The only issue is that, if we re-express all of the filters in that way, they become pretty difficult to optimize into some other sequence of fewer operations. Or, maybe not since the optimizer could be taught to understand what an alpha-matrix looks like or a CFIF really being just a CF.

A common example of optimizing these is whether or not an alpha/opacity is applied as a simple multiply operation or if it involves a matrix operation on the color vector?

Then we get into multiple matrix CF could be combined into a single matrix CF, etc.

But, if we want to have compatibility with the Skia behavior, then IF first, then opacity, then CF last is the order we have to mimic.

[bdero]

Their "natural order" appears to be IF then alpha then CF (consistent with a CF layer wrapping an Opacity layer wrapping an IF layer).

Do you happen to know the criteria for the different orderings? We found that IF->alpha->CF is indeed the ordering used for drawing savelayers, but for solid color draws it seems to be alpha->CF->IF. Enumerating all of the cases is on my todo.

All good points WRT optimizing.

I've been thinking a good first pass might be to have an AsColorMatrixFilter virtual on FilterContents that everything optionally implements (not unlike Skia) -- alternatively we could use DL helpers for this conversion. Then, just before the final filter chain is built:

1. We walk the chain, combining all contiguous filters that can be represented as color matrices.
2. Then, if the first filter is a color matrix and the ColorSource is able to cheaply apply it, we pluck it out of the chain and apply it to the input colors on the CPU. The protocol for ColorSourceContents would provide a way to query for this support and apply a color matrix (FYI @jonahwilliams).

Then perhaps we can scoop up other easy optimizations on a case-by-case basis. For example, if the CF just results in an opacity change, the opacity can be deferred via the snapshot to the final texture draw.

[bdero]

LGTM, thanks!