fix the bounds of nested save layers with a mix of clips and transforms (#31440)

Author: flar

display_list/display_list_unittests.cc

@@ -184,7 +184,8 @@ static constexpr SkIRect TestLatticeSrcRect = {1, 1, 39, 39};
 
 static sk_sp<SkPicture> MakeTestPicture(int w, int h, SkColor color) {
   SkPictureRecorder recorder;
-  SkCanvas* cv = recorder.beginRecording(TestBounds);
+  SkRTreeFactory rtree_factory;
+  SkCanvas* cv = recorder.beginRecording(TestBounds, &rtree_factory);
   SkPaint paint;
   paint.setColor(color);
   paint.setStyle(SkPaint::kFill_Style);
@@ -998,7 +999,8 @@ TEST(DisplayList, DisplayListSaveLayerBoundsWithAlphaFilter) {
     // that modifies alpha channels (save layer bounds are meaningless
     // under those circumstances)
     SkPictureRecorder recorder;
-    SkCanvas* canvas = recorder.beginRecording(build_bounds);
+    SkRTreeFactory rtree_factory;
+    SkCanvas* canvas = recorder.beginRecording(build_bounds, &rtree_factory);
     SkPaint p1;
     p1.setColorFilter(alpha_color_filter);
     canvas->saveLayer(save_bounds, &p1);
@@ -1586,5 +1588,128 @@ TEST(DisplayList, SaveLayerBoundsSnapshotsImageFilter) {
   EXPECT_EQ(bounds, SkRect::MakeLTRB(50, 50, 100, 100));
 }
 
+TEST(DisplayList, FlutterSvgIssue661BoundsWereEmpty) {
+  // See https://github.com/dnfield/flutter_svg/issues/661
+
+  SkPath path1;
+  path1.setFillType(SkPathFillType::kWinding);
+  path1.moveTo(25.54f, 37.52f);
+  path1.cubicTo(20.91f, 37.52f, 16.54f, 33.39f, 13.62f, 30.58f);
+  path1.lineTo(13, 30);
+  path1.lineTo(12.45f, 29.42f);
+  path1.cubicTo(8.39f, 25.15f, 1.61f, 18, 8.37f, 11.27f);
+  path1.cubicTo(10.18f, 9.46f, 12.37f, 9.58f, 14.49f, 11.58f);
+  path1.cubicTo(15.67f, 12.71f, 17.05f, 14.69f, 17.07f, 16.58f);
+  path1.cubicTo(17.0968f, 17.458f, 16.7603f, 18.3081f, 16.14f, 18.93f);
+  path1.cubicTo(15.8168f, 19.239f, 15.4653f, 19.5169f, 15.09f, 19.76f);
+  path1.cubicTo(14.27f, 20.33f, 14.21f, 20.44f, 14.27f, 20.62f);
+  path1.cubicTo(15.1672f, 22.3493f, 16.3239f, 23.9309f, 17.7f, 25.31f);
+  path1.cubicTo(19.0791f, 26.6861f, 20.6607f, 27.8428f, 22.39f, 28.74f);
+  path1.cubicTo(22.57f, 28.8f, 22.69f, 28.74f, 23.25f, 27.92f);
+  path1.cubicTo(23.5f, 27.566f, 23.778f, 27.231f, 24.08f, 26.92f);
+  path1.cubicTo(24.7045f, 26.3048f, 25.5538f, 25.9723f, 26.43f, 26);
+  path1.cubicTo(28.29f, 26, 30.27f, 27.4f, 31.43f, 28.58f);
+  path1.cubicTo(33.43f, 30.67f, 33.55f, 32.9f, 31.74f, 34.7f);
+  path1.cubicTo(30.1477f, 36.4508f, 27.906f, 37.4704f, 25.54f, 37.52f);
+  path1.close();
+  path1.moveTo(11.17f, 12.23f);
+  path1.cubicTo(10.6946f, 12.2571f, 10.2522f, 12.4819f, 9.95f, 12.85f);
+  path1.cubicTo(5.12f, 17.67f, 8.95f, 22.5f, 14.05f, 27.85f);
+  path1.lineTo(14.62f, 28.45f);
+  path1.lineTo(15.16f, 28.96f);
+  path1.cubicTo(20.52f, 34.06f, 25.35f, 37.89f, 30.16f, 33.06f);
+  path1.cubicTo(30.83f, 32.39f, 31.25f, 31.56f, 29.81f, 30.06f);
+  path1.cubicTo(28.9247f, 29.07f, 27.7359f, 28.4018f, 26.43f, 28.16f);
+  path1.cubicTo(26.1476f, 28.1284f, 25.8676f, 28.2367f, 25.68f, 28.45f);
+  path1.cubicTo(25.4633f, 28.6774f, 25.269f, 28.9252f, 25.1f, 29.19f);
+  path1.cubicTo(24.53f, 30.01f, 23.47f, 31.54f, 21.54f, 30.79f);
+  path1.lineTo(21.41f, 30.72f);
+  path1.cubicTo(19.4601f, 29.7156f, 17.6787f, 28.4133f, 16.13f, 26.86f);
+  path1.cubicTo(14.5748f, 25.3106f, 13.2693f, 23.5295f, 12.26f, 21.58f);
+  path1.lineTo(12.2f, 21.44f);
+  path1.cubicTo(11.45f, 19.51f, 12.97f, 18.44f, 13.8f, 17.88f);
+  path1.cubicTo(14.061f, 17.706f, 14.308f, 17.512f, 14.54f, 17.3f);
+  path1.cubicTo(14.7379f, 17.1067f, 14.8404f, 16.8359f, 14.82f, 16.56f);
+  path1.cubicTo(14.5978f, 15.268f, 13.9585f, 14.0843f, 13, 13.19f);
+  path1.cubicTo(12.5398f, 12.642f, 11.8824f, 12.2971f, 11.17f, 12.23f);
+  path1.lineTo(11.17f, 12.23f);
+  path1.close();
+  path1.moveTo(27, 19.34f);
+  path1.lineTo(24.74f, 19.34f);
+  path1.cubicTo(24.7319f, 18.758f, 24.262f, 18.2881f, 23.68f, 18.28f);
+  path1.lineTo(23.68f, 16.05f);
+  path1.lineTo(23.7f, 16.05f);
+  path1.cubicTo(25.5153f, 16.0582f, 26.9863f, 17.5248f, 27, 19.34f);
+  path1.lineTo(27, 19.34f);
+  path1.close();
+  path1.moveTo(32.3f, 19.34f);
+  path1.lineTo(30.07f, 19.34f);
+  path1.cubicTo(30.037f, 15.859f, 27.171f, 13.011f, 23.69f, 13);
+  path1.lineTo(23.69f, 10.72f);
+  path1.cubicTo(28.415f, 10.725f, 32.3f, 14.615f, 32.3f, 19.34f);
+  path1.close();
+
+  SkPath path2;
+  path2.setFillType(SkPathFillType::kWinding);
+  path2.moveTo(37.5f, 19.33f);
+  path2.lineTo(35.27f, 19.33f);
+  path2.cubicTo(35.265f, 12.979f, 30.041f, 7.755f, 23.69f, 7.75f);
+  path2.lineTo(23.69f, 5.52f);
+  path2.cubicTo(31.264f, 5.525f, 37.495f, 11.756f, 37.5f, 19.33f);
+  path2.close();
+
+  DisplayListBuilder builder;
+  {
+    builder.save();
+    builder.clipRect({0, 0, 100, 100}, SkClipOp::kIntersect, true);
+    {
+      builder.save();
+      builder.transform2DAffine(2.17391, 0, -2547.83,  //
+                                0, 2.04082, -500);
+      {
+        builder.save();
+        builder.clipRect({1172, 245, 1218, 294}, SkClipOp::kIntersect, true);
+        {
+          builder.saveLayer(nullptr, true);
+          {
+            builder.save();
+            builder.transform2DAffine(1.4375, 0, 1164.09,  //
+                                      0, 1.53125, 236.548);
+            builder.setAntiAlias(1);
+            builder.setColor(0xffffffff);
+            builder.drawPath(path1);
+            builder.restore();
+          }
+          {
+            builder.save();
+            builder.transform2DAffine(1.4375, 0, 1164.09,  //
+                                      0, 1.53125, 236.548);
+            builder.drawPath(path2);
+            builder.restore();
+          }
+          builder.restore();
+        }
+        builder.restore();
+      }
+      builder.restore();
+    }
+    builder.restore();
+  }
+  sk_sp<DisplayList> display_list = builder.Build();
+  // Prior to the fix, the bounds were empty.
+  EXPECT_FALSE(display_list->bounds().isEmpty());
+  // These are the expected bounds, but testing float values can be
+  // flaky wrt minor changes in the bounds calculations. If this
+  // line has to be revised too often as the DL implementation is
+  // improved and maintained, then we can eliminate this test and
+  // just rely on the "rounded out" bounds test that follows.
+  EXPECT_EQ(display_list->bounds(),
+            SkRect::MakeLTRB(0, 0.00189208984375, 99.9839630127, 100));
+  // This is the more practical result. The bounds are "almost" 0,0,100x100
+  EXPECT_EQ(display_list->bounds().roundOut(), SkIRect::MakeWH(100, 100));
+  EXPECT_EQ(display_list->op_count(), 19);
+  EXPECT_EQ(display_list->bytes(), sizeof(DisplayList) + 304u);
+}
+
 }  // namespace testing
 }  // namespace flutter

display_list/display_list_utils.cc

@@ -247,7 +247,7 @@ void ClipBoundsDispatchHelper::reset(const SkRect* cull_rect) {
 DisplayListBoundsCalculator::DisplayListBoundsCalculator(
     const SkRect* cull_rect)
     : ClipBoundsDispatchHelper(cull_rect) {
-  layer_infos_.emplace_back(std::make_unique<RootLayerData>());
+  layer_infos_.emplace_back(std::make_unique<LayerData>(nullptr));
   accumulator_ = layer_infos_.back()->layer_accumulator();
 }
 void DisplayListBoundsCalculator::setStrokeCap(SkPaint::Cap cap) {
@@ -294,24 +294,36 @@ void DisplayListBoundsCalculator::setMaskBlurFilter(SkBlurStyle style,
 void DisplayListBoundsCalculator::save() {
   SkMatrixDispatchHelper::save();
   ClipBoundsDispatchHelper::save();
-  layer_infos_.emplace_back(std::make_unique<SaveData>(accumulator_));
+  layer_infos_.emplace_back(std::make_unique<LayerData>(accumulator_));
   accumulator_ = layer_infos_.back()->layer_accumulator();
 }
 void DisplayListBoundsCalculator::saveLayer(const SkRect* bounds,
                                             bool with_paint) {
   SkMatrixDispatchHelper::save();
   ClipBoundsDispatchHelper::save();
+
   if (with_paint) {
-    layer_infos_.emplace_back(std::make_unique<SaveLayerData>(
-        accumulator_, image_filter_, paint_nops_on_transparency()));
+    // The actual flood of the outer layer clip will occur after the
+    // (eventual) corresponding restore is called, but rather than
+    // remember this information in the LayerInfo until the restore
+    // method is processed, we just mark the unbounded state up front.
+    if (!paint_nops_on_transparency()) {
+      // We will fill the clip of the outer layer when we restore
+      AccumulateUnbounded();
+    }
+
+    layer_infos_.emplace_back(
+        std::make_unique<LayerData>(accumulator_, image_filter_));
   } else {
     layer_infos_.emplace_back(
-        std::make_unique<SaveLayerData>(accumulator_, nullptr, true));
+        std::make_unique<LayerData>(accumulator_, nullptr));
   }
+
   accumulator_ = layer_infos_.back()->layer_accumulator();
-  // Accumulate the layer in its own coordinate system and then
-  // filter and transform its bounds on restore.
-  SkMatrixDispatchHelper::reset();
+
+  // 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) {
     clipRect(*bounds, SkClipOp::kIntersect, false);
   }
@@ -320,24 +332,53 @@ void DisplayListBoundsCalculator::restore() {
   if (layer_infos_.size() > 1) {
     SkMatrixDispatchHelper::restore();
     ClipBoundsDispatchHelper::restore();
-    accumulator_ = layer_infos_.back()->restore_accumulator();
-    SkRect layer_bounds = layer_infos_.back()->layer_bounds();
-    // Must read unbounded state after layer_bounds
-    bool layer_unbounded = layer_infos_.back()->is_unbounded();
-    layer_infos_.pop_back();
 
-    // We accumulate the bounds even if the layer was unbounded because
-    // the unbounded state may be contained at a higher level, so we at
-    // least accumulate our best estimate about what we have.
-    if (!layer_bounds.isEmpty()) {
-      // We do not use AccumulateOpBounds because the layer info already
-      // applied all bounds modifications based on the attributes that
-      // were in place when it was created. Modifying the bounds further
-      // based on the current attributes would mix attribute states.
-      // The bounds are still transformed and clipped by this method.
-      AccumulateBounds(layer_bounds);
+    // Remember a few pieces of information from the current layer info
+    // for later processing.
+    LayerData* layer_info = layer_infos_.back().get();
+    BoundsAccumulator* outer_accumulator = layer_info->restore_accumulator();
+    bool is_unbounded = layer_info->is_unbounded();
+
+    // Before we pop_back we will get the current layer bounds from the
+    // current accumulator and adjust ot as required based on the filter.
+    SkRect layer_bounds = accumulator_->bounds();
+    sk_sp<SkImageFilter> filter = layer_info->filter();
+    if (filter) {
+      if (filter->canComputeFastBounds()) {
+        SkIRect filter_bounds =
+            filter->filterBounds(layer_bounds.roundOut(), matrix(),
+                                 SkImageFilter::kForward_MapDirection);
+        layer_bounds.set(filter_bounds);
+
+        // We could leave the clipping to the code below that will
+        // finally accumulate the layer bounds, but the bounds do
+        // not normally need clipping unless they were modified by
+        // entering this filtering code path.
+        if (has_clip() && !layer_bounds.intersect(clip_bounds())) {
+          layer_bounds.setEmpty();
+        }
+      } else {
+        // If the filter cannot compute bounds then it might take an
+        // unbounded amount of space. This can sometimes happen if it
+        // modifies transparent black which means its affect will not
+        // be bounded by the transparent pixels outside of the layer
+        // drawable.
+        is_unbounded = true;
+      }
     }
-    if (layer_unbounded) {
+
+    // Restore the accumulator before popping the LayerInfo so that
+    // it nevers points to an out of scope instance.
+    accumulator_ = outer_accumulator;
+    layer_infos_.pop_back();
+
+    // Finally accumulate the impact of the layer into the new scope.
+    // Note that the bounds were already accumulated in device pixels
+    // and clipped to any clips involved so we do not need to go
+    // through any transforms or clips to accuulate them into this
+    // layer.
+    accumulator_->accumulate(layer_bounds);
+    if (is_unbounded) {
       AccumulateUnbounded();
     }
   }