Defer decisions about RasterCache actions until after Preroll is complete

[JsouLiang]

1. I use the AutoGatherLayers (from knopp) to collection the Layer which need to RasterCache
2. Add a level property which mean the layer level in the LayerTree, when we have collection the need RasterCache layer from preroll method and the we will Raster Cache Layer according the level that mean we will do Raster Cache layer from top to bottom .
3. In the new RasterCache(PrerollContext*, matrix) method the layer not do raster cache if the ancestor layer has raster cache

TODO:

• Layer is can do raster cache in current version
  • ImageFilterLayer
  • ColorFilterLayer
  • DisplayListLayer
  • OpacityLayer
  • SkPictureLayer
  • ShaderMaskLayer
• check displayList layer and picture layer can do raster cache
  • RasterCacheableEntry need to differentiate: Layer, DisplayList, SkPicture
• raster cache displayList layer and picture layer

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[flar]

One thing I like about this mechanism is that it essentially breaks the caching out into a separate pass as per flutter/flutter#91923

I think the mechanism should be much simpler as explained in the review comments. Also, it would be nice to include picture and display caching in the list of cacheable items.

Finally, we could prune the dead cache entries (see the SweepOneCache method) before we add new ones in the caching pass. This would require the preroll items to "touch" or "mark" their existing cache items as having been seen so we don't de-cache anything used in the current frame. This advanced pruning could be a follow on PR, or it could be rolled into this mechanism.

[JsouLiang]

@flar hey bro, According your suggest I have change the RasterCache method. And also has some thing need to do, may be you can review the current logic to see if it is correct. Thanks
Current I only change the ImageFilterLayer::Preroll logic. If the ImageFilterLayer::Preroll logic is correct,I will modify other categories layers of the as well

[ColdPaleLight]

@flar @JsouLiang
In general, caching entries from top to bottom according to the level of the layer is a generally correct algorithm, but I think there may be a bad case here. I filed a PR #31925 to try to resolve flutter/flutter#87827 and flutter/flutter#87826 . If these two issues are finally resolved, it means that for particularly complex pictures, they should be cached the first time they are used, and will continue to be kept for a few frames when they are unused. Then if there is such a particularly complex Layer in a descendant of OpaqueLayer or ImageFilterLayer, I guess we should cache this particularly complex Layer instead of OpaqueLayer or ImageFilterLayer.

[wangying3426]

Fine...
It seems to be a right idea which like a "Mark-Cache" algorithm , then we can more flexibly to decide the layers which need to be cached from the perspective of global optimum at each frame.

[flar]

Generally this value is only used during Preroll and will not be needed in the caching methods. You can delete this field and just provide a dummy value when creating the Context used when processing the caching queue.

[flar]

First, I'm not entirely happy with the way the classes break down and inherit methods. I originally suggested creating a CacheableLayer type of class so that only the exact layers that want to perform caching would inherit from it and none of the methods to implement the caching mechanism would exist in Layer or ContainerLayer, but that doesn't seem to be the way that this is going. Every ContainerLayer implements CacheableLayer whether or not its sub class wants to consider caching, and only a portipon of them actually try to cache themselves or their children. If this subclass can't isolate the cacheable methods out of Layer and ContainerLayer, then let's just get rid of it.

Second, we are seeing an issue with the opacity inheritance flags. These were originally implemented as part of Preroll and the children would indicate their ability to inherit the opacity based on a few conditions:

• They were planning to do their own saveLayer in the Paint method and so they could add the opacity as an attribute to the saveLayer "for free" to avoid OpacityLayer having to do its own saveLayer
• They were planning to just drawImage an image to the screen so they could add the opacity to the SkPaint used in the call to drawImage. This applied mainly to TextureLayer and the Picture/DisplayListLayers in the case where they were being drawn from the cache.
• Some layers could pass the opacity through to their child(ren) since their function involved no rendering at all (Transform/ClipLayers).
• Finally some DisplayList objects could inherit the opacity even if they weren't cached.

Most of those cases can be statically determined during preroll except for one - whether or not a Picture or DisplayList is cached. We now don't really know that until after Preroll is done.

In most cases you set it after ShouldBeCached which is a far cry from when it actually should be set. ShouldBeCached simply says that the Picture or DisplayList is eligible based on a static assessment, but there is still a countdown before caching it. It used to be set only after the countdown of pre-cache draws was exceeded and then the picture/DL was cached. You are setting the flag way prematurely (3 entire frames prematurely).

Even if it "ShouldBeCached" and the countdown happens and this is the frame where it gets into the cache, it may not end up being cached because we only cache a maximum of N items per frame and this one may be beyond that limit, or we could simply fail to cache it because it exceeded the maximum allocation size for a texture - we won't know until the cache evaluation phase for much of that.

[JsouLiang]

@flar Hi bro, according to your suggestion, I change the CacheableLayer to an interface, that the ColorFilterLayer, ImageFilterLayer implement the interface that mean they can try to raster cache;
About the check DisplayList should be raster cache, I add the access_threshold_ to limit it.

[flar]

LGTM

[flar]

@ColdPaleLight @dnfield any final review comments?

[flar]

The corresponding benchmark to demonstrate the elimination of the "double caching" issue is in the framework tree and collecting data. Depending on how busy the trees are, it will hopefully collect a good dozen or so results before this PR can be rolled in.

See: https://flutter-dashboard.appspot.com/#/build?taskFilter=cache_use&authorFilter&messageFilter&hashFilter&showMac=true&showWindows=true&showiOS=true&showLinux=true&showAndroid=true&showStaging=false&repo=flutter&branch=master

[ColdPaleLight]

nit: remove it

[ColdPaleLight]

LGTM with nit

[dnfield]

I would really like to see the captures in the two places that are using [=] just explicitly capture what's needed - this can be a source of runtime issues by capturing and then using things that aren't actually safe to use.

That said, It doesn't have to block landing this change and it'd be fine to follow on in a separate PR.

[dnfield]

My other review comments were less important nits or appear to have been addressed.

[flar]

I would really like to see the captures in the two places that are using [=] just explicitly capture what's needed - this can be a source of runtime issues by capturing and then using things that aren't actually safe to use.

That said, It doesn't have to block landing this change and it'd be fine to follow on in a separate PR.

Good point. flutter/flutter#106892

[flar]

Shouldn't this be "intersects"? Is this a bug? I'm curious why it never showed up anywhere. Perhaps because we don't use this cull_rect for much during preroll?