Clip layers reduce rrects and paths to simpler shapes when possible (#164693)

Flutter code can pass clips in the widget tree down as Path objects even if they were originally simpler shapes. We now catch those simplifications in the clip_*_layer code and perform reduced operations in their place.
2025-03-06 21:01:22 -08:00 · 2025-03-06 21:01:22 -08:00 · f1090285df
commit f1090285df
parent a45d325bad
5 changed files with 221 additions and 52 deletions
--- a/engine/src/flutter/flow/layers/clip_path_layer.cc
+++ b/engine/src/flutter/flow/layers/clip_path_layer.cc
@ -14,8 +14,21 @@ const DlRect ClipPathLayer::clip_shape_bounds() const {
 }

 void ClipPathLayer::ApplyClip(LayerStateStack::MutatorContext& mutator) const {
-  clip_shape().WillRenderSkPath();
-  mutator.clipPath(clip_shape(), clip_behavior() != Clip::kHardEdge);
+  bool is_aa = clip_behavior() != Clip::kHardEdge;
+  DlRect rect;
+  if (clip_shape().IsRect(&rect)) {
+    mutator.clipRect(rect, is_aa);
+  } else if (clip_shape().IsOval(&rect)) {
+    mutator.clipRRect(DlRoundRect::MakeOval(rect), is_aa);
+  } else {
+    DlRoundRect rrect;
+    if (clip_shape().IsRoundRect(&rrect)) {
+      mutator.clipRRect(rrect, is_aa);
+    } else {
+      clip_shape().WillRenderSkPath();
+      mutator.clipPath(clip_shape(), is_aa);
+    }
+  }
 }

 }  // namespace flutter
