Revert "[Impeller] hash even less stuff per frame. (#55092)"

This reverts commit d283bc8.

Author: auto-submit[bot]

impeller/display_list/dl_dispatcher.cc

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

impeller/entity/contents/text_contents.cc

@@ -90,10 +90,6 @@ 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");
@@ -173,12 +169,16 @@ 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);
-          FontGlyphAtlas* font_atlas = nullptr;
+          const FontGlyphAtlas* font_atlas =
+              atlas->GetFontGlyphAtlas(font, rounded_scale);
+          if (!font_atlas) {
+            VALIDATION_LOG << "Could not find font in the atlas.";
+            continue;
+          }
 
           // Adjust glyph position based on the subpixel rounding
           // used by the font.
@@ -201,45 +201,22 @@ bool TextContents::Render(const ContentContext& renderer,
           Point screen_offset = (entity_transform * Point(0, 0));
           for (const TextRun::GlyphPosition& glyph_position :
                run.GetGlyphPositions()) {
-            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) {
-              // Note: uses unrounded scale for more accurate subpixel position.
-              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;
+            // 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 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
@@ -289,10 +266,4 @@ 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,8 +62,6 @@ 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

@@ -282,8 +282,7 @@ static bool BulkUpdateAtlasBitmap(const GlyphAtlas& atlas,
     if (!data.has_value()) {
       continue;
     }
-    auto [pos, bounds, placeholder] = data.value();
-    FML_DCHECK(!placeholder);
+    auto [pos, bounds] = data.value();
     Size size = pos.GetSize();
     if (size.IsEmpty()) {
       continue;
@@ -326,9 +325,7 @@ static bool UpdateAtlasBitmap(const GlyphAtlas& atlas,
     if (!data.has_value()) {
       continue;
     }
-    auto [pos, bounds, placeholder] = data.value();
-    FML_DCHECK(!placeholder);
-
+    auto [pos, bounds] = data.value();
     Size size = pos.GetSize();
     if (size.IsEmpty()) {
       continue;
@@ -405,88 +402,61 @@ static Rect ComputeGlyphSize(const SkFont& font,
                         scaled_bounds.fBottom);
 };
 
-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(), rounded_scale);
-        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());
+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));
         }
       }
+    } 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 std::vector<std::shared_ptr<TextFrame>>& text_frames) const {
+    const FontGlyphMap& font_glyph_map) 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 (text_frames.empty()) {
+  if (font_glyph_map.empty()) {
     return last_atlas;
   }
 
@@ -495,7 +465,9 @@ 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.
   // ---------------------------------------------------------------------------
-  auto [new_glyphs, glyph_sizes] = CollectNewGlyphs(last_atlas, text_frames);
+  std::vector<FontGlyphPair> new_glyphs;
+  std::vector<Rect> glyph_sizes;
+  CollectNewGlyphs(last_atlas, font_glyph_map, new_glyphs, glyph_sizes);
   if (new_glyphs.size() == 0) {
     return last_atlas;
   }
@@ -564,11 +536,9 @@ std::shared_ptr<GlyphAtlas> TypographerContextSkia::CreateGlyphAtlas(
     blit_old_atlas = false;
     new_atlas = std::make_shared<GlyphAtlas>(type);
 
-    auto [update_glyphs, update_sizes] =
-        CollectNewGlyphs(new_atlas, text_frames);
-    new_glyphs = std::move(update_glyphs);
-    glyph_sizes = std::move(update_sizes);
-
+    new_glyphs.clear();
+    glyph_sizes.clear();
+    CollectNewGlyphs(new_atlas, font_glyph_map, new_glyphs, glyph_sizes);
     glyph_positions.clear();
     glyph_positions.reserve(new_glyphs.size());
     first_missing_index = 0;

impeller/typographer/backends/skia/typographer_context_skia.h

@@ -27,14 +27,9 @@ class TypographerContextSkia : public TypographerContext {
       GlyphAtlas::Type type,
       HostBuffer& host_buffer,
       const std::shared_ptr<GlyphAtlasContext>& atlas_context,
-      const std::vector<std::shared_ptr<TextFrame>>& text_frames)
-      const override;
+      const FontGlyphMap& font_glyph_map) 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;