DisplayList savelayer opacity peephole optimization #30957

flar · 2022-01-20T21:50:14Z

These changes introduce the ability for saveLayer calls within a DisplayList to implement opacity peephole optimization under very specific conditions. In particular, only a single encapsulated rendering primitive is allowed.

This mechanism will eventually be able to handle multiple overlapping primitives if the bounds calculations are factored into the determination. Currently overlap detection is difficult to do as the bounds are calculated inside a Dispatcher object which only sees the data stream from a DisplayList and cannot access its records to modify their state. In order for the bounds calculator to update the DisplayList to provide information about non-overlapping children it will either need to be incorporated directly into the Builder, or will have to use a new iteration process that allows Dispatcher implementations to update the records they are being dispatched from.

flar · 2022-01-25T20:52:26Z

This is finally good to go for review @gw280 @chinmaygarde

flar · 2022-01-27T00:08:07Z

Here are the results from the benchmarks I'm adding to the framework repo:

flutter/flutter#97336 (comment)

gw280 · 2022-02-03T23:33:11Z

display_list/display_list_builder.h

          cannot_inherit_opacity(false),
          has_compatible_op(false) {}

+    size_t save_layer_offset;


can you add a comment here about what this is? it's not obvious from reading the code.

Big comment added...

gw280 · 2022-02-03T23:39:45Z

display_list/display_list_unittests.cc

+  builder.restore();
+  builder.Build()->Dispatch(expector);
+  EXPECT_EQ(expector.save_layer_count(), 1);
+}


I don't see any tests with nested saveLayer calls. We should probably add one that exercises this optimisation, and one that doesn't.

I added a couple. Can(Cannot(..., Can(....))) and Can(Can(...))

gw280 · 2022-02-03T23:42:00Z

LGTM for the most part. One minor documentation addition needed, and some extra tests for nested saveLayers would be nice.

gw280 · 2022-02-03T23:44:55Z

display_list/display_list_builder.cc

