[Impeller] Replace Impeller opacity peephole delegate with DL variant. #52707

jonahwilliams · 2024-05-10T01:58:04Z

Use the existing DL opacity peephole flags and remove the Impeller variant. Also wires up for experimental canvas.

Work towards flutter/flutter#142054

jonahwilliams · 2024-05-10T01:59:08Z

impeller/aiks/canvas.cc

+    transform_stack_.back().distributed_opacity *= paint.color.alpha;
+    return;
+  } else {
+    FML_LOG(ERROR) << "no dist opacity";


@flar try as I might, I can't seem to get this to fire on any of the opacity peephole tests in the macrobenchmarks app. It seems like can_distribute_opacity is always false with impeller?

This is just running with the regular canvas.

Can't get which to fire? This is the "false" part of the if. Does this mean it is always true?

I mean its never true

It's working for the dl_sk_dispatcher, though...?

(or we would have had benchmark regressions).

but its not working here :)

Is there something different in the impeller DL setup that we're missing for dl opacity peephole? or some other requirement I'm missing?

I was finally able to get this running locally and it worked for me with Grid with alpha SaveLayer on Rows

If you tell me which sub-test types you ran I can look and see why they don't do peephole optimizations. Keep in mind that I created buttons for scenarios that wouldn't work so that we had something to aim for in the future, so not every one of those options will result in opacity peephole success...

…nvas

jonahwilliams · 2024-05-14T23:19:13Z

Can confirm this is working, sorry for the noise!

jonahwilliams · 2024-05-15T15:48:53Z

impeller/aiks/canvas.cc

  TRACE_EVENT0("flutter", "Canvas::saveLayer");
+  if (can_distribute_opacity) {
+    FML_DCHECK(!backdrop_filter);


@flar I'm surprised but I hit this check in some tests. Do I need to assume that even if can_distribute_opacity is true, there may be other paint states (blend mode/ filters) that can still require a save layer?

Which tests? I can take a look and see if there might be something wrong with the DL checks. In particular, I think I only pass SrcOver for now even though other blend modes might cause problems. I disallow ColorFilters, but I allow ImageFilters because according to my investigations the opacity should be applied to the output of the ImageFilter so it shouldn't matter. (It's applied to the input of the ColorFilter, though, so I just disallow them even though I could maybe look at their details and find that some of them are compatible).

Ah, are you specifically referring to backdrop filter vs opacity flag? I notice that I don't turn off the flag in that case, but perhaps I should. The flag is more about the contents and whether the contents are compatible.

I also don't think I check the attributes that apply the SL to the parent. If they aren't SrcOver then I don't think we can do the opacity stuff.

I also don't believe the "old code" (pre-reorg) does this either. Let me get the reorg back in and then I can revisit layer properties and how they affect the flag.

Filed flutter/flutter#148505

flar · 2024-05-15T19:34:25Z

impeller/aiks/aiks_unittests.cc

-                                             Entity::TileMode::kClamp);
-
-  // Paint has image filter, can't elide.
-  delegate = std::make_shared<OpacityPeepholePassDelegate>(paint);


This might be the case where I differ in assumptions. DL allows IF+opacity, but it looks like AIKS expects it to be disallowed...?

We always fold the opacity into the image filter compositing, so in effect its as if the peephole wasn't applied.

flar · 2024-05-15T19:35:01Z

impeller/aiks/aiks_unittests.cc

-  paint.color = Color::Red();
-
-  // Paint has no alpha, can't elide;
-  delegate = std::make_shared<OpacityPeepholePassDelegate>(paint);


Huh? Why can't opaque colors be peepholed?

Just old overly defensive code.

flar · 2024-05-15T19:38:34Z

impeller/aiks/paint_pass_delegate.cc

