[Impeller] better handle allocation herustics of Android slide in page transition.

[jonahwilliams]

For flutter/flutter#158881

The new Android m3 page transition animates a saveLayer w/ opacity + slide in animation. See example video from linked PR:

387832059-a806f25d-8564-4cad-8dfc-eb4585294181.mov

On each frame, we intersect the coverage rect of this saveLayer contents with the screen rect, and shrink it to a partial rectangle. But this changes each frame, which forces us to re-allocate a large texture each frame, causing performance issues.

Why not ignore the cull rect entirely? We sometimes ignore the cull rect for the image filter layer for similar reasons, but from observation, the sizes of these saveLayer can be slightly larger than the screen for flutter gallery. Instead, I attempted to use the cull rect size to shrink the saveLayer by shifting the origin before intersecting.

I think this should be safe to do, as I believe it could only leave the coverage as larger than it would have been and not smaller.

[jonahwilliams]

@flar / @gaaclarke does this make sense to you?

[flar]

The algorithm feels highly speculative and very specific to the sliding transition. The two coverages could differ by a large shift and we'd allocate a coverage that was very greatly larger than we need, wasting rendering time.

I like the fact that it first checks if there is any intersection at all - that at least prevents this over-estimated coverage when the source isn't even visible. But, what if the transformed intersection is 5x5 but the shifted intersection is 500x700? We'd allocate 14k times as many pixels as we need without even batting an eye. And what if that particular case happened to be static so we paid that price on every frame without any indication that it was animating towards a larger intersection?

If this is to prevent thrashing, what if we just increased the granularity of temporary texture allocations to minimize the number without trying to completely eliminate them?

[flar]

Overall I'd vote against this particular heuristic without any other limits or predictive evidence guiding its actions, but I am intrigued by the concept of trying to avoid varying allocations during transitions.

[jonahwilliams]

Re: lack of limits, that is a good point. For the slide in, I think the values are only in the 10 to 15% range. So I could limit this by limiting how much I'm willing to slide it over.

The algorithm feels highly speculative and very specific to the sliding transition

It is highly specific to that transition, but that transition will be the default for all android applications soon - so its going to be extremely widespread.

[flar]

One thought is that we could advertise transitions to the engine by having something along the lines of ui.Canvas.interpolateTransorm(matrixA, matrixB, t) - that way we see the entire range of transforms we will encounter and the t value specifies the specific transition point currently in effect.

[flar]

Re: lack of limits, that is a good point. For the slide in, I think the values are only in the 10 to 15% range. So I could limit this by limiting how much I'm willing to slide it over.

Also, this form of performance improvement is a compromise. It's not "simply" saving us thrashing the cache, it's causing us to render more in lieu of thrashing the cache.

We should only render the true intersection on this frame even if we allocate what we believe will be the entire operation. I don't see that addressed here. Perhaps return a {intersected bounds, potential size} tuple?

[flar]

Another idea, the transform interpolation could be an internal thing with TransformLayer if it could understand that it is part of an animation and tell its engine layer "Hey, I'm going from A to B and currently at state t" without having to modify the ui.Canvas API in a confusing way. (iow - It might be an internal call on DlCanvas invoked from the TransformLayer, but not visible on ui.Canvas).

[jonahwilliams]

I've narrowed down this case so that it only applies to slide in like transations. I agree that this fix is overly specific, and I think the real fix for the allocation problems is flutter/flutter#156503 . This is essentially to round up allocations to some sort of interval sizing. This would almost fix this issue, however for reasons I'm not entirely sure of yet, the framework is reporting that one of the saveLayers is a ~8 or so pixels wider than the onscreen size in a way that doesn't seem entirely stable.

That said, I would like to land this ahead of attempting larger changes as I don't want the new android page transition to be delayed nor do I want the first versions of it to regress overally performance. I do not believe that this change, while overly specific, is harmful, though let benchmarks and framework CI strike me down if I'm wrong.

[flar]

Just some suggestions.

[flar]

Not sure if this needs a code comment. My first reading of this line was "wait, we only do this if there is very nearly no delta?" before I realized it was looking for horizontal or vertical transitions.

[jonahwilliams]

Added more documentation

[flar]

Perhaps add a test where the threshold is too large in X and another in Y.

[jonahwilliams]

Done

[gaaclarke]

I share some concerns like Jim, if we want to go down this route I think we need even more complete documentation.

[gaaclarke]

Suggested change

	// thrasing of the texture cache. Instead, we try to determine the
	// thrashing of the texture cache. Instead, we try to determine the

Why does it cause thrashing?

[jonahwilliams]

because the intersected size is different each frame, while the original size is stable

[gaaclarke]

Can you add that to the comment please?

[gaaclarke]

This magic number needs to be pulled out into a const with documentation about where it comes from.

[jonahwilliams]

done

[flar]

LGTM

[gaaclarke]

Let's make sure to revisit this after we get benchmark results, trying it out well documented sgtm.