fix handling of clipped rendering inside a layer that applies a filter

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++) {

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();

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;
@@ -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

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<Data3x3>(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) {

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<std::unique_ptr<Data>> saved_;
 };