From 7f9deb8308467174861e0108aa31c3e1f4fc029c Mon Sep 17 00:00:00 2001
From: jonahwilliams <jonahwilliams@google.com>
Date: Mon, 6 Mar 2023 13:43:53 -0800
Subject: [PATCH 01/10] [Impeller] basic opacity peephole

---
 impeller/aiks/canvas.cc                       | 11 ++-
 impeller/aiks/paint_pass_delegate.cc          | 93 ++++++++++++++++++-
 impeller/aiks/paint_pass_delegate.h           | 33 ++++++-
 impeller/entity/contents/clip_contents.cc     | 12 +++
 impeller/entity/contents/clip_contents.h      | 11 +++
 .../entity/contents/color_source_contents.cc  |  8 ++
 .../entity/contents/color_source_contents.h   |  6 ++
 impeller/entity/contents/contents.cc          |  8 ++
 impeller/entity/contents/contents.h           | 13 +++
 .../entity/contents/solid_color_contents.cc   | 11 +++
 .../entity/contents/solid_color_contents.h    |  7 ++
 impeller/entity/entity_pass.cc                | 26 +++++-
 impeller/entity/entity_pass.h                 |  6 ++
 impeller/entity/entity_pass_delegate.cc       |  5 +-
 impeller/entity/entity_pass_delegate.h        |  6 +-
 impeller/entity/entity_unittests.cc           |  2 +-
 impeller/entity/geometry.cc                   | 13 +++
 impeller/entity/geometry.h                    | 13 +++
 18 files changed, 275 insertions(+), 9 deletions(-)

