From 7e8fc6005e845f54564e59d0d26f73cb1ca87e88 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Tue, 2 Nov 2021 23:40:48 +0100 Subject: [PATCH] Fix partial repaint when TextureLayer is inside retained layer (flutter/engine#29482) --- engine/src/flutter/flow/diff_context.cc | 17 ++++++++++++-- engine/src/flutter/flow/diff_context.h | 7 ++++++ .../flutter/flow/layers/container_layer.cc | 3 ++- .../src/flutter/flow/layers/texture_layer.cc | 7 ++++++ .../flow/layers/texture_layer_unittests.cc | 22 +++++++++++++++++++ engine/src/flutter/flow/paint_region.h | 14 ++++++++++-- 6 files changed, 65 insertions(+), 5 deletions(-) diff --git a/engine/src/flutter/flow/diff_context.cc b/engine/src/flutter/flow/diff_context.cc index a0a8c4460f..c9782594af 100644 --- a/engine/src/flutter/flow/diff_context.cc +++ b/engine/src/flutter/flow/diff_context.cc @@ -21,6 +21,7 @@ void DiffContext::BeginSubtree() { state_stack_.push_back(state_); state_.rect_index_ = rects_->size(); state_.has_filter_bounds_adjustment = false; + state_.has_texture = false; if (state_.transform_override) { state_.transform = *state_.transform_override; state_.transform_override = std::nullopt; @@ -40,7 +41,8 @@ DiffContext::State::State() : dirty(false), cull_rect(kGiantRect), rect_index_(0), - has_filter_bounds_adjustment(false) {} + has_filter_bounds_adjustment(false), + has_texture(false) {} void DiffContext::PushTransform(const SkMatrix& transform) { state_.transform.preConcat(transform); @@ -140,6 +142,16 @@ void DiffContext::AddLayerBounds(const SkRect& rect) { } } +void DiffContext::MarkSubtreeHasTextureLayer() { + // 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_texture = true; +} + void DiffContext::AddExistingPaintRegion(const PaintRegion& region) { // Adding paint region for retained layer implies that current subtree is not // dirty, so we know, for example, that the inherited transforms must match @@ -162,7 +174,8 @@ PaintRegion DiffContext::CurrentSubtreeRegion() const { bool has_readback = std::any_of( readbacks_.begin(), readbacks_.end(), [&](const Readback& r) { return r.position >= state_.rect_index_; }); - return PaintRegion(rects_, state_.rect_index_, rects_->size(), has_readback); + return PaintRegion(rects_, state_.rect_index_, rects_->size(), has_readback, + state_.has_texture); } void DiffContext::AddDamage(const PaintRegion& damage) { diff --git a/engine/src/flutter/flow/diff_context.h b/engine/src/flutter/flow/diff_context.h index e1e8d55701..b57df7a946 100644 --- a/engine/src/flutter/flow/diff_context.h +++ b/engine/src/flutter/flow/diff_context.h @@ -104,6 +104,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(); + // Add layer bounds to current paint region; rect is in "local" (layer) // coordinates. void AddLayerBounds(const SkRect& rect); @@ -203,6 +207,9 @@ class DiffContext { // Whether this subtree has filter bounds adjustment function. If so, // it will need to be removed from stack when subtree is closed. bool has_filter_bounds_adjustment; + + // Whether there is a texture layer in this subtree. + bool has_texture; }; std::shared_ptr> rects_; diff --git a/engine/src/flutter/flow/layers/container_layer.cc b/engine/src/flutter/flow/layers/container_layer.cc index c78636c4fe..552d413f26 100644 --- a/engine/src/flutter/flow/layers/container_layer.cc +++ b/engine/src/flutter/flow/layers/container_layer.cc @@ -77,7 +77,8 @@ void ContainerLayer::DiffChildren(DiffContext* context, auto layer = layers_[i]; auto prev_layer = prev_layers[i_prev]; auto paint_region = context->GetOldLayerPaintRegion(prev_layer.get()); - if (layer == prev_layer && !paint_region.has_readback()) { + if (layer == prev_layer && !paint_region.has_readback() && + !paint_region.has_texture()) { // 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/engine/src/flutter/flow/layers/texture_layer.cc b/engine/src/flutter/flow/layers/texture_layer.cc index 7f169345a2..e360b50d55 100644 --- a/engine/src/flutter/flow/layers/texture_layer.cc +++ b/engine/src/flutter/flow/layers/texture_layer.cc @@ -28,6 +28,13 @@ void TextureLayer::Diff(DiffContext* context, const Layer* old_layer) { // dirty context->MarkSubtreeDirty(context->GetOldLayerPaintRegion(prev)); } + + // Make sure DiffContext knows there is a TextureLayer in this subtree. + // This prevents ContainerLayer from skipping TextureLayer diffing when + // TextureLayer is inside retained layer. + // See ContainerLayer::DiffChildren + // https://github.com/flutter/flutter/issues/92925 + context->MarkSubtreeHasTextureLayer(); context->AddLayerBounds(SkRect::MakeXYWH(offset_.x(), offset_.y(), size_.width(), size_.height())); context->SetLayerPaintRegion(this, context->CurrentSubtreeRegion()); diff --git a/engine/src/flutter/flow/layers/texture_layer_unittests.cc b/engine/src/flutter/flow/layers/texture_layer_unittests.cc index 4f6adf9944..4d251120ef 100644 --- a/engine/src/flutter/flow/layers/texture_layer_unittests.cc +++ b/engine/src/flutter/flow/layers/texture_layer_unittests.cc @@ -4,6 +4,7 @@ #include "flutter/flow/layers/texture_layer.h" +#include "flutter/flow/testing/diff_context_test.h" #include "flutter/flow/testing/layer_test.h" #include "flutter/flow/testing/mock_layer.h" #include "flutter/flow/testing/mock_texture.h" @@ -94,5 +95,26 @@ TEST_F(TextureLayerTest, PaintingWithLinearSampling) { EXPECT_EQ(mock_canvas().draw_calls(), std::vector()); } +using TextureLayerDiffTest = DiffContextTest; + +TEST_F(TextureLayerDiffTest, TextureInRetainedLayer) { + MockLayerTree tree1; + auto container = std::make_shared(); + tree1.root()->Add(container); + auto layer = std::make_shared( + SkPoint::Make(0, 0), SkSize::Make(100, 100), 0, false, + SkSamplingOptions(SkFilterMode::kLinear)); + container->Add(layer); + + MockLayerTree tree2; + tree2.root()->Add(container); // retained layer + + auto damage = DiffLayerTree(tree1, MockLayerTree()); + EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(0, 0, 100, 100)); + + damage = DiffLayerTree(tree2, tree1); + EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(0, 0, 100, 100)); +} + } // namespace testing } // namespace flutter diff --git a/engine/src/flutter/flow/paint_region.h b/engine/src/flutter/flow/paint_region.h index 929a7b35db..1fd8896b68 100644 --- a/engine/src/flutter/flow/paint_region.h +++ b/engine/src/flutter/flow/paint_region.h @@ -25,8 +25,13 @@ class PaintRegion { PaintRegion(std::shared_ptr> rects, size_t from, size_t to, - bool has_readback) - : rects_(rects), from_(from), to_(to), has_readback_(has_readback) {} + bool has_readback, + bool has_texture) + : rects_(rects), + from_(from), + to_(to), + has_readback_(has_readback), + has_texture_(has_texture) {} std::vector::const_iterator begin() const { FML_DCHECK(is_valid()); @@ -47,11 +52,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_; } + private: std::shared_ptr> rects_; size_t from_ = 0; size_t to_ = 0; bool has_readback_ = false; + bool has_texture_ = false; }; } // namespace flutter