Don't push offset to leaf layers #21619
Conversation
|
Have you considered using |
|
@yjbanov Sure, I'll have a look. Can you point me to your prototype? |
There was a problem hiding this comment.
Ok, I'll preserve the old API. It will disallow me to remove some code that I think would be beneficial to our code health and tech debt in the long run. But we can always change the API and make the decision of whether to remove such code or not later in a separate PR.
|
I think we should continue to support the old API, even if we change the default implementation of OffsetLayer to use transforms and pass through zero instead of passing through the offset. |
There was a problem hiding this comment.
I think I'd prefer a dedicated pushOffset method on SceneBuilder that matches the API of OffsetLayer:
@override
void addToScene(ui.SceneBuilder builder) {
builder.pushOffset(offset.dx, offset.dy);
}The dx and dy can be easily (and more efficiently) converted to a transform matrix by the engine. Knowing the difference between a transform and an offset I can use it to collapse a series of offsets into a single offset as an optimization that reduces the total number of layers.
There was a problem hiding this comment.
Sounds good to me. flutter/engine#6349 is uploaded to handle this. Once that landed, I can change this (either in this PR, or in the next PR).
|
@Hixie Added |
There was a problem hiding this comment.
looks like this is fixing an old bug, can you add a test for this?
|
LGTM. I'm surprised by the premise of this PR, though. why would we need to send anything to the engine but the new offsets on the leaves? |
|
@Hixie here's an example: Frame 1: Frame 2: Today, this is submitted to the Engine as: Frame 1: Frame 2: Note that we mutated the tree under the "opacity" node. After this change, we will submit the layers to the engine unchanged, i.e. the sub-tree rooted at "opacity" is not mutated from frame 1 to frame 2, which tells the engine "nothing's changed under opacity and it is safe to cache it". |
4 participants
For retained rendering, we don't want to push the offset down to each leaf layer. Otherwise, changing an offset layer on the very high level could cascade the change to too many leaves, which means that we can't retain them.
To not push the offset downwards, we simply push a TransformLayer when there's an offset. Skia has a fast path for concatenating scale/translation-only matrix so this operation should be fast (no performance regression is measured on Moto G4).
This also makes our compositor much cleaner as we don't have to pass
layerOffsetall over the places inXXXLayer::addToScene.We can also remove the
offset_field and its related operations fromflow::PictureLayer, flow::TextureLayer, flow::ChildSceneLayerin flutter/engine once this change landed.This is our first step towards #21756