Move layer clip culling to Paint() method to fix child caching #22336
Conversation
|
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@fredlee12345678 thoughts? |
| void set_paint_bounds(const SkRect& paint_bounds) { | ||
| paint_bounds_ = paint_bounds; | ||
| } | ||
|
|
There was a problem hiding this comment.
Let's take this opportunity to add some documentations to Layer::paint_bounds. Before retained layers are introduced, Layer::paint_bounds is simple as each layer only exists for one frame with a single context. After retained layers, we need to explicitly document that Layer::paint_bounds must be independent of its parent context as the layer subtree can be retained and used in another context.
As it's context-independent, I wonder if Preroll is the best place to compute paint_bounds since there's aPrerollContext, and one might be tempted to use it. Shall we compute the paint bounds during the layer construction phase? For example, during SceneBuilder phase, when all children layers are added, call a method such as Layer::finalize to notify that no more children are expected to be added, and the paint_bounds can now be computed in a context-independent way.
There was a problem hiding this comment.
We've been discussing these issues over on the issue page linked above.
I think we are pretty much good to go on pre-computing the bounds at scene construction time, but there is still the issue of the shadow bounds depending on device pixel ratio which I think might be a bug. I briefly played with pre-computing the bounds as a variation of this PR, but decided we can do that in a separate operation - the main objective here is to get rid of the pre-mature culling while not breaking anything else.
I'll add some comments to paint_bounds().
There was a problem hiding this comment.
I updated the comments in the recent commit.
|
(Please ignore my previous comment about performance - I was graphing the wrong A/B results.) It looks like the statistics on the rasterizer times and the profile of the timings is nearly identical (see attached graph image).
|
flow/layers/layer.h
Outdated
| const float frame_device_pixel_ratio; | ||
|
|
||
| bool needs_painting(const SkRect& paint_bounds) { | ||
| return !internal_nodes_canvas->quickReject(paint_bounds); |
There was a problem hiding this comment.
What's the difference between the paint_bounds argument, and the paint_bounds_ member variable? I thought they should agree with each other?
There was a problem hiding this comment.
This method was on PaintContext so it needed to have it passed in.
I've since moved the method back to Layer so it is now "needs_painting(PaintContext& context);"
This code is obsolete.
There was a problem hiding this comment.
New code that now has Layer::needs_painting(PaintContext& context) pushed.
The new code also has Layer::is_empty() because there were 2 or 3 places that wanted to ask if the layer (usually the root) needed to be painted but they hadn't constructed a full PaintContext yet. So, those places will now depend solely on whether the layer is empty.
flar
force-pushed
the
no-paint-bounds-culling-in-preroll
branch
from
e029837 to
e7f9ee7
Compare
flar
changed the title
|
Here are the AB comparison graphs of rasterizer times for the gallery_transitions_perf benchmark running on a Moto G4: |
flar
changed the title
| layer->Add(mock_layer); | ||
|
|
||
| preroll_context()->cull_rect = kEmptyRect; // Cull everything | ||
| preroll_context()->cull_rect = distant_bounds; // Cull everything |
There was a problem hiding this comment.
Is // Cull everything still accurate? Is seems that everything in this test still falls outside of the cull_rect, but it's not as obvious as an empty rect. I'm also curious what motivates the replacement of the empty rect.
There was a problem hiding this comment.
It culls "everything in this test". I'll update the comment. Or maybe split it into two tests?
There was a problem hiding this comment.
Changed this to // Cull these children in latest push
flow/layers/container_layer.cc
Outdated
| // is initially handed to a layer's Paint() method. By the time the | ||
| // layer calls PaintChildren(), though, it may have modified the | ||
| // PaintContext so we can no longer double-check the culling. | ||
| // FML_DCHECK(needs_painting(context)); |
There was a problem hiding this comment.
We prefer removing the code instead of just commenting it out. You can still leave the comment here with something like Don't FML_DCHECK(needs_painting(context)) here! needs_painting(context) is only valid ....
flow/layers/layer.h
Outdated
| needs_system_composite_ = value; | ||
| } | ||
|
|
||
| // Returns the unclipped paint bounds in the layer's local coordinate |
There was a problem hiding this comment.
unclipped seems inaccurate. The paint bounds should consider all clips in the layer subtree, but ignore all clips or cull rects of its ancestors?
flow/layers/texture_layer.cc
Outdated
|
|
||
| void TextureLayer::Paint(PaintContext& context) const { | ||
| TRACE_EVENT0("flutter", "TextureLayer::Paint"); | ||
| // FML_DCHECK(needs_painting(context)); |
There was a problem hiding this comment.
Remove the line that's commented out.
There was a problem hiding this comment.
Read above for my history on this. Note that the line wasn't actually there before. I added it when I started, realized that the tests were broken and commented it out pending investigation. I've now figured out why it was causing problems and working on fixing it rather than removing it.
There was a problem hiding this comment.
I've now fixed the tests (new commits pending) and will restore this line.
There was a problem hiding this comment.
Fixes now committed (pushed).
|
I believe the current code should meet all review feedback so far. |
flar
force-pushed
the
no-paint-bounds-culling-in-preroll
branch
from
9159986 to
4b6bee8
Compare
flar
added
the
waiting for tree to go green
5 participants
Fixes flutter/flutter#69552 by moving layer culling to the
Paint()phase.Also, now all children are culled, not just Clip layers.
Also, preroll can be avoided on retained layers.(No longer seeing this as a goal or a possibility.)The tests are all botched by this change as this is a WIP. I'm seeking feedback on the general approach before I modify the layer tests to match the new preroll/paint rules.(Tests now updated.)The test case in the issue runs correctly with this change and it runs fine in DEBUG mode which means the(old method removed entirely in favor of new methods)FML_DCHECK(false)I inserted in the oldLayer::needs_painting()method is not being triggered during a run of a simple app.