raster cache Pictures embedded inside other DisplayLists by flar · Pull Request #29155 · flutter/engine

flar · 2021-10-13T09:11:01Z

Prepares sub-pictures of DisplayLists during Preroll and then draws them from the cache during Paint.

Needs tests.

Also needs a little more review of the potential to doubly cache the parent and child pictures.

flutter-dashboard · 2021-10-13T09:11:04Z

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.

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.

dnfield · 2021-10-13T17:39:02Z

flow/display_list_canvas.cc

 void DisplayListCanvasDispatcher::drawPicture(const sk_sp<SkPicture> picture,
                                              const SkMatrix* matrix,
                                              bool render_with_attributes) {
+  if (cache_ && !render_with_attributes) {


Am I understanding this correctly? It looks like this is trying to raster cache sub-pictures every time. That will be expensive on memory and processing power...

On what basis do you make that claim?

Ahh, I see now. I was misreading this and thought you were skipping a call to Prepare. I was confused about which class/method was doing what. I can see now the logic I was looking for is in https://github.com/flutter/engine/pull/29155/files#diff-3854149b0d2e47aedb825a57720ae5dbd70e7b0ab78a460bb8b31d0c4f8211dcR147

Ah, yes, this is just setting up a new scan of the DL ops with the same logic applied to the primary DL/Picture.

dnfield · 2021-10-13T17:39:29Z

flow/layers/display_list_layer.cc

    TRACE_EVENT0("flutter", "DisplayListLayer::RasterCache (Preroll)");
    cache->Prepare(context, disp_list, is_complex_, will_change_, matrix,
                   offset_);
+    if (!matrix.hasPerspective()) {


Is this related to this PR?

Yes and no.

Technically we probably shouldn't be doing any caching under perspective transforms. We've been living with it for a while for picture layers and display list layers and I haven't seen any developers note that it is causing problems, mainly because really who does a lot of perspective transforms in UI code. Without a specific test case I don't want to go modifying that code path just yet.

For this new code that test is partially "aiming for more correctness" in that regard, but also in a practical matter the setup for a perspective matrix (working under the naive assumption that it might be worthwhile) would be more involved. So, to simplify the new code by avoiding a more complicated setup that would likely end in tears, I added this check to avoid the recursion and bad output that might ensue.

Could we move this into a separate PR? It seems like it's worth doing, but it should be landed separately to make bisecting easier, and to avoid needing to revert this change for an unrelated reason.

I should make a new Issue for it, but if I remove it from this code, this code itself gets more complicated for a reason that will likely prove unfeasible.

I can add to the issue something along the lines of "if it turns out we can cache under perspective transforms, then the following code needs to remove its test and adjust".

dnfield

We should not be raster caching sub-pictures every time with no limit - the raster cache currently limits picture caching to 3 per frame and only in cases where we'd see them again.

This would probably benefit flutter_svg, but it may not work well with other uses of this API, which are currently few. But it would be surprising to find that canvas.drawPicture always results in raster caching. Forgive me if I'm just missing something in the code here :)

flar · 2021-10-13T21:01:01Z

We should not be raster caching sub-pictures every time with no limit - the raster cache currently limits picture caching to 3 per frame and only in cases where we'd see them again.

I don't get how this changes any of that.

The limit is to how many new pictures are cached, not the total number of cached pictures. We typically end up with way more than 3 pictures cached.

These pictures will only be cached if they are stable - why should this code be rejected under the logic of "and only if they are stable"? That decision is happening here too.

This would probably benefit flutter_svg, but it may not work well with other uses of this API, which are currently few. But it would be surprising to find that canvas.drawPicture always results in raster caching. Forgive me if I'm just missing something in the code here :)

"Always results in raster caching" is not what is happening here. This code makes them eligible for caching under the same rules as the picture objects stored in the picture/dl layers themselves. The difference is that developers don't have to complicate their code to try to isolate them to receive this benefit.

If someone is using drawPicture() on the same picture over and over, then it will be cached as if they were embedding it in a picture layer. Why shouldn't it be? If they are using it on a temporary picture - that picture won't be cached because it won't see reuse.

dnfield · 2021-10-13T21:11:23Z

I figured out what I was reading wrong.

I do think we should move the perspective logic into a separate PR - it'd be fine to land that one before this one if it makes a difference - but it should make it easier to bisect/revert if that causes an unrelated problem.

flar · 2021-10-13T21:21:40Z

I figured out what I was reading wrong.

I do think we should move the perspective logic into a separate PR - it'd be fine to land that one before this one if it makes a difference - but it should make it easier to bisect/revert if that causes an unrelated problem.

This is currently a WIP so there is no real rush, but I would want to find a test case that demonstrates the problem with caching under a perspective transform before I move on that.

Also, there is an outstanding issue to fix our transform layers wrt perspective transforms that could change this whole perspective. One of the problems with caching under a perspective transform is that we are currently using broken one-shot versions of perspective transforms that are only valid for leaf operations. If we had a true perspective transform at that point in the logic we might be able to cache. Unfortunately, I don't think our caching math is up to that challenge without a lot more work.

zanderso · 2021-10-19T20:01:01Z

How would this interact with flutter/flutter#91621 if we did it?

flar · 2021-10-19T21:58:35Z

How would this interact with flutter/flutter#91621 if we did it?

For parent pictures that are degenerate, such as "record [drawPicture(stable_picture)] end_recording" where the picture doesn't look complex enough to cache, this wouldn't be needed with the fix for nested op counts, but it shouldn't hurt anything as long as we make sure that we don't double cache the parent and the child (that work is TBD while this PR is WIP).

For parent pictures that change on every frame, whether or not they are also degenerate, this would allow a complex-but-stable child to be cached anyway. This is the case that commonly happens with the SVG pictures where they can be included in multi-widget pictures that are changing due to content outside of the SVG picture changing, but the SVG picture is stable and rendered with drawPicture(). That specific case is already obsoleted by dnfield/flutter_svg#592, though, so the value depends on how many other such cases we find in other app code.

For parent pictures that are stable and complex, those will be cached themselves and so these child pictures don't need to be cached (modulo this PR needs to address that case and prevent redundant caching).

flutter-dashboard · 2021-11-15T18:14:06Z

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.

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.

flutter-dashboard · 2022-01-12T18:30:48Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

flar · 2022-03-15T23:40:54Z

This has been on the back burner for a while and is massively obsolete. It might be better to start over after a number of PRs land to improve the raster caching.

raster cache Pictures embedded inside other DisplayLists

6767a01

flar added affects: engine Work in progress (WIP) Not ready (yet) for review! labels Oct 13, 2021

flar requested review from chinmaygarde, dnfield and goderbauer October 13, 2021 09:11

flutter-dashboard bot added the needs tests label Oct 13, 2021

google-cla bot added the cla: yes label Oct 13, 2021

apply the matrix parameter of drawPicture during rendering

407efeb

flar mentioned this pull request Oct 13, 2021

Improve performance when an animation is present over the SVG dnfield/flutter_svg#592

Merged

dnfield reviewed Oct 13, 2021

View reviewed changes

dnfield suggested changes Oct 13, 2021

View reviewed changes

satisfy Windows compiler aggressive incomplete class heuristics

9a15ebb

flar mentioned this pull request Oct 15, 2021

TransformLayer should participate in raster caching flutter/flutter#91648

Closed

ds84182 mentioned this pull request Oct 21, 2021

use nested op counts to determine picture complexity for raster cache #29265

Merged

CaseyHillers changed the base branch from master to main November 15, 2021 18:14

fluttergithubbot force-pushed the master branch from b1a8771 to 2962099 Compare November 15, 2021 18:30

godofredoc changed the base branch from master to main November 24, 2021 07:16

flar closed this Mar 15, 2022

Conversation

flar commented Oct 13, 2021

Uh oh!

flutter-dashboard bot commented Oct 13, 2021

Uh oh!

dnfield Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

flar Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

dnfield Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

flar Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

dnfield Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

flar Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

dnfield Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

flar Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

flar commented Oct 13, 2021

Uh oh!

dnfield commented Oct 13, 2021

Uh oh!

flar commented Oct 13, 2021

Uh oh!

zanderso commented Oct 19, 2021

Uh oh!

flar commented Oct 19, 2021

Uh oh!

flutter-dashboard bot commented Nov 15, 2021

Uh oh!

flutter-dashboard bot commented Jan 12, 2022

Uh oh!

flar commented Mar 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

flar Oct 13, 2021 •

edited

Loading