Don't add empty OpacityLayer to the engine#31520
Conversation
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
LGTM. I think there's potentially a Fuchsia FL-something open for this, I'll try to find it.
|
Can we document this somehow? It seems entirely reasonable to say the contract for Opacity is that it must have a child (to keep the layer tree smaller). It may be difficult for consumers to discover that right now though, unless it's documented and I just missed it. |
|
I'll add docs/comments to the @chinmaygarde : is there any other or better place for documenting the engine layers? |
| // Try to avoid an [OpacityLayer] with no children. Painting an [OpacityLayer] | ||
| // in the Flutter engine is very costly due to the creation of an offscreen | ||
| // buffer and render target switches. If there's no child, having the | ||
| // OpacityLayer or not has the same effect. If [OpacityLayer] has no child, its |
There was a problem hiding this comment.
This seems off to me. Why can't we make it cheap in the engine if there's no child? E.g. if the engine gets an OpacityLayer with no layers, it's a no-op layer.
My understanding is that it's still beneficial to omit such layers from the engine side because it would (slightly) shrink the size of the layer tree and thus the real time it takes to traverse it.
There was a problem hiding this comment.
Do you mean "Try to avoid an [OpacityLayer] with no children. Remove that layer if possible to save some tree walks." ?
I guess that we can make an empty OpacityLayer as a no-op in the engine. We're just not currently doing it, and instead doing FML_DCHECK the nonemptiness.
There was a problem hiding this comment.
I think it's fine for the contract to be Don't send me an empty opacity layer, but the explanation here makes me wonder why that is (e.g. why is it so easy to short-circuit it here and not in the engine?).
Should we be pushing the short-circuiting even further up the call stack to the pushLayer call with opacity? Should we add an assert to OpacityLayer that it has at least one child when it's pushed?
There was a problem hiding this comment.
Talked offline - let's make this a doc comment (///) and your new text in there should be good.
kf6gpe
added
framework
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
Would like to get further clarification before submitting this :)
dnfield
added
the
framework
|
LGTM once doc is fixed. |
4 participants
Fixes #31517