From bbdb5d6a3e95c685f2df78a1d2272d7a64287947 Mon Sep 17 00:00:00 2001
From: Brandon DeRosier <bdero@google.com>
Date: Thu, 1 Dec 2022 10:16:24 -0800
Subject: [PATCH] [Impeller] Use glyph bounds to compute the max glyph size
 instead of font metrics (flutter/engine#37998)

---
 .../flutter/impeller/aiks/aiks_unittests.cc   |  37 ++++++++++++++++++
 .../impeller/entity/contents/text_contents.cc |   3 +-
 engine/src/flutter/impeller/fixtures/BUILD.gn |   1 +
 engine/src/flutter/impeller/fixtures/wtf.otf  | Bin 0 -> 2552 bytes
 .../backends/skia/text_frame_skia.cc          |  25 ++++++++++--
 .../backends/skia/text_render_context_skia.cc |   7 ++--
 .../typographer/typographer_unittests.cc      |   4 +-
 7 files changed, 66 insertions(+), 11 deletions(-)
 create mode 100644 engine/src/flutter/impeller/fixtures/wtf.otf

diff --git a/engine/src/flutter/impeller/aiks/aiks_unittests.cc b/engine/src/flutter/impeller/aiks/aiks_unittests.cc
index 9b06d08802..14bdcacaba 100644
--- a/engine/src/flutter/impeller/aiks/aiks_unittests.cc
+++ b/engine/src/flutter/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/engine/src/flutter/impeller/entity/contents/text_contents.cc b/engine/src/flutter/impeller/entity/contents/text_contents.cc
index 1d6727d305..1f30c98155 100644
--- a/engine/src/flutter/impeller/entity/contents/text_contents.cc
+++ b/engine/src/flutter/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/engine/src/flutter/impeller/fixtures/BUILD.gn b/engine/src/flutter/impeller/fixtures/BUILD.gn
index 30d609cd4f..eb81d57237 100644
--- a/engine/src/flutter/impeller/fixtures/BUILD.gn
+++ b/engine/src/flutter/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/engine/src/flutter/impeller/fixtures/wtf.otf b/engine/src/flutter/impeller/fixtures/wtf.otf
new file mode 100644
index 0000000000000000000000000000000000000000..ae0c62c61a728bbf4ae8952dc8134a727dc634ac
GIT binary patch
literal 2552
zcmeYd3Grv(VrXDsW>9c;b5kg{E}Y1~z`BBgfrY`%EyQ>3iqb_444iiu7#L*ygY}If
zX1dfcFfa-*Ffb$}=Oz}27JN};U|=y}U|<Z&NKH&pN;>j}fq`KS0|SFuMn-BPdn!W%
z0|Ubo1_lO|jNFn66V~5i3=Etr7#J8#auX{G7<dC17#J8i7#JAX@)C1XH|_qroPmL{
zfPsPONI`ya$^V@Uh71g>9SjT%3Ji=Or3?&J61;Qb`E9;3a5KMPU|?Xlzjmesgnlvo
zkHl{;_6zKM3=9k$3``(Vh#qEz{}v2UtV(|*{z$N20GkAr1XH{W3;_%bybKHsEG$fn
z6Bt+-7?`@4_AoFo^gw9F2MmUcObiT646K}B9Uuba9uN)20cox&3=Iqhj~F0!LEHwj
ziRmqa0@Me54C`Q`47Ln4P&Ok2AHxkOn~8y);X9Pg%%H;<31zb|2r#aIvRN7U8E-<_
zYz%x%><r8dj4U9xRWUF?*^CS_47;FgCI&u+=TJ5?Lj=QrD4T^rfw2wBW@V6L+zDl~
zF~~5!WN>E4XDDE(WGG_DWJqVoU?^cwV9;PlX3%6%U@&4ZU@&4ZV$fkwU<hHzU`S<9
zU~pr|VJKxNVJKlpWhi1$U~mNUG8pn1iWrI+^uQuH3^@!6NG2A8d8rJ=U_E6FsSGI$
zdJGKC`303lnduoN3L43p3PuJ-Mmh>18L0|xIi)2fsYMEor6n2pMa6mwjyX9BV1>mB
zMXAN9MP;cedJGKV3>gfG3?)#Pp}LD9JR`9rBqJ4S6azyLLn=c$Ln%WJLn1>F14B@1
zdTCB#5d(u8Lq0<uLkXBJVn_#j(TG8h!GJ-5!HPivZVB!H0I4!$&}A@WFk;Y!2N{D~
zeqM=Neo=a=f{~tqf|UZuK`6enQZUptG|~lm8{rpR)`0zktet@&3>=un44GgbC@>f>
z7%&)uT@GR~gryc0XXfWA7#JAp85kHKtbjx$f#3qAF9?=_ra&K1ssWV|3@i)=;JgRP
zeU=Pvj~M>{XJBA3fUy2w{C`xA2dWF4Zy4Dakhx$nLIfidGYcylI|nBhvPy1b&QN1B
zLmd2n8>qa4u#rhd1{M|uMn*<{P)b2cx{OQ=j0~nf_!)l4F#eEb`XR^sLxM$vhj|YV
z%fA|#-(E887i8G^SY;Rv^UE+W2*@zp5tLz2lVRQ@#J~d5%E92u5Xz8TnwRNnWMDv1
zo&&iIIT3>RNG@eyU=U;wVi0B!VGv~yV-RPMV31^xVo+ynXW#|7M2W$VA&g-L!$O8f
zjG~OujG2t<8TT=MV%pKY_lN6V#=U!gZrRJcSL~O>kCp!<*!KP@7yaS-bITuBcD~=<
zWsE<Xer^$Uu>WKJ$Ev$~U*wPAKQZ0i-x)vtko}?3-EH#lkNFRi?rsjgKNEh(h$?XL
z{VrV3_;b~757FfBESuT?ZcP&XF1-G?FxwB=U%7B|HYon^llirzyL<b0jvs|GtbWKQ
z$*^958s%NaShk6|OigstcZn3XpN1^IS+;y<VgHf$Ueq>M=2yj^jPCB^6*8>z{#0Xi
zlMJg!IpdEW8CDU|-&((Q{=WQsue<xB`Y+yp(%s$P^S&qiT>W!(clU3n?+$-Y|GWh9
z2FT05(zY@Gny4ZAoiE`xAN!V{n?+N8b8TY#v+#Q!+z0TGU|21~TJ$5UyZg7$c9~yu
ze#C)Hkzwa6XZ$lkhMf-_B;IAevbHe(?EZONH0it27WTheQ$)YBtpClz_CxMhmT0E-
zp9Gm-9aw@l_Pf%b1aR<h@PW;gVZ8!1^ykc<jiP_<{ki{J?z`NNkRPF}|HN3V?^xZo
zzH{gH?c2BSaC~3U{k!dV>-V<bZLGiBzPE|;eVg^-d;N<a>fhNIe<*+F`kwclllj-v
zd!oOYzF%c+nO8NVyqn{<Ia^g$V@^vR$9IeF+rR7o)^^|dUjN;;o3(LS-KvUp9KTK2
z%DXG;S}Qnyv;4lz+E&|JJ*9qf{mkZB-5fvvclUI&E?KZ(seJdO&dD7UIsO#?{3ZHZ
zP*wG}pnP}q?6{?AJ)J!reeK`9e-}so*4W#9rrYPc(|6PFUOheCJv}|^w=CS>eU#(7
zpxVdZf^xqfu=q#qIeuc#?&HVzMEm(iMfuC~{i+d>Vc9NPKL2N9_r70EY~4JZ-8@`d
zxYkeL;n~E!sh@{ehCxjjl&4i3?EjelvB46sCJ+Bu{xIwA<^Z*Hj6f|4<`;h?AT;v}
g2>Tx|gpVxFV92V(&<APxFfiQn`TPk~mLRv20EVrv+yDRo

literal 0
HcmV?d00001

diff --git a/engine/src/flutter/impeller/typographer/backends/skia/text_frame_skia.cc b/engine/src/flutter/impeller/typographer/backends/skia/text_frame_skia.cc
index 8b649c6c5e..7fc4f5fd97 100644
--- a/engine/src/flutter/impeller/typographer/backends/skia/text_frame_skia.cc
+++ b/engine/src/flutter/impeller/typographer/backends/skia/text_frame_skia.cc
@@ -4,8 +4,12 @@
 
 #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
@@ -13,7 +17,8 @@
 
 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/engine/src/flutter/impeller/typographer/backends/skia/text_render_context_skia.cc b/engine/src/flutter/impeller/typographer/backends/skia/text_render_context_skia.cc
index 1641171f56..58754115fa 100644
--- a/engine/src/flutter/impeller/typographer/backends/skia/text_render_context_skia.cc
+++ b/engine/src/flutter/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/engine/src/flutter/impeller/typographer/typographer_unittests.cc b/engine/src/flutter/impeller/typographer/typographer_unittests.cc
index 7bd81264c4..51d5dfe9b1 100644
--- a/engine/src/flutter/impeller/typographer/typographer_unittests.cc
+++ b/engine/src/flutter/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);
 }