From f1090285dff49c64e1981698ebff487668bb1da7 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Thu, 6 Mar 2025 21:01:22 -0800 Subject: [PATCH] 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. --- .../flutter/flow/layers/clip_path_layer.cc | 17 +- .../flow/layers/clip_path_layer_unittests.cc | 147 +++++++++++++++++- .../flutter/flow/layers/clip_rrect_layer.cc | 7 +- .../flow/layers/clip_rrect_layer_unittests.cc | 50 +++++- .../android/platform_view_android_jni_impl.cc | 52 +------ 5 files changed, 221 insertions(+), 52 deletions(-) diff --git a/engine/src/flutter/flow/layers/clip_path_layer.cc b/engine/src/flutter/flow/layers/clip_path_layer.cc index c3153349d2d..d2006cb25ce 100644 --- 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 diff --git a/engine/src/flutter/flow/layers/clip_path_layer_unittests.cc b/engine/src/flutter/flow/layers/clip_path_layer_unittests.cc index 14e8b87417e..275b0fad89a 100644 --- 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(child_path); auto layer = std::make_shared(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(child_path, child_paint); auto layer = std::make_shared(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(child_path, child_paint); + auto layer = std::make_shared(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(child_path, child_paint); + auto layer = std::make_shared(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(child_path, child_paint); + auto layer = std::make_shared(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); diff --git a/engine/src/flutter/flow/layers/clip_rrect_layer.cc b/engine/src/flutter/flow/layers/clip_rrect_layer.cc index 134972b80e1..6b172d50c58 100644 --- 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 diff --git a/engine/src/flutter/flow/layers/clip_rrect_layer_unittests.cc b/engine/src/flutter/flow/layers/clip_rrect_layer_unittests.cc index c5ce9edc661..69630c7adac 100644 --- 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(child_path, child_paint); auto layer = std::make_shared(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(child_path, child_paint); auto layer = std::make_shared(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(child_path, child_paint); + auto layer = std::make_shared(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); diff --git 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 index b9432e59613..5cc4f40ea7a 100644 --- 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 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 radiisArray( - env, env->NewFloatArray(8)); - env->SetFloatArrayRegion(radiisArray.obj(), 0, 8, radiis); - env->CallVoidMethod(mutatorsStack, - g_mutators_stack_push_cliprrect_method, - static_cast(rect.GetLeft()), // - static_cast(rect.GetTop()), // - static_cast(rect.GetRight()), // - static_cast(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 =