From b2872478130ac1b8870a08145fc0b2a13d356e5b Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Tue, 30 Jul 2024 17:47:26 +0200 Subject: [PATCH 1/3] Do not disable partial repaint based on thread merging state --- flow/diff_context.cc | 14 ++++++++----- flow/diff_context.h | 12 +++++++---- flow/layers/container_layer.cc | 2 +- flow/layers/platform_view_layer.cc | 11 ++++++++++ flow/layers/platform_view_layer.h | 1 + flow/layers/platform_view_layer_unittests.cc | 21 ++++++++++++++++++++ flow/layers/texture_layer.cc | 2 +- flow/paint_region.h | 12 +++++------ shell/common/rasterizer.cc | 10 +--------- 9 files changed, 59 insertions(+), 26 deletions(-) diff --git a/flow/diff_context.cc b/flow/diff_context.cc index 641b06588ba68..f505c0f863a3c 100644 --- a/flow/diff_context.cc +++ b/flow/diff_context.cc @@ -27,7 +27,7 @@ void DiffContext::BeginSubtree() { bool had_integral_transform = state_.integral_transform; state_.rect_index = rects_->size(); state_.has_filter_bounds_adjustment = false; - state_.has_texture = false; + state_.has_volatile_layer = false; state_.integral_transform = false; if (had_integral_transform) { @@ -162,6 +162,10 @@ bool DiffContext::PushCullRect(const SkRect& clip) { return !state_.matrix_clip.device_cull_rect().isEmpty(); } +void DiffContext::ForceFullRepaint() { + damage_ = SkRect::MakeIWH(frame_size_.width(), frame_size_.height()); +} + SkMatrix DiffContext::GetTransform3x3() const { return state_.matrix_clip.matrix_3x3(); } @@ -203,14 +207,14 @@ void DiffContext::AddLayerBounds(const SkRect& rect) { } } -void DiffContext::MarkSubtreeHasTextureLayer() { +void DiffContext::MarkSubtreeHasVolatileLayer() { // Set the has_texture flag on current state and all parent states. That // way we'll know that we can't skip diff for retained layers because // they contain a TextureLayer. for (auto& state : state_stack_) { - state.has_texture = true; + state.has_volatile_layer = true; } - state_.has_texture = true; + state_.has_volatile_layer = true; } void DiffContext::AddExistingPaintRegion(const PaintRegion& region) { @@ -238,7 +242,7 @@ PaintRegion DiffContext::CurrentSubtreeRegion() const { readbacks_.begin(), readbacks_.end(), [&](const Readback& r) { return r.position >= state_.rect_index; }); return PaintRegion(rects_, state_.rect_index, rects_->size(), has_readback, - state_.has_texture); + state_.has_volatile_layer); } void DiffContext::AddDamage(const PaintRegion& damage) { diff --git a/flow/diff_context.h b/flow/diff_context.h index 0eb536c49cbe4..1e0f34ae14db5 100644 --- a/flow/diff_context.h +++ b/flow/diff_context.h @@ -77,6 +77,9 @@ class DiffContext { // Pushes cull rect for current subtree bool PushCullRect(const SkRect& clip); + // Marks entire frame as dirty. + void ForceFullRepaint(); + // Function that adjusts layer bounds (in device coordinates) depending // on filter. using FilterBoundsAdjustment = std::function; @@ -107,9 +110,10 @@ class DiffContext { bool IsSubtreeDirty() const { return state_.dirty; } - // Marks that current subtree contains a TextureLayer. This is needed to - // ensure that we'll Diff the TextureLayer even if inside retained layer. - void MarkSubtreeHasTextureLayer(); + // Marks that current subtree contains a volatile layer. A volatile layer will + // do diffing even if it is a retained subtree. Necessary for TextureLayer + // and PlatformViewLayer. + void MarkSubtreeHasVolatileLayer(); // Add layer bounds to current paint region; rect is in "local" (layer) // coordinates. @@ -231,7 +235,7 @@ class DiffContext { bool has_filter_bounds_adjustment = false; // Whether there is a texture layer in this subtree. - bool has_texture = false; + bool has_volatile_layer = false; }; void MakeTransformIntegral(DisplayListMatrixClipState& matrix_clip); diff --git a/flow/layers/container_layer.cc b/flow/layers/container_layer.cc index 74829417f2432..384559c5c2ab2 100644 --- a/flow/layers/container_layer.cc +++ b/flow/layers/container_layer.cc @@ -78,7 +78,7 @@ void ContainerLayer::DiffChildren(DiffContext* context, auto prev_layer = prev_layers[i_prev]; auto paint_region = context->GetOldLayerPaintRegion(prev_layer.get()); if (layer == prev_layer && !paint_region.has_readback() && - !paint_region.has_texture()) { + !paint_region.has_volatile_layer()) { // for retained layers, stop processing the subtree and add existing // region; We know current subtree is not dirty (every ancestor up to // here matches) so the retained subtree will render identically to diff --git a/flow/layers/platform_view_layer.cc b/flow/layers/platform_view_layer.cc index d641a12665d38..376981ffde94f 100644 --- a/flow/layers/platform_view_layer.cc +++ b/flow/layers/platform_view_layer.cc @@ -13,6 +13,17 @@ PlatformViewLayer::PlatformViewLayer(const SkPoint& offset, int64_t view_id) : offset_(offset), size_(size), view_id_(view_id) {} +void PlatformViewLayer::Diff(DiffContext* context, const Layer* old_layer) { + DiffContext::AutoSubtreeRestore subtree(context); + // Ensure that Diff is called again even if this layer is in retained subtree. + context->MarkSubtreeHasVolatileLayer(); + // It is not necessary to track the actual paint region for the layer due to + // forced full repaint below, but the paint region carries the volatile layer + // flag. + context->SetLayerPaintRegion(this, context->CurrentSubtreeRegion()); + context->ForceFullRepaint(); +} + void PlatformViewLayer::Preroll(PrerollContext* context) { set_paint_bounds(SkRect::MakeXYWH(offset_.x(), offset_.y(), size_.width(), size_.height())); diff --git a/flow/layers/platform_view_layer.h b/flow/layers/platform_view_layer.h index ac5792af55872..360a0d01f9117 100644 --- a/flow/layers/platform_view_layer.h +++ b/flow/layers/platform_view_layer.h @@ -16,6 +16,7 @@ class PlatformViewLayer : public Layer { public: PlatformViewLayer(const SkPoint& offset, const SkSize& size, int64_t view_id); + void Diff(DiffContext* context, const Layer* old_layer) override; void Preroll(PrerollContext* context) override; void Paint(PaintContext& context) const override; diff --git a/flow/layers/platform_view_layer_unittests.cc b/flow/layers/platform_view_layer_unittests.cc index b89447f3a589e..a732740bfd076 100644 --- a/flow/layers/platform_view_layer_unittests.cc +++ b/flow/layers/platform_view_layer_unittests.cc @@ -6,6 +6,7 @@ #include "flutter/flow/layers/platform_view_layer.h" #include "flutter/flow/layers/transform_layer.h" +#include "flutter/flow/testing/diff_context_test.h" #include "flutter/flow/testing/layer_test.h" #include "flutter/flow/testing/mock_embedder.h" #include "flutter/flow/testing/mock_layer.h" @@ -139,5 +140,25 @@ TEST_F(PlatformViewLayerTest, StateTransfer) { transform_layer1->Paint(paint_ctx); } +using PlatformViewLayerDiffTest = DiffContextTest; + +TEST_F(PlatformViewLayerDiffTest, PlatformViewRetainedLayer) { + MockLayerTree tree1(SkISize::Make(800, 600)); + auto container = std::make_shared(); + tree1.root()->Add(container); + auto layer = std::make_shared(SkPoint::Make(100, 100), + SkSize::Make(100, 100), 0); + container->Add(layer); + + MockLayerTree tree2(SkISize::Make(800, 600)); + tree2.root()->Add(container); // retained layer + + auto damage = DiffLayerTree(tree1, MockLayerTree(SkISize::Make(800, 600))); + EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(0, 0, 800, 600)); + + damage = DiffLayerTree(tree2, tree1); + EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(0, 0, 800, 600)); +} + } // namespace testing } // namespace flutter diff --git a/flow/layers/texture_layer.cc b/flow/layers/texture_layer.cc index c36a5eb6274d4..6fa71a9f7ed52 100644 --- a/flow/layers/texture_layer.cc +++ b/flow/layers/texture_layer.cc @@ -34,7 +34,7 @@ void TextureLayer::Diff(DiffContext* context, const Layer* old_layer) { // TextureLayer is inside retained layer. // See ContainerLayer::DiffChildren // https://github.com/flutter/flutter/issues/92925 - context->MarkSubtreeHasTextureLayer(); + context->MarkSubtreeHasVolatileLayer(); context->AddLayerBounds(SkRect::MakeXYWH(offset_.x(), offset_.y(), size_.width(), size_.height())); context->SetLayerPaintRegion(this, context->CurrentSubtreeRegion()); diff --git a/flow/paint_region.h b/flow/paint_region.h index 52f29762f67ad..e034b45821104 100644 --- a/flow/paint_region.h +++ b/flow/paint_region.h @@ -30,12 +30,12 @@ class PaintRegion { size_t from, size_t to, bool has_readback, - bool has_texture) + bool has_volatile_layer) : rects_(std::move(rects)), from_(from), to_(to), has_readback_(has_readback), - has_texture_(has_texture) {} + has_volatile_layer_(has_volatile_layer) {} std::vector::const_iterator begin() const { FML_DCHECK(is_valid()); @@ -56,16 +56,16 @@ class PaintRegion { // that performs readback bool has_readback() const { return has_readback_; } - // Returns whether there is a TextureLayer in subtree represented by this - // region. - bool has_texture() const { return has_texture_; } + // Returns whether there is a TextureLayer or PlatformViewLayer in subtree + // represented by this region. + bool has_volatile_layer() const { return has_volatile_layer_; } private: std::shared_ptr> rects_; size_t from_ = 0; size_t to_ = 0; bool has_readback_ = false; - bool has_texture_ = false; + bool has_volatile_layer_ = false; }; } // namespace flutter diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 6239e22192a79..de0a285a27ae9 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -743,17 +743,9 @@ DrawSurfaceStatus Rasterizer::DrawToSurfaceUnsafe( // when leaf layer tracing is enabled we wish to repaint the whole frame // for accurate performance metrics. if (frame->framebuffer_info().supports_partial_repaint) { - // Disable partial repaint if external_view_embedder_ SubmitFlutterView is - // involved - ExternalViewEmbedder unconditionally clears the entire - // surface and also partial repaint with platform view present is - // something that still need to be figured out. - bool force_full_repaint = - external_view_embedder_ && - (!raster_thread_merger_ || raster_thread_merger_->IsMerged()); - damage = std::make_unique(); auto existing_damage = frame->framebuffer_info().existing_damage; - if (existing_damage.has_value() && !force_full_repaint) { + if (existing_damage.has_value()) { damage->SetPreviousLayerTree(GetLastLayerTree(view_id)); damage->AddAdditionalDamage(existing_damage.value()); damage->SetClipAlignment( From 950e4870f67e68e77d81360243580800ebb04979 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Tue, 30 Jul 2024 23:10:26 +0200 Subject: [PATCH 2/3] Do full repaint after removing platform view --- flow/diff_context.cc | 10 ++++++---- flow/diff_context.h | 6 +++--- flow/layers/platform_view_layer.cc | 8 ++++---- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/flow/diff_context.cc b/flow/diff_context.cc index f505c0f863a3c..18a14cd17675e 100644 --- a/flow/diff_context.cc +++ b/flow/diff_context.cc @@ -162,10 +162,6 @@ bool DiffContext::PushCullRect(const SkRect& clip) { return !state_.matrix_clip.device_cull_rect().isEmpty(); } -void DiffContext::ForceFullRepaint() { - damage_ = SkRect::MakeIWH(frame_size_.width(), frame_size_.height()); -} - SkMatrix DiffContext::GetTransform3x3() const { return state_.matrix_clip.matrix_3x3(); } @@ -207,6 +203,12 @@ void DiffContext::AddLayerBounds(const SkRect& rect) { } } +void DiffContext::RepaintEntireFrame() { + auto frame_rect = SkRect::MakeIWH(frame_size_.width(), frame_size_.height()); + rects_->push_back(frame_rect); + AddDamage(frame_rect); +} + void DiffContext::MarkSubtreeHasVolatileLayer() { // Set the has_texture flag on current state and all parent states. That // way we'll know that we can't skip diff for retained layers because diff --git a/flow/diff_context.h b/flow/diff_context.h index 1e0f34ae14db5..539d5f73cec70 100644 --- a/flow/diff_context.h +++ b/flow/diff_context.h @@ -77,9 +77,6 @@ class DiffContext { // Pushes cull rect for current subtree bool PushCullRect(const SkRect& clip); - // Marks entire frame as dirty. - void ForceFullRepaint(); - // Function that adjusts layer bounds (in device coordinates) depending // on filter. using FilterBoundsAdjustment = std::function; @@ -119,6 +116,9 @@ class DiffContext { // coordinates. void AddLayerBounds(const SkRect& rect); + // Marks entire frame as dirty. + void RepaintEntireFrame(); + // Add entire paint region of retained layer for current subtree. This can // only be used in subtrees that are not dirty, otherwise ancestor transforms // or clips may result in different paint region. diff --git a/flow/layers/platform_view_layer.cc b/flow/layers/platform_view_layer.cc index 376981ffde94f..38b816d31660c 100644 --- a/flow/layers/platform_view_layer.cc +++ b/flow/layers/platform_view_layer.cc @@ -17,11 +17,11 @@ void PlatformViewLayer::Diff(DiffContext* context, const Layer* old_layer) { DiffContext::AutoSubtreeRestore subtree(context); // Ensure that Diff is called again even if this layer is in retained subtree. context->MarkSubtreeHasVolatileLayer(); - // It is not necessary to track the actual paint region for the layer due to - // forced full repaint below, but the paint region carries the volatile layer - // flag. + // Partial repaint is disabled when platform view is present. This will also + // set whole frame as the layer paint region, which will ensure full repaint + // when the layer is removed. + context->RepaintEntireFrame(); context->SetLayerPaintRegion(this, context->CurrentSubtreeRegion()); - context->ForceFullRepaint(); } void PlatformViewLayer::Preroll(PrerollContext* context) { From 121941f77a089a36aca3fd68ba3d928f46aab503 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Tue, 30 Jul 2024 23:16:22 +0200 Subject: [PATCH 3/3] Add test --- flow/layers/platform_view_layer_unittests.cc | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/flow/layers/platform_view_layer_unittests.cc b/flow/layers/platform_view_layer_unittests.cc index a732740bfd076..97e1ee9ce7383 100644 --- a/flow/layers/platform_view_layer_unittests.cc +++ b/flow/layers/platform_view_layer_unittests.cc @@ -160,5 +160,25 @@ TEST_F(PlatformViewLayerDiffTest, PlatformViewRetainedLayer) { EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(0, 0, 800, 600)); } +TEST_F(PlatformViewLayerDiffTest, FullRepaintAfterRemovingLayer) { + MockLayerTree tree1(SkISize::Make(800, 600)); + auto container = std::make_shared(); + tree1.root()->Add(container); + auto layer = std::make_shared(SkPoint::Make(100, 100), + SkSize::Make(100, 100), 0); + container->Add(layer); + + auto damage = DiffLayerTree(tree1, MockLayerTree(SkISize::Make(800, 600))); + EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(0, 0, 800, 600)); + + // Second layer tree with the PlatformViewLayer removed. + MockLayerTree tree2(SkISize::Make(800, 600)); + auto container2 = std::make_shared(); + tree2.root()->Add(container2); + + damage = DiffLayerTree(tree2, tree1); + EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(0, 0, 800, 600)); +} + } // namespace testing } // namespace flutter