+      if (layer_info.is_group_opacity_compatible()) {
+        SaveLayerOp* op = reinterpret_cast<SaveLayerOp*>(
+            storage_.get() + layer_info.save_layer_offset);
+        op->options = op->options.with_can_distribute_opacity();


This is a little terrifying to me. Maybe we should add a warning here that we should make sure we only ever modify data in-place? ie - that nothing we do here increases the size of *op.

And that this type of modification can only be done during building. Once done with building the contents of the records must remain constant so we can trust its output to never change and Equals() to continue working.

Big block comment added...

…er opacity opt

…engine#30957)

zanderso · 2022-02-04T21:55:59Z

SkiaPerf results https://flutter-flutter-perf.skia.org/e/?begin=1643063115&end=1644004520&keys=X32827d8819e8271e025f50e77bf2bec0&requestType=0&xbaroffset=27447

spauldhaliwal · 2022-07-15T17:01:29Z

This merge was referenced in this Flutter 3.0 release post, however, it's not clear from reading the above when / how developers can expect improvements to opacity animations.

It's stated above that the performance optimization should kick in when opacity animations contain a "single rendering primitive". What does this mean? What's a single rendering primitive? Is there a way for developers to test the above improvement and benchmark before and after results?

Any insight into this new improvement would be appreciated. Thanks!

flar · 2022-07-15T17:35:44Z

This merge was referenced in this Flutter 3.0 release post, however, it's not clear from reading the above when / how developers can expect improvements to opacity animations.

It's stated above that the performance optimization should kick in when opacity animations contain a "single rendering primitive". What does this mean? What's a single rendering primitive? Is there a way for developers to test the above improvement and benchmark before and after results?

Any insight into this new improvement would be appreciated. Thanks!

A single rendering primitive is a method call on Canvas. This was not a very customer-facing version of this issue and only describes the behavior that you might observe if you are making your own calls to Canvas, but most developers just use widgets and the interactions with a Canvas object are managed by those widgets and their RenderObjects.

There is another issue for a related mechanism that speaks in terms that are more related to what developers do with their scenes which is #29775 which established the same optimization for engine layers and the opacity layer which is used by the Opacity widget and the Fade animations. In that case the scenario would be an Opacity widget with a single child at the EngineLayer level (the widget itself only allows for a single child widget, but that child widget may end up producing multiple engine layers as it paints). Also, the OpacityLayer can optimize for multiple non-overlapping child layers.

Without a good understanding of how widgets are associated with RenderObjects and RenderObjects produce both Picture and EngineLayer objects, the predictability of either mechanism will not be readily accessible to many developers. The description of how the various trees of widgets and RenderObjects interact is the subject of a number of articles on the web, particularly this video.

spauldhaliwal · 2022-07-15T17:41:19Z

Awesome, thanks so much for the detailed response!

JsouLiang · 2022-08-15T07:37:39Z

display_list/display_list_unittests.cc

+  builder.restore();
+
+  builder.Build()->Dispatch(expector);
+  EXPECT_EQ(expector.save_layer_count(), 2);


Sorry, I don't get what do u want to test, expect the save_count is equal the number we call saveLayer ?
I fount we call twice saveLayer the expector.save_layer_count is 2.
Through the case title NestedSaveLayersCanBothSupportOpacityOptimization I think the save_count will be less than 2, cause it is called SaveLayer Opacity Optimization.

Both saveLayers can inherit and transmit the opacity information from a calling context to their children. That is what is being tested.

Are we use the save_layer_count the check if saveLayer can inherit and transmit the opacity?

I'm wondering if these questions have to do with save/Layer optimizations that might be coming?

The test is just making sure that the interactions of a saveLayer that is trying to figure out if all of its children are compatible works correctly in the presence of a single child that is itself a saveLayer which will also do its own compatibility processing. i.e. do the 2 layers correctly navigate and analyze their own compatibility independently?

The count itself here isn't a primary part of the test, but more used as a watchdog to make sure that the entire list of asynchronously delivered flags that were expected were completely encountered. Primarily the expector will verify that it encounters each flag property in its expectations, in the right order. But, since that class/object is called by another object that controls the start/stop of the iteration, it has no "and you should be done now" call in which it can double-check "thank you, I got all the flags so we are successful!" So if it doesn't get called once per saveLayer, then something was wrong other than getting a mismatch on the flags - so the testing of the count at the end is simply saying "and did we get a matching call for every set of flags that I provided in the constructor?" The count is a simple way to note that we did see all of the flags we were expecting, but that same condition could have been tracked or asserted in different ways (i.e. checking an iterator against the end of the list, or using a "are you done with all of the flags?, etc.).

In retrospect, it probably should have been implemented as:
bool is_complete() { return save_layer_count_ == expected_.size(); }
since the specific count itself is not at issue as much as the fact that it must match the length of the expectations vector. Also, rather than a count, it might have used an iterator and so { return cur_ == end_; } might have been an alternate implementation of "completeness" that wouldn't depend on the internal workings of the expector class.

jjoelson · 2023-06-07T14:25:27Z

Hi all! Sorry to comment on this old merged PR, but is it possible that this optimization could cause opacity to not render properly in golden tests? Since upgrading to Flutter 3.10.0, we're seeing our widgets that use animated Opacity being rendered as fully opaque in our goldens regardless of what value we set as the opacity.

zanderso · 2023-06-07T14:31:53Z

Hi @jjoelson, the team does not triage comments on old PRs. The best way for us to take a look is to file a new issue with full details. Thanks!

flar requested review from chinmaygarde and gw280 January 20, 2022 21:50

flar added the Work in progress (WIP) Not ready (yet) for review! label Jan 24, 2022

flar force-pushed the dl-savelayer-opacity-peephole-prototype branch from 6086ee4 to ce1ddfb Compare January 24, 2022 23:12

flar changed the title ~~WIP: DisplayList savelayer opacity peephole prototype~~ DisplayList savelayer opacity peephole prototype Jan 25, 2022

flar mentioned this pull request Jan 26, 2022

Add benchmarks to measure impact of alpha saveLayers in DisplayLists flutter/flutter#97336

Merged

flar changed the title ~~DisplayList savelayer opacity peephole prototype~~ DisplayList savelayer opacity peephole Jan 27, 2022

flar changed the title ~~DisplayList savelayer opacity peephole~~ DisplayList savelayer opacity peephole optimization Jan 27, 2022

flar removed the Work in progress (WIP) Not ready (yet) for review! label Jan 27, 2022

flar requested review from chinmaygarde and gw280 and removed request for chinmaygarde and gw280 February 3, 2022 21:13

gw280 reviewed Feb 3, 2022

View reviewed changes

flar requested a review from gw280 February 4, 2022 01:17

gw280 approved these changes Feb 4, 2022

View reviewed changes

flar added 10 commits February 3, 2022 22:16

prototype of single-op opacity peephole optimization in DL savelayers

5105981

move save layer optimization properties into a new flags class

d9cc98a

rename to SaveLayerOptions and simplify push/restore_opacity

e9be389

fix a bug in the SaveLayerOptions and add a rendering test of saveLay…

9fe3aa3

…er opacity opt

add saveLayer opacity optimization tests

818596f

explicit constructor suggested by pre-submit checks

af7b1d4

add extra rendering ops to saveLayer tracing test to avoid optimization

49a90ac

comment fix

c82d872

simplify SaveLayerOptions flags

0d9d682

review feedback

f243b71

flar force-pushed the dl-savelayer-opacity-peephole-prototype branch from 7ab1bee to f243b71 Compare February 4, 2022 06:59

flar added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 4, 2022

fluttergithubbot merged commit dbe4cc8 into flutter:main Feb 4, 2022

engine-flutter-autoroll mentioned this pull request Feb 4, 2022

Roll Engine from 0d39b6dd2853 to dbe4cc89ba33 (1 revision) flutter/flutter#97787

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 4, 2022

dbe4cc8 DisplayList savelayer opacity peephole optimization (flutter/…

d7b6842

…engine#30957)

zanderso mentioned this pull request Feb 4, 2022

Compensate for change to saveLayer flutter/impeller#6

Merged

flar mentioned this pull request Feb 13, 2022

ImageFilterLayer for simple matrix transforms leads to unnecessary render target switches flutter/flutter#97987

Open

jmagman mentioned this pull request Mar 9, 2022

error: definition of implicit copy assignment operator for 'SaveLayerOptions' is deprecated because it has a user-declared copy constructor flutter/flutter#99795

Closed

JsouLiang reviewed Aug 15, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DisplayList savelayer opacity peephole optimization #30957

DisplayList savelayer opacity peephole optimization #30957

flar commented Jan 20, 2022 •

edited

Loading

flar commented Jan 25, 2022

flar commented Jan 27, 2022

gw280 Feb 3, 2022

flar Feb 4, 2022

gw280 Feb 3, 2022

flar Feb 4, 2022

gw280 commented Feb 3, 2022

gw280 Feb 3, 2022

flar Feb 4, 2022

flar Feb 4, 2022

zanderso commented Feb 4, 2022

spauldhaliwal commented Jul 15, 2022

flar commented Jul 15, 2022

spauldhaliwal commented Jul 15, 2022

JsouLiang Aug 15, 2022 •

edited

Loading

flar Aug 15, 2022

JsouLiang Aug 15, 2022 •

edited

Loading

flar Aug 15, 2022

jjoelson commented Jun 7, 2023 •

edited

Loading

zanderso commented Jun 7, 2023

DisplayList savelayer opacity peephole optimization #30957

DisplayList savelayer opacity peephole optimization #30957

Conversation

flar commented Jan 20, 2022 • edited Loading

flar commented Jan 25, 2022

flar commented Jan 27, 2022

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

gw280 commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso commented Feb 4, 2022

spauldhaliwal commented Jul 15, 2022

flar commented Jul 15, 2022

spauldhaliwal commented Jul 15, 2022

JsouLiang Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JsouLiang Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoelson commented Jun 7, 2023 • edited Loading

zanderso commented Jun 7, 2023

flar commented Jan 20, 2022 •

edited

Loading

JsouLiang Aug 15, 2022 •

edited

Loading

JsouLiang Aug 15, 2022 •

edited

Loading

jjoelson commented Jun 7, 2023 •

edited

Loading