[Impeller] flood input coverage with destructive color filter. #55758

jonahwilliams · 2024-10-09T00:31:50Z

Fixes flutter/flutter#126389
Fixes flutter/flutter#137090
Fixes flutter/flutter#154035

If this bit is set, the output of a color filter should flood to the input of a filter to parent clip. We're not checking this right now and so all color filter content is treated as bounded.

This needs to flood the input as the image filter (which occurs afterwards) should process the flooded input.

flutter-dashboard · 2024-10-10T16:18:54Z

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 #55758 at sha fc71765

bdero

LGTM!

bdero · 2024-10-16T18:02:40Z

Hmm, actually I think something is going awry with TranslucentSaveLayerWithColorAndImageFilterDrawsCorrectly.

The red source color on the SaveLayer paint's color filter is leaking through even through the blend mode is set to kDstOver.

jonahwilliams · 2024-10-16T19:20:51Z

https://github.com/flutter/engine/blob/main/display_list/effects/dl_color_filter.cc#L55 The Dl ColorFilter marks dstOver with a non-transparent color as modifying transparent black

FYI @flar

flar · 2024-10-16T20:38:25Z

https://github.com/flutter/engine/blob/main/display_list/effects/dl_color_filter.cc#L55 The Dl ColorFilter marks dstOver with a non-transparent color as modifying transparent black

FYI @flar

Isn't that correct?

Dst is transparent, color is not. The result with dstOver would be the color which means it modifies transparent black...?

See

engine/display_list/testing/dl_rendering_unittests.cc

Line 4297 in 066a3c7

