Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Move layer clip culling to Paint() method to fix child caching #22336

Merged
fluttergithubbot merged 9 commits into from
Nov 13, 2020
Merged

Move layer clip culling to Paint() method to fix child caching #22336

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f68b19d
prototype to fix the problem of clip layers preventing child caching
flar Nov 5, 2020
c1e1049
remove kUnvisitedRect, the code base is not yet ready to reuse paint_…
flar Nov 7, 2020
19a6e6b
add comments about the scope of Layer::paint_bounds()
flar Nov 9, 2020
fc39170
switch culling to layer::needs_painting(PaintContext)
flar Nov 11, 2020
e9f9c83
Remove needs_painting DCHECK from PaintChildren
flar Nov 12, 2020
54ebbe3
remove dead code and fix formatting
flar Nov 12, 2020
c0b7c3b
fix texture layer death tests and enable DCHECK in TextureLayer::Paint
flar Nov 12, 2020
5d7faf2
adjust comments as per review feedback
flar Nov 12, 2020
4b6bee8
add Skia bug number to workaround comment
flar Nov 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
remove kUnvisitedRect, the code base is not yet ready to reuse paint_… 
…bounds
  • Loading branch information
@flar
flar committed Nov 13, 2020
commit c1e10490a60c7b96965e4db22b7431d8f9dfd17a
3 changes: 2 additions & 1 deletion flow/layers/clip_path_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,13 @@ void ClipPathLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
Layer::AutoPrerollSaveLayerState save =
Layer::AutoPrerollSaveLayerState::Create(context, UsesSaveLayer());
context->mutators_stack.PushClipPath(clip_path_);

SkRect child_paint_bounds = SkRect::MakeEmpty();
PrerollChildren(context, matrix, &child_paint_bounds);

if (child_paint_bounds.intersect(clip_path_bounds)) {
set_paint_bounds(child_paint_bounds);
}

context->mutators_stack.Pop();
context->cull_rect = previous_cull_rect;
}
Expand Down
5 changes: 2 additions & 3 deletions flow/layers/clip_rect_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,16 @@ void ClipRectLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {

SkRect previous_cull_rect = context->cull_rect;
context->cull_rect.intersect(clip_rect_);
// TRACE_EVENT_INSTANT0("flutter", "children inside clip rect");

Layer::AutoPrerollSaveLayerState save =
Layer::AutoPrerollSaveLayerState::Create(context, UsesSaveLayer());
context->mutators_stack.PushClipRect(clip_rect_);

SkRect child_paint_bounds = SkRect::MakeEmpty();
PrerollChildren(context, matrix, &child_paint_bounds);

if (child_paint_bounds.intersect(clip_rect_)) {
set_paint_bounds(child_paint_bounds);
}

context->mutators_stack.Pop();
context->cull_rect = previous_cull_rect;
}
Expand Down
4 changes: 2 additions & 2 deletions flow/layers/clip_rrect_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,16 @@ void ClipRRectLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
SkRect previous_cull_rect = context->cull_rect;
SkRect clip_rrect_bounds = clip_rrect_.getBounds();
context->cull_rect.intersect(clip_rrect_bounds);

Layer::AutoPrerollSaveLayerState save =
Layer::AutoPrerollSaveLayerState::Create(context, UsesSaveLayer());
context->mutators_stack.PushClipRRect(clip_rrect_);

SkRect child_paint_bounds = SkRect::MakeEmpty();
PrerollChildren(context, matrix, &child_paint_bounds);

if (child_paint_bounds.intersect(clip_rrect_bounds)) {
set_paint_bounds(child_paint_bounds);
}

context->mutators_stack.Pop();
context->cull_rect = previous_cull_rect;
}
Expand Down
9 changes: 3 additions & 6 deletions flow/layers/container_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,7 @@ void ContainerLayer::PrerollChildren(PrerollContext* context,
// sibling tree.
context->has_platform_view = false;

if (layer->paint_bounds() == kUnvisitedRect) {
layer->Preroll(context, child_matrix);
}
layer->Preroll(context, child_matrix);

if (layer->needs_system_composite()) {
set_needs_system_composite(true);
Expand Down Expand Up @@ -88,9 +86,8 @@ void ContainerLayer::PaintChildren(PaintContext& context) const {
void ContainerLayer::TryToPrepareRasterCache(PrerollContext* context,
Layer* layer,
const SkMatrix& matrix) {
if (!context->has_platform_view && context->raster_cache
// && SkRect::Intersects(context->cull_rect, layer->paint_bounds())
) {
if (!context->has_platform_view && context->raster_cache &&
SkRect::Intersects(context->cull_rect, layer->paint_bounds())) {
context->raster_cache->Prepare(context, layer, matrix);
}
}
Expand Down
2 changes: 1 addition & 1 deletion flow/layers/layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
namespace flutter {

Layer::Layer()
: paint_bounds_(kUnvisitedRect),
: paint_bounds_(SkRect::MakeEmpty()),
unique_id_(NextUniqueID()),
needs_system_composite_(false) {}

Expand Down
10 changes: 8 additions & 2 deletions flow/layers/layer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
namespace flutter {

static constexpr SkRect kGiantRect = SkRect::MakeLTRB(-1E9F, -1E9F, 1E9F, 1E9F);
static constexpr SkRect kUnvisitedRect = SkRect::MakeLTRB(-1E9F, -1E9F, -1E9F, -1E9F);

// This should be an exact copy of the Clip enum in painting.dart.
enum Clip { none, hardEdge, antiAlias, antiAliasWithSaveLayer };
Expand Down Expand Up @@ -176,7 +175,14 @@ class Layer {
paint_bounds_ = paint_bounds;
}

Copy link
Contributor

@liyuqian liyuqian Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@flar flar Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor Author

@flar flar Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comments in the recent commit.

bool needs_painting() const { FML_DCHECK(false); return false; }
// This can be removed once we wean the unit tests off of calling it...
// Unit tests currently fail anyway because the mock PaintContext objects
// don't have a valid clip set to support the new needs_painting()
// mechanism, so more work is required...
bool needs_painting() const {
FML_LOG(ERROR) << "!!!!! Layer::needs_painting() CALLED !!!!!";
return !paint_bounds_.isEmpty();
}

uint64_t unique_id() const { return unique_id_; }

Expand Down