--- a/engine/src/flutter/flow/layers/clip_path_layer_unittests.cc
+++ b/engine/src/flutter/flow/layers/clip_path_layer_unittests.cc
@ -63,7 +63,8 @@ TEST_F(ClipPathLayerTest, PaintingCulledLayerDies) {
  const DlRect layer_bounds = DlRect::MakeXYWH(0.5, 1.0, 5.0, 6.0);
  const DlRect distant_bounds = DlRect::MakeXYWH(100.0, 100.0, 10.0, 10.0);
  const DlPath child_path = DlPath::MakeRect(child_bounds);
-  const DlPath layer_path = DlPath::MakeRect(layer_bounds);
+  const DlPath layer_path = DlPath::MakeRect(layer_bounds) +
+                            DlPath::MakeRect(layer_bounds.Expand(-0.1, -0.1));
  auto mock_layer = std::make_shared<MockLayer>(child_path);
  auto layer = std::make_shared<ClipPathLayer>(layer_path, Clip::kHardEdge);
  layer->Add(mock_layer);
@ -103,7 +104,8 @@ TEST_F(ClipPathLayerTest, ChildOutsideBounds) {
  const DlRect child_bounds = DlRect::MakeXYWH(2.5, 5.0, 4.5, 4.0);
  const DlRect clip_bounds = DlRect::MakeXYWH(0.5, 1.0, 5.0, 6.0);
  const DlPath child_path = DlPath::MakeRect(child_bounds);
-  const DlPath clip_path = DlPath::MakeRect(clip_bounds);
+  const DlPath clip_path = DlPath::MakeRect(clip_bounds) +
+                           DlPath::MakeRect(clip_bounds.Expand(-0.1f, -0.1f));
  const DlPaint child_paint = DlPaint(DlColor::kYellow());
  auto mock_layer = std::make_shared<MockLayer>(child_path, child_paint);
  auto layer = std::make_shared<ClipPathLayer>(clip_path, Clip::kHardEdge);
@ -141,6 +143,147 @@ TEST_F(ClipPathLayerTest, ChildOutsideBounds) {
  // would trip an FML_DCHECK
 }

+TEST_F(ClipPathLayerTest, RectPathReducesToClipRect) {
+  const DlMatrix initial_matrix = DlMatrix::MakeTranslation({0.5f, 1.0f});
+  const DlRect child_bounds = DlRect::MakeXYWH(1.0, 2.0, 2.0, 2.0);
+  const DlRect layer_bounds = DlRect::MakeXYWH(0.5, 1.0, 5.0, 6.0);
+  const DlPath child_path = DlPath::MakeRect(child_bounds) +
+                            DlPath::MakeOval(child_bounds.Expand(-0.1f));
+  const DlPath layer_path = DlPath::MakeRect(layer_bounds);
+  const DlPaint child_paint = DlPaint(DlColor::kYellow());
+  auto mock_layer = std::make_shared<MockLayer>(child_path, child_paint);
+  auto layer = std::make_shared<ClipPathLayer>(layer_path, Clip::kHardEdge);
+  layer->Add(mock_layer);
+
+  preroll_context()->state_stack.set_preroll_delegate(initial_matrix);
+  layer->Preroll(preroll_context());
+
+  // Untouched
+  EXPECT_EQ(preroll_context()->state_stack.device_cull_rect(), kGiantRect);
+  EXPECT_TRUE(preroll_context()->state_stack.is_empty());
+
+  EXPECT_EQ(mock_layer->paint_bounds(), child_bounds);
+  EXPECT_EQ(layer->paint_bounds(), mock_layer->paint_bounds());
+  EXPECT_EQ(layer->child_paint_bounds(), child_bounds);
+  EXPECT_TRUE(mock_layer->needs_painting(paint_context()));
+  EXPECT_TRUE(layer->needs_painting(paint_context()));
+  EXPECT_EQ(mock_layer->parent_cull_rect(), layer_bounds);
+  EXPECT_EQ(mock_layer->parent_matrix(), initial_matrix);
+  EXPECT_EQ(mock_layer->parent_mutators(),
+            std::vector({Mutator(layer_bounds)}));
+
+  layer->Paint(display_list_paint_context());
+  DisplayListBuilder expected_builder;
+  /* (ClipPath)layer::Paint */ {
+    expected_builder.Save();
+    {
+      expected_builder.ClipRect(layer_bounds);
+      /* mock_layer::Paint */ {
+        expected_builder.DrawPath(child_path, child_paint);
+      }
+    }
+    expected_builder.Restore();
+  }
+  EXPECT_TRUE(DisplayListsEQ_Verbose(display_list(), expected_builder.Build()));
+}
+
+TEST_F(ClipPathLayerTest, OvalPathReducesToClipRRect) {
+  const DlMatrix initial_matrix = DlMatrix::MakeTranslation({0.5f, 1.0f});
+  const DlRect child_bounds = DlRect::MakeXYWH(1.0, 2.0, 2.0, 2.0);
+  const DlRect layer_bounds = DlRect::MakeXYWH(0.5, 1.0, 5.0, 6.0);
+  const DlPath child_path = DlPath::MakeRect(child_bounds) +
+                            DlPath::MakeOval(child_bounds.Expand(-0.1f));
+  const DlPath layer_path = DlPath::MakeOval(layer_bounds);
+  const DlPaint child_paint = DlPaint(DlColor::kYellow());
+  auto mock_layer = std::make_shared<MockLayer>(child_path, child_paint);
+  auto layer = std::make_shared<ClipPathLayer>(layer_path, Clip::kHardEdge);
+  layer->Add(mock_layer);
+
+  preroll_context()->state_stack.set_preroll_delegate(initial_matrix);
+  layer->Preroll(preroll_context());
+
+  // Untouched
+  EXPECT_EQ(preroll_context()->state_stack.device_cull_rect(), kGiantRect);
+  EXPECT_TRUE(preroll_context()->state_stack.is_empty());
+
+  EXPECT_EQ(mock_layer->paint_bounds(), child_bounds);
+  EXPECT_EQ(layer->paint_bounds(), mock_layer->paint_bounds());
+  EXPECT_EQ(layer->child_paint_bounds(), child_bounds);
+  EXPECT_TRUE(mock_layer->needs_painting(paint_context()));
+  EXPECT_TRUE(layer->needs_painting(paint_context()));
+  EXPECT_EQ(mock_layer->parent_cull_rect(), layer_bounds);
+  EXPECT_EQ(mock_layer->parent_matrix(), initial_matrix);
+  EXPECT_EQ(mock_layer->parent_mutators(),
+            std::vector({Mutator(DlRoundRect::MakeOval(layer_bounds))}));
+
+  layer->Paint(display_list_paint_context());
+  DisplayListBuilder expected_builder;
+  /* (ClipPath)layer::Paint */ {
+    expected_builder.Save();
+    {
+      expected_builder.ClipRoundRect(DlRoundRect::MakeOval(layer_bounds));
+      /* mock_layer::Paint */ {
+        expected_builder.DrawPath(child_path, child_paint);
+      }
+    }
+    expected_builder.Restore();
+  }
+  EXPECT_TRUE(DisplayListsEQ_Verbose(display_list(), expected_builder.Build()));
+}
+
+TEST_F(ClipPathLayerTest, RRectPathReducesToClipRRect) {
+  const DlMatrix initial_matrix = DlMatrix::MakeTranslation({0.5f, 1.0f});
+  const DlRect child_bounds = DlRect::MakeXYWH(1.0, 2.0, 2.0, 2.0);
+  const DlRect layer_bounds = DlRect::MakeXYWH(0.5, 1.0, 5.0, 6.0);
+  const DlPath child_path = DlPath::MakeRect(child_bounds) +
+                            DlPath::MakeOval(child_bounds.Expand(-0.1f));
+  const DlRoundRect layer_rrect =
+      DlRoundRect::MakeRectXY(layer_bounds, 0.1f, 0.1f);
+  const DlPath layer_path = DlPath::MakeRoundRect(layer_rrect);
+  const DlPaint child_paint = DlPaint(DlColor::kYellow());
+  auto mock_layer = std::make_shared<MockLayer>(child_path, child_paint);
+  auto layer = std::make_shared<ClipPathLayer>(layer_path, Clip::kHardEdge);
+  layer->Add(mock_layer);
+  DlRoundRect converted_rrect;
+
+  // The conversion back to an RRect by "IsRoundRect" is lossy, so we need
+  // to use the version that will be reduced by the clip_path_layer.
+  EXPECT_TRUE(layer_path.IsRoundRect(&converted_rrect));
+
+  preroll_context()->state_stack.set_preroll_delegate(initial_matrix);
+  layer->Preroll(preroll_context());
+
+  // Untouched
+  EXPECT_EQ(preroll_context()->state_stack.device_cull_rect(), kGiantRect);
+  EXPECT_TRUE(preroll_context()->state_stack.is_empty());
+
+  EXPECT_EQ(mock_layer->paint_bounds(), child_bounds);
+  EXPECT_EQ(layer->paint_bounds(), mock_layer->paint_bounds());
+  EXPECT_EQ(layer->child_paint_bounds(), child_bounds);
+  EXPECT_TRUE(mock_layer->needs_painting(paint_context()));
+  EXPECT_TRUE(layer->needs_painting(paint_context()));
+  EXPECT_EQ(mock_layer->parent_cull_rect(), layer_bounds);
+  EXPECT_EQ(mock_layer->parent_matrix(), initial_matrix);
+  EXPECT_EQ(mock_layer->parent_mutators(),
+            std::vector({Mutator(converted_rrect)}));
+
+  layer->Paint(display_list_paint_context());
+  EXPECT_EQ(display_list()->GetOpType(1u),
+            DisplayListOpType::kClipIntersectRoundRect);
+  DisplayListBuilder expected_builder;
+  /* (ClipPath)layer::Paint */ {
+    expected_builder.Save();
+    {
+      expected_builder.ClipRoundRect(converted_rrect);
+      /* mock_layer::Paint */ {
+        expected_builder.DrawPath(child_path, child_paint);
+      }
+    }
+    expected_builder.Restore();
+  }
+  EXPECT_TRUE(DisplayListsEQ_Verbose(display_list(), expected_builder.Build()));
+}
+
 TEST_F(ClipPathLayerTest, FullyContainedChild) {
  const DlMatrix initial_matrix = DlMatrix::MakeTranslation({0.5f, 1.0f});
  const DlRect child_bounds = DlRect::MakeXYWH(1.0, 2.0, 2.0, 2.0);
--- a/engine/src/flutter/flow/layers/clip_rrect_layer.cc
+++ b/engine/src/flutter/flow/layers/clip_rrect_layer.cc
@ -15,7 +15,12 @@ const DlRect ClipRRectLayer::clip_shape_bounds() const {
 }

 void ClipRRectLayer::ApplyClip(LayerStateStack::MutatorContext& mutator) const {
-  mutator.clipRRect(clip_shape(), clip_behavior() != Clip::kHardEdge);
+  bool is_aa = clip_behavior() != Clip::kHardEdge;
+  if (clip_shape().IsRect()) {
+    mutator.clipRect(clip_shape().GetBounds(), is_aa);
+  } else {
+    mutator.clipRRect(clip_shape(), is_aa);
+  }
 }

 }  // namespace flutter
--- a/engine/src/flutter/flow/layers/clip_rrect_layer_unittests.cc
+++ b/engine/src/flutter/flow/layers/clip_rrect_layer_unittests.cc
@ -64,7 +64,8 @@ TEST_F(ClipRRectLayerTest, PaintingCulledLayerDies) {
  const DlRect layer_bounds = DlRect::MakeXYWH(0.5, 1.0, 5.0, 6.0);
  const DlRect distant_bounds = DlRect::MakeXYWH(100.0, 100.0, 10.0, 10.0);
  const DlPath child_path = DlPath::MakeRect(child_bounds);
-  const DlRoundRect layer_rrect = DlRoundRect::MakeRect(layer_bounds);
+  const DlRoundRect layer_rrect =
+      DlRoundRect::MakeRectXY(layer_bounds, 0.1f, 0.1f);
  const DlPaint child_paint = DlPaint(DlColor::kYellow());
  auto mock_layer = std::make_shared<MockLayer>(child_path, child_paint);
  auto layer = std::make_shared<ClipRRectLayer>(layer_rrect, Clip::kHardEdge);
@ -105,7 +106,8 @@ TEST_F(ClipRRectLayerTest, ChildOutsideBounds) {
  const DlRect child_bounds = DlRect::MakeXYWH(2.5, 5.0, 4.5, 4.0);
  const DlRect clip_bounds = DlRect::MakeXYWH(0.5, 1.0, 5.0, 6.0);
  const DlPath child_path = DlPath::MakeRect(child_bounds);
-  const DlRoundRect clip_rrect = DlRoundRect::MakeRect(clip_bounds);
+  const DlRoundRect clip_rrect =
+      DlRoundRect::MakeRectXY(clip_bounds, 0.1f, 0.1f);
  const DlPaint child_paint = DlPaint(DlColor::kYellow());
  auto mock_layer = std::make_shared<MockLayer>(child_path, child_paint);
  auto layer = std::make_shared<ClipRRectLayer>(clip_rrect, Clip::kHardEdge);
@ -143,6 +145,50 @@ TEST_F(ClipRRectLayerTest, ChildOutsideBounds) {
  // would trip an FML_DCHECK
 }

+TEST_F(ClipRRectLayerTest, RectRRectReducesToClipRect) {
+  const DlMatrix initial_matrix = DlMatrix::MakeTranslation({0.5f, 1.0f});
+  const DlRect child_bounds = DlRect::MakeXYWH(1.0, 2.0, 2.0, 2.0);
+  const DlRect layer_bounds = DlRect::MakeXYWH(0.5, 1.0, 5.0, 6.0);
+  const DlPath child_path = DlPath::MakeRect(child_bounds) +
+                            DlPath::MakeOval(child_bounds.Expand(-0.1f));
+  const DlRoundRect layer_rrect = DlRoundRect::MakeRect(layer_bounds);
+  const DlPaint child_paint = DlPaint(DlColor::kYellow());
+  auto mock_layer = std::make_shared<MockLayer>(child_path, child_paint);
+  auto layer = std::make_shared<ClipRRectLayer>(layer_rrect, Clip::kHardEdge);
+  layer->Add(mock_layer);
+
+  preroll_context()->state_stack.set_preroll_delegate(initial_matrix);
+  layer->Preroll(preroll_context());
+
+  // Untouched
+  EXPECT_EQ(preroll_context()->state_stack.device_cull_rect(), kGiantRect);
+  EXPECT_TRUE(preroll_context()->state_stack.is_empty());
+
+  EXPECT_EQ(mock_layer->paint_bounds(), child_bounds);
+  EXPECT_EQ(layer->paint_bounds(), mock_layer->paint_bounds());
+  EXPECT_EQ(layer->child_paint_bounds(), child_bounds);
+  EXPECT_TRUE(mock_layer->needs_painting(paint_context()));
+  EXPECT_TRUE(layer->needs_painting(paint_context()));
+  EXPECT_EQ(mock_layer->parent_cull_rect(), layer_bounds);
+  EXPECT_EQ(mock_layer->parent_matrix(), initial_matrix);
+  EXPECT_EQ(mock_layer->parent_mutators(),
+            std::vector({Mutator(layer_bounds)}));
+
+  layer->Paint(display_list_paint_context());
+  DisplayListBuilder expected_builder;
+  /* (ClipRRect)layer::Paint */ {
+    expected_builder.Save();
+    {
+      expected_builder.ClipRect(layer_bounds);
+      /* mock_layer::Paint */ {
+        expected_builder.DrawPath(child_path, child_paint);
+      }
+    }
+    expected_builder.Restore();
+  }
+  EXPECT_TRUE(DisplayListsEQ_Verbose(display_list(), expected_builder.Build()));
+}
+
 TEST_F(ClipRRectLayerTest, FullyContainedChild) {
  const DlMatrix initial_matrix = DlMatrix::MakeTranslation({0.5f, 1.0f});
  const DlRect child_bounds = DlRect::MakeXYWH(1.0, 2.0, 2.0, 2.0);
--- a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc
+++ b/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc
@ -2123,51 +2123,13 @@ void PlatformViewAndroidJNIImpl::onDisplayPlatformView2(
      }
      case MutatorType::kClipPath: {
        auto& dlPath = (*iter)->GetPath();
-        // Sometimes a kClipPath mutator is actually housing a simpler
-        // shape. This isn't usually an issue, but the impeller Path version
-        // of an oval or round rect may be too approximated to really match
-        // well between the rendering operations (which check for simpler
-        // shapes) and handing a raw path to the platform. To make things
-        // match better, we do the same shape reduction checks here as most
-        // renderers perform (we don't look for a Rect shape, though as
-        // those match pretty well on their own).
-        //
-        // This should eventually be handled at a higher level, as in the
-        // clip_path_layer.
-        // See https://github.com/flutter/flutter/issues/164666
-        std::optional<DlRoundRect> path_rrect;
-        {
-          DlRect rect;
-          if (dlPath.IsOval(&rect)) {
-            path_rrect = DlRoundRect::MakeOval(rect);
-          } else {
-            DlRoundRect rrect;
-            if (dlPath.IsRoundRect(&rrect)) {
-              path_rrect = rrect;
-            }
-          }
-        }
-        if (path_rrect.has_value()) {
-          const DlRect& rect = path_rrect->GetBounds();
-          const DlRoundingRadii& radii = path_rrect->GetRadii();
-          SkScalar radiis[8] = {
-              radii.top_left.width,     radii.top_left.height,
-              radii.top_right.width,    radii.top_right.height,
-              radii.bottom_right.width, radii.bottom_right.height,
-              radii.bottom_left.width,  radii.bottom_left.height,
-          };
-          fml::jni::ScopedJavaLocalRef<jfloatArray> radiisArray(
-              env, env->NewFloatArray(8));
-          env->SetFloatArrayRegion(radiisArray.obj(), 0, 8, radiis);
-          env->CallVoidMethod(mutatorsStack,
-                              g_mutators_stack_push_cliprrect_method,
-                              static_cast<int>(rect.GetLeft()),    //
-                              static_cast<int>(rect.GetTop()),     //
-                              static_cast<int>(rect.GetRight()),   //
-                              static_cast<int>(rect.GetBottom()),  //
-                              radiisArray.obj());
-          break;
-        }
+        // The layer mutator mechanism should have already caught and
+        // redirected these simplified path cases, which is important because
+        // the conics they generate (in the case of oval and rrect) will
+        // not match the results of an impeller path conversion very closely.
+        FML_DCHECK(!dlPath.IsRect());
+        FML_DCHECK(!dlPath.IsOval());
+        FML_DCHECK(!dlPath.IsRoundRect());

        // Define and populate an Android Path with data from the DlPath
        jobject androidPath =