TEST_F(DisplayListRendering, BlendColorFilterModifyTransparencyCheck) {

bdero · 2024-10-16T21:11:41Z

Oh, this is actually correct behavior. @flar is right. I confused kDst and kDstOver. Because most of the destination is transparent, the red source color should be leaking through everywhere except for the partially translucent black rectangle.

So this LGTM.

flar · 2024-10-16T21:26:05Z

Oh, this is actually correct behavior. @flar is right. I confused kDst and kDstOver. Because most of the destination is transparent, the red source color should be leaking through everywhere except for the partially translucent black rectangle.

So this LGTM.

But we should see if the sudden change in behavior is masking what the test was originally trying to test. In particular the test mentioned above is missing the blue rectangle now. Was the blue rectangle important? If so, then let's adjust the test to match its own purpose...

bdero · 2024-10-16T21:50:45Z

For TranslucentSaveLayerWithBlendColorFilterDrawsCorrectly, TranslucentSaveLayerWithColorAndImageFilterDrawsCorrectly I think the value that the blue rectangle adds is a visual comparison with a filter-less SaveLayer, so I think just swapping the order around to make the blue rectangle appear over the flooded red area would keep that value.
ForegroundBlendSubpassCollapseOptimization looks good as is. The before image just looks incorrect.
For BlendModePlusAlphaColorFilterWideGamut, the dark green background is being blended with the SaveLayer drawn over it with kPlus. I don't think exposing the dark green background adds any value, so it seems fine as-is.

But if we want to prevent any or all of these goldens from changing, it can be done by passing bounds rects to the SaveLayers of course.

jonahwilliams · 2024-10-16T22:06:23Z

I updated TranslucentSaveLayerWithBlendColorFilterDrawsCorrectly to clip so it matches the old golden, for the rest I'll update.

bdero · 2024-10-16T22:46:34Z

impeller/display_list/aiks_dl_unittests.cc

  paint.setColorFilter(
      DlBlendColorFilter::Make(DlColor::kRed(), DlBlendMode::kDstOver));
+  builder.Save();
+  builder.ClipRect(SkRect::MakeXYWH(100, 500, 300, 300));


Should we fix TranslucentSaveLayerWithColorAndImageFilterDrawsCorrectly too?

The test name has "Translucent SaveLayer" in it which makes me think that the expanded flood is part of the behavior it is wanting to demonstrate. By clipping so tightly there is no indication of the flooding when such a combination happens. Perhaps expand the clip by 10 pixels or so, which will both show the flooding without completely obscuring the blue rectangle?

flutter-dashboard · 2024-10-16T22:54:11Z

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

Changes reported for pull request #55758 at sha b9d93a4

flar · 2024-10-17T01:01:54Z

For TranslucentSaveLayerWithBlendColorFilterDrawsCorrectly, TranslucentSaveLayerWithColorAndImageFilterDrawsCorrectly I think the value that the blue rectangle adds is a visual comparison with a filter-less SaveLayer, so I think just swapping the order around to make the blue rectangle appear over the flooded red area would keep that value.

ForegroundBlendSubpassCollapseOptimization looks good as is. The before image just looks incorrect.

For BlendModePlusAlphaColorFilterWideGamut, the dark green background is being blended with the SaveLayer drawn over it with kPlus. I don't think exposing the dark green background adds any value, so it seems fine as-is.

But if we want to prevent any or all of these goldens from changing, it can be done by passing bounds rects to the SaveLayers of course.

I was going to say "add a clip" which it looks like Jonah did. If no changes to the goldens are the biggest concern then clipping to the SL region would do that. If you want to also have this test show that it overflows (the new behavior) then the clip could be slightly bigger so that it both shows all the old content and also shows that the overflow is happening without obscuring the other part of the test.

jonahwilliams · 2024-10-17T01:20:44Z

I did add some new test goldens that show the flooding, that is DestructiveBlendColorFilterFloodsClip

…er. (flutter/engine#55758)

…157077) flutter/engine@c69ed59...7d1abd9 2024-10-17 [email protected] Roll Fuchsia Linux SDK from OTfEfbaoT9c0HcprI... to 9F_NaKPd2twhbPwP7... (flutter/engine#55926) 2024-10-17 [email protected] [Impeller] flood input coverage with destructive color filter. (flutter/engine#55758) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from OTfEfbaoT9c0 to 9F_NaKPd2twh 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] 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

…lutter#157077) flutter/engine@c69ed59...7d1abd9 2024-10-17 [email protected] Roll Fuchsia Linux SDK from OTfEfbaoT9c0HcprI... to 9F_NaKPd2twhbPwP7... (flutter/engine#55926) 2024-10-17 [email protected] [Impeller] flood input coverage with destructive color filter. (flutter/engine#55758) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from OTfEfbaoT9c0 to 9F_NaKPd2twh 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] 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

…er/engine#55758) Fixes flutter#126389 Fixes flutter#137090 Fixes flutter#154035 If this bit is set, the output of a color filter should flood to the input of a filter to parent clip. We're not checking this right now and so all color filter content is treated as bounded. This needs to flood the input as the image filter (which occurs afterwards) should process the flooded input.

[Impeller] flood input coverage with destructive color filter.

f393625

jonahwilliams requested review from flar and gaaclarke October 9, 2024 00:31

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

update goldens.

fc71765

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

bdero approved these changes Oct 16, 2024

View reviewed changes

++

b9d93a4

bdero reviewed Oct 16, 2024

View reviewed changes

one more.

534500b

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

auto-submit bot merged commit 8344836 into flutter:main Oct 17, 2024

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

Roll Flutter Engine from c69ed59b01a5 to 83448365b912 (1 revision) flutter/flutter#157075

Closed

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

83448365b [Impeller] flood input coverage with destructive color filt…

e08aaa0

…er. (flutter/engine#55758)

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

Roll Flutter Engine from c69ed59b01a5 to 7d1abd99e0c0 (2 revisions) flutter/flutter#157077

Merged

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

83448365b [Impeller] flood input coverage with destructive color filt…

4e2acc8

…er. (flutter/engine#55758)

[Impeller] flood input coverage with destructive color filter. #55758

[Impeller] flood input coverage with destructive color filter. #55758

Uh oh!

Conversation

jonahwilliams commented Oct 9, 2024

Uh oh!

flutter-dashboard bot commented Oct 10, 2024

Uh oh!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

bdero commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Oct 16, 2024

Uh oh!

flar commented Oct 16, 2024

Uh oh!

bdero commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flar commented Oct 16, 2024

Uh oh!

bdero commented Oct 16, 2024

Uh oh!

jonahwilliams commented Oct 16, 2024

Uh oh!

bdero Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

flar Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Oct 16, 2024

Uh oh!

flar commented Oct 17, 2024

Uh oh!

jonahwilliams commented Oct 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bdero commented Oct 16, 2024 •

edited

Loading

bdero commented Oct 16, 2024 •

edited

Loading

bdero Oct 16, 2024 •

edited

Loading