[Impeller] Pack 👏 the 👏 atlas 👏

impeller/entity/contents/text_contents.cc

@@ -128,10 +128,6 @@ static bool CommonRender(
 
   for (const auto& run : frame.GetRuns()) {
     auto font = run.GetFont();
-    auto glyph_size_ = font.GetMetrics().GetBoundingBox().size;
-    auto glyph_size = Point{static_cast<Scalar>(glyph_size_.width),
-                            static_cast<Scalar>(glyph_size_.height)};
-    auto metrics_offset = font.GetMetrics().min_extent;
 
     for (const auto& glyph_position : run.GetGlyphPositions()) {
       FontGlyphPair font_glyph_pair{font, glyph_position.glyph};
@@ -144,13 +140,14 @@ static bool CommonRender(
       auto atlas_position = atlas_glyph_pos->origin;
       auto atlas_glyph_size =
           Point{atlas_glyph_pos->size.width, atlas_glyph_pos->size.height};
-      auto offset_glyph_position = glyph_position.position + metrics_offset;
+      auto offset_glyph_position =
+          glyph_position.position + glyph_position.glyph.bounds.origin;
 
       for (const auto& point : unit_points) {
         typename VS::PerVertexData vtx;
         vtx.unit_position = point;
         vtx.destination_position = offset_glyph_position + Point(0.5, 0.5);
-        vtx.destination_size = glyph_size - Point(1.0, 1.0);
+        vtx.destination_size = Point(glyph_position.glyph.bounds.size);
         vtx.source_position = atlas_position + Point(0.5, 0.5);
         vtx.source_glyph_size = atlas_glyph_size - Point(1.0, 1.0);
         if constexpr (std::is_same_v<TPipeline, GlyphAtlasPipeline>) {

impeller/typographer/backends/skia/text_frame_skia.cc

@@ -27,26 +27,14 @@ static Font ToFont(const SkTextBlobRunIterator& run, Scalar scale) {
   Font::Metrics metrics;
   metrics.scale = scale;
   metrics.point_size = font.getSize();
-  metrics.ascent = sk_metrics.fAscent;
-  metrics.descent = sk_metrics.fDescent;
-  metrics.min_extent = {sk_metrics.fXMin, sk_metrics.fTop};
-  metrics.max_extent = {sk_metrics.fXMax, sk_metrics.fBottom};
-
-  std::vector<SkRect> glyph_bounds;
-  SkPaint paint;
-
-  glyph_bounds.resize(run.glyphCount());
-  run.font().getBounds(run.glyphs(), run.glyphCount(), glyph_bounds.data(),
-                       nullptr);
-  for (auto& bounds : glyph_bounds) {
-    metrics.min_extent = metrics.min_extent.Min({bounds.fLeft, bounds.fTop});
-    metrics.max_extent =
-        metrics.max_extent.Max({bounds.fRight, bounds.fBottom});
-  }
 
   return Font{std::move(typeface), metrics};
 }
 
+static Rect ToRect(const SkRect& rect) {
+  return Rect::MakeLTRB(rect.fLeft, rect.fTop, rect.fRight, rect.fBottom);
+}
+
 TextFrame TextFrameFromTextBlob(const sk_sp<SkTextBlob>& blob, Scalar scale) {
   if (!blob) {
     return {};
@@ -71,7 +59,11 @@ TextFrame TextFrameFromTextBlob(const sk_sp<SkTextBlob>& blob, Scalar scale) {
       case SkTextBlobRunIterator::kHorizontal_Positioning:
         FML_DLOG(ERROR) << "Unimplemented.";
         break;
-      case SkTextBlobRunIterator::kFull_Positioning:
+      case SkTextBlobRunIterator::kFull_Positioning: {
+        std::vector<SkRect> glyph_bounds;
+        glyph_bounds.resize(glyph_count);
+        run.font().getBounds(glyphs, glyph_count, glyph_bounds.data(), nullptr);
+
         for (auto i = 0u; i < glyph_count; i++) {
           // kFull_Positioning has two scalars per glyph.
           const SkPoint* glyph_points = run.points();
@@ -80,10 +72,11 @@ TextFrame TextFrameFromTextBlob(const sk_sp<SkTextBlob>& blob, Scalar scale) {
                                  ? Glyph::Type::kBitmap
                                  : Glyph::Type::kPath;
 
-          text_run.AddGlyph(Glyph{glyphs[i], type},
+          text_run.AddGlyph(Glyph{glyphs[i], type, ToRect(glyph_bounds[i])},
                             Point{point->x(), point->y()});
         }
         break;
+      }
       case SkTextBlobRunIterator::kRSXform_Positioning:
         FML_DLOG(ERROR) << "Unimplemented.";
         break;

impeller/typographer/backends/skia/text_render_context_skia.cc

@@ -71,16 +71,16 @@ static size_t PairsFitInAtlasOfSize(const FontGlyphPair::Vector& pairs,
   glyph_positions.clear();
   glyph_positions.reserve(pairs.size());
 
-  // TODO(114563): We might be able to remove this per-glyph padding if we fix
-  //               the underlying causes of the overlap.
+  // TODO(bdero): We might be able to remove this per-glyph padding if we fix
+  //              the underlying causes of the overlap.
+  //              https://github.com/flutter/flutter/issues/114563
   constexpr auto padding = 2;
 
   for (size_t i = 0; i < pairs.size(); i++) {
     const auto& pair = pairs[i];
 
     const auto glyph_size =
-        ISize::Ceil(pair.font.GetMetrics().GetBoundingBox().size *
-                    pair.font.GetMetrics().scale);
+        ISize::Ceil((pair.glyph.bounds * pair.font.GetMetrics().scale).size);
     SkIPoint16 location_in_atlas;
     if (!rect_packer->addRect(glyph_size.width + padding,   //
                               glyph_size.height + padding,  //
@@ -288,13 +288,14 @@ static std::shared_ptr<SkBitmap> CreateAtlasBitmap(const GlyphAtlas& atlas,
     glyph_paint.setColor(glyph_color);
     canvas->resetMatrix();
     canvas->scale(metrics.scale, metrics.scale);
-    canvas->drawGlyphs(1u,         // count
-                       &glyph_id,  // glyphs
-                       &position,  // positions
-                       SkPoint::Make(-metrics.min_extent.x,
-                                     -metrics.min_extent.y),  // origin
-                       sk_font,                               // font
-                       glyph_paint                            // paint
+    canvas->drawGlyphs(
+        1u,         // count
+        &glyph_id,  // glyphs
+        &position,  // positions
+        SkPoint::Make(-font_glyph.glyph.bounds.GetLeft(),
+                      -font_glyph.glyph.bounds.GetTop()),  // origin
+        sk_font,                                           // font
+        glyph_paint                                        // paint
     );
     return true;
   });

impeller/typographer/font.h

@@ -38,46 +38,9 @@ class Font : public Comparable<Font> {
     /// The point size of the font.
     ///
     Scalar point_size = 12.0f;
-    //--------------------------------------------------------------------------
-    /// The font ascent relative to the baseline. This is usually negative as
-    /// moving upwards (ascending) in an upper-left-origin coordinate system
-    /// yields smaller numbers.
-    ///
-    Scalar ascent = 0.0f;
-    //--------------------------------------------------------------------------
-    /// The font descent relative to the baseline. This is usually positive as
-    /// moving downwards (descending) in an upper-left-origin coordinate system
-    /// yields larger numbers.
-    ///
-    Scalar descent = 0.0f;
-    //--------------------------------------------------------------------------
-    /// The minimum glyph extents relative to the origin. Typically negative in
-    /// an upper-left-origin coordinate system.
-    ///
-    Point min_extent;
-    //--------------------------------------------------------------------------
-    /// The maximum glyph extents relative to the origin. Typically positive in
-    /// an upper-left-origin coordinate system.
-    ///
-    Point max_extent;
-
-    //--------------------------------------------------------------------------
-    /// @brief      The union of the bounding boxes of all the glyphs.
-    ///
-    /// @return     The bounding box.
-    ///
-    constexpr Rect GetBoundingBox() const {
-      return Rect::MakeLTRB(min_extent.x,  //
-                            min_extent.y,  //
-                            max_extent.x,  //
-                            max_extent.y   //
-      );
-    }
 
     constexpr bool operator==(const Metrics& o) const {
-      return scale == o.scale && point_size == o.point_size &&
-             ascent == o.ascent && descent == o.descent &&
-             min_extent == o.min_extent && max_extent == o.max_extent;
+      return scale == o.scale && point_size == o.point_size;
     }
   };
 
@@ -113,6 +76,6 @@ class Font : public Comparable<Font> {
 template <>
 struct std::hash<impeller::Font::Metrics> {
   constexpr std::size_t operator()(const impeller::Font::Metrics& m) const {
-    return fml::HashCombine(m.scale, m.point_size, m.ascent, m.descent);
+    return fml::HashCombine(m.scale, m.point_size);
   }
 };

impeller/typographer/glyph.h

@@ -8,6 +8,7 @@
 #include <functional>
 
 #include "flutter/fml/macros.h"
+#include "impeller/geometry/rect.h"
 
 namespace impeller {
 
@@ -23,11 +24,18 @@ struct Glyph {
   uint16_t index = 0;
 
   //------------------------------------------------------------------------------
-  /// @brief      Whether the glyph is a path or a bitmap.
+  /// @brief  Whether the glyph is a path or a bitmap.
   ///
   Type type = Type::kPath;
 
-  Glyph(uint16_t p_index, Type p_type) : index(p_index), type(p_type) {}
+  //------------------------------------------------------------------------------
+  /// @brief  Visibility coverage of the glyph in text run space (relative to
+  ///         the baseline, no scaling applied).
+  ///
+  Rect bounds;
+
+  Glyph(uint16_t p_index, Type p_type, Rect p_bounds)
+      : index(p_index), type(p_type), bounds(p_bounds) {}
 };
 
 }  // namespace impeller

impeller/typographer/text_frame.cc

@@ -14,10 +14,10 @@ std::optional<Rect> TextFrame::GetBounds() const {
   std::optional<Rect> result;
 
   for (const auto& run : runs_) {
-    const auto glyph_bounds = run.GetFont().GetMetrics().GetBoundingBox();
     for (const auto& glyph_position : run.GetGlyphPositions()) {
-      Rect glyph_rect = Rect(glyph_position.position + glyph_bounds.origin,
-                             glyph_bounds.size);
+      Rect glyph_rect =
+          Rect(glyph_position.position + glyph_position.glyph.bounds.origin,
+               glyph_position.glyph.bounds.size);
       result = result.has_value() ? result->Union(glyph_rect) : glyph_rect;
     }
   }

impeller/typographer/typographer_unittests.cc

@@ -108,8 +108,6 @@ TEST_P(TypographerTest, GlyphAtlasWithOddUniqueGlyphSize) {
   ASSERT_NE(atlas, nullptr);
   ASSERT_NE(atlas->GetTexture(), nullptr);
 
-  // The 3 unique glyphs should not evenly fit into a square texture, so we
-  // should have a rectangular one.
   ASSERT_EQ(atlas->GetTexture()->GetSize().width,
             atlas->GetTexture()->GetSize().height);
 }
@@ -166,7 +164,7 @@ TEST_P(TypographerTest, GlyphAtlasWithLotsOfdUniqueGlyphSize) {
   ASSERT_NE(atlas, nullptr);
   ASSERT_NE(atlas->GetTexture(), nullptr);
 
-  ASSERT_EQ(atlas->GetTexture()->GetSize().width,
+  ASSERT_EQ(atlas->GetTexture()->GetSize().width * 2,
             atlas->GetTexture()->GetSize().height);
 }