DisplayList savelayer opacity peephole optimization #30957
Conversation
flar
force-pushed
the
dl-savelayer-opacity-peephole-prototype
branch
from
6086ee4
to
ce1ddfb
Compare
flar
changed the title
|
This is finally good to go for review @gw280 @chinmaygarde |
|
Here are the results from the benchmarks I'm adding to the framework repo: |
flar
changed the title
flar
changed the title
flar
requested review from
chinmaygarde and
gw280
and removed request for
chinmaygarde and
gw280
| cannot_inherit_opacity(false), | ||
| has_compatible_op(false) {} | ||
|
|
||
| size_t save_layer_offset; |
There was a problem hiding this comment.
can you add a comment here about what this is? it's not obvious from reading the code.
| builder.restore(); | ||
| builder.Build()->Dispatch(expector); | ||
| EXPECT_EQ(expector.save_layer_count(), 1); | ||
| } |
There was a problem hiding this comment.
I don't see any tests with nested saveLayer calls. We should probably add one that exercises this optimisation, and one that doesn't.
There was a problem hiding this comment.
I added a couple. Can(Cannot(..., Can(....))) and Can(Can(...))
|
LGTM for the most part. One minor documentation addition needed, and some extra tests for nested saveLayers would be nice. |
| 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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Big block comment added...
flar
force-pushed
the
dl-savelayer-opacity-peephole-prototype
branch
from
7ab1bee
to
f243b71
Compare
flar
added
the
waiting for tree to go green
|
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. |
|
Awesome, thanks so much for the detailed response! |
| builder.restore(); | ||
|
|
||
| builder.Build()->Dispatch(expector); | ||
| EXPECT_EQ(expector.save_layer_count(), 2); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Both saveLayers can inherit and transmit the opacity information from a calling context to their children. That is what is being tested.
There was a problem hiding this comment.
Are we use the save_layer_count the check if saveLayer can inherit and transmit the opacity?
There was a problem hiding this comment.
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.
|
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 |
|
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! |
These changes introduce the ability for
saveLayercalls 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.