Don't let subpass coverage exceed entity coverage.

Author: chinmaygarde

impeller/aiks/aiks_unittests.cc

@@ -197,9 +197,8 @@ TEST_F(AiksTest, CanPerformSaveLayerWithBounds) {
   ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
 }
 
-TEST_F(
-    AiksTest,
-    DISABLED_CanPerformSaveLayerWithBoundsAndLargerIntermediateIsNotAllocated) {
+TEST_F(AiksTest,
+       CanPerformSaveLayerWithBoundsAndLargerIntermediateIsNotAllocated) {
   Canvas canvas;
 
   Paint red;

impeller/aiks/canvas.cc

@@ -132,6 +132,7 @@ void Canvas::ClipPath(Path path) {
   entity.SetPath(std::move(path));
   entity.SetContents(std::make_shared<ClipContents>());
   entity.SetStencilDepth(GetStencilDepth());
+  entity.SetAddsToCoverage(false);
 
   GetCurrentPass().AddEntity(std::move(entity));
 }

impeller/entity/contents.cc

@@ -205,7 +205,12 @@ bool TextureContents::Render(const ContentRenderer& renderer,
   using FS = TextureFillFragmentShader;
 
   const auto coverage_rect = entity.GetPath().GetBoundingBox();
-  if (coverage_rect.size.IsEmpty()) {
+
+  if (!coverage_rect.has_value()) {
+    return true;
+  }
+
+  if (coverage_rect->size.IsEmpty()) {
     return true;
   }
 
@@ -226,7 +231,7 @@ bool TextureContents::Render(const ContentRenderer& renderer,
           VS::PerVertexData data;
           data.vertices = vtx;
           data.texture_coords =
-              ((vtx - coverage_rect.origin) / coverage_rect.size);
+              ((vtx - coverage_rect->origin) / coverage_rect->size);
           vertex_builder.AppendVertex(data);
         });
     if (!tess_result) {

impeller/entity/entity.cc

@@ -29,6 +29,22 @@ void Entity::SetPath(Path path) {
   path_ = std::move(path);
 }
 
+void Entity::SetAddsToCoverage(bool adds) {
+  adds_to_coverage_ = adds;
+}
+
+bool Entity::AddsToCoverage() const {
+  return adds_to_coverage_;
+}
+
+std::optional<Rect> Entity::GetCoverage() const {
+  if (!adds_to_coverage_) {
+    return std::nullopt;
+  }
+
+  return path_.GetBoundingBox();
+}
+
 void Entity::SetContents(std::shared_ptr<Contents> contents) {
   contents_ = std::move(contents);
 }

impeller/entity/entity.h

@@ -30,6 +30,12 @@ class Entity {
 
   void SetPath(Path path);
 
+  void SetAddsToCoverage(bool adds);
+
+  bool AddsToCoverage() const;
+
+  std::optional<Rect> GetCoverage() const;
+
   void SetContents(std::shared_ptr<Contents> contents);
 
   const std::shared_ptr<Contents>& GetContents() const;
@@ -47,6 +53,7 @@ class Entity {
   std::shared_ptr<Contents> contents_;
   Path path_;
   uint32_t stencil_depth_ = 0u;
+  bool adds_to_coverage_ = true;
 };
 
 }  // namespace impeller

impeller/entity/entity_pass.cc

@@ -43,27 +43,41 @@ size_t EntityPass::GetSubpassesDepth() const {
   return max_subpass_depth + 1u;
 }
 
-Rect EntityPass::GetCoverageRect() const {
-  std::optional<Point> min, max;
+std::optional<Rect> EntityPass::GetEntitiesCoverage() const {
+  std::optional<Rect> result;
   for (const auto& entity : entities_) {
-    auto coverage = entity.GetPath().GetMinMaxCoveragePoints();
-    if (!coverage.has_value()) {
+    auto coverage = entity.GetCoverage();
+    if (!result.has_value() && coverage.has_value()) {
+      result = coverage;
       continue;
     }
-    if (!min.has_value()) {
-      min = coverage->first;
-    }
-    if (!max.has_value()) {
-      max = coverage->second;
+    if (!coverage.has_value()) {
+      continue;
     }
-    min = min->Min(coverage->first);
-    max = max->Max(coverage->second);
+    result = result->Union(coverage.value());
+  }
+  return result;
+}
+
+std::optional<Rect> EntityPass::GetSubpassCoverage(
+    const EntityPass& subpass) const {
+  auto entities_coverage = subpass.GetEntitiesCoverage();
+  // The entities don't cover anything. There is nothing to do.
+  if (!entities_coverage.has_value()) {
+    return std::nullopt;
   }
-  if (!min.has_value() || !max.has_value()) {
-    return {};
+
+  // The delegates don't have an opinion on what the entity coverage has to be.
+  // Just use that as-is.
+  auto delegate_coverage = delegate_->GetCoverageRect();
+  if (!delegate_coverage.has_value()) {
+    return entities_coverage;
   }
-  const auto diff = *max - *min;
-  return {min->x, min->y, diff.x, diff.y};
+
+  // If the delete 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());
 }
 
 EntityPass* EntityPass::GetSuperpass() const {
@@ -74,18 +88,6 @@ const EntityPass::Subpasses& EntityPass::GetSubpasses() const {
   return subpasses_;
 }
 
-Rect EntityPass::GetSubpassCoverage(const EntityPass& subpass) const {
-  auto subpass_coverage = subpass.GetCoverageRect();
-  auto delegate_coverage =
-      delegate_->GetCoverageRect().value_or(subpass_coverage);
-  Rect coverage;
-  coverage.origin = subpass_coverage.origin;
-  // TODO(csg): This must still be restricted to the max texture size. Or,
-  // decide if this must be done by the allocator.
-  coverage.size = subpass_coverage.size.Min(delegate_coverage.size);
-  return coverage;
-}
-
 EntityPass* EntityPass::AddSubpass(std::unique_ptr<EntityPass> pass) {
   if (!pass) {
     return nullptr;
@@ -117,7 +119,11 @@ bool EntityPass::Render(ContentRenderer& renderer,
 
     const auto subpass_coverage = GetSubpassCoverage(*subpass);
 
-    if (subpass_coverage.IsEmpty()) {
+    if (!subpass_coverage.has_value()) {
+      continue;
+    }
+
+    if (subpass_coverage->size.IsEmpty()) {
       // It is not an error to have an empty subpass. But subpasses that can't
       // create their intermediates must trip errors.
       continue;
@@ -126,7 +132,7 @@ bool EntityPass::Render(ContentRenderer& renderer,
     auto context = renderer.GetContext();
 
     auto subpass_target = RenderTarget::CreateOffscreen(
-        *context, ISize::Ceil(subpass_coverage.size));
+        *context, ISize::Ceil(subpass_coverage->size));
 
     auto subpass_texture = subpass_target.GetRenderTargetTexture();
 
@@ -178,7 +184,8 @@ bool EntityPass::Render(ContentRenderer& renderer,
     }
 
     Entity entity;
-    entity.SetPath(PathBuilder{}.AddRect(subpass_coverage).CreatePath());
+    entity.SetPath(
+        PathBuilder{}.AddRect(subpass_coverage.value()).CreatePath());
     entity.SetContents(std::move(offscreen_texture_contents));
     entity.SetStencilDepth(stencil_depth_);
     entity.SetTransformation(xformation_);

impeller/entity/entity_pass.h

@@ -33,10 +33,6 @@ class EntityPass {
 
   std::unique_ptr<EntityPass> Clone() const;
 
-  Rect GetCoverageRect() const;
-
-  // TODO(csg): This prevents an optimization where the coverage can be
-  // calculated once in SetEntities an memoized.
   void AddEntity(Entity entity);
 
   void SetEntities(Entities entities);
@@ -66,7 +62,9 @@ class EntityPass {
   std::unique_ptr<EntityPassDelegate> delegate_ =
       EntityPassDelegate::MakeDefault();
 
-  Rect GetSubpassCoverage(const EntityPass& subpass) const;
+  std::optional<Rect> GetSubpassCoverage(const EntityPass& subpass) const;
+
+  std::optional<Rect> GetEntitiesCoverage() const;
 
   FML_DISALLOW_COPY_AND_ASSIGN(EntityPass);
 };

impeller/geometry/geometry_unittests.cc

@@ -185,7 +185,8 @@ TEST(GeometryTest, BoundingBoxCubic) {
   path.AddCubicComponent({120, 160}, {25, 200}, {220, 260}, {220, 40});
   auto box = path.GetBoundingBox();
   Rect expected(93.9101, 40, 126.09, 158.862);
-  ASSERT_RECT_NEAR(box, expected);
+  ASSERT_TRUE(box.has_value());
+  ASSERT_RECT_NEAR(box.value(), expected);
 }
 
 TEST(GeometryTest, BoundingBoxOfCompositePathIsCorrect) {
@@ -194,7 +195,8 @@ TEST(GeometryTest, BoundingBoxOfCompositePathIsCorrect) {
   auto path = builder.CreatePath();
   auto actual = path.GetBoundingBox();
   Rect expected(10, 10, 300, 300);
-  ASSERT_RECT_NEAR(actual, expected);
+  ASSERT_TRUE(actual.has_value());
+  ASSERT_RECT_NEAR(actual.value(), expected);
 }
 
 TEST(GeometryTest, CanGenerateMipCounts) {

impeller/geometry/path.cc

@@ -173,15 +173,15 @@ std::vector<Point> Path::CreatePolyline(
   return points;
 }
 
-Rect Path::GetBoundingBox() const {
+std::optional<Rect> Path::GetBoundingBox() const {
   auto min_max = GetMinMaxCoveragePoints();
   if (!min_max.has_value()) {
-    return {};
+    return std::nullopt;
   }
   auto min = min_max->first;
   auto max = min_max->second;
   const auto difference = max - min;
-  return {min.x, min.y, difference.x, difference.y};
+  return Rect{min.x, min.y, difference.x, difference.y};
 }
 
 std::optional<std::pair<Point, Point>> Path::GetMinMaxCoveragePoints() const {

impeller/geometry/path.h

@@ -67,7 +67,7 @@ class Path {
   std::vector<Point> CreatePolyline(
       const SmoothingApproximation& approximation = {}) const;
 
-  Rect GetBoundingBox() const;
+  std::optional<Rect> GetBoundingBox() const;
 
   std::optional<std::pair<Point, Point>> GetMinMaxCoveragePoints() const;
 

impeller/renderer/renderer_unittests.cc

@@ -266,7 +266,7 @@ TEST_F(RendererTest, CanRenderToTexture) {
 
 TEST_F(RendererTest, CanRenderPath) {
   auto path = PathBuilder{}.AddCircle({550, 550}, 500).CreatePath();
-  ASSERT_FALSE(path.GetBoundingBox().IsZero());
+  ASSERT_FALSE(path.GetBoundingBox().has_value());
 
   using BoxPipeline = PipelineT<BoxFadeVertexShader, BoxFadeFragmentShader>;
   using VS = BoxFadeVertexShader;