From baf55892d96003acc880b55efd675cd573578577 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 19 Jul 2023 11:40:31 -0700 Subject: [PATCH 1/2] fix handling of clipped rendering inside a layer that applies a filter (#43787) When a saveLayer is rendered with an ImageFilter that modifies the bounds of the rendered pixels, and some of the content of that saveLayer did not intersect the clip, but the filtered output of that content did intersect the clip, we might not accumulate the bounds of those rendering operations into the DisplayList bounds. This bug was not encountered during practical testing, but showed up on some testing with the new NOP culling code. For now, this bug only affects the accuracy of the reported bounds of the DisplayList, but that can affect raster caching and potentially the layer culling done in the LayerTree. It might also affect the accuracy of the RTree associated with the DisplayList, which would only show up in rare circumstances when the indicated operation was used in the presence of an embedded view. --- display_list/display_list_unittests.cc | 64 +++++++++++++++++++ display_list/dl_builder.cc | 21 ++++-- .../testing/dl_rendering_unittests.cc | 54 ++++++++++++++-- display_list/utils/dl_matrix_clip_tracker.cc | 18 +++++- display_list/utils/dl_matrix_clip_tracker.h | 18 ++++++ 5 files changed, 165 insertions(+), 10 deletions(-) diff --git a/display_list/display_list_unittests.cc b/display_list/display_list_unittests.cc index 28f3c005d488a..8489e00e42c57 100644 --- a/display_list/display_list_unittests.cc +++ b/display_list/display_list_unittests.cc @@ -503,6 +503,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 64c4acf421435..bb9dcc0e386b7 100644 --- a/display_list/testing/dl_rendering_unittests.cc +++ b/display_list/testing/dl_rendering_unittests.cc @@ -863,9 +863,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; @@ -2033,7 +2033,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!"; } } @@ -3454,6 +3455,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 98cfdf6132587..daeef0810dfeb 100644 --- a/display_list/utils/dl_matrix_clip_tracker.cc +++ b/display_list/utils/dl_matrix_clip_tracker.cc @@ -103,7 +103,8 @@ static bool 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 c6efaaf78c9d1..94ef0ee6683de 100644 --- a/display_list/utils/dl_matrix_clip_tracker.h +++ b/display_list/utils/dl_matrix_clip_tracker.h @@ -27,6 +27,21 @@ 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(); } bool using_4x4_matrix() const { return current_->is_4x4(); } @@ -104,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) {} @@ -114,6 +131,7 @@ class DisplayListMatrixClipTracker { friend class Data3x3; friend class Data4x4; + SkRect original_cull_rect_; Data* current_; std::vector> saved_; }; From fcb3c89787809bd0f1c881e20559f54b59eb8d4f Mon Sep 17 00:00:00 2001 From: Remon Helmond Date: Tue, 26 Sep 2023 08:31:31 +0100 Subject: [PATCH 2/2] remove unused 3x3 function --- display_list/utils/dl_matrix_clip_tracker.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/display_list/utils/dl_matrix_clip_tracker.h b/display_list/utils/dl_matrix_clip_tracker.h index 94ef0ee6683de..71d99f179b6f7 100644 --- a/display_list/utils/dl_matrix_clip_tracker.h +++ b/display_list/utils/dl_matrix_clip_tracker.h @@ -40,8 +40,6 @@ class DisplayListMatrixClipTracker { 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(); } bool using_4x4_matrix() const { return current_->is_4x4(); }