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
Next Next commit
prototype to fix the problem of clip layers preventing child caching
  • Loading branch information
@flar
flar committed Nov 13, 2020
commit f68b19d4498e4ca517d7602044b0ee663bd239f0
2 changes: 1 addition & 1 deletion flow/layers/backdrop_filter_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ void BackdropFilterLayer::Preroll(PrerollContext* context,

void BackdropFilterLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "BackdropFilterLayer::Paint");
FML_DCHECK(needs_painting());
FML_DCHECK(context.needs_painting(paint_bounds()));

Layer::AutoSaveLayer save = Layer::AutoSaveLayer::Create(
context,
Expand Down
31 changes: 11 additions & 20 deletions flow/layers/clip_path_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,17 @@ void ClipPathLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {

SkRect previous_cull_rect = context->cull_rect;
SkRect clip_path_bounds = clip_path_.getBounds();
children_inside_clip_ = context->cull_rect.intersect(clip_path_bounds);
if (children_inside_clip_) {
TRACE_EVENT_INSTANT0("flutter", "children inside clip rect");

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.intersect(clip_path_bounds);
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 All @@ -54,12 +50,7 @@ void ClipPathLayer::UpdateScene(std::shared_ptr<SceneUpdateContext> context) {

void ClipPathLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "ClipPathLayer::Paint");
FML_DCHECK(needs_painting());

if (!children_inside_clip_) {
TRACE_EVENT_INSTANT0("flutter", "children not inside clip rect, skipping");
return;
}
FML_DCHECK(context.needs_painting(paint_bounds()));

SkAutoCanvasRestore save(context.internal_nodes_canvas, true);
context.internal_nodes_canvas->clipPath(clip_path_,
Expand Down
1 change: 0 additions & 1 deletion flow/layers/clip_path_layer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ class ClipPathLayer : public ContainerLayer {
private:
SkPath clip_path_;
Clip clip_behavior_;
bool children_inside_clip_ = false;

FML_DISALLOW_COPY_AND_ASSIGN(ClipPathLayer);
};
Expand Down
29 changes: 11 additions & 18 deletions flow/layers/clip_rect_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,19 @@ void ClipRectLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
TRACE_EVENT0("flutter", "ClipRectLayer::Preroll");

SkRect previous_cull_rect = context->cull_rect;
children_inside_clip_ = context->cull_rect.intersect(clip_rect_);
if (children_inside_clip_) {
TRACE_EVENT_INSTANT0("flutter", "children inside clip 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);
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();
if (child_paint_bounds.intersect(clip_rect_)) {
set_paint_bounds(child_paint_bounds);
}
context->mutators_stack.Pop();
context->cull_rect = previous_cull_rect;
}

Expand All @@ -49,12 +47,7 @@ void ClipRectLayer::UpdateScene(std::shared_ptr<SceneUpdateContext> context) {

void ClipRectLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "ClipRectLayer::Paint");
FML_DCHECK(needs_painting());

if (!children_inside_clip_) {
TRACE_EVENT_INSTANT0("flutter", "children not inside clip rect, skipping");
return;
}
FML_DCHECK(context.needs_painting(paint_bounds()));

SkAutoCanvasRestore save(context.internal_nodes_canvas, true);
context.internal_nodes_canvas->clipRect(clip_rect_,
Expand Down
1 change: 0 additions & 1 deletion flow/layers/clip_rect_layer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ class ClipRectLayer : public ContainerLayer {
private:
SkRect clip_rect_;
Clip clip_behavior_;
bool children_inside_clip_ = false;

FML_DISALLOW_COPY_AND_ASSIGN(ClipRectLayer);
};
Expand Down
28 changes: 10 additions & 18 deletions flow/layers/clip_rrect_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,18 @@ void ClipRRectLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {

SkRect previous_cull_rect = context->cull_rect;
SkRect clip_rrect_bounds = clip_rrect_.getBounds();
children_inside_clip_ = context->cull_rect.intersect(clip_rrect_bounds);
if (children_inside_clip_) {
TRACE_EVENT_INSTANT0("flutter", "children inside clip rect");
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);
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();
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 All @@ -50,12 +47,7 @@ void ClipRRectLayer::UpdateScene(std::shared_ptr<SceneUpdateContext> context) {

void ClipRRectLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "ClipRRectLayer::Paint");
FML_DCHECK(needs_painting());

if (!children_inside_clip_) {
TRACE_EVENT_INSTANT0("flutter", "children not inside clip rect, skipping");
return;
}
FML_DCHECK(context.needs_painting(paint_bounds()));

SkAutoCanvasRestore save(context.internal_nodes_canvas, true);
context.internal_nodes_canvas->clipRRect(clip_rrect_,
Expand Down
1 change: 0 additions & 1 deletion flow/layers/clip_rrect_layer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ class ClipRRectLayer : public ContainerLayer {
private:
SkRRect clip_rrect_;
Clip clip_behavior_;
bool children_inside_clip_ = false;

FML_DISALLOW_COPY_AND_ASSIGN(ClipRRectLayer);
};
Expand Down
2 changes: 1 addition & 1 deletion flow/layers/color_filter_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ void ColorFilterLayer::Preroll(PrerollContext* context,

void ColorFilterLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "ColorFilterLayer::Paint");
FML_DCHECK(needs_painting());
FML_DCHECK(context.needs_painting(paint_bounds()));

SkPaint paint;
paint.setColorFilter(filter_);
Expand Down
15 changes: 9 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 @@ -23,7 +23,7 @@ void ContainerLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
}

void ContainerLayer::Paint(PaintContext& context) const {
FML_DCHECK(needs_painting());
FML_DCHECK(context.needs_painting(paint_bounds()));

PaintChildren(context);
}
Expand All @@ -49,7 +49,9 @@ void ContainerLayer::PrerollChildren(PrerollContext* context,
// sibling tree.
context->has_platform_view = false;

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

if (layer->needs_system_composite()) {
set_needs_system_composite(true);
Expand All @@ -72,12 +74,12 @@ void ContainerLayer::PrerollChildren(PrerollContext* context,
}

void ContainerLayer::PaintChildren(PaintContext& context) const {
FML_DCHECK(needs_painting());
FML_DCHECK(context.needs_painting(paint_bounds()));

// Intentionally not tracing here as there should be no self-time
// and the trace event on this common function has a small overhead.
for (auto& layer : layers_) {
if (layer->needs_painting()) {
if (context.needs_painting(layer->paint_bounds())) {
layer->Paint(context);
}
}
Expand All @@ -86,8 +88,9 @@ 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/image_filter_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ void ImageFilterLayer::Preroll(PrerollContext* context,

void ImageFilterLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "ImageFilterLayer::Paint");
FML_DCHECK(needs_painting());
FML_DCHECK(context.needs_painting(paint_bounds()));

if (context.raster_cache) {
if (context.raster_cache->Draw(this, *context.leaf_nodes_canvas)) {
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_(SkRect::MakeEmpty()),
: paint_bounds_(kUnvisitedRect),
unique_id_(NextUniqueID()),
needs_system_composite_(false) {}

Expand Down
7 changes: 6 additions & 1 deletion flow/layers/layer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
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 @@ -122,6 +123,10 @@ class Layer {
const RasterCache* raster_cache;
const bool checkerboard_offscreen_layers;
const float frame_device_pixel_ratio;

bool needs_painting(const SkRect& paint_bounds) {
return !internal_nodes_canvas->quickReject(paint_bounds);
Copy link
Contributor

@liyuqian liyuqian Nov 10, 2020

Choose a reason for hiding this comment

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

What's the difference between the paint_bounds argument, and the paint_bounds_ member variable? I thought they should agree with each other?

Copy link
Contributor Author

@flar flar Nov 10, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@flar flar Nov 12, 2020

Choose a reason for hiding this comment

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

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.

}
};

// Calls SkCanvas::saveLayer and restores the layer upon destruction. Also
Expand Down Expand Up @@ -171,7 +176,7 @@ 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 { return !paint_bounds_.isEmpty(); }
bool needs_painting() const { FML_DCHECK(false); return false; }

uint64_t unique_id() const { return unique_id_; }

Expand Down
4 changes: 2 additions & 2 deletions flow/layers/layer_tree.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ void LayerTree::Paint(CompositorContext::ScopedFrame& frame,
checkerboard_offscreen_layers_,
device_pixel_ratio_};

if (root_layer_->needs_painting()) {
if (context.needs_painting(root_layer_->paint_bounds())) {
root_layer_->Paint(context);
}
}
Expand Down Expand Up @@ -176,7 +176,7 @@ sk_sp<SkPicture> LayerTree::Flatten(const SkRect& bounds) {
if (root_layer_) {
root_layer_->Preroll(&preroll_context, root_surface_transformation);
// The needs painting flag may be set after the preroll. So check it after.
if (root_layer_->needs_painting()) {
if (paint_context.needs_painting(root_layer_->paint_bounds())) {
root_layer_->Paint(paint_context);
}
}
Expand Down
2 changes: 1 addition & 1 deletion flow/layers/opacity_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ void OpacityLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {

void OpacityLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "OpacityLayer::Paint");
FML_DCHECK(needs_painting());
FML_DCHECK(context.needs_painting(paint_bounds()));

SkPaint paint;
paint.setAlpha(alpha_);
Expand Down
2 changes: 1 addition & 1 deletion flow/layers/physical_shape_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ void PhysicalShapeLayer::Preroll(PrerollContext* context,

void PhysicalShapeLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "PhysicalShapeLayer::Paint");
FML_DCHECK(needs_painting());
FML_DCHECK(context.needs_painting(paint_bounds()));

if (elevation_ != 0) {
DrawShadow(context.leaf_nodes_canvas, path_, shadow_color_, elevation_,
Expand Down
2 changes: 1 addition & 1 deletion flow/layers/picture_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ void PictureLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
void PictureLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "PictureLayer::Paint");
FML_DCHECK(picture_.get());
FML_DCHECK(needs_painting());
FML_DCHECK(context.needs_painting(paint_bounds()));

SkAutoCanvasRestore save(context.leaf_nodes_canvas, true);
context.leaf_nodes_canvas->translate(offset_.x(), offset_.y());
Expand Down
2 changes: 1 addition & 1 deletion flow/layers/shader_mask_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ void ShaderMaskLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {

void ShaderMaskLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "ShaderMaskLayer::Paint");
FML_DCHECK(needs_painting());
FML_DCHECK(context.needs_painting(paint_bounds()));

Layer::AutoSaveLayer save =
Layer::AutoSaveLayer::Create(context, paint_bounds(), nullptr);
Expand Down
1 change: 1 addition & 0 deletions flow/layers/texture_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ void TextureLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {

void TextureLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "TextureLayer::Paint");
FML_DCHECK(context.needs_painting(paint_bounds()));

std::shared_ptr<Texture> texture =
context.texture_registry.GetTexture(texture_id_);
Expand Down
2 changes: 1 addition & 1 deletion flow/layers/transform_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ void TransformLayer::UpdateScene(std::shared_ptr<SceneUpdateContext> context) {

void TransformLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "TransformLayer::Paint");
FML_DCHECK(needs_painting());
FML_DCHECK(context.needs_painting(paint_bounds()));

SkAutoCanvasRestore save(context.internal_nodes_canvas, true);
context.internal_nodes_canvas->concat(transform_);
Expand Down
2 changes: 1 addition & 1 deletion flow/raster_cache.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ std::unique_ptr<RasterCacheResult> RasterCache::RasterizeLayer(
context->has_platform_view ? nullptr : context->raster_cache,
context->checkerboard_offscreen_layers,
context->frame_device_pixel_ratio};
if (layer->needs_painting()) {
if (paintContext.needs_painting(layer->paint_bounds())) {
layer->Paint(paintContext);
}
});
Expand Down