Revert "[Impeller] cache even more text frame data to skip lookups."

impeller/display_list/dl_dispatcher.cc

@@ -1105,9 +1105,8 @@ void FirstPassDispatcher::drawTextFrame(
     // we do not double-apply the alpha.
     properties.color = paint_.color.WithAlpha(1.0);
   }
-  auto scale = TextFrame::RoundScaledFontSize(
-      (matrix_ * Matrix::MakeTranslation(Point(x, y))).GetMaxBasisLengthXY());
-
+  auto scale =
+      (matrix_ * Matrix::MakeTranslation(Point(x, y))).GetMaxBasisLengthXY();
   renderer_.GetLazyGlyphAtlas()->AddTextFrame(
       text_frame,                                       //
       scale,                                            //

impeller/entity/contents/text_contents.cc

@@ -175,7 +175,8 @@ bool TextContents::Render(const ContentContext& renderer,
         size_t bounds_offset = 0u;
         for (const TextRun& run : frame_->GetRuns()) {
           const Font& font = run.GetFont();
-          Scalar rounded_scale = TextFrame::RoundScaledFontSize(scale_);
+          Scalar rounded_scale = TextFrame::RoundScaledFontSize(
+              scale_, font.GetMetrics().point_size);
           FontGlyphAtlas* font_atlas = nullptr;
 
           // Adjust glyph position based on the subpixel rounding
@@ -219,9 +220,9 @@ bool TextContents::Render(const ContentContext& renderer,
                 VALIDATION_LOG << "Could not find font in the atlas.";
                 continue;
               }
+              // Note: uses unrounded scale for more accurate subpixel position.
               Point subpixel = TextFrame::ComputeSubpixelPosition(
-                  glyph_position, font.GetAxisAlignment(), offset_,
-                  rounded_scale);
+                  glyph_position, font.GetAxisAlignment(), offset_, scale_);
 
               std::optional<FrameBounds> maybe_atlas_glyph_bounds =
                   font_atlas->FindGlyphBounds(SubpixelGlyph{

impeller/entity/entity_unittests.cc

@@ -2120,11 +2120,11 @@ TEST_P(EntityTest, ColorFilterContentsWithLargeGeometry) {
 }
 
 TEST_P(EntityTest, TextContentsCeilsGlyphScaleToDecimal) {
-  ASSERT_EQ(TextFrame::RoundScaledFontSize(0.4321111f), 0.43f);
-  ASSERT_EQ(TextFrame::RoundScaledFontSize(0.5321111f), 0.53f);
-  ASSERT_EQ(TextFrame::RoundScaledFontSize(2.1f), 2.1f);
-  ASSERT_EQ(TextFrame::RoundScaledFontSize(0.0f), 0.0f);
-  ASSERT_EQ(TextFrame::RoundScaledFontSize(100000000.0f), 48.0f);
+  ASSERT_EQ(TextFrame::RoundScaledFontSize(0.4321111f, 12), 0.43f);
+  ASSERT_EQ(TextFrame::RoundScaledFontSize(0.5321111f, 12), 0.53f);
+  ASSERT_EQ(TextFrame::RoundScaledFontSize(2.1f, 12), 2.1f);
+  ASSERT_EQ(TextFrame::RoundScaledFontSize(0.0f, 12), 0.0f);
+  ASSERT_EQ(TextFrame::RoundScaledFontSize(100000000.0f, 12), 48.0f);
 }
 
 TEST_P(EntityTest, SpecializationConstantsAreAppliedToVariants) {

impeller/typographer/backends/skia/typographer_context_skia.cc

@@ -413,20 +413,18 @@ TypographerContextSkia::CollectNewGlyphs(
     const std::vector<std::shared_ptr<TextFrame>>& text_frames) {
   std::vector<FontGlyphPair> new_glyphs;
   std::vector<Rect> glyph_sizes;
-  size_t generation_id = atlas->GetAtlasGeneration();
   for (const auto& frame : text_frames) {
-    if (frame->IsFrameComplete() &&
-        frame->GetAtlasGeneration() == generation_id &&
-        !frame->GetFrameBounds(0).is_placeholder) {
-      continue;
-    }
+    // TODO(jonahwilliams): unless we destroy the atlas (which we know about),
+    // we could probably guarantee that a text frame that is complete does not
+    // need to be processed unless the scale or properties changed. I'm leaving
+    // this as a future optimization.
     frame->ClearFrameBounds();
-    frame->SetAtlasGeneration(generation_id);
 
     for (const auto& run : frame->GetRuns()) {
       auto metrics = run.GetFont().GetMetrics();
 
-      auto rounded_scale = TextFrame::RoundScaledFontSize(frame->GetScale());
+      auto rounded_scale =
+          TextFrame::RoundScaledFontSize(frame->GetScale(), metrics.point_size);
       ScaledFont scaled_font{.font = run.GetFont(), .scale = rounded_scale};
 
       FontGlyphAtlas* font_glyph_atlas =
@@ -568,8 +566,7 @@ std::shared_ptr<GlyphAtlas> TypographerContextSkia::CreateGlyphAtlas(
   if (atlas_context->GetAtlasSize().height >= max_texture_height ||
       context.GetBackendType() == Context::BackendType::kOpenGLES) {
     blit_old_atlas = false;
-    new_atlas = std::make_shared<GlyphAtlas>(
-        type, /*initial_generation=*/last_atlas->GetAtlasGeneration() + 1);
+    new_atlas = std::make_shared<GlyphAtlas>(type);
 
     auto [update_glyphs, update_sizes] =
         CollectNewGlyphs(new_atlas, text_frames);

impeller/typographer/font_glyph_pair.h

@@ -20,17 +20,6 @@ struct GlyphProperties {
   Join stroke_join = Join::kMiter;
   Scalar stroke_miter = 4.0;
   bool stroke = false;
-
-  struct Equal {
-    constexpr bool operator()(const impeller::GlyphProperties& lhs,
-                              const impeller::GlyphProperties& rhs) const {
-      return lhs.color.ToARGB() == rhs.color.ToARGB() &&
-             lhs.stroke == rhs.stroke && lhs.stroke_cap == rhs.stroke_cap &&
-             lhs.stroke_join == rhs.stroke_join &&
-             lhs.stroke_miter == rhs.stroke_miter &&
-             lhs.stroke_width == rhs.stroke_width;
-    }
-  };
 };
 
 //------------------------------------------------------------------------------
@@ -96,8 +85,12 @@ struct SubpixelGlyph {
              lhs.glyph.type == rhs.glyph.type &&
              lhs.subpixel_offset == rhs.subpixel_offset &&
              lhs.properties.has_value() && rhs.properties.has_value() &&
-             GlyphProperties::Equal{}(lhs.properties.value(),
-                                      rhs.properties.value());
+             lhs.properties->color.ToARGB() == rhs.properties->color.ToARGB() &&
+             lhs.properties->stroke == rhs.properties->stroke &&
+             lhs.properties->stroke_cap == rhs.properties->stroke_cap &&
+             lhs.properties->stroke_join == rhs.properties->stroke_join &&
+             lhs.properties->stroke_miter == rhs.properties->stroke_miter &&
+             lhs.properties->stroke_width == rhs.properties->stroke_width;
     }
   };
 };

impeller/typographer/glyph_atlas.cc

@@ -12,8 +12,7 @@
 namespace impeller {
 
 GlyphAtlasContext::GlyphAtlasContext(GlyphAtlas::Type type)
-    : atlas_(std::make_shared<GlyphAtlas>(type, /*initial_generation=*/0)),
-      atlas_size_(ISize(0, 0)) {}
+    : atlas_(std::make_shared<GlyphAtlas>(type)), atlas_size_(ISize(0, 0)) {}
 
 GlyphAtlasContext::~GlyphAtlasContext() {}
 
@@ -46,8 +45,7 @@ void GlyphAtlasContext::UpdateRectPacker(
   rect_packer_ = std::move(rect_packer);
 }
 
-GlyphAtlas::GlyphAtlas(Type type, size_t initial_generation)
-    : type_(type), generation_(initial_generation) {}
+GlyphAtlas::GlyphAtlas(Type type) : type_(type) {}
 
 GlyphAtlas::~GlyphAtlas() = default;
 
@@ -67,14 +65,6 @@ void GlyphAtlas::SetTexture(std::shared_ptr<Texture> texture) {
   texture_ = std::move(texture);
 }
 
-size_t GlyphAtlas::GetAtlasGeneration() const {
-  return generation_;
-}
-
-void GlyphAtlas::SetAtlasGeneration(size_t generation) {
-  generation_ = generation;
-}
-
 void GlyphAtlas::AddTypefaceGlyphPositionAndBounds(const FontGlyphPair& pair,
                                                    Rect position,
                                                    Rect bounds) {

impeller/typographer/glyph_atlas.h

@@ -58,9 +58,8 @@ class GlyphAtlas {
   /// @brief      Create an empty glyph atlas.
   ///
   /// @param[in]  type  How the glyphs are represented in the texture.
-  /// @param[in]  initial_generation the atlas generation.
   ///
-  GlyphAtlas(Type type, size_t initial_generation);
+  explicit GlyphAtlas(Type type);
 
   ~GlyphAtlas();
 
@@ -143,22 +142,9 @@ class GlyphAtlas {
   ///
   FontGlyphAtlas* GetOrCreateFontGlyphAtlas(const ScaledFont& scaled_font);
 
-  //----------------------------------------------------------------------------
-  /// @brief      Retrieve the generation id for this glyph atlas.
-  ///
-  ///             The generation id is used to match with a TextFrame to
-  ///             determine if the frame is guaranteed to already be populated
-  ///             in the atlas.
-  size_t GetAtlasGeneration() const;
-
-  //----------------------------------------------------------------------------
-  /// @brief      Update the atlas generation.
-  void SetAtlasGeneration(size_t value);
-
  private:
   const Type type_;
   std::shared_ptr<Texture> texture_;
-  size_t generation_ = 0;
 
   std::unordered_map<ScaledFont,
                      FontGlyphAtlas,

impeller/typographer/text_frame.cc

@@ -3,25 +3,11 @@
 // found in the LICENSE file.
 
 #include "impeller/typographer/text_frame.h"
-#include "impeller/geometry/scalar.h"
 #include "impeller/typographer/font.h"
 #include "impeller/typographer/font_glyph_pair.h"
 
 namespace impeller {
 
-namespace {
-static bool TextPropertiesEquals(const std::optional<GlyphProperties>& a,
-                                 const std::optional<GlyphProperties>& b) {
-  if (!a.has_value() && !b.has_value()) {
-    return true;
-  }
-  if (a.has_value() && b.has_value()) {
-    return GlyphProperties::Equal{}(a.value(), b.value());
-  }
-  return false;
-}
-}  // namespace
-
 TextFrame::TextFrame() = default;
 
 TextFrame::TextFrame(std::vector<TextRun>& runs, Rect bounds, bool has_color)
@@ -51,7 +37,7 @@ bool TextFrame::HasColor() const {
 }
 
 // static
-Scalar TextFrame::RoundScaledFontSize(Scalar scale) {
+Scalar TextFrame::RoundScaledFontSize(Scalar scale, Scalar point_size) {
   // An arbitrarily chosen maximum text scale to ensure that regardless of the
   // CTM, a glyph will fit in the atlas. If we clamp significantly, this may
   // reduce fidelity but is preferable to the alternative of failing to render.
@@ -101,12 +87,6 @@ Point TextFrame::ComputeSubpixelPosition(
 void TextFrame::SetPerFrameData(Scalar scale,
                                 Point offset,
                                 std::optional<GlyphProperties> properties) {
-  if (!ScalarNearlyEqual(scale_, scale) ||
-      !ScalarNearlyEqual(offset_.x, offset.x) ||
-      !ScalarNearlyEqual(offset_.y, offset.y) ||
-      !TextPropertiesEquals(properties_, properties)) {
-    bound_values_.clear();
-  }
   scale_ = scale;
   offset_ = offset;
   properties_ = properties;
@@ -141,15 +121,7 @@ bool TextFrame::IsFrameComplete() const {
 }
 
 const FrameBounds& TextFrame::GetFrameBounds(size_t index) const {
-  return bound_values_[index];
-}
-
-size_t TextFrame::GetAtlasGeneration() const {
-  return generation_;
-}
-
-void TextFrame::SetAtlasGeneration(size_t value) {
-  generation_ = value;
+  return bound_values_.at(index);
 }
 
 }  // namespace impeller

impeller/typographer/text_frame.h

@@ -32,7 +32,7 @@ class TextFrame {
       Point offset,
       Scalar scale);
 
-  static Scalar RoundScaledFontSize(Scalar scale);
+  static Scalar RoundScaledFontSize(Scalar scale, Scalar point_size);
 
   //----------------------------------------------------------------------------
   /// @brief      The conservative bounding box for this text frame.
@@ -84,18 +84,13 @@ class TextFrame {
                        Point offset,
                        std::optional<GlyphProperties> properties);
 
-  // A generation id for the glyph atlas this text run was associated
-  // with. As long as the frame generation matches the atlas generation,
-  // the contents are guaranteed to be populated and do not need to be
-  // processed.
-  size_t GetAtlasGeneration() const;
-
   TextFrame& operator=(TextFrame&& other) = default;
 
   TextFrame(const TextFrame& other) = default;
 
  private:
   friend class TypographerContextSkia;
+  friend class TypographerContextSTB;
   friend class LazyGlyphAtlas;
 
   Scalar GetScale() const;
@@ -108,17 +103,14 @@ class TextFrame {
 
   void ClearFrameBounds();
 
-  void SetAtlasGeneration(size_t value);
-
   std::vector<TextRun> runs_;
   Rect bounds_;
   bool has_color_;
 
   // Data that is cached when rendering the text frame and is only
-  // valid for the current atlas generation.
+  // valid for a single frame.
   std::vector<FrameBounds> bound_values_;
   Scalar scale_ = 0;
-  size_t generation_ = 0;
   Point offset_;
   std::optional<GlyphProperties> properties_;
 };