Revert "[Impeller] Use the scissor to limit all draws by clip coverage. (#51698)"

This reverts commit e0f86b2.

Author: auto-submit[bot]

impeller/entity/entity_pass.cc

@@ -25,7 +25,6 @@
 #include "impeller/entity/inline_pass_context.h"
 #include "impeller/geometry/color.h"
 #include "impeller/geometry/rect.h"
-#include "impeller/geometry/size.h"
 #include "impeller/renderer/command_buffer.h"
 
 #ifdef IMPELLER_DEBUG
@@ -753,25 +752,6 @@ EntityPass::EntityResult EntityPass::GetEntityForElement(
   FML_UNREACHABLE();
 }
 
-static void SetClipScissor(std::optional<Rect> clip_coverage,
-                           RenderPass& pass,
-                           Point global_pass_position) {
-  if constexpr (!ContentContext::kEnableStencilThenCover) {
-    return;
-  }
-  // Set the scissor to the clip coverage area. We do this prior to rendering
-  // the clip itself and all its contents.
-  IRect scissor;
-  if (clip_coverage.has_value()) {
-    clip_coverage = clip_coverage->Shift(-global_pass_position);
-    scissor = IRect::RoundOut(clip_coverage.value());
-    // The scissor rect must not exceed the size of the render target.
-    scissor = scissor.Intersection(IRect::MakeSize(pass.GetRenderTargetSize()))
-                  .value_or(IRect());
-  }
-  pass.SetScissor(scissor);
-}
-
 bool EntityPass::RenderElement(Entity& element_entity,
                                size_t clip_depth_floor,
                                InlinePassContext& pass_context,
@@ -791,10 +771,8 @@ bool EntityPass::RenderElement(Entity& element_entity,
     // Restore any clips that were recorded before the backdrop filter was
     // applied.
     auto& replay_entities = clip_coverage_stack.GetReplayEntities();
-    for (const auto& replay : replay_entities) {
-      SetClipScissor(clip_coverage_stack.CurrentClipCoverage(), *result.pass,
-                     global_pass_position);
-      if (!replay.entity.Render(renderer, *result.pass)) {
+    for (const auto& entity : replay_entities) {
+      if (!entity.Render(renderer, *result.pass)) {
         VALIDATION_LOG << "Failed to render entity for clip restore.";
       }
     }
@@ -848,18 +826,11 @@ bool EntityPass::RenderElement(Entity& element_entity,
   element_entity.GetContents()->SetCoverageHint(
       Rect::Intersection(element_coverage_hint, current_clip_coverage));
 
-  EntityPassClipStack::ClipStateResult clip_state_result =
-      clip_coverage_stack.ApplyClipState(clip_coverage, element_entity,
-                                         clip_depth_floor,
-                                         global_pass_position);
-
-  if (clip_state_result.clip_did_change) {
-    // We only need to update the pass scissor if the clip state has changed.
-    SetClipScissor(clip_coverage_stack.CurrentClipCoverage(), *result.pass,
-                   global_pass_position);
-  }
-
-  if (!clip_state_result.should_render) {
+  if (!clip_coverage_stack.AppendClipCoverage(clip_coverage, element_entity,
+                                              clip_depth_floor,
+                                              global_pass_position)) {
+    // If the entity's coverage change did not change the clip coverage, we
+    // don't need to render it.
     return true;
   }
 

impeller/entity/entity_pass_clip_stack.cc

@@ -49,23 +49,20 @@ EntityPassClipStack::GetClipCoverageLayers() const {
   return subpass_state_.back().clip_coverage;
 }
 
-EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState(
-    Contents::ClipCoverage global_clip_coverage,
+bool EntityPassClipStack::AppendClipCoverage(
+    Contents::ClipCoverage clip_coverage,
     Entity& entity,
     size_t clip_depth_floor,
     Point global_pass_position) {
-  ClipStateResult result = {.should_render = false, .clip_did_change = false};
-
   auto& subpass_state = GetCurrentSubpassState();
-  switch (global_clip_coverage.type) {
+  switch (clip_coverage.type) {
     case Contents::ClipCoverage::Type::kNoChange:
       break;
     case Contents::ClipCoverage::Type::kAppend: {
       auto op = CurrentClipCoverage();
       subpass_state.clip_coverage.push_back(
-          ClipCoverageLayer{.coverage = global_clip_coverage.coverage,
+          ClipCoverageLayer{.coverage = clip_coverage.coverage,
                             .clip_depth = entity.GetClipDepth() + 1});
-      result.clip_did_change = true;
 
       FML_DCHECK(subpass_state.clip_coverage.back().clip_depth ==
                  subpass_state.clip_coverage.front().clip_depth +
@@ -74,14 +71,14 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState(
       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;
+        return false;
       }
     } break;
     case Contents::ClipCoverage::Type::kRestore: {
       if (subpass_state.clip_coverage.back().clip_depth <=
           entity.GetClipDepth()) {
         // Drop clip restores that will do nothing.
-        return result;
+        return false;
       }
 
       auto restoration_index = entity.GetClipDepth() -
@@ -99,19 +96,18 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState(
         restore_coverage = restore_coverage->Shift(-global_pass_position);
       }
       subpass_state.clip_coverage.resize(restoration_index + 1);
-      result.clip_did_change = true;
 
       if constexpr (ContentContext::kEnableStencilThenCover) {
         // Skip all clip restores when stencil-then-cover is enabled.
         if (subpass_state.clip_coverage.back().coverage.has_value()) {
-          RecordEntity(entity, global_clip_coverage.type, Rect());
+          RecordEntity(entity, clip_coverage.type);
         }
-        return result;
+        return false;
       }
 
       if (!subpass_state.clip_coverage.back().coverage.has_value()) {
         // Running this restore op won't make anything renderable, so skip it.
-        return result;
+        return false;
       }
 
       auto restore_contents =
@@ -134,23 +130,19 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState(
 #endif
 
   entity.SetClipDepth(entity.GetClipDepth() - clip_depth_floor);
-  RecordEntity(entity, global_clip_coverage.type,
-               subpass_state.clip_coverage.back().coverage);
+  RecordEntity(entity, clip_coverage.type);
 
-  result.should_render = true;
-  return result;
+  return true;
 }
 
 void EntityPassClipStack::RecordEntity(const Entity& entity,
-                                       Contents::ClipCoverage::Type type,
-                                       std::optional<Rect> clip_coverage) {
+                                       Contents::ClipCoverage::Type type) {
   auto& subpass_state = GetCurrentSubpassState();
   switch (type) {
     case Contents::ClipCoverage::Type::kNoChange:
       return;
     case Contents::ClipCoverage::Type::kAppend:
-      subpass_state.rendered_clip_entities.push_back(
-          {.entity = entity.Clone(), .clip_coverage = clip_coverage});
+      subpass_state.rendered_clip_entities.push_back(entity.Clone());
       break;
     case Contents::ClipCoverage::Type::kRestore:
       if (!subpass_state.rendered_clip_entities.empty()) {
@@ -165,8 +157,7 @@ EntityPassClipStack::GetCurrentSubpassState() {
   return subpass_state_.back();
 }
 
-const std::vector<EntityPassClipStack::ReplayResult>&
-EntityPassClipStack::GetReplayEntities() const {
+const std::vector<Entity>& EntityPassClipStack::GetReplayEntities() const {
   return subpass_state_.back().rendered_clip_entities;
 }
 

impeller/entity/entity_pass_clip_stack.h

@@ -6,8 +6,6 @@
 #define FLUTTER_IMPELLER_ENTITY_ENTITY_PASS_CLIP_STACK_H_
 
 #include "impeller/entity/contents/contents.h"
-#include "impeller/entity/entity.h"
-#include "impeller/geometry/rect.h"
 
 namespace impeller {
 
@@ -23,20 +21,6 @@ struct ClipCoverageLayer {
 ///        stencil buffer is left in an identical state.
 class EntityPassClipStack {
  public:
-  struct ReplayResult {
-    Entity entity;
-    std::optional<Rect> clip_coverage;
-  };
-
-  struct ClipStateResult {
-    /// Whether or not the Entity should be rendered. If false, the Entity may
-    /// be safely skipped.
-    bool should_render = false;
-    /// Whether or not the current clip coverage changed during the call to
-    /// `ApplyClipState`.
-    bool clip_did_change = false;
-  };
-
   /// Create a new [EntityPassClipStack] with an initialized coverage rect.
   explicit EntityPassClipStack(const Rect& initial_coverage_rect);
 
@@ -50,27 +34,24 @@ class EntityPassClipStack {
 
   bool HasCoverage() const;
 
-  /// @brief  Applies the current clip state to an Entity. If the given Entity
-  ///         is a clip operation, then the clip state is updated accordingly.
-  ClipStateResult ApplyClipState(Contents::ClipCoverage global_clip_coverage,
-                                 Entity& entity,
-                                 size_t clip_depth_floor,
-                                 Point global_pass_position);
+  /// Returns true if entity should be rendered.
+  bool AppendClipCoverage(Contents::ClipCoverage clip_coverage,
+                          Entity& entity,
+                          size_t clip_depth_floor,
+                          Point global_pass_position);
 
   // Visible for testing.
-  void RecordEntity(const Entity& entity,
-                    Contents::ClipCoverage::Type type,
-                    std::optional<Rect> clip_coverage);
+  void RecordEntity(const Entity& entity, Contents::ClipCoverage::Type type);
 
   // Visible for testing.
-  const std::vector<ReplayResult>& GetReplayEntities() const;
+  const std::vector<Entity>& GetReplayEntities() const;
 
   // Visible for testing.
   const std::vector<ClipCoverageLayer> GetClipCoverageLayers() const;
 
  private:
   struct SubpassState {
-    std::vector<ReplayResult> rendered_clip_entities;
+    std::vector<Entity> rendered_clip_entities;
     std::vector<ClipCoverageLayer> clip_coverage;
   };
 

impeller/entity/entity_pass_unittests.cc

@@ -17,26 +17,16 @@ TEST(EntityPassClipStackTest, CanPushAndPopEntities) {
   EXPECT_TRUE(recorder.GetReplayEntities().empty());
 
   Entity entity;
-  recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend,
-                        Rect::MakeLTRB(0, 0, 100, 100));
+  recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend);
   EXPECT_EQ(recorder.GetReplayEntities().size(), 1u);
 
-  recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend,
-                        Rect::MakeLTRB(0, 0, 50, 50));
+  recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend);
   EXPECT_EQ(recorder.GetReplayEntities().size(), 2u);
-  ASSERT_TRUE(recorder.GetReplayEntities()[0].clip_coverage.has_value());
-  ASSERT_TRUE(recorder.GetReplayEntities()[1].clip_coverage.has_value());
-  // NOLINTBEGIN(bugprone-unchecked-optional-access)
-  EXPECT_EQ(recorder.GetReplayEntities()[0].clip_coverage.value(),
-            Rect::MakeLTRB(0, 0, 100, 100));
-  EXPECT_EQ(recorder.GetReplayEntities()[1].clip_coverage.value(),
-            Rect::MakeLTRB(0, 0, 50, 50));
-  // NOLINTEND(bugprone-unchecked-optional-access)
-
-  recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore, Rect());
+
+  recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore);
   EXPECT_EQ(recorder.GetReplayEntities().size(), 1u);
 
-  recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore, Rect());
+  recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore);
   EXPECT_TRUE(recorder.GetReplayEntities().empty());
 }
 
@@ -47,7 +37,7 @@ TEST(EntityPassClipStackTest, CanPopEntitiesSafely) {
   EXPECT_TRUE(recorder.GetReplayEntities().empty());
 
   Entity entity;
-  recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore, Rect());
+  recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore);
   EXPECT_TRUE(recorder.GetReplayEntities().empty());
 }
 
@@ -58,8 +48,7 @@ TEST(EntityPassClipStackTest, CanAppendNoChange) {
   EXPECT_TRUE(recorder.GetReplayEntities().empty());
 
   Entity entity;
-  recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kNoChange,
-                        Rect());
+  recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kNoChange);
   EXPECT_TRUE(recorder.GetReplayEntities().empty());
 }
 
@@ -72,14 +61,12 @@ TEST(EntityPassClipStackTest, AppendCoverageNoChange) {
   EXPECT_EQ(recorder.GetClipCoverageLayers()[0].clip_depth, 0u);
 
   Entity entity;
-  EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState(
+  recorder.AppendClipCoverage(
       Contents::ClipCoverage{
           .type = Contents::ClipCoverage::Type::kNoChange,
           .coverage = std::nullopt,
       },
       entity, 0, Point(0, 0));
-  EXPECT_TRUE(result.should_render);
-  EXPECT_FALSE(result.clip_did_change);
 
   EXPECT_EQ(recorder.GetClipCoverageLayers()[0].coverage,
             Rect::MakeSize(Size::MakeWH(100, 100)));
@@ -95,14 +82,12 @@ TEST(EntityPassClipStackTest, AppendAndRestoreClipCoverage) {
   // Push a clip.
   Entity entity;
   entity.SetClipDepth(0);
-  EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState(
+  recorder.AppendClipCoverage(
       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);
 
   ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 2u);
   EXPECT_EQ(recorder.GetClipCoverageLayers()[1].coverage,
@@ -112,7 +97,7 @@ TEST(EntityPassClipStackTest, AppendAndRestoreClipCoverage) {
 
   // Restore the clip.
   entity.SetClipDepth(0);
-  recorder.ApplyClipState(
+  recorder.AppendClipCoverage(
       Contents::ClipCoverage{
           .type = Contents::ClipCoverage::Type::kRestore,
           .coverage = Rect::MakeLTRB(50, 50, 55, 55),
@@ -135,14 +120,12 @@ TEST(EntityPassClipStackTest, UnbalancedRestore) {
   // Restore the clip.
   Entity entity;
   entity.SetClipDepth(0);
-  EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState(
+  recorder.AppendClipCoverage(
       Contents::ClipCoverage{
           .type = Contents::ClipCoverage::Type::kRestore,
           .coverage = Rect::MakeLTRB(50, 50, 55, 55),
       },
       entity, 0, Point(0, 0));
-  EXPECT_FALSE(result.should_render);
-  EXPECT_FALSE(result.clip_did_change);
 
   ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u);
   EXPECT_EQ(recorder.GetClipCoverageLayers()[0].coverage,
@@ -160,16 +143,12 @@ TEST(EntityPassClipStackTest, ClipAndRestoreWithSubpasses) {
   // Push a clip.
   Entity entity;
   entity.SetClipDepth(0u);
-  {
-    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);
-  }
+  recorder.AppendClipCoverage(
+      Contents::ClipCoverage{
+          .type = Contents::ClipCoverage::Type::kAppend,
+          .coverage = Rect::MakeLTRB(50, 50, 55, 55),
+      },
+      entity, 0, Point(0, 0));
 
   ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 2u);
   EXPECT_EQ(recorder.GetClipCoverageLayers()[1].coverage,
@@ -184,16 +163,12 @@ TEST(EntityPassClipStackTest, ClipAndRestoreWithSubpasses) {
             Rect::MakeLTRB(50, 50, 55, 55));
 
   entity.SetClipDepth(1);
-  {
-    EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState(
-        Contents::ClipCoverage{
-            .type = Contents::ClipCoverage::Type::kAppend,
-            .coverage = Rect::MakeLTRB(54, 54, 55, 55),
-        },
-        entity, 0, Point(0, 0));
-    EXPECT_TRUE(result.should_render);
-    EXPECT_TRUE(result.clip_did_change);
-  }
+  recorder.AppendClipCoverage(
+      Contents::ClipCoverage{
+          .type = Contents::ClipCoverage::Type::kAppend,
+          .coverage = Rect::MakeLTRB(54, 54, 55, 55),
+      },
+      entity, 0, Point(0, 0));
 
   EXPECT_EQ(recorder.GetClipCoverageLayers()[1].coverage,
             Rect::MakeLTRB(54, 54, 55, 55));