[Impeller] Skip rect clips that do nothing. (flutter#43948)

Resolves flutter/flutter#131100

* Skip rect clips that we know won't further restrict the clip shape.
* Don't append clips to enforce subpass bounds. We currently don't collapse passes when subpass clips are added anyhow, so just a restriction to subpass collapsing in order to keep the current behavior intact.
* Small refactor to reduce confusion: Just place the subpass bounds limit into `EntityPass` itself instead of the delegate.

Built on flutter#43946 (because iOS currently fails validation without it).

Gallery home screen before/after:

![compare](https://github.com/flutter/engine/assets/919017/34608633-6b3b-4979-be2b-f2685b32168f)

ci/licenses_golden/excluded_files

@@ -132,6 +132,7 @@
 ../../../flutter/impeller/docs
 ../../../flutter/impeller/entity/contents/filters/inputs/filter_input_unittests.cc
 ../../../flutter/impeller/entity/entity_unittests.cc
+../../../flutter/impeller/entity/geometry/geometry_unittests.cc
 ../../../flutter/impeller/fixtures
 ../../../flutter/impeller/geometry/README.md
 ../../../flutter/impeller/geometry/geometry_unittests.cc

impeller/aiks/aiks_unittests.cc

@@ -1979,7 +1979,7 @@ TEST_P(AiksTest, OpacityPeepHoleApplicationTest) {
       ColorFilter::MakeBlend(BlendMode::kSourceOver, Color::Blue());
 
   // Paint has color filter, can't elide.
-  auto delegate = std::make_shared<OpacityPeepholePassDelegate>(paint, rect);
+  auto delegate = std::make_shared<OpacityPeepholePassDelegate>(paint);
   ASSERT_FALSE(delegate->CanCollapseIntoParentPass(entity_pass.get()));
 
   paint.color_filter = nullptr;
@@ -1991,14 +1991,14 @@ TEST_P(AiksTest, OpacityPeepHoleApplicationTest) {
   };
 
   // Paint has image filter, can't elide.
-  delegate = std::make_shared<OpacityPeepholePassDelegate>(paint, rect);
+  delegate = std::make_shared<OpacityPeepholePassDelegate>(paint);
   ASSERT_FALSE(delegate->CanCollapseIntoParentPass(entity_pass.get()));
 
   paint.image_filter = nullptr;
   paint.color = Color::Red();
 
   // Paint has no alpha, can't elide;
-  delegate = std::make_shared<OpacityPeepholePassDelegate>(paint, rect);
+  delegate = std::make_shared<OpacityPeepholePassDelegate>(paint);
   ASSERT_FALSE(delegate->CanCollapseIntoParentPass(entity_pass.get()));
 
   // Positive test.
@@ -2008,7 +2008,7 @@ TEST_P(AiksTest, OpacityPeepHoleApplicationTest) {
   entity_pass->AddEntity(entity);
   paint.color = Color::Red().WithAlpha(0.5);
 
-  delegate = std::make_shared<OpacityPeepholePassDelegate>(paint, rect);
+  delegate = std::make_shared<OpacityPeepholePassDelegate>(paint);
   ASSERT_TRUE(delegate->CanCollapseIntoParentPass(entity_pass.get()));
 }
 
@@ -2114,6 +2114,30 @@ TEST_P(AiksTest, DrawRectAbsorbsClearsNegative) {
   ASSERT_EQ(render_pass->GetCommands().size(), 2llu);
 }
 
+TEST_P(AiksTest, ClipRectElidesNoOpClips) {
+  Canvas canvas(Rect(0, 0, 100, 100));
+  canvas.ClipRect(Rect(0, 0, 100, 100));
+  canvas.ClipRect(Rect(-100, -100, 300, 300));
+  canvas.DrawPaint({.color = Color::Red(), .blend_mode = BlendMode::kSource});
+  canvas.DrawPaint({.color = Color::CornflowerBlue().WithAlpha(0.75),
+                    .blend_mode = BlendMode::kSourceOver});
+
+  Picture picture = canvas.EndRecordingAsPicture();
+  auto expected = Color::Red().Blend(Color::CornflowerBlue().WithAlpha(0.75),
+                                     BlendMode::kSourceOver);
+  ASSERT_EQ(picture.pass->GetClearColor(), expected);
+
+  std::shared_ptr<ContextSpy> spy = ContextSpy::Make();
+  std::shared_ptr<Context> real_context = GetContext();
+  std::shared_ptr<ContextMock> mock_context = spy->MakeContext(real_context);
+  AiksContext renderer(mock_context);
+  std::shared_ptr<Image> image = picture.ToImage(renderer, {300, 300});
+
+  ASSERT_EQ(spy->render_passes_.size(), 1llu);
+  std::shared_ptr<RenderPass> render_pass = spy->render_passes_[0];
+  ASSERT_EQ(render_pass->GetCommands().size(), 0llu);
+}
+
 TEST_P(AiksTest, CollapsedDrawPaintInSubpass) {
   Canvas canvas;
   canvas.DrawPaint(

impeller/aiks/canvas.cc

@@ -280,7 +280,16 @@ void Canvas::ClipPath(const Path& path, Entity::ClipOperation clip_op) {
 }
 
 void Canvas::ClipRect(const Rect& rect, Entity::ClipOperation clip_op) {
-  ClipGeometry(Geometry::MakeRect(rect), clip_op);
+  auto geometry = Geometry::MakeRect(rect);
+  auto& cull_rect = xformation_stack_.back().cull_rect;
+  if (clip_op == Entity::ClipOperation::kIntersect &&                        //
+      cull_rect.has_value() &&                                               //
+      geometry->CoversArea(xformation_stack_.back().xformation, *cull_rect)  //
+  ) {
+    return;  // This clip will do nothing, so skip it.
+  }
+
+  ClipGeometry(std::move(geometry), clip_op);
   switch (clip_op) {
     case Entity::ClipOperation::kIntersect:
       IntersectCulling(rect);
@@ -298,7 +307,21 @@ void Canvas::ClipRRect(const Rect& rect,
                   .SetConvexity(Convexity::kConvex)
                   .AddRoundedRect(rect, corner_radius)
                   .TakePath();
-  ClipGeometry(Geometry::MakeFillPath(path), clip_op);
+
+  std::optional<Rect> inner_rect = (corner_radius * 2 < rect.size.width &&
+                                    corner_radius * 2 < rect.size.height)
+                                       ? rect.Expand(-corner_radius)
+                                       : std::make_optional<Rect>();
+  auto geometry = Geometry::MakeFillPath(path, inner_rect);
+  auto& cull_rect = xformation_stack_.back().cull_rect;
+  if (clip_op == Entity::ClipOperation::kIntersect &&                        //
+      cull_rect.has_value() &&                                               //
+      geometry->CoversArea(xformation_stack_.back().xformation, *cull_rect)  //
+  ) {
+    return;  // This clip will do nothing, so skip it.
+  }
+
+  ClipGeometry(std::move(geometry), clip_op);
   switch (clip_op) {
     case Entity::ClipOperation::kIntersect:
       IntersectCulling(rect);
@@ -491,23 +514,14 @@ void Canvas::SaveLayer(const Paint& paint,
   Save(true, paint.blend_mode, backdrop_filter);
 
   auto& new_layer_pass = GetCurrentPass();
+  new_layer_pass.SetBoundsLimit(bounds);
 
   // Only apply opacity peephole on default blending.
   if (paint.blend_mode == BlendMode::kSourceOver) {
     new_layer_pass.SetDelegate(
-        std::make_unique<OpacityPeepholePassDelegate>(paint, bounds));
+        std::make_unique<OpacityPeepholePassDelegate>(paint));
   } else {
-    new_layer_pass.SetDelegate(
-        std::make_unique<PaintPassDelegate>(paint, bounds));
-  }
-
-  if (bounds.has_value() && !backdrop_filter) {
-    // Render target switches due to a save layer can be elided. In such cases
-    // where passes are collapsed into their parent, the clipping effect to
-    // the size of the render target that would have been allocated will be
-    // absent. Explicitly add back a clip to reproduce that behavior. Since
-    // clips never require a render target switch, this is a cheap operation.
-    ClipRect(bounds.value());
+    new_layer_pass.SetDelegate(std::make_unique<PaintPassDelegate>(paint));
   }
 }
 

impeller/aiks/paint_pass_delegate.cc

@@ -17,17 +17,11 @@ namespace impeller {
 /// PaintPassDelegate
 /// ----------------------------------------------
 
-PaintPassDelegate::PaintPassDelegate(Paint paint, std::optional<Rect> coverage)
-    : paint_(std::move(paint)), coverage_(coverage) {}
+PaintPassDelegate::PaintPassDelegate(Paint paint) : paint_(std::move(paint)) {}
 
 // |EntityPassDelgate|
 PaintPassDelegate::~PaintPassDelegate() = default;
 
-// |EntityPassDelgate|
-std::optional<Rect> PaintPassDelegate::GetCoverageRect() {
-  return coverage_;
-}
-
 // |EntityPassDelgate|
 bool PaintPassDelegate::CanElide() {
   return paint_.blend_mode == BlendMode::kDestination;
@@ -56,19 +50,12 @@ std::shared_ptr<Contents> PaintPassDelegate::CreateContentsForSubpassTarget(
 /// OpacityPeepholePassDelegate
 /// ----------------------------------------------
 
-OpacityPeepholePassDelegate::OpacityPeepholePassDelegate(
-    Paint paint,
-    std::optional<Rect> coverage)
-    : paint_(std::move(paint)), coverage_(coverage) {}
+OpacityPeepholePassDelegate::OpacityPeepholePassDelegate(Paint paint)
+    : paint_(std::move(paint)) {}
 
 // |EntityPassDelgate|
 OpacityPeepholePassDelegate::~OpacityPeepholePassDelegate() = default;
 
-// |EntityPassDelgate|
-std::optional<Rect> OpacityPeepholePassDelegate::GetCoverageRect() {
-  return coverage_;
-}
-
 // |EntityPassDelgate|
 bool OpacityPeepholePassDelegate::CanElide() {
   return paint_.blend_mode == BlendMode::kDestination;
@@ -77,6 +64,11 @@ bool OpacityPeepholePassDelegate::CanElide() {
 // |EntityPassDelgate|
 bool OpacityPeepholePassDelegate::CanCollapseIntoParentPass(
     EntityPass* entity_pass) {
+  // Passes with absorbed clips can not be safely collapsed.
+  if (entity_pass->GetBoundsLimit().has_value()) {
+    return false;
+  }
+
   // OpacityPeepholePassDelegate will only get used if the pass's blend mode is
   // SourceOver, so no need to check here.
   if (paint_.color.alpha <= 0.0 || paint_.color.alpha >= 1.0 ||

impeller/aiks/paint_pass_delegate.h

@@ -16,14 +16,11 @@ class EntityPass;
 
 class PaintPassDelegate final : public EntityPassDelegate {
  public:
-  PaintPassDelegate(Paint paint, std::optional<Rect> coverage);
+  explicit PaintPassDelegate(Paint paint);
 
   // |EntityPassDelgate|
   ~PaintPassDelegate() override;
 
-  // |EntityPassDelegate|
-  std::optional<Rect> GetCoverageRect() override;
-
   // |EntityPassDelgate|
   bool CanElide() override;
 
@@ -37,7 +34,6 @@ class PaintPassDelegate final : public EntityPassDelegate {
 
  private:
   const Paint paint_;
-  const std::optional<Rect> coverage_;
 
   FML_DISALLOW_COPY_AND_ASSIGN(PaintPassDelegate);
 };
@@ -49,14 +45,11 @@ class PaintPassDelegate final : public EntityPassDelegate {
 /// cannot forward to child subpass delegates.
 class OpacityPeepholePassDelegate final : public EntityPassDelegate {
  public:
-  OpacityPeepholePassDelegate(Paint paint, std::optional<Rect> coverage);
+  explicit OpacityPeepholePassDelegate(Paint paint);
 
   // |EntityPassDelgate|
   ~OpacityPeepholePassDelegate() override;
 
-  // |EntityPassDelegate|
-  std::optional<Rect> GetCoverageRect() override;
-
   // |EntityPassDelgate|
   bool CanElide() override;
 
@@ -70,7 +63,6 @@ class OpacityPeepholePassDelegate final : public EntityPassDelegate {
 
  private:
   const Paint paint_;
-  const std::optional<Rect> coverage_;
 
   FML_DISALLOW_COPY_AND_ASSIGN(OpacityPeepholePassDelegate);
 };

impeller/entity/BUILD.gn

@@ -270,6 +270,7 @@ impeller_component("entity_unittests") {
     "entity_playground.cc",
     "entity_playground.h",
     "entity_unittests.cc",
+    "geometry/geometry_unittests.cc",
   ]
 
   deps = [

impeller/entity/entity_pass.cc

@@ -62,6 +62,14 @@ void EntityPass::SetDelegate(std::unique_ptr<EntityPassDelegate> delegate) {
   delegate_ = std::move(delegate);
 }
 
+void EntityPass::SetBoundsLimit(std::optional<Rect> bounds_limit) {
+  bounds_limit_ = bounds_limit;
+}
+
+std::optional<Rect> EntityPass::GetBoundsLimit() const {
+  return bounds_limit_;
+}
+
 void EntityPass::AddEntity(Entity entity) {
   if (entity.GetBlendMode() == BlendMode::kSourceOver &&
       entity.GetContents()->IsOpaque()) {
@@ -129,20 +137,12 @@ std::optional<Rect> EntityPass::GetSubpassCoverage(
     return std::nullopt;
   }
 
-  // The delegates don't have an opinion on what the entity coverage has to be.
-  // Just use that as-is.
-  auto delegate_coverage = subpass.delegate_->GetCoverageRect();
-  if (!delegate_coverage.has_value()) {
+  if (!subpass.bounds_limit_.has_value()) {
     return entities_coverage;
   }
-  // The delegate coverage hint is in given in local space, so apply the subpass
-  // transformation.
-  delegate_coverage = delegate_coverage->TransformBounds(subpass.xformation_);
-
-  // If the delegate tells us the coverage is smaller than it needs to be, then
-  // great. OTOH, if the delegate is being wasteful, limit coverage to what is
-  // actually needed.
-  return entities_coverage->Intersection(delegate_coverage.value());
+  auto user_bounds_coverage =
+      subpass.bounds_limit_->TransformBounds(subpass.xformation_);
+  return entities_coverage->Intersection(user_bounds_coverage);
 }
 
 EntityPass* EntityPass::GetSuperpass() const {

impeller/entity/entity_pass.h

@@ -54,6 +54,19 @@ class EntityPass {
 
   void SetDelegate(std::unique_ptr<EntityPassDelegate> delgate);
 
+  /// @brief  Set the bounds limit, which is provided by the user when creating
+  ///         a SaveLayer. This is a hint that allows the user to communicate
+  ///         that it's OK to not render content outside of the bounds.
+  ///
+  ///         For consistency with Skia, we effectively treat this like a
+  ///         rectangle clip by forcing the subpass texture size to never exceed
+  ///         it.
+  void SetBoundsLimit(std::optional<Rect> bounds_limit);
+
+  /// @brief  Get the bounds limit, which is provided by the user when creating
+  ///         a SaveLayer.
+  std::optional<Rect> GetBoundsLimit() const;
+
   size_t GetSubpassesDepth() const;
 
   std::unique_ptr<EntityPass> Clone() const;
@@ -223,6 +236,7 @@ class EntityPass {
   BlendMode blend_mode_ = BlendMode::kSourceOver;
   bool flood_clip_ = false;
   bool enable_offscreen_debug_checkerboard_ = false;
+  std::optional<Rect> bounds_limit_;
 
   /// These values are incremented whenever something is added to the pass that
   /// requires reading from the backdrop texture. Currently, this can happen in

impeller/entity/entity_pass_delegate.cc

@@ -18,9 +18,6 @@ class DefaultEntityPassDelegate final : public EntityPassDelegate {
   // |EntityPassDelegate|
   ~DefaultEntityPassDelegate() override = default;
 
-  // |EntityPassDelegate|
-  std::optional<Rect> GetCoverageRect() override { return std::nullopt; }
-
   // |EntityPassDelegate|
   bool CanElide() override { return false; }
 

impeller/entity/entity_pass_delegate.h

@@ -20,8 +20,6 @@ class EntityPassDelegate {
 
   EntityPassDelegate();
 
-  virtual std::optional<Rect> GetCoverageRect() = 0;
-
   virtual ~EntityPassDelegate();
 
   virtual bool CanElide() = 0;

impeller/entity/entity_unittests.cc

@@ -70,15 +70,11 @@ TEST_P(EntityTest, CanCreateEntity) {
 
 class TestPassDelegate final : public EntityPassDelegate {
  public:
-  explicit TestPassDelegate(std::optional<Rect> coverage, bool collapse = false)
-      : coverage_(coverage), collapse_(collapse) {}
+  explicit TestPassDelegate(bool collapse = false) : collapse_(collapse) {}
 
   // |EntityPassDelegate|
   ~TestPassDelegate() override = default;
 
-  // |EntityPassDelegate|
-  std::optional<Rect> GetCoverageRect() override { return coverage_; }
-
   // |EntityPassDelgate|
   bool CanElide() override { return false; }
 
@@ -107,12 +103,12 @@ auto CreatePassWithRectPath(Rect rect,
   entity.SetContents(SolidColorContents::Make(
       PathBuilder{}.AddRect(rect).TakePath(), Color::Red()));
   subpass->AddEntity(entity);
-  subpass->SetDelegate(
-      std::make_unique<TestPassDelegate>(bounds_hint, collapse));
+  subpass->SetDelegate(std::make_unique<TestPassDelegate>(collapse));
+  subpass->SetBoundsLimit(bounds_hint);
   return subpass;
 }
 
-TEST_P(EntityTest, EntityPassCoverageRespectsDelegateBoundsHint) {
+TEST_P(EntityTest, EntityPassRespectsSubpassBoundsLimit) {
   EntityPass pass;
 
   auto subpass0 = CreatePassWithRectPath(Rect::MakeLTRB(0, 0, 100, 100),

impeller/entity/geometry/fill_path_geometry.cc

@@ -6,7 +6,9 @@
 
 namespace impeller {
 
-FillPathGeometry::FillPathGeometry(const Path& path) : path_(path) {}
+FillPathGeometry::FillPathGeometry(const Path& path,
+                                   std::optional<Rect> inner_rect)
+    : path_(path), inner_rect_(inner_rect) {}
 
 FillPathGeometry::~FillPathGeometry() = default;
 
@@ -147,4 +149,16 @@ std::optional<Rect> FillPathGeometry::GetCoverage(
   return path_.GetTransformedBoundingBox(transform);
 }
 
+bool FillPathGeometry::CoversArea(const Matrix& transform,
+                                  const Rect& rect) const {
+  if (!inner_rect_.has_value()) {
+    return false;
+  }
+  if (!transform.IsTranslationScaleOnly()) {
+    return false;
+  }
+  Rect coverage = inner_rect_->TransformBounds(transform);
+  return coverage.Contains(rect);
+}
+
 }  // namespace impeller