[Impeller] fix clip culling with exp canvas.

impeller/aiks/experimental_canvas.cc

@@ -10,6 +10,7 @@
 #include "impeller/base/validation.h"
 #include "impeller/core/allocator.h"
 #include "impeller/core/formats.h"
+#include "impeller/entity/contents/clip_contents.h"
 #include "impeller/entity/contents/framebuffer_blend_contents.h"
 #include "impeller/entity/contents/text_contents.h"
 #include "impeller/entity/entity.h"
@@ -431,6 +432,12 @@ void ExperimentalCanvas::SaveLayer(
   entry.rendering_mode = Entity::RenderingMode::kSubpassAppendSnapshotTransform;
   transform_stack_.emplace_back(entry);
 
+  // The current clip aiks clip culling can not handle image filters.
+  // Remove this once we've migrated to exp canvas and removed it.
+  if (paint.image_filter) {
+    transform_stack_.back().cull_rect = std::nullopt;
+  }
+
   // Start non-collapsed subpasses with a fresh clip coverage stack limited by
   // the subpass coverage. This is important because image filters applied to
   // save layers may transform the subpass texture after it's rendered,
@@ -752,6 +759,7 @@ void ExperimentalCanvas::AddClipEntityToCurrentPass(Entity entity) {
   auto transform = entity.GetTransform();
   entity.SetTransform(
       Matrix::MakeTranslation(Vector3(-GetGlobalPassPosition())) * transform);
+
   // Ideally the clip depth would be greater than the current rendering
   // depth because any rendering calls that follow this clip operation will
   // pre-increment the depth and then be rendering above our clip depth,

impeller/entity/contents/clip_contents.cc

@@ -53,8 +53,11 @@ Contents::ClipCoverage ClipContents::GetClipCoverage(
     case Entity::ClipOperation::kDifference:
       // This can be optimized further by considering cases when the bounds of
       // the current stencil will shrink.
-      return {.type = ClipCoverage::Type::kAppend,
-              .coverage = current_clip_coverage};
+      return {
+          .type = ClipCoverage::Type::kAppend,  //
+          .is_difference_or_non_square = true,  //
+          .coverage = current_clip_coverage     //
+      };
     case Entity::ClipOperation::kIntersect:
       if (!geometry_) {
         return {.type = ClipCoverage::Type::kAppend, .coverage = std::nullopt};
@@ -64,8 +67,9 @@ Contents::ClipCoverage ClipContents::GetClipCoverage(
         return {.type = ClipCoverage::Type::kAppend, .coverage = std::nullopt};
       }
       return {
-          .type = ClipCoverage::Type::kAppend,
-          .coverage = current_clip_coverage->Intersection(coverage.value()),
+          .type = ClipCoverage::Type::kAppend,                                //
+          .is_difference_or_non_square = !geometry_->IsAxisAlignedRect(),     //
+          .coverage = current_clip_coverage->Intersection(coverage.value()),  //
       };
   }
   FML_UNREACHABLE();
@@ -91,19 +95,6 @@ bool ClipContents::Render(const ContentContext& renderer,
 
   using VS = ClipPipeline::VertexShader;
 
-  if (clip_op_ == Entity::ClipOperation::kIntersect &&
-      geometry_->IsAxisAlignedRect() &&
-      entity.GetTransform().IsTranslationScaleOnly()) {
-    std::optional<Rect> coverage =
-        geometry_->GetCoverage(entity.GetTransform());
-    if (coverage.has_value() &&
-        coverage->Contains(Rect::MakeSize(pass.GetRenderTargetSize()))) {
-      // Skip axis-aligned intersect clips that cover the whole render target
-      // since they won't draw anything to the depth buffer.
-      return true;
-    }
-  }
-
   VS::FrameInfo info;
   info.depth = GetShaderClipDepth(entity);
 

impeller/entity/contents/contents.h

@@ -38,6 +38,17 @@ class Contents {
     enum class Type { kNoChange, kAppend, kRestore };
 
     Type type = Type::kNoChange;
+    // TODO(jonahwilliams): this should probably use the Entity::ClipOperation
+    // enum, but that has transitive import errors.
+    bool is_difference_or_non_square = false;
+
+    /// @brief This coverage is the outer coverage of the clip.
+    ///
+    /// For example, if the clip is a circular clip, this is the rectangle that
+    /// contains the circle and not the rectangle that is contained within the
+    /// circle. This means that we cannot use the coverage alone to determine if
+    /// a clip can be culled, and instead also use the somewhat hacky
+    /// "is_difference_or_non_square" field.
     std::optional<Rect> coverage = std::nullopt;
   };
 

impeller/entity/entity_pass_clip_stack.cc

@@ -60,7 +60,7 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState(
     case Contents::ClipCoverage::Type::kNoChange:
       break;
     case Contents::ClipCoverage::Type::kAppend: {
-      auto op = CurrentClipCoverage();
+      auto maybe_coverage = CurrentClipCoverage();
 
       // Compute the previous clip height.
       size_t previous_clip_height = 0;
@@ -72,6 +72,24 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState(
         previous_clip_height = clip_height_floor;
       }
 
+      if (!maybe_coverage.has_value()) {
+        // Running this append op won't impact the clip buffer because the
+        // whole screen is already being clipped, so skip it.
+        return result;
+      }
+      auto op = maybe_coverage.value();
+
+      // If the new clip coverage is bigger than the existing coverage for
+      // intersect clips, we do not need to change the clip region.
+      if (!global_clip_coverage.is_difference_or_non_square &&
+          global_clip_coverage.coverage.has_value() &&
+          global_clip_coverage.coverage.value().Contains(op)) {
+        subpass_state.clip_coverage.push_back(ClipCoverageLayer{
+            .coverage = op, .clip_height = previous_clip_height + 1});
+
+        return result;
+      }
+
       subpass_state.clip_coverage.push_back(
           ClipCoverageLayer{.coverage = global_clip_coverage.coverage,
                             .clip_height = previous_clip_height + 1});
@@ -81,11 +99,6 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState(
                  subpass_state.clip_coverage.front().clip_height +
                      subpass_state.clip_coverage.size() - 1);
 
-      if (!op.has_value()) {
-        // Running this append op won't impact the clip buffer because the
-        // whole screen is already being clipped, so skip it.
-        return result;
-      }
     } break;
     case Contents::ClipCoverage::Type::kRestore: {
       ClipRestoreContents* restore_contents =

impeller/entity/entity_pass_unittests.cc

@@ -130,6 +130,115 @@ TEST(EntityPassClipStackTest, AppendAndRestoreClipCoverage) {
   EXPECT_EQ(recorder.GetReplayEntities().size(), 0u);
 }
 
+// Append two clip coverages, the second is larger the first. This
+// should result in the second clip not requiring any update.
+TEST(EntityPassClipStackTest, AppendLargerClipCoverage) {
+  EntityPassClipStack recorder =
+      EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100));
+
+  ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u);
+
+  // Push a clip.
+  Entity entity;
+  EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState(
+      Contents::ClipCoverage{
+          .type = Contents::ClipCoverage::Type::kAppend,
+          .coverage = Rect::MakeLTRB(50, 50, 55, 55),
+      },
+      entity, 0, Point(0, 0));
+  EXPECT_TRUE(result.should_render);
+  EXPECT_TRUE(result.clip_did_change);
+
+  // Push a clip with larger coverage than the previous state.
+  result = recorder.ApplyClipState(
+      Contents::ClipCoverage{
+          .type = Contents::ClipCoverage::Type::kAppend,
+          .coverage = Rect::MakeLTRB(0, 0, 100, 100),
+      },
+      entity, 0, Point(0, 0));
+
+  EXPECT_FALSE(result.should_render);
+  EXPECT_FALSE(result.clip_did_change);
+}
+
+// Since clip entities return the outer coverage we can only cull axis aligned
+// rectangles and intersect clips.
+TEST(EntityPassClipStackTest,
+     AppendLargerClipCoverageWithDifferenceOrNonSquare) {
+  EntityPassClipStack recorder =
+      EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100));
+
+  ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u);
+
+  // Push a clip.
+  Entity entity;
+  EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState(
+      Contents::ClipCoverage{
+          .type = Contents::ClipCoverage::Type::kAppend,
+          .coverage = Rect::MakeLTRB(50, 50, 55, 55),
+      },
+      entity, 0, Point(0, 0));
+  EXPECT_TRUE(result.should_render);
+  EXPECT_TRUE(result.clip_did_change);
+
+  // Push a clip with larger coverage than the previous state.
+  result = recorder.ApplyClipState(
+      Contents::ClipCoverage{
+          .type = Contents::ClipCoverage::Type::kAppend,
+          .is_difference_or_non_square = true,
+          .coverage = Rect::MakeLTRB(0, 0, 100, 100),
+      },
+      entity, 0, Point(0, 0));
+
+  EXPECT_TRUE(result.should_render);
+  EXPECT_TRUE(result.clip_did_change);
+}
+
+TEST(EntityPassClipStackTest, AppendDecreasingSizeClipCoverage) {
+  EntityPassClipStack recorder =
+      EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100));
+
+  ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u);
+
+  // Push Clips that shrink in size. All should be applied.
+  Entity entity;
+
+  for (auto i = 1; i < 20; i++) {
+    EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState(
+        Contents::ClipCoverage{
+            .type = Contents::ClipCoverage::Type::kAppend,
+            .coverage = Rect::MakeLTRB(i, i, 100 - i, 100 - i),
+        },
+        entity, 0, Point(0, 0));
+    EXPECT_TRUE(result.should_render);
+    EXPECT_TRUE(result.clip_did_change);
+    EXPECT_EQ(recorder.CurrentClipCoverage(),
+              Rect::MakeLTRB(i, i, 100 - i, 100 - i));
+  }
+}
+
+TEST(EntityPassClipStackTest, AppendIncreasingSizeClipCoverage) {
+  EntityPassClipStack recorder =
+      EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100));
+
+  ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u);
+
+  // Push Clips that grow in size. All should be skipped.
+  Entity entity;
+
+  for (auto i = 1; i < 20; i++) {
+    EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState(
+        Contents::ClipCoverage{
+            .type = Contents::ClipCoverage::Type::kAppend,
+            .coverage = Rect::MakeLTRB(0 - i, 0 - i, 100 + i, 100 + i),
+        },
+        entity, 0, Point(0, 0));
+    EXPECT_FALSE(result.should_render);
+    EXPECT_FALSE(result.clip_did_change);
+    EXPECT_EQ(recorder.CurrentClipCoverage(), Rect::MakeLTRB(0, 0, 100, 100));
+  }
+}
+
 TEST(EntityPassClipStackTest, UnbalancedRestore) {
   EntityPassClipStack recorder =
       EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100));