From cf93b71b17f60f2e6f06025eba4560f37a06f93a Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Tue, 18 Jul 2023 14:02:35 -0700 Subject: [PATCH 1/2] fix handling of clipped rendering inside a layer that applies a filter --- display_list/display_list_unittests.cc | 64 +++++++++++++++++++ display_list/dl_builder.cc | 21 ++++-- .../testing/dl_rendering_unittests.cc | 48 +++++++++++++- display_list/utils/dl_matrix_clip_tracker.cc | 18 +++++- display_list/utils/dl_matrix_clip_tracker.h | 16 +++++ 5 files changed, 160 insertions(+), 7 deletions(-) diff --git a/display_list/display_list_unittests.cc b/display_list/display_list_unittests.cc index 4c16f6e4438dc..62b58a022ce70 100644 --- a/display_list/display_list_unittests.cc +++ b/display_list/display_list_unittests.cc @@ -504,6 +504,70 @@ TEST_F(DisplayListTest, BuilderInitialClipBoundsNonZero) { ASSERT_EQ(builder.GetDestinationClipBounds(), clip_bounds); } +TEST_F(DisplayListTest, UnclippedSaveLayerContentAccountsForFilter) { + SkRect cull_rect = SkRect::MakeLTRB(0.0f, 0.0f, 300.0f, 300.0f); + SkRect clip_rect = SkRect::MakeLTRB(100.0f, 100.0f, 200.0f, 200.0f); + SkRect draw_rect = SkRect::MakeLTRB(50.0f, 140.0f, 101.0f, 160.0f); + auto filter = DlBlurImageFilter::Make(10.0f, 10.0f, DlTileMode::kDecal); + DlPaint layer_paint = DlPaint().setImageFilter(filter); + + ASSERT_TRUE(clip_rect.intersects(draw_rect)); + ASSERT_TRUE(cull_rect.contains(clip_rect)); + ASSERT_TRUE(cull_rect.contains(draw_rect)); + + DisplayListBuilder builder; + builder.Save(); + { + builder.ClipRect(clip_rect, ClipOp::kIntersect, false); + builder.SaveLayer(&cull_rect, &layer_paint); + { // + builder.DrawRect(draw_rect, DlPaint()); + } + builder.Restore(); + } + builder.Restore(); + auto display_list = builder.Build(); + + ASSERT_EQ(display_list->op_count(), 6u); + + SkRect result_rect = draw_rect.makeOutset(30.0f, 30.0f); + ASSERT_TRUE(result_rect.intersect(clip_rect)); + ASSERT_EQ(result_rect, SkRect::MakeLTRB(100.0f, 110.0f, 131.0f, 190.0f)); + ASSERT_EQ(display_list->bounds(), result_rect); +} + +TEST_F(DisplayListTest, ClippedSaveLayerContentAccountsForFilter) { + SkRect cull_rect = SkRect::MakeLTRB(0.0f, 0.0f, 300.0f, 300.0f); + SkRect clip_rect = SkRect::MakeLTRB(100.0f, 100.0f, 200.0f, 200.0f); + SkRect draw_rect = SkRect::MakeLTRB(50.0f, 140.0f, 99.0f, 160.0f); + auto filter = DlBlurImageFilter::Make(10.0f, 10.0f, DlTileMode::kDecal); + DlPaint layer_paint = DlPaint().setImageFilter(filter); + + ASSERT_FALSE(clip_rect.intersects(draw_rect)); + ASSERT_TRUE(cull_rect.contains(clip_rect)); + ASSERT_TRUE(cull_rect.contains(draw_rect)); + + DisplayListBuilder builder; + builder.Save(); + { + builder.ClipRect(clip_rect, ClipOp::kIntersect, false); + builder.SaveLayer(&cull_rect, &layer_paint); + { // + builder.DrawRect(draw_rect, DlPaint()); + } + builder.Restore(); + } + builder.Restore(); + auto display_list = builder.Build(); + + ASSERT_EQ(display_list->op_count(), 6u); + + SkRect result_rect = draw_rect.makeOutset(30.0f, 30.0f); + ASSERT_TRUE(result_rect.intersect(clip_rect)); + ASSERT_EQ(result_rect, SkRect::MakeLTRB(100.0f, 110.0f, 129.0f, 190.0f)); + ASSERT_EQ(display_list->bounds(), result_rect); +} + TEST_F(DisplayListTest, SingleOpSizes) { for (auto& group : allGroups) { for (size_t i = 0; i < group.variants.size(); i++) { diff --git a/display_list/dl_builder.cc b/display_list/dl_builder.cc index ea2a132580780..1536d54b4160d 100644 --- a/display_list/dl_builder.cc +++ b/display_list/dl_builder.cc @@ -522,12 +522,25 @@ void DisplayListBuilder::saveLayer(const SkRect* bounds, } } - // Even though Skia claims that the bounds are only a hint, they actually - // use them as the temporary layer bounds during rendering the layer, so - // we set them as if a clip operation were performed. - if (bounds) { + if (options.renders_with_attributes() && current_.getImageFilter()) { + // We use |resetCullRect| here because we will be accumulating bounds of + // primitives before applying the filter to those bounds. We might + // encounter a primitive whose bounds are clipped, but whose filtered + // bounds will not be clipped. If the individual rendering ops bounds + // are clipped, it will not contribute to the overall bounds which + // could lead to inaccurate (subset) bounds of the DisplayList. + // We need to reset the cull rect here to avoid this premature clipping. + // The filtered bounds will be clipped to the existing clip rect when + // this layer is restored. + // If bounds is null then the original cull_rect will be used. + tracker_.resetCullRect(bounds); + } else if (bounds) { + // Even though Skia claims that the bounds are only a hint, they actually + // use them as the temporary layer bounds during rendering the layer, so + // we set them as if a clip operation were performed. tracker_.clipRect(*bounds, ClipOp::kIntersect, false); } + if (backdrop) { // A backdrop will affect up to the entire surface, bounded by the clip AccumulateUnbounded(); diff --git a/display_list/testing/dl_rendering_unittests.cc b/display_list/testing/dl_rendering_unittests.cc index 55afd24d806d6..1d7b9c5ff52e0 100644 --- a/display_list/testing/dl_rendering_unittests.cc +++ b/display_list/testing/dl_rendering_unittests.cc @@ -2032,7 +2032,8 @@ class CanvasCompareTester { if (!dl_bounds.contains(sk_bounds)) { FML_LOG(ERROR) << "DisplayList bounds are too small!"; } - if (!sk_bounds.roundOut().contains(dl_bounds.roundOut())) { + if (!dl_bounds.isEmpty() && + !sk_bounds.roundOut().contains(dl_bounds.roundOut())) { FML_LOG(ERROR) << "###### DisplayList bounds larger than reference!"; } } @@ -3453,6 +3454,51 @@ TEST_F(DisplayListCanvas, DrawShadowDpr) { CanvasCompareTester::DefaultTolerance.addBoundsPadding(3, 3)); } +TEST_F(DisplayListCanvas, SaveLayerClippedContentStillFilters) { + // draw rect is just outside of render bounds on the right + const SkRect draw_rect = SkRect::MakeLTRB( // + kRenderRight + 1, // + kRenderTop, // + kTestBounds.fRight, // + kRenderBottom // + ); + TestParameters test_params( + [=](SkCanvas* canvas, const SkPaint& paint) { + auto layer_filter = + SkImageFilters::Blur(10.0f, 10.0f, SkTileMode::kDecal, nullptr); + SkPaint layer_paint; + layer_paint.setImageFilter(layer_filter); + canvas->save(); + canvas->clipRect(kRenderBounds, SkClipOp::kIntersect, false); + canvas->saveLayer(&kTestBounds, &layer_paint); + canvas->drawRect(draw_rect, paint); + canvas->restore(); + canvas->restore(); + }, + [=](DlCanvas* canvas, const DlPaint& paint) { + auto layer_filter = + DlBlurImageFilter::Make(10.0f, 10.0f, DlTileMode::kDecal); + DlPaint layer_paint; + layer_paint.setImageFilter(layer_filter); + canvas->Save(); + canvas->ClipRect(kRenderBounds, ClipOp::kIntersect, false); + canvas->SaveLayer(&kTestBounds, &layer_paint); + canvas->DrawRect(draw_rect, paint); + canvas->Restore(); + canvas->Restore(); + }, + kSaveLayerWithPaintFlags); + CaseParameters case_params("Filtered SaveLayer with clipped content"); + BoundsTolerance tolerance = BoundsTolerance().addAbsolutePadding(6.0f, 6.0f); + + for (auto& provider : CanvasCompareTester::kTestProviders) { + RenderEnvironment env = RenderEnvironment::MakeN32(provider.get()); + env.init_ref(test_params.sk_renderer(), test_params.dl_renderer()); + CanvasCompareTester::quickCompareToReference(env, "default"); + CanvasCompareTester::RenderWith(test_params, env, tolerance, case_params); + } +} + TEST_F(DisplayListCanvas, SaveLayerConsolidation) { float commutable_color_matrix[]{ // clang-format off diff --git a/display_list/utils/dl_matrix_clip_tracker.cc b/display_list/utils/dl_matrix_clip_tracker.cc index 69164b803ef70..2dd0e8fa31a39 100644 --- a/display_list/utils/dl_matrix_clip_tracker.cc +++ b/display_list/utils/dl_matrix_clip_tracker.cc @@ -103,7 +103,8 @@ bool DisplayListMatrixClipTracker::is_3x3(const SkM44& m) { DisplayListMatrixClipTracker::DisplayListMatrixClipTracker( const SkRect& cull_rect, - const SkMatrix& matrix) { + const SkMatrix& matrix) + : original_cull_rect_(cull_rect) { // isEmpty protects us against NaN as we normalize any empty cull rects SkRect cull = cull_rect.isEmpty() ? SkRect::MakeEmpty() : cull_rect; saved_.emplace_back(std::make_unique(matrix, cull)); @@ -113,7 +114,8 @@ DisplayListMatrixClipTracker::DisplayListMatrixClipTracker( DisplayListMatrixClipTracker::DisplayListMatrixClipTracker( const SkRect& cull_rect, - const SkM44& m44) { + const SkM44& m44) + : original_cull_rect_(cull_rect) { // isEmpty protects us against NaN as we normalize any empty cull rects SkRect cull = cull_rect.isEmpty() ? SkRect::MakeEmpty() : cull_rect; if (is_3x3(m44)) { @@ -282,6 +284,18 @@ bool DisplayListMatrixClipTracker::Data::content_culled( return !mapped.intersects(cull_rect_); } +void DisplayListMatrixClipTracker::Data::resetBounds(const SkRect& cull_rect) { + if (!cull_rect.isEmpty()) { + SkRect rect; + mapRect(cull_rect, &rect); + if (!rect.isEmpty()) { + cull_rect_ = rect; + return; + } + } + cull_rect_.setEmpty(); +} + void DisplayListMatrixClipTracker::Data::clipBounds(const SkRect& clip, ClipOp op, bool is_aa) { diff --git a/display_list/utils/dl_matrix_clip_tracker.h b/display_list/utils/dl_matrix_clip_tracker.h index a0d74b85f02af..94ef0ee6683de 100644 --- a/display_list/utils/dl_matrix_clip_tracker.h +++ b/display_list/utils/dl_matrix_clip_tracker.h @@ -27,6 +27,19 @@ class DisplayListMatrixClipTracker { DisplayListMatrixClipTracker(const SkRect& cull_rect, const SkMatrix& matrix); DisplayListMatrixClipTracker(const SkRect& cull_rect, const SkM44& matrix); + // This method should almost never be used as it breaks the encapsulation + // of the enclosing clips. However it is needed for practical purposes in + // some rare cases - such as when a saveLayer is collecting rendering + // operations prior to applying a filter on the entire layer bounds and + // some of those operations fall outside the enclosing clip, but their + // filtered content will spread out from where they were rendered on the + // layer into the enclosing clipped area. + // Omitting the |cull_rect| argument, or passing nullptr, will restore the + // cull rect to the initial value it had when the tracker was constructed. + void resetCullRect(const SkRect* cull_rect = nullptr) { + current_->resetBounds(cull_rect ? *cull_rect : original_cull_rect_); + } + static bool is_3x3(const SkM44& m44); SkRect base_device_cull_rect() const { return saved_[0]->device_cull_rect(); } @@ -106,6 +119,8 @@ class DisplayListMatrixClipTracker { virtual void clipBounds(const SkRect& clip, ClipOp op, bool is_aa); + virtual void resetBounds(const SkRect& cull_rect); + protected: Data(const SkRect& rect) : cull_rect_(rect) {} @@ -116,6 +131,7 @@ class DisplayListMatrixClipTracker { friend class Data3x3; friend class Data4x4; + SkRect original_cull_rect_; Data* current_; std::vector> saved_; }; From a91101c99e6ec110994cc73af2abc75cfc198a38 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 19 Jul 2023 10:51:45 -0700 Subject: [PATCH 2/2] don't store lambdas as references --- display_list/testing/dl_rendering_unittests.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/display_list/testing/dl_rendering_unittests.cc b/display_list/testing/dl_rendering_unittests.cc index 1d7b9c5ff52e0..76b16cc2ccc3e 100644 --- a/display_list/testing/dl_rendering_unittests.cc +++ b/display_list/testing/dl_rendering_unittests.cc @@ -862,9 +862,9 @@ class TestParameters { } private: - const SkRenderer& sk_renderer_; - const DlRenderer& dl_renderer_; - const DisplayListAttributeFlags& flags_; + const SkRenderer sk_renderer_; + const DlRenderer dl_renderer_; + const DisplayListAttributeFlags flags_; bool is_draw_text_blob_ = false; bool is_draw_display_list_ = false;