[Impeller] Reland: hash less stuff per frame. (#55742)

Reverted due to missing glyph when rolling into the framework. Problem was that we should be always using the unrounded rounded scale to compute subpixel position but we sometimes used the rounded scale.

---

Currently we have multiple stages of hashing while font rendering, which is relatively expensive for the actualy required workload. First, we hash the contents of all text frames to compute the unique set of glyphs per frame. Then we diff this hash set against the hashmap of glyphs within the atlas. Finally we hash and lookup the final rendered bounds for each glyph.

We can simplify this to 2. hash lookups for glyphs not yet in the atlas and 1. hash lookup for glyphs that are in the atlas. This is done by combing the step where we uniquely compute glyphs per frame with the diff against the current atlas. When this lookup is performed, we also store the glyph position (if found) in the text_frame itself - which allows text contents to skip the last hash, as long as the glyph has already been rendered.

See #55092

Author: Jonah Williams

impeller/display_list/dl_dispatcher.cc

@@ -1112,10 +1112,13 @@ void TextFrameDispatcher::drawTextFrame(
   }
   auto scale =
       (matrix_ * Matrix::MakeTranslation(Point(x, y))).GetMaxBasisLengthXY();
-  renderer_.GetLazyGlyphAtlas()->AddTextFrame(*text_frame,  //
-                                              scale,        //
-                                              Point(x, y),  //
-                                              properties    //
+  renderer_.GetLazyGlyphAtlas()->AddTextFrame(
+      text_frame,                                       //
+      scale,                                            //
+      Point(x, y),                                      //
+      (properties.stroke || text_frame->HasColor())     //
+          ? std::optional<GlyphProperties>(properties)  //
+          : std::nullopt                                //
   );
 }
 

impeller/entity/contents/text_contents.cc

@@ -17,7 +17,6 @@
 #include "impeller/geometry/point.h"
 #include "impeller/renderer/render_pass.h"
 #include "impeller/typographer/glyph_atlas.h"
-#include "impeller/typographer/lazy_glyph_atlas.h"
 
 namespace impeller {
 
@@ -90,6 +89,10 @@ bool TextContents::Render(const ContentContext& renderer,
     VALIDATION_LOG << "Cannot render glyphs without prepared atlas.";
     return false;
   }
+  if (!frame_->IsFrameComplete()) {
+    VALIDATION_LOG << "Failed to find font glyph bounds.";
+    return false;
+  }
 
   // Information shared by all glyph draw calls.
   pass.SetCommandLabel("TextFrame");
@@ -169,16 +172,12 @@ bool TextContents::Render(const ContentContext& renderer,
         VS::PerVertexData* vtx_contents =
             reinterpret_cast<VS::PerVertexData*>(contents);
         size_t i = 0u;
+        size_t bounds_offset = 0u;
         for (const TextRun& run : frame_->GetRuns()) {
           const Font& font = run.GetFont();
           Scalar rounded_scale = TextFrame::RoundScaledFontSize(
               scale_, font.GetMetrics().point_size);
-          const FontGlyphAtlas* font_atlas =
-              atlas->GetFontGlyphAtlas(font, rounded_scale);
-          if (!font_atlas) {
-            VALIDATION_LOG << "Could not find font in the atlas.";
-            continue;
-          }
+          FontGlyphAtlas* font_atlas = nullptr;
 
           // Adjust glyph position based on the subpixel rounding
           // used by the font.
@@ -201,22 +200,44 @@ bool TextContents::Render(const ContentContext& renderer,
           Point screen_offset = (entity_transform * Point(0, 0));
           for (const TextRun::GlyphPosition& glyph_position :
                run.GetGlyphPositions()) {
-            // Note: uses unrounded scale for more accurate subpixel position.
-            Point subpixel = TextFrame::ComputeSubpixelPosition(
-                glyph_position, font.GetAxisAlignment(), offset_, scale_);
-            std::optional<std::pair<Rect, Rect>> maybe_atlas_glyph_bounds =
-                font_atlas->FindGlyphBounds(SubpixelGlyph{
-                    glyph_position.glyph, subpixel,
-                    (properties_.stroke || frame_->HasColor())
-                        ? std::optional<GlyphProperties>(properties_)
-                        : std::nullopt});
-            if (!maybe_atlas_glyph_bounds.has_value()) {
-              VALIDATION_LOG << "Could not find glyph position in the atlas.";
-              continue;
+            const FrameBounds& frame_bounds =
+                frame_->GetFrameBounds(bounds_offset);
+            bounds_offset++;
+            auto atlas_glyph_bounds = frame_bounds.atlas_bounds;
+            auto glyph_bounds = frame_bounds.glyph_bounds;
+
+            // If frame_bounds.is_placeholder is true, this is the first frame
+            // the glyph has been rendered and so its atlas position was not
+            // known when the glyph was recorded. Perform a slow lookup into the
+            // glyph atlas hash table.
+            if (frame_bounds.is_placeholder) {
+              if (!font_atlas) {
+                font_atlas = atlas->GetOrCreateFontGlyphAtlas(
+                    ScaledFont{font, rounded_scale});
+              }
+
+              if (!font_atlas) {
+                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_, scale_);
+
+              std::optional<FrameBounds> maybe_atlas_glyph_bounds =
+                  font_atlas->FindGlyphBounds(SubpixelGlyph{
+                      glyph_position.glyph,  //
+                      subpixel,              //
+                      GetGlyphProperties()   //
+                  });
+              if (!maybe_atlas_glyph_bounds.has_value()) {
+                VALIDATION_LOG << "Could not find glyph position in the atlas.";
+                continue;
+              }
+              atlas_glyph_bounds =
+                  maybe_atlas_glyph_bounds.value().atlas_bounds;
             }
-            const Rect& atlas_glyph_bounds =
-                maybe_atlas_glyph_bounds.value().first;
-            Rect glyph_bounds = maybe_atlas_glyph_bounds.value().second;
+
             Rect scaled_bounds = glyph_bounds.Scale(1.0 / rounded_scale);
             // For each glyph, we compute two rectangles. One for the vertex
             // positions and one for the texture coordinates (UVs). The atlas
@@ -263,4 +284,10 @@ bool TextContents::Render(const ContentContext& renderer,
   return pass.Draw().ok();
 }
 
+std::optional<GlyphProperties> TextContents::GetGlyphProperties() const {
+  return (properties_.stroke || frame_->HasColor())
+             ? std::optional<GlyphProperties>(properties_)
+             : std::nullopt;
+}
+
 }  // namespace impeller

impeller/entity/contents/text_contents.h

@@ -62,6 +62,8 @@ class TextContents final : public Contents {
               RenderPass& pass) const override;
 
  private:
+  std::optional<GlyphProperties> GetGlyphProperties() const;
+
   std::shared_ptr<TextFrame> frame_;
   Scalar scale_ = 1.0;
   Scalar inherited_opacity_ = 1.0;

impeller/typographer/backends/skia/typographer_context_skia.cc

@@ -283,7 +283,8 @@ static bool BulkUpdateAtlasBitmap(const GlyphAtlas& atlas,
     if (!data.has_value()) {
       continue;
     }
-    auto [pos, bounds] = data.value();
+    auto [pos, bounds, placeholder] = data.value();
+    FML_DCHECK(!placeholder);
     Size size = pos.GetSize();
     if (size.IsEmpty()) {
       continue;
@@ -326,7 +327,9 @@ static bool UpdateAtlasBitmap(const GlyphAtlas& atlas,
     if (!data.has_value()) {
       continue;
     }
-    auto [pos, bounds] = data.value();
+    auto [pos, bounds, placeholder] = data.value();
+    FML_DCHECK(!placeholder);
+
     Size size = pos.GetSize();
     if (size.IsEmpty()) {
       continue;
@@ -404,61 +407,88 @@ static Rect ComputeGlyphSize(const SkFont& font,
                         scaled_bounds.fBottom);
 };
 
-static void CollectNewGlyphs(const std::shared_ptr<GlyphAtlas>& atlas,
-                             const FontGlyphMap& font_glyph_map,
-                             std::vector<FontGlyphPair>& new_glyphs,
-                             std::vector<Rect>& glyph_sizes) {
-  for (const auto& font_value : font_glyph_map) {
-    const ScaledFont& scaled_font = font_value.first;
-    const FontGlyphAtlas* font_glyph_atlas =
-        atlas->GetFontGlyphAtlas(scaled_font.font, scaled_font.scale);
-
-    auto metrics = scaled_font.font.GetMetrics();
-
-    SkFont sk_font(
-        TypefaceSkia::Cast(*scaled_font.font.GetTypeface()).GetSkiaTypeface(),
-        metrics.point_size, metrics.scaleX, metrics.skewX);
-    sk_font.setEdging(SkFont::Edging::kAntiAlias);
-    sk_font.setHinting(SkFontHinting::kSlight);
-    sk_font.setEmbolden(metrics.embolden);
-    // Rather than computing the bounds at the requested point size and scaling
-    // up the bounds, we scale up the font size and request the bounds. This
-    // seems to give more accurate bounds information.
-    sk_font.setSize(sk_font.getSize() * scaled_font.scale);
-    sk_font.setSubpixel(true);
-
-    if (font_glyph_atlas) {
-      for (const SubpixelGlyph& glyph : font_value.second) {
-        if (!font_glyph_atlas->FindGlyphBounds(glyph)) {
-          new_glyphs.emplace_back(scaled_font, glyph);
-          glyph_sizes.push_back(
-              ComputeGlyphSize(sk_font, glyph, scaled_font.scale));
+std::pair<std::vector<FontGlyphPair>, std::vector<Rect>>
+TypographerContextSkia::CollectNewGlyphs(
+    const std::shared_ptr<GlyphAtlas>& atlas,
+    const std::vector<std::shared_ptr<TextFrame>>& text_frames) {
+  std::vector<FontGlyphPair> new_glyphs;
+  std::vector<Rect> glyph_sizes;
+  for (const auto& frame : text_frames) {
+    // 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();
+
+    for (const auto& run : frame->GetRuns()) {
+      auto metrics = run.GetFont().GetMetrics();
+
+      auto rounded_scale =
+          TextFrame::RoundScaledFontSize(frame->GetScale(), metrics.point_size);
+      ScaledFont scaled_font{.font = run.GetFont(), .scale = rounded_scale};
+
+      FontGlyphAtlas* font_glyph_atlas =
+          atlas->GetOrCreateFontGlyphAtlas(scaled_font);
+      FML_DCHECK(!!font_glyph_atlas);
+
+      SkFont sk_font(
+          TypefaceSkia::Cast(*scaled_font.font.GetTypeface()).GetSkiaTypeface(),
+          metrics.point_size, metrics.scaleX, metrics.skewX);
+      sk_font.setEdging(SkFont::Edging::kAntiAlias);
+      sk_font.setHinting(SkFontHinting::kSlight);
+      sk_font.setEmbolden(metrics.embolden);
+      // Rather than computing the bounds at the requested point size and
+      // scaling up the bounds, we scale up the font size and request the
+      // bounds. This seems to give more accurate bounds information.
+      sk_font.setSize(sk_font.getSize() * scaled_font.scale);
+      sk_font.setSubpixel(true);
+
+      for (const auto& glyph_position : run.GetGlyphPositions()) {
+        Point subpixel = TextFrame::ComputeSubpixelPosition(
+            glyph_position, scaled_font.font.GetAxisAlignment(),
+            frame->GetOffset(), frame->GetScale());
+        SubpixelGlyph subpixel_glyph(glyph_position.glyph, subpixel,
+                                     frame->GetProperties());
+        const auto& font_glyph_bounds =
+            font_glyph_atlas->FindGlyphBounds(subpixel_glyph);
+
+        if (!font_glyph_bounds.has_value()) {
+          new_glyphs.push_back(FontGlyphPair{scaled_font, subpixel_glyph});
+          auto glyph_bounds =
+              ComputeGlyphSize(sk_font, subpixel_glyph, scaled_font.scale);
+          glyph_sizes.push_back(glyph_bounds);
+
+          auto frame_bounds = FrameBounds{
+              Rect::MakeLTRB(0, 0, 0, 0),  //
+              glyph_bounds,                //
+              /*placeholder=*/true         //
+          };
+
+          frame->AppendFrameBounds(frame_bounds);
+          font_glyph_atlas->AppendGlyph(subpixel_glyph, frame_bounds);
+        } else {
+          frame->AppendFrameBounds(font_glyph_bounds.value());
         }
       }
-    } else {
-      for (const SubpixelGlyph& glyph : font_value.second) {
-        new_glyphs.emplace_back(scaled_font, glyph);
-        glyph_sizes.push_back(
-            ComputeGlyphSize(sk_font, glyph, scaled_font.scale));
-      }
     }
   }
+  return {std::move(new_glyphs), std::move(glyph_sizes)};
 }
 
 std::shared_ptr<GlyphAtlas> TypographerContextSkia::CreateGlyphAtlas(
     Context& context,
     GlyphAtlas::Type type,
     HostBuffer& host_buffer,
     const std::shared_ptr<GlyphAtlasContext>& atlas_context,
-    const FontGlyphMap& font_glyph_map) const {
+    const std::vector<std::shared_ptr<TextFrame>>& text_frames) const {
   TRACE_EVENT0("impeller", __FUNCTION__);
   if (!IsValid()) {
     return nullptr;
   }
   std::shared_ptr<GlyphAtlas> last_atlas = atlas_context->GetGlyphAtlas();
   FML_DCHECK(last_atlas->GetType() == type);
 
-  if (font_glyph_map.empty()) {
+  if (text_frames.empty()) {
     return last_atlas;
   }
 
@@ -467,9 +497,7 @@ std::shared_ptr<GlyphAtlas> TypographerContextSkia::CreateGlyphAtlas(
   //         with the current atlas and reuse if possible. For each new font and
   //         glyph pair, compute the glyph size at scale.
   // ---------------------------------------------------------------------------
-  std::vector<FontGlyphPair> new_glyphs;
-  std::vector<Rect> glyph_sizes;
-  CollectNewGlyphs(last_atlas, font_glyph_map, new_glyphs, glyph_sizes);
+  auto [new_glyphs, glyph_sizes] = CollectNewGlyphs(last_atlas, text_frames);
   if (new_glyphs.size() == 0) {
     return last_atlas;
   }
@@ -540,9 +568,11 @@ std::shared_ptr<GlyphAtlas> TypographerContextSkia::CreateGlyphAtlas(
     blit_old_atlas = false;
     new_atlas = std::make_shared<GlyphAtlas>(type);
 
-    new_glyphs.clear();
-    glyph_sizes.clear();
-    CollectNewGlyphs(new_atlas, font_glyph_map, new_glyphs, glyph_sizes);
+    auto [update_glyphs, update_sizes] =
+        CollectNewGlyphs(new_atlas, text_frames);
+    new_glyphs = std::move(update_glyphs);
+    glyph_sizes = std::move(update_sizes);
+
     glyph_positions.clear();
     glyph_positions.reserve(new_glyphs.size());
     first_missing_index = 0;

impeller/typographer/backends/skia/typographer_context_skia.h

@@ -27,9 +27,14 @@ class TypographerContextSkia : public TypographerContext {
       GlyphAtlas::Type type,
       HostBuffer& host_buffer,
       const std::shared_ptr<GlyphAtlasContext>& atlas_context,
-      const FontGlyphMap& font_glyph_map) const override;
+      const std::vector<std::shared_ptr<TextFrame>>& text_frames)
+      const override;
 
  private:
+  static std::pair<std::vector<FontGlyphPair>, std::vector<Rect>>
+  CollectNewGlyphs(const std::shared_ptr<GlyphAtlas>& atlas,
+                   const std::vector<std::shared_ptr<TextFrame>>& text_frames);
+
   TypographerContextSkia(const TypographerContextSkia&) = delete;
 
   TypographerContextSkia& operator=(const TypographerContextSkia&) = delete;

impeller/typographer/font_glyph_pair.h

@@ -5,10 +5,6 @@
 #ifndef FLUTTER_IMPELLER_TYPOGRAPHER_FONT_GLYPH_PAIR_H_
 #define FLUTTER_IMPELLER_TYPOGRAPHER_FONT_GLYPH_PAIR_H_
 
-#include <unordered_map>
-#include <unordered_set>
-
-#include "fml/hash_combine.h"
 #include "impeller/geometry/color.h"
 #include "impeller/geometry/path.h"
 #include "impeller/geometry/point.h"
@@ -99,23 +95,15 @@ struct SubpixelGlyph {
   };
 };
 
-using FontGlyphMap =
-    std::unordered_map<ScaledFont,
-                       std::unordered_set<SubpixelGlyph,
-                                          SubpixelGlyph::Hash,
-                                          SubpixelGlyph::Equal>,
-                       ScaledFont::Hash,
-                       ScaledFont::Equal>;
-
 //------------------------------------------------------------------------------
 /// @brief      A font along with a glyph in that font rendered at a particular
 ///             scale and subpixel position.
 ///
 struct FontGlyphPair {
   FontGlyphPair(const ScaledFont& sf, const SubpixelGlyph& g)
       : scaled_font(sf), glyph(g) {}
-  const ScaledFont& scaled_font;
-  const SubpixelGlyph& glyph;
+  ScaledFont scaled_font;
+  SubpixelGlyph glyph;
 };
 
 }  // namespace impeller