From 2c24669e122087d90a5e66ffa675b146f9e6815e Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Tue, 10 Oct 2023 03:19:52 -0700 Subject: [PATCH 1/6] [Impeller] Don't cull readbacks outside damage rects. --- flow/diff_context.cc | 8 ++------ flow/layers/backdrop_filter_layer_unittests.cc | 3 +-- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/flow/diff_context.cc b/flow/diff_context.cc index 612fe563fa9c6..7b9fb7e081518 100644 --- a/flow/diff_context.cc +++ b/flow/diff_context.cc @@ -124,12 +124,8 @@ Damage DiffContext::ComputeDamage(const SkIRect& accumulated_buffer_damage, for (const auto& r : readbacks_) { SkRect rect = SkRect::Make(r.rect); - if (rect.intersects(frame_damage)) { - frame_damage.join(rect); - } - if (rect.intersects(buffer_damage)) { - buffer_damage.join(rect); - } + frame_damage.join(rect); + buffer_damage.join(rect); } Damage res; diff --git a/flow/layers/backdrop_filter_layer_unittests.cc b/flow/layers/backdrop_filter_layer_unittests.cc index e73433889ff5d..a8bba9a30da9c 100644 --- a/flow/layers/backdrop_filter_layer_unittests.cc +++ b/flow/layers/backdrop_filter_layer_unittests.cc @@ -469,8 +469,7 @@ TEST_F(BackdropLayerDiffTest, BackdropLayer) { auto path1 = SkPath().addRect(SkRect::MakeLTRB(180, 180, 190, 190)); l4.root()->Add(std::make_shared(path1)); damage = DiffLayerTree(l4, l3); - - EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(180, 180, 190, 190)); + EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(0, 0, 190, 190)); MockLayerTree l5; l5.root()->Add(scale); From db3702da5bde67d958936fc44374cf671b0dc2ee Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Tue, 10 Oct 2023 15:56:39 +0200 Subject: [PATCH 2/6] Only add readback to damage if paint rect intersects --- flow/diff_context.cc | 23 ++++++++++++++++++----- flow/diff_context.h | 12 ++++++++---- flow/layers/backdrop_filter_layer.cc | 2 +- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/flow/diff_context.cc b/flow/diff_context.cc index 7b9fb7e081518..4f9ebdc96e75a 100644 --- a/flow/diff_context.cc +++ b/flow/diff_context.cc @@ -123,9 +123,20 @@ Damage DiffContext::ComputeDamage(const SkIRect& accumulated_buffer_damage, SkRect frame_damage(damage_); for (const auto& r : readbacks_) { - SkRect rect = SkRect::Make(r.rect); - frame_damage.join(rect); - buffer_damage.join(rect); + SkRect paint_rect = SkRect::Make(r.paint_rect); + SkRect readback_rect = SkRect::Make(r.readback_rect); + // Changes either in readback or paint rect require repainting both readback + // and paint rect. + if (paint_rect.intersects(frame_damage) || + readback_rect.intersects(frame_damage)) { + frame_damage.join(readback_rect); + frame_damage.join(paint_rect); + } + if (paint_rect.intersects(buffer_damage) || + readback_rect.intersects(buffer_damage)) { + buffer_damage.join(readback_rect); + buffer_damage.join(paint_rect); + } } Damage res; @@ -216,9 +227,11 @@ void DiffContext::AddExistingPaintRegion(const PaintRegion& region) { } } -void DiffContext::AddReadbackRegion(const SkIRect& rect) { +void DiffContext::AddReadbackRegion(const SkIRect& paint_rect, + const SkIRect& readback_rect) { Readback readback; - readback.rect = rect; + readback.paint_rect = paint_rect; + readback.readback_rect = readback_rect; readback.position = rects_->size(); // Push empty rect as a placeholder for position in current subtree rects_->push_back(SkRect::MakeEmpty()); diff --git a/flow/diff_context.h b/flow/diff_context.h index 029fb49f933fc..2373a9ff75f7c 100644 --- a/flow/diff_context.h +++ b/flow/diff_context.h @@ -123,8 +123,9 @@ class DiffContext { // The idea of readback region is that if any part of the readback region // needs to be repainted, then the whole readback region must be repainted; // - // Readback rect is in screen coordinates. - void AddReadbackRegion(const SkIRect& rect); + // Paint rect and readback rect is in screen coordinates. + void AddReadbackRegion(const SkIRect& paint_rect, + const SkIRect& readback_rect); // Returns the paint region for current subtree; Each rect in paint region is // in screen coordinates; Once a layer accumulates the paint regions of its @@ -261,8 +262,11 @@ class DiffContext { // determine if subtree has any readback size_t position; - // readback area, in screen coordinates - SkIRect rect; + // Paint region of the filter performing readback, in screen coordinates. + SkIRect paint_rect; + + // Readback area of the filter, in screen coordinates. + SkIRect readback_rect; }; std::vector readbacks_; diff --git a/flow/layers/backdrop_filter_layer.cc b/flow/layers/backdrop_filter_layer.cc index 44c3fca4a6acd..32707c4f77e6d 100644 --- a/flow/layers/backdrop_filter_layer.cc +++ b/flow/layers/backdrop_filter_layer.cc @@ -31,7 +31,7 @@ void BackdropFilterLayer::Diff(DiffContext* context, const Layer* old_layer) { SkIRect filter_input_bounds; // in screen coordinates filter_->get_input_device_bounds( filter_target_bounds, context->GetTransform3x3(), filter_input_bounds); - context->AddReadbackRegion(filter_input_bounds); + context->AddReadbackRegion(filter_target_bounds, filter_input_bounds); } DiffChildren(context, prev); From a156b34a79cc347225a5846cd4cfd79eed646297 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Tue, 10 Oct 2023 17:14:17 +0200 Subject: [PATCH 3/6] Add test for readback outside of paint area --- .../layers/backdrop_filter_layer_unittests.cc | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/flow/layers/backdrop_filter_layer_unittests.cc b/flow/layers/backdrop_filter_layer_unittests.cc index a8bba9a30da9c..2c4aa2657a393 100644 --- a/flow/layers/backdrop_filter_layer_unittests.cc +++ b/flow/layers/backdrop_filter_layer_unittests.cc @@ -469,7 +469,7 @@ TEST_F(BackdropLayerDiffTest, BackdropLayer) { auto path1 = SkPath().addRect(SkRect::MakeLTRB(180, 180, 190, 190)); l4.root()->Add(std::make_shared(path1)); damage = DiffLayerTree(l4, l3); - EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(0, 0, 190, 190)); + EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(180, 180, 190, 190)); MockLayerTree l5; l5.root()->Add(scale); @@ -481,6 +481,31 @@ TEST_F(BackdropLayerDiffTest, BackdropLayer) { EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(0, 0, 190, 190)); } +TEST_F(BackdropLayerDiffTest, ReadbackOutsideOfPaintArea) { + auto filter = DlMatrixImageFilter(SkMatrix::Translate(50, 50), + DlImageSampling::kLinear); + + MockLayerTree l1(SkISize::Make(100, 100)); + + auto clip = std::make_shared(SkRect::MakeLTRB(60, 60, 80, 80), + Clip::hardEdge); + clip->Add(std::make_shared(filter.shared(), + DlBlendMode::kSrcOver)); + l1.root()->Add(clip); + auto damage = DiffLayerTree(l1, MockLayerTree(SkISize::Make(100, 100))); + + EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(10, 10, 80, 80)); + + MockLayerTree l2(SkISize::Make(100, 100)); + // path inside readback area must trigger whole readback repaint + filter + // repaint. + auto path2 = SkPath().addRect(SkRect::MakeLTRB(10, 10, 20, 20)); + l2.root()->Add(clip); + l2.root()->Add(std::make_shared(path2)); + damage = DiffLayerTree(l2, l1); + EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(10, 10, 80, 80)); +} + TEST_F(BackdropLayerDiffTest, BackdropLayerInvalidTransform) { auto filter = DlBlurImageFilter(10, 10, DlTileMode::kClamp); From 1a92aa4a2894caa55af51a0165612973e661d935 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Tue, 10 Oct 2023 17:16:33 +0200 Subject: [PATCH 4/6] Do not check buffer_damage for intersection Buffer damage is just frame_damage + accumulated buffer damage and there's no point intersecting against accumulated_buffer_damage. --- flow/diff_context.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/flow/diff_context.cc b/flow/diff_context.cc index 4f9ebdc96e75a..54cbc5e80742b 100644 --- a/flow/diff_context.cc +++ b/flow/diff_context.cc @@ -131,9 +131,6 @@ Damage DiffContext::ComputeDamage(const SkIRect& accumulated_buffer_damage, readback_rect.intersects(frame_damage)) { frame_damage.join(readback_rect); frame_damage.join(paint_rect); - } - if (paint_rect.intersects(buffer_damage) || - readback_rect.intersects(buffer_damage)) { buffer_damage.join(readback_rect); buffer_damage.join(paint_rect); } From 27fb8f82983879451e45c5afae3b0d9035467278 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Tue, 10 Oct 2023 20:16:23 +0200 Subject: [PATCH 5/6] Add comment --- flow/diff_context.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/flow/diff_context.h b/flow/diff_context.h index 2373a9ff75f7c..0eff8ef5db4f6 100644 --- a/flow/diff_context.h +++ b/flow/diff_context.h @@ -123,7 +123,10 @@ class DiffContext { // The idea of readback region is that if any part of the readback region // needs to be repainted, then the whole readback region must be repainted; // - // Paint rect and readback rect is in screen coordinates. + // paint_rect - rectangle where the filter paints contents (in screen + // coordinates) + // readback_rect - rectangle where the filter samples from (in screen + // coordinates) void AddReadbackRegion(const SkIRect& paint_rect, const SkIRect& readback_rect); From d8e86380bf879b0559387919f754405003d0bd7f Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Tue, 10 Oct 2023 20:42:30 +0200 Subject: [PATCH 6/6] Make the origin calculation in unit test more readable --- flow/layers/backdrop_filter_layer_unittests.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/flow/layers/backdrop_filter_layer_unittests.cc b/flow/layers/backdrop_filter_layer_unittests.cc index 2c4aa2657a393..5e38f74db9f44 100644 --- a/flow/layers/backdrop_filter_layer_unittests.cc +++ b/flow/layers/backdrop_filter_layer_unittests.cc @@ -494,16 +494,16 @@ TEST_F(BackdropLayerDiffTest, ReadbackOutsideOfPaintArea) { l1.root()->Add(clip); auto damage = DiffLayerTree(l1, MockLayerTree(SkISize::Make(100, 100))); - EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(10, 10, 80, 80)); + EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(60 - 50, 60 - 50, 80, 80)); MockLayerTree l2(SkISize::Make(100, 100)); // path inside readback area must trigger whole readback repaint + filter // repaint. - auto path2 = SkPath().addRect(SkRect::MakeLTRB(10, 10, 20, 20)); + auto path2 = SkPath().addRect(SkRect::MakeXYWH(60 - 50, 60 - 50, 10, 10)); l2.root()->Add(clip); l2.root()->Add(std::make_shared(path2)); damage = DiffLayerTree(l2, l1); - EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(10, 10, 80, 80)); + EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(60 - 50, 60 - 50, 80, 80)); } TEST_F(BackdropLayerDiffTest, BackdropLayerInvalidTransform) {