-
-  // OpacityPeepholePassDelegate will only get used if the pass's blend mode is
-  // SourceOver, so no need to check here.
-  if (paint_.color.alpha <= 0.0 || paint_.color.alpha >= 1.0 ||


If the alpha is 0 then we should skip the pass entirely, so I'm not sure why it is saying that it "can't collapse into parent" here (i.e. return false).

Also, if the paint is opaque, then that isn't a reason not to collapse the pass...? It just means you have no partial opacity to distribute, but you can still collapse it and distribute the opacity of 1.0 (i.e. NOP on modifying the opacity).

I think I was just being overly conservative at the time.

chinmaygarde · 2024-05-23T20:06:44Z

Are we making progress on this?

jonahwilliams · 2024-05-26T18:32:53Z

Need to get back to it and address feedback, but was waiting on the DL reorg patch to reland.

flar · 2024-05-28T19:29:06Z

The reorg has rolled all the way now, so work depending on it should be good to go...

…nvas

jonahwilliams · 2024-05-29T00:21:29Z

@flar

The DL opacity peephole is still missing cases that the impeller version got in the wonderous app. On the main carosel view, both the clouds (while in motion only) and the swipe up indicator are impacted by impeller but not display list:

flar · 2024-05-29T02:38:26Z

It looks like entities allow kSrc with opaque which DL doesn't allow. I'll look into supporting that, but I'll have to figure out how to emulate it in the Skia Dispatcher.

engine/impeller/entity/entity.cc

Line 127 in 21f1d14

if (!((blend_mode_ == BlendMode::kSource && contents_->IsOpaque()) ||

jonahwilliams · 2024-05-29T02:43:38Z

I don't think there are any src blend mode entities in this app, this check is supposed to help resolve the interactions between srcOver -> src promotion and opacity peephole

…nvas

impeller/display_list/dl_dispatcher.cc

flutter-dashboard · 2024-05-31T04:02:47Z

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 #52707 at sha 49c7376

flar

I think the inherited opacity doesn't transfer to a child SaveLayer's blit back, but maybe I'm misreading the changes...?

flar · 2024-05-31T04:48:53Z

impeller/aiks/canvas.cc

-  } else {
-    new_layer_pass.SetDelegate(std::make_shared<PaintPassDelegate>(paint));
-  }
+  new_layer_pass.SetDelegate(std::make_shared<PaintPassDelegate>(paint));


Does this transfer the existing distributed opacity from the transform_stack.back() to the new layer pass?

Basically, if this saveLayer didn't pass along the distributed opacity above, then it needs to apply it to the blit-back to the parent and I don't see where that is happening. Should PaintPassDelegate carry along an extra parameter for the distributed opacity from outside the saveLayer?

Good catch, this is fixed and a test case is added.

flar · 2024-05-31T04:54:28Z

impeller/display_list/aiks_dl_opacity_unittests.cc

+  alpha.setColor(DlColor::kRed().modulateOpacity(0.5));
+
+  builder.SaveLayer(nullptr, &alpha);
+  builder.DrawRect(SkRect::MakeXYWH(000, 000, 100, 100), red);


Why are these XY numbers written as octal literals?

Also, don't they overlap? That should disable the group opacity...?

I... don't know. Call it a brain fart. Fixed.

flar · 2024-05-31T20:32:42Z

impeller/display_list/aiks_dl_opacity_unittests.cc

+  alpha.setColor(DlColor::kRed().modulateOpacity(0.5));
+
+  builder.SaveLayer(nullptr, &alpha);
+  builder.DrawRect(SkRect::MakeXYWH(020, 020, 100, 100), green);


More octal numbers. Not an issue in this case, but I guess this is someone's way of lining up rows of numbers without the formatter complaining? Clever as long as you are aware that you are mising octal and decimal...

agh let me fix this too

flar

LGTM

flutter-dashboard · 2024-05-31T21:38:29Z

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

Changes reported for pull request #52707 at sha 1f90efb

…DL variant. (flutter/engine#52707)

…149462) flutter/engine@990f1ce...40b868e 2024-05-31 jonahwilliams@google.com [Impeller] make strokes < 0.5 physical pixels visually thinner. (flutter/engine#53154) 2024-05-31 jonahwilliams@google.com [Impeller] Replace Impeller opacity peephole delegate with DL variant. (flutter/engine#52707) 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 jacksongardner@google.com,rmistry@google.com,zra@google.com 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] test DL opacity peephole.

9bdf84b

github-actions bot added the e: impeller label May 10, 2024

jonahwilliams commented May 10, 2024

View reviewed changes

jonahwilliams added 2 commits May 14, 2024 13:27

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

6deb635

…nvas

Delete impeller opacity peephole.

3d5739a

++

326af19

jonahwilliams changed the title ~~[Impeller] test DL opacity peephole.~~ [Impeller] Replace Impeller opacity peephole delegate with DL variant. May 15, 2024

jonahwilliams marked this pull request as ready for review May 15, 2024 00:29

add test and port existing opacity test.

d66b1a3

jonahwilliams commented May 15, 2024

View reviewed changes

flar reviewed May 15, 2024

View reviewed changes

jonahwilliams added 2 commits May 28, 2024 15:17

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

03f4ffb

…nvas

updates to check paint state.

0d36fe0

jonahwilliams added 2 commits May 30, 2024 16:36

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

6a1a416

…nvas

fix missing default value.

55ad6ee

jonahwilliams commented May 31, 2024

View reviewed changes

impeller/display_list/dl_dispatcher.cc Show resolved Hide resolved

license

74f8f84

jonahwilliams requested a review from flar May 31, 2024 02:03

update goldens.

49c7376

flutter-dashboard bot added the will affect goldens label May 31, 2024

flar requested changes May 31, 2024

View reviewed changes

fix distribution to save layers and add unit tests.

302e125

jonahwilliams requested a review from flar May 31, 2024 16:55

jonahwilliams added 2 commits May 31, 2024 09:56

update impeller goldens.

06ca207

Update impeller_golden_tests_output.txt

88cd532

flar reviewed May 31, 2024

View reviewed changes

flar approved these changes May 31, 2024

View reviewed changes

Update aiks_dl_opacity_unittests.cc

1f90efb

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

auto-submit bot merged commit 21a8158 into flutter:main May 31, 2024
31 checks passed

engine-flutter-autoroll mentioned this pull request May 31, 2024

Roll Flutter Engine from 990f1ceac5d5 to 40b868efcc46 (2 revisions) flutter/flutter#149462

Merged

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

21a8158ca [Impeller] Replace Impeller opacity peephole delegate with …

172911b

…DL variant. (flutter/engine#52707)

[Impeller] Replace Impeller opacity peephole delegate with DL variant. #52707

[Impeller] Replace Impeller opacity peephole delegate with DL variant. #52707

Conversation

jonahwilliams commented May 10, 2024 • edited

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

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

chinmaygarde commented May 23, 2024

jonahwilliams commented May 26, 2024

flar commented May 28, 2024

jonahwilliams commented May 29, 2024

flar commented May 29, 2024

jonahwilliams commented May 29, 2024

flutter-dashboard bot commented May 31, 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

flar left a comment

Choose a reason for hiding this comment

flutter-dashboard bot commented May 31, 2024

jonahwilliams commented May 10, 2024 •

edited