From 02bee3abf09c1ea8f235073b3f5a8a5ea5c884fe Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Tue, 18 Jul 2023 10:21:42 -0700 Subject: [PATCH] [Impeller] Avoid inserting additional save layers based on clip configuration. (flutter/engine#43759) Fixes https://github.com/flutter/flutter/issues/130775 On the Skia backend, antiAliasWithSaveLayer is the highest fidelity clipping option. In the Impeller backend, there isn't any difference in how we clip, since the stencil buffer is always used. Nevertheless we were still inserting the save layer, which results in an extra offscreen texture and is wasteful. Track if impeller is enabled in the diff/preroll/paint context and avoid inserting a save layer. --- engine/src/flutter/flow/compositor_context.cc | 8 ++-- engine/src/flutter/flow/compositor_context.h | 3 +- engine/src/flutter/flow/diff_context.cc | 6 ++- engine/src/flutter/flow/diff_context.h | 6 ++- .../flow/layers/clip_rrect_layer_unittests.cc | 45 +++++++++++++++++++ .../flutter/flow/layers/clip_shape_layer.h | 15 ++++--- engine/src/flutter/flow/layers/layer.h | 3 ++ engine/src/flutter/flow/layers/layer_tree.cc | 2 + .../flutter/flow/testing/diff_context_test.cc | 6 ++- .../flutter/flow/testing/diff_context_test.h | 3 +- engine/src/flutter/flow/testing/layer_test.h | 6 +++ 11 files changed, 88 insertions(+), 15 deletions(-) diff --git a/engine/src/flutter/flow/compositor_context.cc b/engine/src/flutter/flow/compositor_context.cc index 446cf968f7f..2c7593c1625 100644 --- a/engine/src/flutter/flow/compositor_context.cc +++ b/engine/src/flutter/flow/compositor_context.cc @@ -13,13 +13,14 @@ namespace flutter { std::optional FrameDamage::ComputeClipRect( flutter::LayerTree& layer_tree, - bool has_raster_cache) { + bool has_raster_cache, + bool impeller_enabled) { if (layer_tree.root_layer()) { PaintRegionMap empty_paint_region_map; DiffContext context(layer_tree.frame_size(), layer_tree.paint_region_map(), prev_layer_tree_ ? prev_layer_tree_->paint_region_map() : empty_paint_region_map, - has_raster_cache); + has_raster_cache, impeller_enabled); context.PushCullRect(SkRect::MakeIWH(layer_tree.frame_size().width(), layer_tree.frame_size().height())); { @@ -121,7 +122,8 @@ RasterStatus CompositorContext::ScopedFrame::Raster( std::optional clip_rect; if (frame_damage) { - clip_rect = frame_damage->ComputeClipRect(layer_tree, !ignore_raster_cache); + clip_rect = frame_damage->ComputeClipRect(layer_tree, !ignore_raster_cache, + !gr_context_); if (aiks_context_ && !ShouldPerformPartialRepaint(clip_rect, layer_tree.frame_size())) { diff --git a/engine/src/flutter/flow/compositor_context.h b/engine/src/flutter/flow/compositor_context.h index 646ca2f9c64..30aae4736c9 100644 --- a/engine/src/flutter/flow/compositor_context.h +++ b/engine/src/flutter/flow/compositor_context.h @@ -82,7 +82,8 @@ class FrameDamage { // but the paint region of layer_tree will be calculated so that it can be // used for diffing of subsequent frames. std::optional ComputeClipRect(flutter::LayerTree& layer_tree, - bool has_raster_cache); + bool has_raster_cache, + bool impeller_enabled); // See Damage::frame_damage. std::optional GetFrameDamage() const { diff --git a/engine/src/flutter/flow/diff_context.cc b/engine/src/flutter/flow/diff_context.cc index 160b5c899fa..612fe563fa9 100644 --- a/engine/src/flutter/flow/diff_context.cc +++ b/engine/src/flutter/flow/diff_context.cc @@ -10,13 +10,15 @@ namespace flutter { DiffContext::DiffContext(SkISize frame_size, PaintRegionMap& this_frame_paint_region_map, const PaintRegionMap& last_frame_paint_region_map, - bool has_raster_cache) + bool has_raster_cache, + bool impeller_enabled) : clip_tracker_(DisplayListMatrixClipTracker(kGiantRect, SkMatrix::I())), rects_(std::make_shared>()), frame_size_(frame_size), this_frame_paint_region_map_(this_frame_paint_region_map), last_frame_paint_region_map_(last_frame_paint_region_map), - has_raster_cache_(has_raster_cache) {} + has_raster_cache_(has_raster_cache), + impeller_enabled_(impeller_enabled) {} void DiffContext::BeginSubtree() { state_stack_.push_back(state_); diff --git a/engine/src/flutter/flow/diff_context.h b/engine/src/flutter/flow/diff_context.h index e273fdb25d9..029fb49f933 100644 --- a/engine/src/flutter/flow/diff_context.h +++ b/engine/src/flutter/flow/diff_context.h @@ -46,7 +46,8 @@ class DiffContext { explicit DiffContext(SkISize frame_size, PaintRegionMap& this_frame_paint_region_map, const PaintRegionMap& last_frame_paint_region_map, - bool has_raster_cache); + bool has_raster_cache, + bool impeller_enabled); // Starts a new subtree. void BeginSubtree(); @@ -161,6 +162,8 @@ class DiffContext { // cached. bool has_raster_cache() const { return has_raster_cache_; } + bool impeller_enabled() const { return impeller_enabled_; } + class Statistics { public: // Picture replaced by different picture @@ -245,6 +248,7 @@ class DiffContext { PaintRegionMap& this_frame_paint_region_map_; const PaintRegionMap& last_frame_paint_region_map_; bool has_raster_cache_; + bool impeller_enabled_; void AddDamage(const SkRect& rect); diff --git a/engine/src/flutter/flow/layers/clip_rrect_layer_unittests.cc b/engine/src/flutter/flow/layers/clip_rrect_layer_unittests.cc index 1c3672d7cc6..98d6493ba7b 100644 --- a/engine/src/flutter/flow/layers/clip_rrect_layer_unittests.cc +++ b/engine/src/flutter/flow/layers/clip_rrect_layer_unittests.cc @@ -595,6 +595,51 @@ TEST_F(ClipRRectLayerTest, EmptyClipDoesNotCullPlatformView) { EXPECT_EQ(embedder.painted_views(), std::vector({view_id})); } +TEST_F(ClipRRectLayerTest, AntiAliasWithSaveLayerIgnoresSaveLayerImpeller) { + enable_impeller(); + + auto path1 = SkPath().addRect({10, 10, 30, 30}); + auto mock1 = MockLayer::MakeOpacityCompatible(path1); + auto path2 = SkPath().addRect({20, 20, 40, 40}); + auto mock2 = MockLayer::MakeOpacityCompatible(path2); + auto children_bounds = path1.getBounds(); + children_bounds.join(path2.getBounds()); + SkRect clip_rect = SkRect::MakeWH(500, 500); + SkRRect clip_rrect = SkRRect::MakeRectXY(clip_rect, 20, 20); + auto clip_rrect_layer = std::make_shared( + clip_rrect, Clip::antiAliasWithSaveLayer); + clip_rrect_layer->Add(mock1); + clip_rrect_layer->Add(mock2); + + // ClipRectLayer will pass through compatibility from multiple + // non-overlapping compatible children + PrerollContext* context = preroll_context(); + clip_rrect_layer->Preroll(context); + EXPECT_EQ(context->renderable_state_flags, 0); + + DisplayListBuilder expected_builder; + /* OpacityLayer::Paint() */ { + expected_builder.Save(); + { + /* ClipRectLayer::Paint() */ { + expected_builder.Save(); + expected_builder.ClipRRect(clip_rrect, ClipOp::kIntersect, true); + /* child layer1 paint */ { + expected_builder.DrawPath(path1, DlPaint()); + } + /* child layer2 paint */ { // + expected_builder.DrawPath(path2, DlPaint()); + } + // expected_builder.Restore(); + } + } + expected_builder.Restore(); + } + + clip_rrect_layer->Paint(display_list_paint_context()); + EXPECT_TRUE(DisplayListsEQ_Verbose(expected_builder.Build(), display_list())); +} + } // namespace testing } // namespace flutter diff --git a/engine/src/flutter/flow/layers/clip_shape_layer.h b/engine/src/flutter/flow/layers/clip_shape_layer.h index 69c479fe61a..d33267853e3 100644 --- a/engine/src/flutter/flow/layers/clip_shape_layer.h +++ b/engine/src/flutter/flow/layers/clip_shape_layer.h @@ -32,7 +32,8 @@ class ClipShapeLayer : public CacheableContainerLayer { context->MarkSubtreeDirty(context->GetOldLayerPaintRegion(old_layer)); } } - if (UsesSaveLayer() && context->has_raster_cache()) { + if (UsesSaveLayer(context->impeller_enabled()) && + context->has_raster_cache()) { context->WillPaintWithIntegralTransform(); } if (context->PushCullRect(clip_shape_bounds())) { @@ -42,7 +43,7 @@ class ClipShapeLayer : public CacheableContainerLayer { } void Preroll(PrerollContext* context) override { - bool uses_save_layer = UsesSaveLayer(); + bool uses_save_layer = UsesSaveLayer(context->impeller_enabled); // We can use the raster_cache for children only when the use_save_layer is // true so if use_save_layer is false we pass the layer_raster_item is @@ -52,7 +53,8 @@ class ClipShapeLayer : public CacheableContainerLayer { context, context->state_stack.transform_3x3()); Layer::AutoPrerollSaveLayerState save = - Layer::AutoPrerollSaveLayerState::Create(context, UsesSaveLayer()); + Layer::AutoPrerollSaveLayerState::Create( + context, UsesSaveLayer(context->impeller_enabled)); auto mutator = context->state_stack.save(); ApplyClip(mutator); @@ -78,7 +80,7 @@ class ClipShapeLayer : public CacheableContainerLayer { auto mutator = context.state_stack.save(); ApplyClip(mutator); - if (!UsesSaveLayer()) { + if (!UsesSaveLayer(context.impeller_enabled)) { PaintChildren(context); return; } @@ -99,7 +101,10 @@ class ClipShapeLayer : public CacheableContainerLayer { PaintChildren(context); } - bool UsesSaveLayer() const { + bool UsesSaveLayer(bool enable_impeller) const { + if (enable_impeller) { + return false; + } return clip_behavior_ == Clip::antiAliasWithSaveLayer; } diff --git a/engine/src/flutter/flow/layers/layer.h b/engine/src/flutter/flow/layers/layer.h index 04a501284bb..eaae9d4279a 100644 --- a/engine/src/flutter/flow/layers/layer.h +++ b/engine/src/flutter/flow/layers/layer.h @@ -71,6 +71,8 @@ struct PrerollContext { // presence of a texture layer during Preroll. bool has_texture_layer = false; + bool impeller_enabled = false; + // The list of flags that describe which rendering state attributes // (such as opacity, ColorFilter, ImageFilter) a given layer can // render itself without requiring the parent to perform a protective @@ -117,6 +119,7 @@ struct PaintContext { // only when leaf layer tracing is enabled. LayerSnapshotStore* layer_snapshot_store = nullptr; bool enable_leaf_layer_tracing = false; + bool impeller_enabled = false; impeller::AiksContext* aiks_context; }; diff --git a/engine/src/flutter/flow/layers/layer_tree.cc b/engine/src/flutter/flow/layers/layer_tree.cc index bb7492a83fc..e8aea74efd7 100644 --- a/engine/src/flutter/flow/layers/layer_tree.cc +++ b/engine/src/flutter/flow/layers/layer_tree.cc @@ -60,6 +60,7 @@ bool LayerTree::Preroll(CompositorContext::ScopedFrame& frame, .raster_time = frame.context().raster_time(), .ui_time = frame.context().ui_time(), .texture_registry = frame.context().texture_registry(), + .impeller_enabled = !frame.gr_context(), .raster_cached_entries = &raster_cache_items_, // clang-format on }; @@ -139,6 +140,7 @@ void LayerTree::Paint(CompositorContext::ScopedFrame& frame, .raster_cache = cache, .layer_snapshot_store = snapshot_store, .enable_leaf_layer_tracing = enable_leaf_layer_tracing_, + .impeller_enabled = !!frame.aiks_context(), .aiks_context = frame.aiks_context(), // clang-format on }; diff --git a/engine/src/flutter/flow/testing/diff_context_test.cc b/engine/src/flutter/flow/testing/diff_context_test.cc index 20153a0140d..5a277b9cccd 100644 --- a/engine/src/flutter/flow/testing/diff_context_test.cc +++ b/engine/src/flutter/flow/testing/diff_context_test.cc @@ -17,11 +17,13 @@ Damage DiffContextTest::DiffLayerTree(MockLayerTree& layer_tree, const SkIRect& additional_damage, int horizontal_clip_alignment, int vertical_clip_alignment, - bool use_raster_cache) { + bool use_raster_cache, + bool impeller_enabled) { FML_CHECK(layer_tree.size() == old_layer_tree.size()); DiffContext dc(layer_tree.size(), layer_tree.paint_region_map(), - old_layer_tree.paint_region_map(), use_raster_cache); + old_layer_tree.paint_region_map(), use_raster_cache, + impeller_enabled); dc.PushCullRect( SkRect::MakeIWH(layer_tree.size().width(), layer_tree.size().height())); layer_tree.root()->Diff(&dc, old_layer_tree.root()); diff --git a/engine/src/flutter/flow/testing/diff_context_test.h b/engine/src/flutter/flow/testing/diff_context_test.h index 297a2bdf5f7..4fb3baafbc7 100644 --- a/engine/src/flutter/flow/testing/diff_context_test.h +++ b/engine/src/flutter/flow/testing/diff_context_test.h @@ -41,7 +41,8 @@ class DiffContextTest : public LayerTest { const SkIRect& additional_damage = SkIRect::MakeEmpty(), int horizontal_clip_alignment = 0, int vertical_alignment = 0, - bool use_raster_cache = true); + bool use_raster_cache = true, + bool impeller_enabled = false); // Create display list consisting of filled rect with given color; Being able // to specify different color is useful to test deep comparison of pictures diff --git a/engine/src/flutter/flow/testing/layer_test.h b/engine/src/flutter/flow/testing/layer_test.h index e74ef6fcdda..178c307426c 100644 --- a/engine/src/flutter/flow/testing/layer_test.h +++ b/engine/src/flutter/flow/testing/layer_test.h @@ -195,6 +195,12 @@ class LayerTestBase : public CanvasTestBase { paint_context_.layer_snapshot_store = nullptr; } + void enable_impeller() { + preroll_context_.impeller_enabled = true; + paint_context_.impeller_enabled = true; + display_list_paint_context_.impeller_enabled = true; + } + private: void set_raster_cache_(std::unique_ptr raster_cache) { raster_cache_ = std::move(raster_cache);