[Impeller] Use glyph bounds to compute the max glyph size instead of …

…font metrics (#37998)
flutter · Dec 1, 2022 · 303e26e · 303e26e
1 parent d75b7ce
commit 303e26e
Show file tree

Hide file tree

Showing 7 changed files with 66 additions and 11 deletions.
diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc
@@ -1118,6 +1118,43 @@ TEST_P(AiksTest, CanRenderTextInSaveLayer) {
   ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
 }
 
+TEST_P(AiksTest, CanRenderTextOutsideBoundaries) {
+  Canvas canvas;
+  canvas.Translate({200, 150});
+
+  // Construct the text blob.
+  auto mapping = OpenFixtureAsSkData("wtf.otf");
+  ASSERT_NE(mapping, nullptr);
+
+  Scalar font_size = 80;
+  SkFont sk_font(SkTypeface::MakeFromData(mapping), font_size);
+
+  Paint text_paint;
+  text_paint.color = Color::White().WithAlpha(0.8);
+
+  struct {
+    Point position;
+    const char* text;
+  } text[] = {{Point(0, 0), "0F0F0F0"},
+              {Point(1, 2), "789"},
+              {Point(1, 3), "456"},
+              {Point(1, 4), "123"},
+              {Point(0, 6), "0F0F0F0"}};
+  for (auto& t : text) {
+    canvas.Save();
+    canvas.Translate(t.position * Point(font_size * 2, font_size * 1.1));
+    {
+      auto blob = SkTextBlob::MakeFromString(t.text, sk_font);
+      ASSERT_NE(blob, nullptr);
+      auto frame = TextFrameFromTextBlob(blob);
+      canvas.DrawTextFrame(frame, Point(), text_paint);
+    }
+    canvas.Restore();
+  }
+
+  ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
+}
+
 TEST_P(AiksTest, CanDrawPaint) {
   Paint paint;
   paint.color = Color::MediumTurquoise();

diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc
@@ -131,8 +131,7 @@ static bool CommonRender(
     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 =
-        Point{font.GetMetrics().min_extent.x, font.GetMetrics().ascent};
+    auto metrics_offset = font.GetMetrics().min_extent;
 
     for (const auto& glyph_position : run.GetGlyphPositions()) {
       FontGlyphPair font_glyph_pair{font, glyph_position.glyph};

diff --git a/impeller/fixtures/BUILD.gn b/impeller/fixtures/BUILD.gn
@@ -73,6 +73,7 @@ test_fixtures("file_fixtures") {
     "table_mountain_pz.png",
     "test_texture.frag",
     "types.h",
+    "wtf.otf",
   ]
   if (host_os == "mac") {
     fixtures += [ "/System/Library/Fonts/Apple Color Emoji.ttc" ]

diff --git a/impeller/fixtures/wtf.otf b/impeller/fixtures/wtf.otf
diff --git a/impeller/typographer/backends/skia/text_frame_skia.cc b/impeller/typographer/backends/skia/text_frame_skia.cc
@@ -4,16 +4,21 @@
 
 #include "impeller/typographer/backends/skia/text_frame_skia.h"
 
+#include <vector>
+
 #include "flutter/fml/logging.h"
 #include "impeller/typographer/backends/skia/typeface_skia.h"
+#include "include/core/SkFontTypes.h"
+#include "include/core/SkRect.h"
 #include "third_party/skia/include/core/SkFont.h"
 #include "third_party/skia/include/core/SkFontMetrics.h"
 #include "third_party/skia/src/core/SkStrikeSpec.h"    // nogncheck
 #include "third_party/skia/src/core/SkTextBlobPriv.h"  // nogncheck
 
 namespace impeller {
 
-static Font ToFont(const SkFont& font, Scalar scale) {
+static Font ToFont(const SkTextBlobRunIterator& run, Scalar scale) {
+  auto& font = run.font();
   auto typeface = std::make_shared<TypefaceSkia>(font.refTypefaceOrDefault());
 
   SkFontMetrics sk_metrics;
@@ -24,8 +29,20 @@ static Font ToFont(const SkFont& font, Scalar scale) {
   metrics.point_size = font.getSize();
   metrics.ascent = sk_metrics.fAscent;
   metrics.descent = sk_metrics.fDescent;
-  metrics.min_extent = {sk_metrics.fXMin, sk_metrics.fAscent};
-  metrics.max_extent = {sk_metrics.fXMax, 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};
 }
@@ -38,7 +55,7 @@ TextFrame TextFrameFromTextBlob(const sk_sp<SkTextBlob>& blob, Scalar scale) {
   TextFrame frame;
 
   for (SkTextBlobRunIterator run(blob.get()); !run.done(); run.next()) {
-    TextRun text_run(ToFont(run.font(), scale));
+    TextRun text_run(ToFont(run, scale));
 
     // TODO(jonahwilliams): ask Skia for a public API to look this up.
     // https://github.com/flutter/flutter/issues/112005

diff --git a/impeller/typographer/backends/skia/text_render_context_skia.cc b/impeller/typographer/backends/skia/text_render_context_skia.cc
@@ -77,6 +77,7 @@ static size_t PairsFitInAtlasOfSize(const FontGlyphPair::Vector& pairs,
 
   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);
@@ -291,9 +292,9 @@ static std::shared_ptr<SkBitmap> CreateAtlasBitmap(const GlyphAtlas& atlas,
                        &glyph_id,  // glyphs
                        &position,  // positions
                        SkPoint::Make(-metrics.min_extent.x,
-                                     -metrics.ascent),  // origin
-                       sk_font,                         // font
-                       glyph_paint                      // paint
+                                     -metrics.min_extent.y),  // origin
+                       sk_font,                               // font
+                       glyph_paint                            // paint
     );
     return true;
   });

diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc
@@ -110,7 +110,7 @@ TEST_P(TypographerTest, GlyphAtlasWithOddUniqueGlyphSize) {
 
   // 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 * 2,
+  ASSERT_EQ(atlas->GetTexture()->GetSize().width,
             atlas->GetTexture()->GetSize().height);
 }
 
@@ -166,7 +166,7 @@ TEST_P(TypographerTest, GlyphAtlasWithLotsOfdUniqueGlyphSize) {
   ASSERT_NE(atlas, nullptr);
   ASSERT_NE(atlas->GetTexture(), nullptr);
 
-  ASSERT_EQ(atlas->GetTexture()->GetSize().width * 2,
+  ASSERT_EQ(atlas->GetTexture()->GetSize().width,
             atlas->GetTexture()->GetSize().height);
 }