diff --git a/impeller/aiks/canvas.cc b/impeller/aiks/canvas.cc
index 0b86986c34a3e..222792fcc8697 100644
--- a/impeller/aiks/canvas.cc
+++ b/impeller/aiks/canvas.cc
@@ -362,8 +362,15 @@ void Canvas::SaveLayer(
   Save(true, paint.blend_mode, backdrop_filter);
 
   auto& new_layer_pass = GetCurrentPass();
-  new_layer_pass.SetDelegate(
-      std::make_unique<PaintPassDelegate>(paint, bounds));
+
+  // Only apply opacity peephole on default blending.
+  if (paint.blend_mode == BlendMode::kSourceOver) {
+    new_layer_pass.SetDelegate(
+        std::make_unique<OpacityPeepholePassDelegate>(paint, bounds));
+  } else {
+    new_layer_pass.SetDelegate(
+        std::make_unique<PaintPassDelegate>(paint, bounds));
+  }
 
   if (bounds.has_value() && !backdrop_filter.has_value()) {
     // Render target switches due to a save layer can be elided. In such cases
diff --git a/impeller/aiks/paint_pass_delegate.cc b/impeller/aiks/paint_pass_delegate.cc
index a06128a534ba2..8d1792e55464a 100644
--- a/impeller/aiks/paint_pass_delegate.cc
+++ b/impeller/aiks/paint_pass_delegate.cc
@@ -6,10 +6,14 @@
 
 #include "impeller/entity/contents/contents.h"
 #include "impeller/entity/contents/texture_contents.h"
+#include "impeller/entity/entity_pass.h"
 #include "impeller/geometry/path_builder.h"
 
 namespace impeller {
 
+/// PaintPassDelegate
+/// ----------------------------------------------
+
 PaintPassDelegate::PaintPassDelegate(Paint paint, std::optional<Rect> coverage)
     : paint_(std::move(paint)), coverage_(coverage) {}
 
@@ -27,7 +31,7 @@ bool PaintPassDelegate::CanElide() {
 }
 
 // |EntityPassDelgate|
-bool PaintPassDelegate::CanCollapseIntoParentPass() {
+bool PaintPassDelegate::CanCollapseIntoParentPass(EntityPass* entity_pass) {
   return false;
 }
 
@@ -44,4 +48,91 @@ std::shared_ptr<Contents> PaintPassDelegate::CreateContentsForSubpassTarget(
                                             effect_transform);
 }
 
+/// OpacityPeepholePassDelegate
+/// ----------------------------------------------
+
+OpacityPeepholePassDelegate::OpacityPeepholePassDelegate(
+    Paint paint,
+    std::optional<Rect> coverage)
+    : paint_(std::move(paint)), coverage_(coverage) {}
+
+// |EntityPassDelgate|
+OpacityPeepholePassDelegate::~OpacityPeepholePassDelegate() = default;
+
+// |EntityPassDelgate|
+std::optional<Rect> OpacityPeepholePassDelegate::GetCoverageRect() {
+  return coverage_;
+}
+
+// |EntityPassDelgate|
+bool OpacityPeepholePassDelegate::CanElide() {
+  return paint_.blend_mode == BlendMode::kDestination;
+}
+
+// |EntityPassDelgate|
+bool OpacityPeepholePassDelegate::CanCollapseIntoParentPass(
+    EntityPass* entity_pass) {
+  // Note: determing whether any coverage intersects has quadradic complexity in
+  // the number of rectangles, and depending on whether or not we cache at
+  // different levels of the entity tree may end up cubic. In the interest of
+  // proving whether or not this optimization is valuable, we only consider very
+  // simple peephole optimizations here - where there is a single drawing
+  // command wrapped in save layer. This would indicate something like an
+  // Opacity or FadeTransition wrapping a very simple widget, like in the
+  // CupertinoPicker.
+  if (entity_pass->GetEntityCount() > 3) {
+    // Single paint command with a save layer would be:
+    // 1. clip
+    // 2. draw command
+    // 3. restore.
+    return false;
+  }
+  bool all_can_accept = true;
+  std::vector<Rect> all_coverages;
+  auto i = 0;
+  auto had_subpass = entity_pass->IterateAllFlatEntities([&](Entity& entity) {
+    auto contents = entity.GetContents();
+    if (!contents->CanAcceptOpacity()) {
+      std::cerr << "Cant accept opacity" << i << std::endl;
+      all_can_accept = false;
+      return false;
+    }
+    i++;
+    auto maybe_coverage = contents->GetCoverage(entity);
+    if (maybe_coverage.has_value()) {
+      auto coverage = maybe_coverage.value();
+      for (const auto& cv : all_coverages) {
+        if (cv.IntersectsWithRect(coverage)) {
+          all_can_accept = false;
+          return false;
+        }
+      }
+      all_coverages.push_back(coverage);
+    }
+    return true;
+  });
+  if (had_subpass || !all_can_accept) {
+    return false;
+  }
+  entity_pass->IterateAllFlatEntities([&](Entity& entity) {
+    entity.GetContents()->InheritOpacity(paint_.color.alpha);
+    return true;
+  });
+  return true;
+}
+
+// |EntityPassDelgate|
+std::shared_ptr<Contents>
+OpacityPeepholePassDelegate::CreateContentsForSubpassTarget(
+    std::shared_ptr<Texture> target,
+    const Matrix& effect_transform) {
+  auto contents = TextureContents::MakeRect(Rect::MakeSize(target->GetSize()));
+  contents->SetTexture(target);
+  contents->SetSourceRect(Rect::MakeSize(target->GetSize()));
+  contents->SetOpacity(paint_.color.alpha);
+  contents->SetDeferApplyingOpacity(true);
+  return paint_.WithFiltersForSubpassTarget(std::move(contents),
+                                            effect_transform);
+}
+
 }  // namespace impeller
diff --git a/impeller/aiks/paint_pass_delegate.h b/impeller/aiks/paint_pass_delegate.h
index 128f7b6abb14a..586b3a2f78526 100644
--- a/impeller/aiks/paint_pass_delegate.h
+++ b/impeller/aiks/paint_pass_delegate.h
@@ -12,6 +12,8 @@
 
 namespace impeller {
 
+class EntityPass;
+
 class PaintPassDelegate final : public EntityPassDelegate {
  public:
   PaintPassDelegate(Paint paint, std::optional<Rect> coverage);
@@ -26,7 +28,7 @@ class PaintPassDelegate final : public EntityPassDelegate {
   bool CanElide() override;
 
   // |EntityPassDelgate|
-  bool CanCollapseIntoParentPass() override;
+  bool CanCollapseIntoParentPass(EntityPass* entity_pass) override;
 
   // |EntityPassDelgate|
   std::shared_ptr<Contents> CreateContentsForSubpassTarget(
@@ -40,4 +42,33 @@ class PaintPassDelegate final : public EntityPassDelegate {
   FML_DISALLOW_COPY_AND_ASSIGN(PaintPassDelegate);
 };
 
+class OpacityPeepholePassDelegate final : public EntityPassDelegate {
+ public:
+  OpacityPeepholePassDelegate(Paint paint, std::optional<Rect> coverage);
+
+  // |EntityPassDelgate|
+  ~OpacityPeepholePassDelegate() override;
+
+  // |EntityPassDelegate|
+  std::optional<Rect> GetCoverageRect() override;
+
+  // |EntityPassDelgate|
+  bool CanElide() override;
+
+  // |EntityPassDelgate|
+  bool CanCollapseIntoParentPass(EntityPass* entity_pass) override;
+
+  // |EntityPassDelgate|
+  std::shared_ptr<Contents> CreateContentsForSubpassTarget(
+      std::shared_ptr<Texture> target,
+      const Matrix& effect_transform) override;
+
+ private:
+  const Paint paint_;
+  const std::optional<Rect> coverage_;
+
+  FML_DISALLOW_COPY_AND_ASSIGN(OpacityPeepholePassDelegate);
+};
+
+
 }  // namespace impeller
diff --git a/impeller/entity/contents/clip_contents.cc b/impeller/entity/contents/clip_contents.cc
index 5f55c726c8297..fdd4983251110 100644
--- a/impeller/entity/contents/clip_contents.cc
+++ b/impeller/entity/contents/clip_contents.cc
@@ -70,6 +70,12 @@ bool ClipContents::ShouldRender(
   return true;
 }
 
+bool ClipContents::CanAcceptOpacity() const {
+  return true;
+}
+
+void ClipContents::InheritOpacity(Scalar opacity) {}
+
 bool ClipContents::Render(const ContentContext& renderer,
                           const Entity& entity,
                           RenderPass& pass) const {
@@ -166,6 +172,12 @@ bool ClipRestoreContents::ShouldRender(
   return true;
 }
 
+bool ClipRestoreContents::CanAcceptOpacity() const {
+  return true;
+}
+
+void ClipRestoreContents::InheritOpacity(Scalar opacity) {}
+
 bool ClipRestoreContents::Render(const ContentContext& renderer,
                                  const Entity& entity,
                                  RenderPass& pass) const {
diff --git a/impeller/entity/contents/clip_contents.h b/impeller/entity/contents/clip_contents.h
index 86606d2f59db7..570e37201ff18 100644
--- a/impeller/entity/contents/clip_contents.h
+++ b/impeller/entity/contents/clip_contents.h
@@ -41,6 +41,11 @@ class ClipContents final : public Contents {
   bool Render(const ContentContext& renderer,
               const Entity& entity,
               RenderPass& pass) const override;
+  // |Contents|
+  bool CanAcceptOpacity() const override;
+
+  // |Contents|
+  void InheritOpacity(Scalar opacity) override;
 
  private:
   std::unique_ptr<Geometry> geometry_;
@@ -78,6 +83,12 @@ class ClipRestoreContents final : public Contents {
               const Entity& entity,
               RenderPass& pass) const override;
 
+  // |Contents|
+  bool CanAcceptOpacity() const override;
+
+  // |Contents|
+  void InheritOpacity(Scalar opacity) override;
+
  private:
   std::optional<Rect> restore_coverage_;
 
diff --git a/impeller/entity/contents/color_source_contents.cc b/impeller/entity/contents/color_source_contents.cc
index 6f0d88d2e8f36..c42cee8895b59 100644
--- a/impeller/entity/contents/color_source_contents.cc
+++ b/impeller/entity/contents/color_source_contents.cc
@@ -42,6 +42,14 @@ std::optional<Rect> ColorSourceContents::GetCoverage(
   return geometry_->GetCoverage(entity.GetTransformation());
 };
 
+bool ColorSourceContents::CanAcceptOpacity() const {
+  return !geometry_->MaybeHasOverlapping();
+}
+
+void ColorSourceContents::InheritOpacity(Scalar opacity) {
+  SetAlpha(GetAlpha() * opacity);
+}
+
 bool ColorSourceContents::ShouldRender(
     const Entity& entity,
     const std::optional<Rect>& stencil_coverage) const {
diff --git a/impeller/entity/contents/color_source_contents.h b/impeller/entity/contents/color_source_contents.h
index 89a86cf47ea78..7b6637857c5be 100644
--- a/impeller/entity/contents/color_source_contents.h
+++ b/impeller/entity/contents/color_source_contents.h
@@ -31,6 +31,12 @@ class ColorSourceContents : public Contents {
   bool ShouldRender(const Entity& entity,
                     const std::optional<Rect>& stencil_coverage) const override;
 
+  // | Contents|
+  bool CanAcceptOpacity() const override;
+
+  // | Contents|
+  void InheritOpacity(Scalar opacity) override;
+
  protected:
   const std::shared_ptr<Geometry>& GetGeometry() const;
 
diff --git a/impeller/entity/contents/contents.cc b/impeller/entity/contents/contents.cc
index 437454d97f9e7..6e1d3832f70f6 100644
--- a/impeller/entity/contents/contents.cc
+++ b/impeller/entity/contents/contents.cc
@@ -110,6 +110,14 @@ std::optional<Snapshot> Contents::RenderToSnapshot(
   return snapshot;
 }
 
+bool Contents::CanAcceptOpacity() const {
+  return false;
+}
+
+void Contents::InheritOpacity(Scalar opacity) {
+  FML_UNREACHABLE();
+}
+
 bool Contents::ShouldRender(const Entity& entity,
                             const std::optional<Rect>& stencil_coverage) const {
   if (!stencil_coverage.has_value()) {
diff --git a/impeller/entity/contents/contents.h b/impeller/entity/contents/contents.h
index a4ff06b93cd5a..747ae844c7723 100644
--- a/impeller/entity/contents/contents.h
+++ b/impeller/entity/contents/contents.h
@@ -83,6 +83,19 @@ class Contents {
 
   void SetColorSourceSize(Size size) { color_source_size_ = size; }
 
+  /// @brief Whether or not this contents can accept the opacity peephole
+  ///        optimization.
+  ///
+  ///        By default all contents return false. Contents are responsible
+  ///        for determining whether or not their own geometries intersect in
+  ///        a way that makes accepting opacity impossible. It is always safe
+  ///        to return false, especially if computing overlap would be computationally
+  ///        expensive.
+  virtual bool CanAcceptOpacity() const;
+
+  /// @brief Inherit the provided opacity.
+  virtual void InheritOpacity(Scalar opacity);
+
  protected:
 
  private:
diff --git a/impeller/entity/contents/solid_color_contents.cc b/impeller/entity/contents/solid_color_contents.cc
index c515a45162737..9a40de8cc74a7 100644
--- a/impeller/entity/contents/solid_color_contents.cc
+++ b/impeller/entity/contents/solid_color_contents.cc
@@ -28,6 +28,17 @@ void SolidColorContents::SetGeometry(std::shared_ptr<Geometry> geometry) {
   geometry_ = std::move(geometry);
 }
 
+// | Contents|
+bool SolidColorContents::CanAcceptOpacity() const {
+  return !geometry_->MaybeHasOverlapping();
+}
+
+// | Contents|
+void SolidColorContents::InheritOpacity(Scalar opacity) {
+  auto color = color_;
+  color_ = color.WithAlpha(color.alpha * opacity);
+}
+
 std::optional<Rect> SolidColorContents::GetCoverage(
     const Entity& entity) const {
   if (color_.IsTransparent()) {
diff --git a/impeller/entity/contents/solid_color_contents.h b/impeller/entity/contents/solid_color_contents.h
index 81148b133232a..15808b2a62114 100644
--- a/impeller/entity/contents/solid_color_contents.h
+++ b/impeller/entity/contents/solid_color_contents.h
@@ -35,6 +35,13 @@ class SolidColorContents final : public Contents {
 
   const Color& GetColor() const;
 
+  // | Contents|
+  bool CanAcceptOpacity() const override;
+
+  // | Contents|
+  void InheritOpacity(Scalar opacity) override;
+
+
   // |Contents|
   std::optional<Rect> GetCoverage(const Entity& entity) const override;
 
diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc
index 31ff319661140..f5641a00670c5 100644
--- a/impeller/entity/entity_pass.cc
+++ b/impeller/entity/entity_pass.cc
@@ -294,8 +294,8 @@ EntityPass::EntityResult EntityPass::GetEntityForElement(
       return EntityPass::EntityResult::Skip();
     }
 
-    if (subpass->delegate_->CanCollapseIntoParentPass() &&
-        !subpass->backdrop_filter_proc_.has_value()) {
+    if (!subpass->backdrop_filter_proc_.has_value() &&
+        subpass->delegate_->CanCollapseIntoParentPass(subpass)) {
       // Directly render into the parent target and move on.
       if (!subpass->OnRender(renderer, root_pass_size,
                              pass_context.GetRenderTarget(), position, position,
@@ -624,6 +624,28 @@ void EntityPass::IterateAllEntities(
   }
 }
 
+bool EntityPass::IterateAllFlatEntities(
+    const std::function<bool(Entity&)>& iterator) {
+  if (!iterator) {
+    return true;
+  }
+
+  for (auto& element : elements_) {
+    if (auto entity = std::get_if<Entity>(&element)) {
+      if (!iterator(*entity)) {
+        return false;
+      }
+      continue;
+    }
+    return true;
+  }
+  return false;
+}
+
+size_t EntityPass::GetEntityCount() const {
+  return elements_.size();
+}
+
 std::unique_ptr<EntityPass> EntityPass::Clone() const {
   std::vector<Element> new_elements;
   new_elements.reserve(elements_.size());
diff --git a/impeller/entity/entity_pass.h b/impeller/entity/entity_pass.h
index 5ba236d4e4ac8..02ccc78a40822 100644
--- a/impeller/entity/entity_pass.h
+++ b/impeller/entity/entity_pass.h
@@ -52,6 +52,12 @@ class EntityPass {
 
   void IterateAllEntities(const std::function<bool(Entity&)>& iterator);
 
+  /// @brief Iterates all entities on this pass, returning if there is a subpass.
+  bool IterateAllFlatEntities(const std::function<bool(Entity&)>& iterator);
+
+  /// @brief Return the number of entities on this pass.
+  size_t GetEntityCount() const;
+
   void SetTransformation(Matrix xformation);
 
   void SetStencilDepth(size_t stencil_depth);
diff --git a/impeller/entity/entity_pass_delegate.cc b/impeller/entity/entity_pass_delegate.cc
index 529c8698f68bd..eccd880086394 100644
--- a/impeller/entity/entity_pass_delegate.cc
+++ b/impeller/entity/entity_pass_delegate.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "impeller/entity/entity_pass_delegate.h"
+#include "impeller/entity/entity_pass.h"
 
 namespace impeller {
 
@@ -24,7 +25,9 @@ class DefaultEntityPassDelegate final : public EntityPassDelegate {
   bool CanElide() override { return false; }
 
   // |EntityPassDelegate|
-  bool CanCollapseIntoParentPass() override { return true; }
+  bool CanCollapseIntoParentPass(EntityPass* entity_pass) override {
+    return true;
+  }
 
   // |EntityPassDelegate|
   std::shared_ptr<Contents> CreateContentsForSubpassTarget(
diff --git a/impeller/entity/entity_pass_delegate.h b/impeller/entity/entity_pass_delegate.h
index 28f485c1237f7..119d78dda8798 100644
--- a/impeller/entity/entity_pass_delegate.h
+++ b/impeller/entity/entity_pass_delegate.h
@@ -12,6 +12,8 @@
 
 namespace impeller {
 
+class EntityPass;
+
 class EntityPassDelegate {
  public:
   static std::unique_ptr<EntityPassDelegate> MakeDefault();
@@ -24,7 +26,9 @@ class EntityPassDelegate {
 
   virtual bool CanElide() = 0;
 
-  virtual bool CanCollapseIntoParentPass() = 0;
+  /// @brief  Whether or not this entity pass can be collapsed into the parent.
+  ///         If true, this method may modify the entities for the current pass.
+  virtual bool CanCollapseIntoParentPass(EntityPass* entity_pass) = 0;
 
   virtual std::shared_ptr<Contents> CreateContentsForSubpassTarget(
       std::shared_ptr<Texture> target,
diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc
index b018ccecad09f..1f48348796f3f 100644
--- a/impeller/entity/entity_unittests.cc
+++ b/impeller/entity/entity_unittests.cc
@@ -74,7 +74,7 @@ class TestPassDelegate final : public EntityPassDelegate {
   bool CanElide() override { return false; }
 
   // |EntityPassDelgate|
-  bool CanCollapseIntoParentPass() override { return false; }
+  bool CanCollapseIntoParentPass(EntityPass* entity_pass) override { return false; }
 
   // |EntityPassDelgate|
   std::shared_ptr<Contents> CreateContentsForSubpassTarget(
diff --git a/impeller/entity/geometry.cc b/impeller/entity/geometry.cc
index b610e03dd8eef..ab8215f6a5744 100644
--- a/impeller/entity/geometry.cc
+++ b/impeller/entity/geometry.cc
@@ -51,6 +51,10 @@ std::unique_ptr<Geometry> Geometry::MakeRect(Rect rect) {
   return std::make_unique<RectGeometry>(rect);
 }
 
+bool Geometry::MaybeHasOverlapping() const {
+  return true;
+}
+
 /////// Path Geometry ///////
 
 FillPathGeometry::FillPathGeometry(const Path& path) : path_(path) {}
@@ -523,6 +527,10 @@ std::optional<Rect> CoverGeometry::GetCoverage(const Matrix& transform) const {
   return Rect::MakeMaximum();
 }
 
+bool CoverGeometry::MaybeHasOverlapping() const {
+  return false;
+}
+
 /////// Rect Geometry ///////
 
 RectGeometry::RectGeometry(Rect rect) : rect_(rect) {}
@@ -559,4 +567,9 @@ std::optional<Rect> RectGeometry::GetCoverage(const Matrix& transform) const {
   return rect_.TransformBounds(transform);
 }
 
+bool RectGeometry::MaybeHasOverlapping() const {
+  return false;
+}
+
+
 }  // namespace impeller
diff --git a/impeller/entity/geometry.h b/impeller/entity/geometry.h
index edc59cd5c62ac..363c4556195a1 100644
--- a/impeller/entity/geometry.h
+++ b/impeller/entity/geometry.h
@@ -74,6 +74,13 @@ class Geometry {
   virtual GeometryVertexType GetVertexType() const = 0;
 
   virtual std::optional<Rect> GetCoverage(const Matrix& transform) const = 0;
+
+  /// @brief Whether this geometry has any overlapping painting that would cause
+  ///        opacity peephole to render incorrectly.
+  ///
+  ///        It is always safe to return `true` from this method, and it is advisable to
+  ///        do so if computing overlap would be computationally complex.
+  virtual bool MaybeHasOverlapping() const;
 };
 
 /// @brief A geometry that is created from a vertices object.
@@ -208,6 +215,9 @@ class CoverGeometry : public Geometry {
   // |Geometry|
   std::optional<Rect> GetCoverage(const Matrix& transform) const override;
 
+  // |Geometry|
+  bool MaybeHasOverlapping() const override;
+
   FML_DISALLOW_COPY_AND_ASSIGN(CoverGeometry);
 };
 
@@ -229,6 +239,9 @@ class RectGeometry : public Geometry {
   // |Geometry|
   std::optional<Rect> GetCoverage(const Matrix& transform) const override;
 
+  // |Geometry|
+  bool MaybeHasOverlapping() const override;
+
   Rect rect_;
 
   FML_DISALLOW_COPY_AND_ASSIGN(RectGeometry);

From e9c957cfe27e76aca7f526bc213756959bfaa36e Mon Sep 17 00:00:00 2001
From: jonahwilliams <jonahwilliams@google.com>
Date: Mon, 6 Mar 2023 16:14:48 -0800
Subject: [PATCH 02/10] [Impeller] minimal opacity peephole for cupertino
 picker

---
 impeller/aiks/paint_pass_delegate.cc          |  3 +-
 impeller/aiks/paint_pass_delegate.h           |  1 -
 impeller/entity/contents/clip_contents.cc     |  4 +--
 impeller/entity/contents/clip_contents.h      |  4 +--
 .../entity/contents/color_source_contents.cc  |  4 +--
 .../entity/contents/color_source_contents.h   |  2 +-
 impeller/entity/contents/contents.cc          |  2 +-
 impeller/entity/contents/contents.h           |  6 ++--
 .../entity/contents/solid_color_contents.cc   |  4 +--
 .../entity/contents/solid_color_contents.h    |  3 +-
 impeller/entity/contents/text_contents.cc     | 16 ++++++++--
 impeller/entity/contents/text_contents.h      |  4 +++
 impeller/entity/entity_pass.h                 |  3 +-
 impeller/entity/geometry.cc                   |  8 ++---
 impeller/entity/geometry.h                    | 11 ++++---
 impeller/typographer/text_frame.cc            | 32 +++++++++++++++++++
 impeller/typographer/text_frame.h             |  2 ++
 17 files changed, 78 insertions(+), 31 deletions(-)

diff --git a/impeller/aiks/paint_pass_delegate.cc b/impeller/aiks/paint_pass_delegate.cc
index 8d1792e55464a..65afc78935d45 100644
--- a/impeller/aiks/paint_pass_delegate.cc
+++ b/impeller/aiks/paint_pass_delegate.cc
@@ -92,8 +92,7 @@ bool OpacityPeepholePassDelegate::CanCollapseIntoParentPass(
   auto i = 0;
   auto had_subpass = entity_pass->IterateAllFlatEntities([&](Entity& entity) {
     auto contents = entity.GetContents();
-    if (!contents->CanAcceptOpacity()) {
-      std::cerr << "Cant accept opacity" << i << std::endl;
+    if (!contents->CanAcceptOpacity(entity)) {
       all_can_accept = false;
       return false;
     }
diff --git a/impeller/aiks/paint_pass_delegate.h b/impeller/aiks/paint_pass_delegate.h
index 586b3a2f78526..208dec3a9f420 100644
--- a/impeller/aiks/paint_pass_delegate.h
+++ b/impeller/aiks/paint_pass_delegate.h
@@ -70,5 +70,4 @@ class OpacityPeepholePassDelegate final : public EntityPassDelegate {
   FML_DISALLOW_COPY_AND_ASSIGN(OpacityPeepholePassDelegate);
 };
 
-
 }  // namespace impeller
diff --git a/impeller/entity/contents/clip_contents.cc b/impeller/entity/contents/clip_contents.cc
index fdd4983251110..1689a0d9c804b 100644
--- a/impeller/entity/contents/clip_contents.cc
+++ b/impeller/entity/contents/clip_contents.cc
@@ -70,7 +70,7 @@ bool ClipContents::ShouldRender(
   return true;
 }
 
-bool ClipContents::CanAcceptOpacity() const {
+bool ClipContents::CanAcceptOpacity(const Entity& entity) const {
   return true;
 }
 
@@ -172,7 +172,7 @@ bool ClipRestoreContents::ShouldRender(
   return true;
 }
 
-bool ClipRestoreContents::CanAcceptOpacity() const {
+bool ClipRestoreContents::CanAcceptOpacity(const Entity& entity) const {
   return true;
 }
 
diff --git a/impeller/entity/contents/clip_contents.h b/impeller/entity/contents/clip_contents.h
index 570e37201ff18..704470c5d7ad9 100644
--- a/impeller/entity/contents/clip_contents.h
+++ b/impeller/entity/contents/clip_contents.h
@@ -42,7 +42,7 @@ class ClipContents final : public Contents {
               const Entity& entity,
               RenderPass& pass) const override;
   // |Contents|
-  bool CanAcceptOpacity() const override;
+  bool CanAcceptOpacity(const Entity& entity) const override;
 
   // |Contents|
   void InheritOpacity(Scalar opacity) override;
@@ -84,7 +84,7 @@ class ClipRestoreContents final : public Contents {
               RenderPass& pass) const override;
 
   // |Contents|
-  bool CanAcceptOpacity() const override;
+  bool CanAcceptOpacity(const Entity& entity) const override;
 
   // |Contents|
   void InheritOpacity(Scalar opacity) override;
diff --git a/impeller/entity/contents/color_source_contents.cc b/impeller/entity/contents/color_source_contents.cc
index c42cee8895b59..22b68bb2ac034 100644
--- a/impeller/entity/contents/color_source_contents.cc
+++ b/impeller/entity/contents/color_source_contents.cc
@@ -42,8 +42,8 @@ std::optional<Rect> ColorSourceContents::GetCoverage(
   return geometry_->GetCoverage(entity.GetTransformation());
 };
 
-bool ColorSourceContents::CanAcceptOpacity() const {
-  return !geometry_->MaybeHasOverlapping();
+bool ColorSourceContents::CanAcceptOpacity(const Entity& entity) const {
+  return !geometry_->MaybeHasOverlapping(entity);
 }
 
 void ColorSourceContents::InheritOpacity(Scalar opacity) {
diff --git a/impeller/entity/contents/color_source_contents.h b/impeller/entity/contents/color_source_contents.h
index 7b6637857c5be..b510dfb97e6fd 100644
--- a/impeller/entity/contents/color_source_contents.h
+++ b/impeller/entity/contents/color_source_contents.h
@@ -32,7 +32,7 @@ class ColorSourceContents : public Contents {
                     const std::optional<Rect>& stencil_coverage) const override;
 
   // | Contents|
-  bool CanAcceptOpacity() const override;
+  bool CanAcceptOpacity(const Entity& entity) const override;
 
   // | Contents|
   void InheritOpacity(Scalar opacity) override;
diff --git a/impeller/entity/contents/contents.cc b/impeller/entity/contents/contents.cc
index 6e1d3832f70f6..93f431ca09e84 100644
--- a/impeller/entity/contents/contents.cc
+++ b/impeller/entity/contents/contents.cc
@@ -110,7 +110,7 @@ std::optional<Snapshot> Contents::RenderToSnapshot(
   return snapshot;
 }
 
-bool Contents::CanAcceptOpacity() const {
+bool Contents::CanAcceptOpacity(const Entity& entity) const {
   return false;
 }
 
diff --git a/impeller/entity/contents/contents.h b/impeller/entity/contents/contents.h
index 747ae844c7723..b74b2e2d3c9fb 100644
--- a/impeller/entity/contents/contents.h
+++ b/impeller/entity/contents/contents.h
@@ -89,9 +89,9 @@ class Contents {
   ///        By default all contents return false. Contents are responsible
   ///        for determining whether or not their own geometries intersect in
   ///        a way that makes accepting opacity impossible. It is always safe
-  ///        to return false, especially if computing overlap would be computationally
-  ///        expensive.
-  virtual bool CanAcceptOpacity() const;
+  ///        to return false, especially if computing overlap would be
+  ///        computationally expensive.
+  virtual bool CanAcceptOpacity(const Entity& entity) const;
 
   /// @brief Inherit the provided opacity.
   virtual void InheritOpacity(Scalar opacity);
diff --git a/impeller/entity/contents/solid_color_contents.cc b/impeller/entity/contents/solid_color_contents.cc
index 9a40de8cc74a7..d2dc87a1952e0 100644
--- a/impeller/entity/contents/solid_color_contents.cc
+++ b/impeller/entity/contents/solid_color_contents.cc
@@ -29,8 +29,8 @@ void SolidColorContents::SetGeometry(std::shared_ptr<Geometry> geometry) {
 }
 
 // | Contents|
-bool SolidColorContents::CanAcceptOpacity() const {
-  return !geometry_->MaybeHasOverlapping();
+bool SolidColorContents::CanAcceptOpacity(const Entity& entity) const {
+  return !geometry_->MaybeHasOverlapping(entity);
 }
 
 // | Contents|
diff --git a/impeller/entity/contents/solid_color_contents.h b/impeller/entity/contents/solid_color_contents.h
index 15808b2a62114..00839e47ada76 100644
--- a/impeller/entity/contents/solid_color_contents.h
+++ b/impeller/entity/contents/solid_color_contents.h
@@ -36,12 +36,11 @@ class SolidColorContents final : public Contents {
   const Color& GetColor() const;
 
   // | Contents|
-  bool CanAcceptOpacity() const override;
+  bool CanAcceptOpacity(const Entity& entity) const override;
 
   // | Contents|
   void InheritOpacity(Scalar opacity) override;
 
-
   // |Contents|
   std::optional<Rect> GetCoverage(const Entity& entity) const override;
 
diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc
index 141ffaccea293..e0c5e43173bf7 100644
--- a/impeller/entity/contents/text_contents.cc
+++ b/impeller/entity/contents/text_contents.cc
@@ -4,6 +4,7 @@
 
 #include "impeller/entity/contents/text_contents.h"
 
+#include <iostream>
 #include <optional>
 #include <type_traits>
 #include <utility>
@@ -50,6 +51,15 @@ void TextContents::SetColor(Color color) {
   color_ = color;
 }
 
+bool TextContents::CanAcceptOpacity(const Entity& entity) const {
+  return !frame_.MaybeHasOverlapping();
+}
+
+void TextContents::InheritOpacity(Scalar opacity) {
+  auto color = color_;
+  color_ = color.WithAlpha(color.alpha * opacity);
+}
+
 std::optional<Rect> TextContents::GetCoverage(const Entity& entity) const {
   auto bounds = frame_.GetBounds();
   if (!bounds.has_value()) {
@@ -117,9 +127,9 @@ static bool CommonRender(
   // interpolated vertex information is also used in the fragment shader to
   // sample from the glyph atlas.
 
-  const std::array<Point, 4> unit_points = {Point{0, 0}, Point{1, 0},
-                                            Point{0, 1}, Point{1, 1}};
-  const std::array<uint32_t, 6> indices = {0, 1, 2, 1, 2, 3};
+  constexpr std::array<Point, 4> unit_points = {Point{0, 0}, Point{1, 0},
+                                                Point{0, 1}, Point{1, 1}};
+  constexpr std::array<uint32_t, 6> indices = {0, 1, 2, 1, 2, 3};
 
   VertexBufferBuilder<typename VS::PerVertexData> vertex_builder;
 
diff --git a/impeller/entity/contents/text_contents.h b/impeller/entity/contents/text_contents.h
index a0cec70a95103..74d2f64e465b7 100644
--- a/impeller/entity/contents/text_contents.h
+++ b/impeller/entity/contents/text_contents.h
@@ -32,6 +32,10 @@ class TextContents final : public Contents {
 
   void SetColor(Color color);
 
+  bool CanAcceptOpacity(const Entity& entity) const override;
+
+  void InheritOpacity(Scalar opacity) override;
+
   // |Contents|
   std::optional<Rect> GetCoverage(const Entity& entity) const override;
 
diff --git a/impeller/entity/entity_pass.h b/impeller/entity/entity_pass.h
index f7db9f23f050b..cca9e634bc9d6 100644
--- a/impeller/entity/entity_pass.h
+++ b/impeller/entity/entity_pass.h
@@ -52,7 +52,8 @@ class EntityPass {
 
   void IterateAllEntities(const std::function<bool(Entity&)>& iterator);
 
-  /// @brief Iterates all entities on this pass, returning if there is a subpass.
+  /// @brief Iterates all entities on this pass, returning if there is a
+  /// subpass.
   bool IterateAllFlatEntities(const std::function<bool(Entity&)>& iterator);
 
   /// @brief Return the number of entities on this pass.
diff --git a/impeller/entity/geometry.cc b/impeller/entity/geometry.cc
index ab8215f6a5744..df57bb84f709b 100644
--- a/impeller/entity/geometry.cc
+++ b/impeller/entity/geometry.cc
@@ -4,6 +4,7 @@
 
 #include "impeller/entity/geometry.h"
 #include "impeller/entity/contents/content_context.h"
+#include "impeller/entity/entity.h"
 #include "impeller/entity/position_color.vert.h"
 #include "impeller/geometry/matrix.h"
 #include "impeller/geometry/path_builder.h"
@@ -51,7 +52,7 @@ std::unique_ptr<Geometry> Geometry::MakeRect(Rect rect) {
   return std::make_unique<RectGeometry>(rect);
 }
 
-bool Geometry::MaybeHasOverlapping() const {
+bool Geometry::MaybeHasOverlapping(const Entity& entity) const {
   return true;
 }
 
@@ -527,7 +528,7 @@ std::optional<Rect> CoverGeometry::GetCoverage(const Matrix& transform) const {
   return Rect::MakeMaximum();
 }
 
-bool CoverGeometry::MaybeHasOverlapping() const {
+bool CoverGeometry::MaybeHasOverlapping(const Entity& entity) const {
   return false;
 }
 
@@ -567,9 +568,8 @@ std::optional<Rect> RectGeometry::GetCoverage(const Matrix& transform) const {
   return rect_.TransformBounds(transform);
 }
 
-bool RectGeometry::MaybeHasOverlapping() const {
+bool RectGeometry::MaybeHasOverlapping(const Entity& entity) const {
   return false;
 }
 
-
 }  // namespace impeller
diff --git a/impeller/entity/geometry.h b/impeller/entity/geometry.h
index 363c4556195a1..0a5958dc4b146 100644
--- a/impeller/entity/geometry.h
+++ b/impeller/entity/geometry.h
@@ -78,9 +78,10 @@ class Geometry {
   /// @brief Whether this geometry has any overlapping painting that would cause
   ///        opacity peephole to render incorrectly.
   ///
-  ///        It is always safe to return `true` from this method, and it is advisable to
-  ///        do so if computing overlap would be computationally complex.
-  virtual bool MaybeHasOverlapping() const;
+  ///        It is always safe to return `true` from this method, and it is
+  ///        advisable to do so if computing overlap would be computationally
+  ///        complex.
+  virtual bool MaybeHasOverlapping(const Entity& entity) const;
 };
 
 /// @brief A geometry that is created from a vertices object.
@@ -216,7 +217,7 @@ class CoverGeometry : public Geometry {
   std::optional<Rect> GetCoverage(const Matrix& transform) const override;
 
   // |Geometry|
-  bool MaybeHasOverlapping() const override;
+  bool MaybeHasOverlapping(const Entity& entity) const override;
 
   FML_DISALLOW_COPY_AND_ASSIGN(CoverGeometry);
 };
@@ -240,7 +241,7 @@ class RectGeometry : public Geometry {
   std::optional<Rect> GetCoverage(const Matrix& transform) const override;
 
   // |Geometry|
-  bool MaybeHasOverlapping() const override;
+  bool MaybeHasOverlapping(const Entity& entity) const override;
 
   Rect rect_;
 
diff --git a/impeller/typographer/text_frame.cc b/impeller/typographer/text_frame.cc
index ff05c28f41061..d7c241b899283 100644
--- a/impeller/typographer/text_frame.cc
+++ b/impeller/typographer/text_frame.cc
@@ -46,4 +46,36 @@ bool TextFrame::HasColor() const {
   return has_color_;
 }
 
+bool TextFrame::MaybeHasOverlapping() const {
+  if (runs_.size() > 1) {
+    return true;
+  }
+  auto glyph_positions = runs_[0].GetGlyphPositions();
+  if (glyph_positions.size() > 10) {
+    return true;
+  }
+  if (glyph_positions.size() == 1) {
+    return false;
+  }
+  // To avoid quadradic behavior the overlapping is checked against an
+  // accumulated bounds rect. This gives faster but less precise information
+  // on text runs.
+  auto first_position = glyph_positions[0];
+  auto overlapping_rect =
+      Rect(first_position.position + first_position.glyph.bounds.origin,
+           first_position.glyph.bounds.size);
+  for (auto i = 1u; i < glyph_positions.size(); i++) {
+    auto glyph_position = glyph_positions[i];
+    auto glyph_rect =
+        Rect(glyph_position.position + glyph_position.glyph.bounds.origin,
+             glyph_position.glyph.bounds.size);
+    auto intersection = glyph_rect.Intersection(overlapping_rect);
+    if (intersection.has_value()) {
+      return true;
+    }
+    overlapping_rect = overlapping_rect.Union(glyph_rect);
+  }
+  return false;
+}
+
 }  // namespace impeller
diff --git a/impeller/typographer/text_frame.h b/impeller/typographer/text_frame.h
index f64bef492e842..5525dd976fa6c 100644
--- a/impeller/typographer/text_frame.h
+++ b/impeller/typographer/text_frame.h
@@ -52,6 +52,8 @@ class TextFrame {
   ///
   const std::vector<TextRun>& GetRuns() const;
 
+  bool MaybeHasOverlapping() const;
+
   //----------------------------------------------------------------------------
   /// @brief      Whether any run in this frame has color.
   bool HasColor() const;

From b72fd092dab2fffefafbe2836676edb3db98d22e Mon Sep 17 00:00:00 2001
From: jonahwilliams <jonahwilliams@google.com>
Date: Tue, 7 Mar 2023 11:42:48 -0800
Subject: [PATCH 03/10] add trivial unittest and update texture contents to
 accept opacity

---
 .../entity/contents/color_source_contents.h   |  4 +-
 impeller/entity/contents/text_contents.cc     |  4 ++
 impeller/entity/contents/text_contents.h      |  2 +
 impeller/entity/contents/texture_contents.cc  | 12 ++++
 impeller/entity/contents/texture_contents.h   |  8 +++
 impeller/entity/entity_unittests.cc           | 66 +++++++++++++++++++
 6 files changed, 94 insertions(+), 2 deletions(-)

diff --git a/impeller/entity/contents/color_source_contents.h b/impeller/entity/contents/color_source_contents.h
index b510dfb97e6fd..bfccc575d090b 100644
--- a/impeller/entity/contents/color_source_contents.h
+++ b/impeller/entity/contents/color_source_contents.h
@@ -37,13 +37,13 @@ class ColorSourceContents : public Contents {
   // | Contents|
   void InheritOpacity(Scalar opacity) override;
 
+  Scalar GetAlpha() const;
+
  protected:
   const std::shared_ptr<Geometry>& GetGeometry() const;
 
   const Matrix& GetInverseMatrix() const;
 
-  Scalar GetAlpha() const;
-
  private:
   std::shared_ptr<Geometry> geometry_;
   Matrix inverse_matrix_;
diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc
index e0c5e43173bf7..0a17e09837a15 100644
--- a/impeller/entity/contents/text_contents.cc
+++ b/impeller/entity/contents/text_contents.cc
@@ -51,6 +51,10 @@ void TextContents::SetColor(Color color) {
   color_ = color;
 }
 
+Color TextContents::GetColor() const {
+  return color_;
+}
+
 bool TextContents::CanAcceptOpacity(const Entity& entity) const {
   return !frame_.MaybeHasOverlapping();
 }
diff --git a/impeller/entity/contents/text_contents.h b/impeller/entity/contents/text_contents.h
index 74d2f64e465b7..8e9b53539d3af 100644
--- a/impeller/entity/contents/text_contents.h
+++ b/impeller/entity/contents/text_contents.h
@@ -32,6 +32,8 @@ class TextContents final : public Contents {
 
   void SetColor(Color color);
 
+  Color GetColor() const;
+
   bool CanAcceptOpacity(const Entity& entity) const override;
 
   void InheritOpacity(Scalar opacity) override;
diff --git a/impeller/entity/contents/texture_contents.cc b/impeller/entity/contents/texture_contents.cc
index 7375598dd87ff..135b466c217e5 100644
--- a/impeller/entity/contents/texture_contents.cc
+++ b/impeller/entity/contents/texture_contents.cc
@@ -57,6 +57,18 @@ void TextureContents::SetStencilEnabled(bool enabled) {
   stencil_enabled_ = enabled;
 }
 
+bool TextureContents::CanAcceptOpacity(const Entity& entity) const {
+  return true;
+}
+
+void TextureContents::InheritOpacity(Scalar opacity) {
+  opacity_ = opacity_ * opacity;
+}
+
+Scalar TextureContents::GetOpacity() const {
+  return opacity_;
+}
+
 std::optional<Rect> TextureContents::GetCoverage(const Entity& entity) const {
   if (opacity_ == 0) {
     return std::nullopt;
diff --git a/impeller/entity/contents/texture_contents.h b/impeller/entity/contents/texture_contents.h
index fec23fba6505a..ca2a3ae167a1b 100644
--- a/impeller/entity/contents/texture_contents.h
+++ b/impeller/entity/contents/texture_contents.h
@@ -46,6 +46,8 @@ class TextureContents final : public Contents {
 
   void SetOpacity(Scalar opacity);
 
+  Scalar GetOpacity() const;
+
   void SetStencilEnabled(bool enabled);
 
   // |Contents|
@@ -63,6 +65,12 @@ class TextureContents final : public Contents {
               const Entity& entity,
               RenderPass& pass) const override;
 
+  // |Contents|
+  bool CanAcceptOpacity(const Entity& entity) const override;
+
+  // |Contents|
+  void InheritOpacity(Scalar opacity) override;
+
   void SetDeferApplyingOpacity(bool defer_applying_opacity);
 
  private:
diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc
index fd7053979341f..ee2a70801b072 100644
--- a/impeller/entity/entity_unittests.cc
+++ b/impeller/entity/entity_unittests.cc
@@ -26,6 +26,7 @@
 #include "impeller/entity/contents/solid_color_contents.h"
 #include "impeller/entity/contents/text_contents.h"
 #include "impeller/entity/contents/texture_contents.h"
+#include "impeller/entity/contents/tiled_texture_contents.h"
 #include "impeller/entity/contents/vertices_contents.h"
 #include "impeller/entity/entity.h"
 #include "impeller/entity/entity_pass.h"
@@ -2347,5 +2348,70 @@ TEST_P(EntityTest, RuntimeEffect) {
   ASSERT_TRUE(OpenPlaygroundHere(callback));
 }
 
+TEST_P(EntityTest, InheritOpacityTest) {
+  Entity entity;
+
+  // Texture contents can always accept opacity.
+  auto texture_contents = std::make_shared<TextureContents>();
+  texture_contents->SetOpacity(0.5);
+  ASSERT_TRUE(texture_contents->CanAcceptOpacity(entity));
+
+  texture_contents->InheritOpacity(0.5);
+  ASSERT_EQ(texture_contents->GetOpacity(), 0.25);
+
+  // Solid color contents can accept opacity if their geometry
+  // doesn't overlap.
+  auto solid_color = std::make_shared<SolidColorContents>();
+  solid_color->SetGeometry(
+      Geometry::MakeRect(Rect::MakeLTRB(100, 100, 200, 200)));
+  solid_color->SetColor(Color::Blue().WithAlpha(0.5));
+
+  ASSERT_TRUE(solid_color->CanAcceptOpacity(entity));
+
+  solid_color->InheritOpacity(0.5);
+  ASSERT_EQ(solid_color->GetColor().alpha, 0.25);
+
+  // Color source contents can accept opacity if their geometry
+  // doesn't overlap.
+  auto tiled_texture = std::make_shared<TiledTextureContents>();
+  tiled_texture->SetGeometry(
+      Geometry::MakeRect(Rect::MakeLTRB(100, 100, 200, 200)));
+  tiled_texture->SetAlpha(0.5);
+
+  ASSERT_TRUE(tiled_texture->CanAcceptOpacity(entity));
+
+  tiled_texture->InheritOpacity(0.5);
+  ASSERT_EQ(tiled_texture->GetAlpha(), 0.25);
+
+  // Text contents can accept opacity if the text frames do not
+  // overlap
+  SkFont font;
+  font.setSize(30);
+  auto blob = SkTextBlob::MakeFromString("A", font);
+  auto frame = TextFrameFromTextBlob(blob);
+  auto lazy_glyph_atlas = std::make_shared<LazyGlyphAtlas>();
+  lazy_glyph_atlas->AddTextFrame(frame);
+
+  auto text_contents = std::make_shared<TextContents>();
+  text_contents->SetTextFrame(frame);
+  text_contents->SetColor(Color::Blue().WithAlpha(0.5));
+
+  ASSERT_TRUE(text_contents->CanAcceptOpacity(entity));
+
+  text_contents->InheritOpacity(0.5);
+  ASSERT_EQ(text_contents->GetColor().alpha, 0.25);
+
+  // Clips and restores trivially accept opacity.
+  ASSERT_TRUE(ClipContents().CanAcceptOpacity(entity));
+  ASSERT_TRUE(ClipRestoreContents().CanAcceptOpacity(entity));
+
+  // Potentially overlapping geometry always returns false.
+  auto solid_color_2 = std::make_shared<TiledTextureContents>();
+  Path path = PathBuilder{}.MoveTo({100, 100}).LineTo({100, 200}).TakePath();
+  solid_color_2->SetGeometry(Geometry::MakeStrokePath(path, 5.0));
+
+  ASSERT_FALSE(solid_color_2->CanAcceptOpacity(entity));
+}
+
 }  // namespace testing
 }  // namespace impeller

From 454c7b3fb2b320b6b86bc35a34dda5fcac02cdb8 Mon Sep 17 00:00:00 2001
From: jonahwilliams <jonahwilliams@google.com>
Date: Fri, 10 Mar 2023 09:52:29 -0800
Subject: [PATCH 04/10] add more tests

---
 impeller/typographer/text_frame.h             |  8 ++++++++
 impeller/typographer/typographer_unittests.cc | 13 +++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/impeller/typographer/text_frame.h b/impeller/typographer/text_frame.h
index 5525dd976fa6c..e43af08b2d6ea 100644
--- a/impeller/typographer/text_frame.h
+++ b/impeller/typographer/text_frame.h
@@ -52,6 +52,14 @@ class TextFrame {
   ///
   const std::vector<TextRun>& GetRuns() const;
 
+  //----------------------------------------------------------------------------
+  /// @brief      Whether any of the glyphs of this run are potentially
+  /// overlapping
+  ///
+  ///             It is always safe to return true from this method. Generally,
+  ///             any large blobs of text should return true to avoid
+  ///             computationally complex calculations. This information is used
+  ///             to apply opacity peephole optimizations to text blobs.
   bool MaybeHasOverlapping() const;
 
   //----------------------------------------------------------------------------
diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc
index 70718e2be20c7..e8b6258fe3466 100644
--- a/impeller/typographer/typographer_unittests.cc
+++ b/impeller/typographer/typographer_unittests.cc
@@ -253,5 +253,18 @@ TEST_P(TypographerTest, FontGlyphPairTypeChangesHashAndEquals) {
   ASSERT_FALSE(FontGlyphPair::Equal{}(pair_1, pair_3));
 }
 
+TEST_P(TypographerTest, MaybeHasOverlapping) {
+  SkFont sk_font;
+  auto frame = TextFrameFromTextBlob(SkTextBlob::MakeFromString("1", sk_font));
+  // Single character has no overlapping
+  ASSERT_FALSE(frame.MaybeHasOverlapping());
+
+  auto frame_2 =
+      TextFrameFromTextBlob(SkTextBlob::MakeFromString("123456789", sk_font));
+  // Characters probably have overlap due to low fidelity text metrics, but this
+  // could be fixed.
+  ASSERT_TRUE(frame_2.MaybeHasOverlapping());
+}
+
 }  // namespace testing
 }  // namespace impeller

From 966bf069f22c1f4cab622e92442fab0e358824d8 Mon Sep 17 00:00:00 2001
From: jonahwilliams <jonahwilliams@google.com>
Date: Fri, 10 Mar 2023 10:28:05 -0800
Subject: [PATCH 05/10] ++

---
 impeller/aiks/paint_pass_delegate.cc | 2 --
 1 file changed, 2 deletions(-)

diff --git a/impeller/aiks/paint_pass_delegate.cc b/impeller/aiks/paint_pass_delegate.cc
index 65afc78935d45..844d78b1fdeef 100644
--- a/impeller/aiks/paint_pass_delegate.cc
+++ b/impeller/aiks/paint_pass_delegate.cc
@@ -89,14 +89,12 @@ bool OpacityPeepholePassDelegate::CanCollapseIntoParentPass(
   }
   bool all_can_accept = true;
   std::vector<Rect> all_coverages;
-  auto i = 0;
   auto had_subpass = entity_pass->IterateAllFlatEntities([&](Entity& entity) {
     auto contents = entity.GetContents();
     if (!contents->CanAcceptOpacity(entity)) {
       all_can_accept = false;
       return false;
     }
-    i++;
     auto maybe_coverage = contents->GetCoverage(entity);
     if (maybe_coverage.has_value()) {
       auto coverage = maybe_coverage.value();

From 1587dd6f27065b79f62fc28046c59919f372081a Mon Sep 17 00:00:00 2001
From: jonahwilliams <jonahwilliams@google.com>
Date: Mon, 13 Mar 2023 15:07:00 -0700
Subject: [PATCH 06/10] fix compile error

---
 impeller/entity/contents/color_source_contents.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/impeller/entity/contents/color_source_contents.h b/impeller/entity/contents/color_source_contents.h
index 3f13d67d51f9a..02e0ca039d417 100644
--- a/impeller/entity/contents/color_source_contents.h
+++ b/impeller/entity/contents/color_source_contents.h
@@ -44,10 +44,6 @@ class ColorSourceContents : public Contents {
  protected:
   const std::shared_ptr<Geometry>& GetGeometry() const;
 
-  const Matrix& GetInverseMatrix() const;
-
-  Scalar GetAlpha() const;
-
  private:
   std::shared_ptr<Geometry> geometry_;
   Matrix inverse_matrix_;

From fe306d70a653aa8cad43061f0fd6e5471b27e1f9 Mon Sep 17 00:00:00 2001
From: jonahwilliams <jonahwilliams@google.com>
Date: Mon, 13 Mar 2023 15:42:19 -0700
Subject: [PATCH 07/10] dont capture everything

---
 impeller/aiks/paint_pass_delegate.cc | 38 +++++++++++++++-------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/impeller/aiks/paint_pass_delegate.cc b/impeller/aiks/paint_pass_delegate.cc
index 844d78b1fdeef..a9688e5102c26 100644
--- a/impeller/aiks/paint_pass_delegate.cc
+++ b/impeller/aiks/paint_pass_delegate.cc
@@ -89,30 +89,32 @@ bool OpacityPeepholePassDelegate::CanCollapseIntoParentPass(
   }
   bool all_can_accept = true;
   std::vector<Rect> all_coverages;
-  auto had_subpass = entity_pass->IterateAllFlatEntities([&](Entity& entity) {
-    auto contents = entity.GetContents();
-    if (!contents->CanAcceptOpacity(entity)) {
-      all_can_accept = false;
-      return false;
-    }
-    auto maybe_coverage = contents->GetCoverage(entity);
-    if (maybe_coverage.has_value()) {
-      auto coverage = maybe_coverage.value();
-      for (const auto& cv : all_coverages) {
-        if (cv.IntersectsWithRect(coverage)) {
+  auto had_subpass = entity_pass->IterateAllFlatEntities(
+      [&all_coverages, &all_can_accept](Entity& entity) {
+        auto contents = entity.GetContents();
+        if (!contents->CanAcceptOpacity(entity)) {
           all_can_accept = false;
           return false;
         }
-      }
-      all_coverages.push_back(coverage);
-    }
-    return true;
-  });
+        auto maybe_coverage = contents->GetCoverage(entity);
+        if (maybe_coverage.has_value()) {
+          auto coverage = maybe_coverage.value();
+          for (const auto& cv : all_coverages) {
+            if (cv.IntersectsWithRect(coverage)) {
+              all_can_accept = false;
+              return false;
+            }
+          }
+          all_coverages.push_back(coverage);
+        }
+        return true;
+      });
   if (had_subpass || !all_can_accept) {
     return false;
   }
-  entity_pass->IterateAllFlatEntities([&](Entity& entity) {
-    entity.GetContents()->InheritOpacity(paint_.color.alpha);
+  auto alpha = paint_.color.alpha;
+  entity_pass->IterateAllFlatEntities([&alpha](Entity& entity) {
+    entity.GetContents()->InheritOpacity(alpha);
     return true;
   });
   return true;

From 080dc6232bc31b7460376bb39ac494c1469137d7 Mon Sep 17 00:00:00 2001
From: jonahwilliams <jonahwilliams@google.com>
Date: Mon, 13 Mar 2023 15:45:40 -0700
Subject: [PATCH 08/10] remove iostream

---
 impeller/entity/contents/text_contents.cc | 1 -
 1 file changed, 1 deletion(-)

diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc
index 882c7deb89d5a..ed6b4efb8206a 100644
--- a/impeller/entity/contents/text_contents.cc
+++ b/impeller/entity/contents/text_contents.cc
@@ -4,7 +4,6 @@
 
 #include "impeller/entity/contents/text_contents.h"
 
-#include <iostream>
 #include <optional>
 #include <type_traits>
 #include <utility>

From 1494463eb4303592eb77413a6652fe9846996211 Mon Sep 17 00:00:00 2001
From: jonahwilliams <jonahwilliams@google.com>
Date: Tue, 14 Mar 2023 17:18:08 -0700
Subject: [PATCH 09/10] dont worry about geom overlap

---
 impeller/aiks/paint_pass_delegate.h               |  5 +++++
 impeller/entity/contents/color_source_contents.cc |  2 +-
 impeller/entity/contents/solid_color_contents.cc  |  2 +-
 impeller/entity/geometry.cc                       | 12 ------------
 impeller/entity/geometry.h                        | 14 --------------
 5 files changed, 7 insertions(+), 28 deletions(-)

diff --git a/impeller/aiks/paint_pass_delegate.h b/impeller/aiks/paint_pass_delegate.h
index 208dec3a9f420..9217943c8a741 100644
--- a/impeller/aiks/paint_pass_delegate.h
+++ b/impeller/aiks/paint_pass_delegate.h
@@ -42,6 +42,11 @@ class PaintPassDelegate final : public EntityPassDelegate {
   FML_DISALLOW_COPY_AND_ASSIGN(PaintPassDelegate);
 };
 
+/// A delegate that attempts to forward opacity from a save layer to
+/// child contents.
+///
+/// Currently this has a hardcoded limit of 3 entities in a pass, and
+/// cannot forward to child subpass delegates.
 class OpacityPeepholePassDelegate final : public EntityPassDelegate {
  public:
   OpacityPeepholePassDelegate(Paint paint, std::optional<Rect> coverage);
diff --git a/impeller/entity/contents/color_source_contents.cc b/impeller/entity/contents/color_source_contents.cc
index 22b68bb2ac034..bfc0f753479f0 100644
--- a/impeller/entity/contents/color_source_contents.cc
+++ b/impeller/entity/contents/color_source_contents.cc
@@ -43,7 +43,7 @@ std::optional<Rect> ColorSourceContents::GetCoverage(
 };
 
 bool ColorSourceContents::CanAcceptOpacity(const Entity& entity) const {
-  return !geometry_->MaybeHasOverlapping(entity);
+  return true;
 }
 
 void ColorSourceContents::InheritOpacity(Scalar opacity) {
diff --git a/impeller/entity/contents/solid_color_contents.cc b/impeller/entity/contents/solid_color_contents.cc
index d2dc87a1952e0..da9571b204807 100644
--- a/impeller/entity/contents/solid_color_contents.cc
+++ b/impeller/entity/contents/solid_color_contents.cc
@@ -30,7 +30,7 @@ void SolidColorContents::SetGeometry(std::shared_ptr<Geometry> geometry) {
 
 // | Contents|
 bool SolidColorContents::CanAcceptOpacity(const Entity& entity) const {
-  return !geometry_->MaybeHasOverlapping(entity);
+  return true;
 }
 
 // | Contents|
diff --git a/impeller/entity/geometry.cc b/impeller/entity/geometry.cc
index df57bb84f709b..2ad20a000afb9 100644
--- a/impeller/entity/geometry.cc
+++ b/impeller/entity/geometry.cc
@@ -52,10 +52,6 @@ std::unique_ptr<Geometry> Geometry::MakeRect(Rect rect) {
   return std::make_unique<RectGeometry>(rect);
 }
 
-bool Geometry::MaybeHasOverlapping(const Entity& entity) const {
-  return true;
-}
-
 /////// Path Geometry ///////
 
 FillPathGeometry::FillPathGeometry(const Path& path) : path_(path) {}
@@ -528,10 +524,6 @@ std::optional<Rect> CoverGeometry::GetCoverage(const Matrix& transform) const {
   return Rect::MakeMaximum();
 }
 
-bool CoverGeometry::MaybeHasOverlapping(const Entity& entity) const {
-  return false;
-}
-
 /////// Rect Geometry ///////
 
 RectGeometry::RectGeometry(Rect rect) : rect_(rect) {}
@@ -568,8 +560,4 @@ std::optional<Rect> RectGeometry::GetCoverage(const Matrix& transform) const {
   return rect_.TransformBounds(transform);
 }
 
-bool RectGeometry::MaybeHasOverlapping(const Entity& entity) const {
-  return false;
-}
-
 }  // namespace impeller
diff --git a/impeller/entity/geometry.h b/impeller/entity/geometry.h
index 0a5958dc4b146..edc59cd5c62ac 100644
--- a/impeller/entity/geometry.h
+++ b/impeller/entity/geometry.h
@@ -74,14 +74,6 @@ class Geometry {
   virtual GeometryVertexType GetVertexType() const = 0;
 
   virtual std::optional<Rect> GetCoverage(const Matrix& transform) const = 0;
-
-  /// @brief Whether this geometry has any overlapping painting that would cause
-  ///        opacity peephole to render incorrectly.
-  ///
-  ///        It is always safe to return `true` from this method, and it is
-  ///        advisable to do so if computing overlap would be computationally
-  ///        complex.
-  virtual bool MaybeHasOverlapping(const Entity& entity) const;
 };
 
 /// @brief A geometry that is created from a vertices object.
@@ -216,9 +208,6 @@ class CoverGeometry : public Geometry {
   // |Geometry|
   std::optional<Rect> GetCoverage(const Matrix& transform) const override;
 
-  // |Geometry|
-  bool MaybeHasOverlapping(const Entity& entity) const override;
-
   FML_DISALLOW_COPY_AND_ASSIGN(CoverGeometry);
 };
 
@@ -240,9 +229,6 @@ class RectGeometry : public Geometry {
   // |Geometry|
   std::optional<Rect> GetCoverage(const Matrix& transform) const override;
 
-  // |Geometry|
-  bool MaybeHasOverlapping(const Entity& entity) const override;
-
   Rect rect_;
 
   FML_DISALLOW_COPY_AND_ASSIGN(RectGeometry);

From 6dd02239b88e357ccb3ab76b3bac36ea91fdfd54 Mon Sep 17 00:00:00 2001
From: jonahwilliams <jonahwilliams@google.com>
Date: Tue, 14 Mar 2023 19:03:47 -0700
Subject: [PATCH 10/10] bdero review

---
 impeller/aiks/paint_pass_delegate.cc | 4 ++--
 impeller/entity/entity_pass.cc       | 2 +-
 impeller/entity/entity_pass.h        | 8 +++++---
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/impeller/aiks/paint_pass_delegate.cc b/impeller/aiks/paint_pass_delegate.cc
index a9688e5102c26..d80d6a3a5333d 100644
--- a/impeller/aiks/paint_pass_delegate.cc
+++ b/impeller/aiks/paint_pass_delegate.cc
@@ -89,7 +89,7 @@ bool OpacityPeepholePassDelegate::CanCollapseIntoParentPass(
   }
   bool all_can_accept = true;
   std::vector<Rect> all_coverages;
-  auto had_subpass = entity_pass->IterateAllFlatEntities(
+  auto had_subpass = entity_pass->IterateUntilSubpass(
       [&all_coverages, &all_can_accept](Entity& entity) {
         auto contents = entity.GetContents();
         if (!contents->CanAcceptOpacity(entity)) {
@@ -113,7 +113,7 @@ bool OpacityPeepholePassDelegate::CanCollapseIntoParentPass(
     return false;
   }
   auto alpha = paint_.color.alpha;
-  entity_pass->IterateAllFlatEntities([&alpha](Entity& entity) {
+  entity_pass->IterateUntilSubpass([&alpha](Entity& entity) {
     entity.GetContents()->InheritOpacity(alpha);
     return true;
   });
diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc
index 8d0c44baafccc..27343bf92bcac 100644
--- a/impeller/entity/entity_pass.cc
+++ b/impeller/entity/entity_pass.cc
@@ -623,7 +623,7 @@ void EntityPass::IterateAllEntities(
   }
 }
 
-bool EntityPass::IterateAllFlatEntities(
+bool EntityPass::IterateUntilSubpass(
     const std::function<bool(Entity&)>& iterator) {
   if (!iterator) {
     return true;
diff --git a/impeller/entity/entity_pass.h b/impeller/entity/entity_pass.h
index 3f8abf386a2d4..6f009202b0857 100644
--- a/impeller/entity/entity_pass.h
+++ b/impeller/entity/entity_pass.h
@@ -52,9 +52,11 @@ class EntityPass {
 
   void IterateAllEntities(const std::function<bool(Entity&)>& iterator);
 
-  /// @brief Iterates all entities on this pass, returning if there is a
-  /// subpass.
-  bool IterateAllFlatEntities(const std::function<bool(Entity&)>& iterator);
+  /// @brief  Iterate entities in this pass up until the first subpass is found.
+  ///         This is useful for limiting look-ahead optimizations.
+  ///
+  /// @return Returns whether a subpass was encountered.
+  bool IterateUntilSubpass(const std::function<bool(Entity&)>& iterator);
 
   /// @brief Return the number of entities on this pass.
   size_t GetEntityCount() const;