From c2394077c36dd26c7e7a2747168a6d609a954c43 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Wed, 22 Dec 2021 15:20:53 +0100 Subject: [PATCH 1/2] Fix crash in BackdropFilterLayer::Diff --- flow/layers/backdrop_filter_layer.cc | 28 ++++++++++++++----- .../layers/backdrop_filter_layer_unittests.cc | 24 ++++++++++++++++ 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/flow/layers/backdrop_filter_layer.cc b/flow/layers/backdrop_filter_layer.cc index 548db3227d72e..98f4130fa0be5 100644 --- a/flow/layers/backdrop_filter_layer.cc +++ b/flow/layers/backdrop_filter_layer.cc @@ -26,15 +26,29 @@ void BackdropFilterLayer::Diff(DiffContext* context, const Layer* old_layer) { if (filter_) { // convert paint bounds and filter to screen coordinates - context->GetTransform().mapRect(&paint_bounds); - auto input_filter_bounds = paint_bounds.roundOut(); auto filter = filter_->makeWithLocalMatrix(context->GetTransform()); + if (filter) { + context->GetTransform().mapRect(&paint_bounds); + auto input_filter_bounds = paint_bounds.roundOut(); + auto filter_bounds = // in screen coordinates + filter->filterBounds(input_filter_bounds, SkMatrix::I(), + SkImageFilter::kReverse_MapDirection); - auto filter_bounds = // in screen coordinates - filter->filterBounds(input_filter_bounds, SkMatrix::I(), - SkImageFilter::kReverse_MapDirection); - - context->AddReadbackRegion(filter_bounds); + context->AddReadbackRegion(filter_bounds); + } else { + // Slightly less precise alternative that works even if filter can not be + // transformed. It might cover larger area than the branch before, because + // filterBounds() first rounds out to integer and and then we transform + // the bounds, as opposed to transforming first and rounding second. + // See https://github.com/flutter/flutter/issues/95211 + auto input_filter_bounds = paint_bounds.roundOut(); + auto filter_bounds = // in local coordinates + filter_->filterBounds(input_filter_bounds, SkMatrix::I(), + SkImageFilter::kReverse_MapDirection); + auto screen_bounds = SkRect::Make(filter_bounds); + context->GetTransform().mapRect(&screen_bounds); + context->AddReadbackRegion(screen_bounds.roundOut()); + } } DiffChildren(context, prev); diff --git a/flow/layers/backdrop_filter_layer_unittests.cc b/flow/layers/backdrop_filter_layer_unittests.cc index 7a5a066fd7344..f56ff4e168197 100644 --- a/flow/layers/backdrop_filter_layer_unittests.cc +++ b/flow/layers/backdrop_filter_layer_unittests.cc @@ -327,5 +327,29 @@ TEST_F(BackdropLayerDiffTest, BackdropLayer) { EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(0, 0, 190, 190)); } +TEST_F(BackdropLayerDiffTest, BackdropLayerInvalidTransform) { + auto filter = SkImageFilters::Blur(10, 10, SkTileMode::kClamp, nullptr); + + { + // tests later assume 30px readback area, fail early if that's not the case + auto readback = filter->filterBounds(SkIRect::MakeWH(10, 10), SkMatrix::I(), + SkImageFilter::kReverse_MapDirection); + EXPECT_EQ(readback, SkIRect::MakeLTRB(-30, -30, 40, 40)); + } + + MockLayerTree l1(SkISize::Make(100, 100)); + SkMatrix transform; + transform.setPerspX(0.1); + transform.setPerspY(0.1); + + auto transform_layer = std::make_shared(transform); + l1.root()->Add(transform_layer); + transform_layer->Add( + std::make_shared(filter, SkBlendMode::kSrcOver)); + + auto damage = DiffLayerTree(l1, MockLayerTree(SkISize::Make(100, 100))); + EXPECT_EQ(damage.frame_damage, SkIRect::MakeWH(5, 5)); +} + } // namespace testing } // namespace flutter From caa158d81bb6ded376d1142d762148fcedbce287 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Wed, 22 Dec 2021 22:12:05 +0100 Subject: [PATCH 2/2] Pass context transform to filterBound instead of scaling the filter --- flow/layers/backdrop_filter_layer.cc | 30 ++++--------------- .../layers/backdrop_filter_layer_unittests.cc | 2 +- 2 files changed, 7 insertions(+), 25 deletions(-) diff --git a/flow/layers/backdrop_filter_layer.cc b/flow/layers/backdrop_filter_layer.cc index 98f4130fa0be5..8e8ef2628a16b 100644 --- a/flow/layers/backdrop_filter_layer.cc +++ b/flow/layers/backdrop_filter_layer.cc @@ -25,30 +25,12 @@ void BackdropFilterLayer::Diff(DiffContext* context, const Layer* old_layer) { context->AddLayerBounds(paint_bounds); if (filter_) { - // convert paint bounds and filter to screen coordinates - auto filter = filter_->makeWithLocalMatrix(context->GetTransform()); - if (filter) { - context->GetTransform().mapRect(&paint_bounds); - auto input_filter_bounds = paint_bounds.roundOut(); - auto filter_bounds = // in screen coordinates - filter->filterBounds(input_filter_bounds, SkMatrix::I(), - SkImageFilter::kReverse_MapDirection); - - context->AddReadbackRegion(filter_bounds); - } else { - // Slightly less precise alternative that works even if filter can not be - // transformed. It might cover larger area than the branch before, because - // filterBounds() first rounds out to integer and and then we transform - // the bounds, as opposed to transforming first and rounding second. - // See https://github.com/flutter/flutter/issues/95211 - auto input_filter_bounds = paint_bounds.roundOut(); - auto filter_bounds = // in local coordinates - filter_->filterBounds(input_filter_bounds, SkMatrix::I(), - SkImageFilter::kReverse_MapDirection); - auto screen_bounds = SkRect::Make(filter_bounds); - context->GetTransform().mapRect(&screen_bounds); - context->AddReadbackRegion(screen_bounds.roundOut()); - } + context->GetTransform().mapRect(&paint_bounds); + auto input_filter_bounds = paint_bounds.roundOut(); + auto filter_bounds = // in screen coordinates + filter_->filterBounds(input_filter_bounds, context->GetTransform(), + SkImageFilter::kReverse_MapDirection); + context->AddReadbackRegion(filter_bounds); } DiffChildren(context, prev); diff --git a/flow/layers/backdrop_filter_layer_unittests.cc b/flow/layers/backdrop_filter_layer_unittests.cc index f56ff4e168197..667b0dc713d28 100644 --- a/flow/layers/backdrop_filter_layer_unittests.cc +++ b/flow/layers/backdrop_filter_layer_unittests.cc @@ -348,7 +348,7 @@ TEST_F(BackdropLayerDiffTest, BackdropLayerInvalidTransform) { std::make_shared(filter, SkBlendMode::kSrcOver)); auto damage = DiffLayerTree(l1, MockLayerTree(SkISize::Make(100, 100))); - EXPECT_EQ(damage.frame_damage, SkIRect::MakeWH(5, 5)); + EXPECT_EQ(damage.frame_damage, SkIRect::MakeWH(15, 15)); } } // namespace testing