[Impeller] Fix text glyph bounds on Android. (flutter#43919)

Resolves flutter/flutter#128624. It turns out that `SkFont::getBounds()` snaps results to integers on Android, but not iOS. By scaling the font up and scaling the resulting per-glyph bounds back down, we can ensure that the results are always precise enough. I also found errors with our usage of the computed bounds, but those were comparatively minor fixes.
izouxv · Jul 22, 2023 · 4ad970f · 4ad970f
1 parent 30f0f6b
commit 4ad970f
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 8 deletions.
diff --git a/impeller/typographer/backends/skia/text_frame_skia.cc b/impeller/typographer/backends/skia/text_frame_skia.cc
@@ -37,6 +37,8 @@ static Rect ToRect(const SkRect& rect) {
   return Rect::MakeLTRB(rect.fLeft, rect.fTop, rect.fRight, rect.fBottom);
 }
 
+static constexpr Scalar kScaleSize = 100000.0f;
+
 TextFrame TextFrameFromTextBlob(const sk_sp<SkTextBlob>& blob) {
   if (!blob) {
     return {};
@@ -64,7 +66,14 @@ TextFrame TextFrameFromTextBlob(const sk_sp<SkTextBlob>& blob) {
       case SkTextBlobRunIterator::kFull_Positioning: {
         std::vector<SkRect> glyph_bounds;
         glyph_bounds.resize(glyph_count);
-        run.font().getBounds(glyphs, glyph_count, glyph_bounds.data(), nullptr);
+        SkFont font = run.font();
+        auto font_size = font.getSize();
+        // For some platforms (including Android), `SkFont::getBounds()` snaps
+        // the computed bounds to integers. And so we scale up the font size
+        // prior to fetching the bounds to ensure that the returned bounds are
+        // always precise enough.
+        font.setSize(kScaleSize);
+        font.getBounds(glyphs, glyph_count, glyph_bounds.data(), nullptr);
 
         for (auto i = 0u; i < glyph_count; i++) {
           // kFull_Positioning has two scalars per glyph.
@@ -74,8 +83,10 @@ TextFrame TextFrameFromTextBlob(const sk_sp<SkTextBlob>& blob) {
                                  ? Glyph::Type::kBitmap
                                  : Glyph::Type::kPath;
 
-          text_run.AddGlyph(Glyph{glyphs[i], type, ToRect(glyph_bounds[i])},
-                            Point{point->x(), point->y()});
+          text_run.AddGlyph(
+              Glyph{glyphs[i], type,
+                    ToRect(glyph_bounds[i]).Scale(font_size / kScaleSize)},
+              Point{point->x(), point->y()});
         }
         break;
       }

diff --git a/impeller/typographer/backends/skia/text_render_context_skia.cc b/impeller/typographer/backends/skia/text_render_context_skia.cc
@@ -56,7 +56,7 @@ static size_t PairsFitInAtlasOfSize(
   for (auto it = pairs.begin(); it != pairs.end(); ++i, ++it) {
     const auto& pair = *it;
 
-    const auto glyph_size = ISize::Ceil((pair.glyph.bounds * pair.scale).size);
+    const auto glyph_size = ISize::Ceil(pair.glyph.bounds.size * pair.scale);
     IPoint16 location_in_atlas;
     if (!rect_packer->addRect(glyph_size.width + kPadding,   //
                               glyph_size.height + kPadding,  //
@@ -93,7 +93,7 @@ static bool CanAppendToExistingAtlas(
   for (size_t i = 0; i < extra_pairs.size(); i++) {
     const FontGlyphPair& pair = extra_pairs[i];
 
-    const auto glyph_size = ISize::Ceil((pair.glyph.bounds * pair.scale).size);
+    const auto glyph_size = ISize::Ceil(pair.glyph.bounds.size * pair.scale);
     IPoint16 location_in_atlas;
     if (!rect_packer->addRect(glyph_size.width + kPadding,   //
                               glyph_size.height + kPadding,  //

diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc
@@ -311,9 +311,7 @@ TEST_P(TypographerTest, MaybeHasOverlapping) {
 
   auto frame_2 =
       TextFrameFromTextBlob(SkTextBlob::MakeFromString("123456789", sk_font));
-  // Characters probably have overlap due to low fidelity text metrics, but this
-  // could be fixed.
-  ASSERT_TRUE(frame_2.MaybeHasOverlapping());
+  ASSERT_FALSE(frame_2.MaybeHasOverlapping());
 }
 
 TEST_P(TypographerTest, RectanglePackerAddsNonoverlapingRectangles) {