Fixed translated text's subpixel alignment. (flutter#162824)

fixes flutter#162776 fixes flutter#149652 Previously we weren't calculating the subpixel offset correctly. We weren't using the transform of the object being drawn to get global coordinates, now we are. ## Video of flutter#149652 after PR https://github.com/user-attachments/assets/3e9063d5-f70c-46d0-a7a4-892819b247b8 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
navaronbracke · Feb 7, 2025 · 68e785e · 68e785e
1 parent 44203b6
commit 68e785e
Show file tree

Hide file tree

Showing 12 changed files with 188 additions and 23 deletions.
diff --git a/engine/src/flutter/impeller/display_list/dl_dispatcher.cc b/engine/src/flutter/impeller/display_list/dl_dispatcher.cc
@@ -1129,9 +1129,10 @@ void FirstPassDispatcher::drawTextFrame(
       (matrix_ * Matrix::MakeTranslation(Point(x, y))).GetMaxBasisLengthXY());
 
   renderer_.GetLazyGlyphAtlas()->AddTextFrame(
-      text_frame,                                       //
-      scale,                                            //
-      Point(x, y),                                      //
+      text_frame,   //
+      scale,        //
+      Point(x, y),  //
+      matrix_,
       (properties.stroke || text_frame->HasColor())     //
           ? std::optional<GlyphProperties>(properties)  //
           : std::nullopt                                //

diff --git a/engine/src/flutter/impeller/display_list/dl_golden_unittests.cc b/engine/src/flutter/impeller/display_list/dl_golden_unittests.cc
@@ -452,5 +452,158 @@ TEST_P(DlGoldenTest, MaintainsSpace) {
     last_space = space;
   }
 }
+
+namespace {
+struct LeftmostIntensity {
+  int32_t x;
+  int32_t value;
+};
+
+/// Returns the highest value in the green channel for leftmost column that
+/// isn't all black.
+LeftmostIntensity CalculateLeftmostIntensity(
+    const impeller::testing::Screenshot* img) {
+  LeftmostIntensity result = {.x = static_cast<int32_t>(img->GetWidth()),
+                              .value = 0};
+  const uint32_t* ptr = reinterpret_cast<const uint32_t*>(img->GetBytes());
+  for (size_t i = 0; i < img->GetHeight(); ++i) {
+    for (int32_t j = 0; j < static_cast<int32_t>(img->GetWidth()); ++j) {
+      if (((*ptr & 0x00ffffff) != 0)) {
+        if (j < result.x) {
+          result.x = j;
+          result.value = (*ptr & 0xff00) >> 8;
+        } else if (j == result.x) {
+          result.value =
+              std::max(static_cast<int32_t>(*ptr & 0xff), result.value);
+        }
+      }
+      ptr++;
+    }
+  }
+  return result;
+}
+}  // namespace
+
+// Checks that the left most edge of the glyph is fading out as we push
+// it to the right by fractional pixels.
+TEST_P(DlGoldenTest, Subpixel) {
+  SetWindowSize(impeller::ISize(1024, 200));
+  impeller::Scalar font_size = 200;
+  auto callback = [&](Scalar offset_x) -> sk_sp<DisplayList> {
+    DisplayListBuilder builder;
+    DlPaint paint;
+    paint.setColor(DlColor::ARGB(1, 0, 0, 0));
+    builder.DrawPaint(paint);
+    RenderTextInCanvasSkia(&builder, "ui", "Roboto-Regular.ttf",
+                           DlPoint::MakeXY(offset_x, 180),
+                           TextRenderOptions{
+                               .font_size = font_size,
+                               .is_subpixel = true,
+                           });
+    return builder.Build();
+  };
+
+  LeftmostIntensity intensity[5];
+  for (int i = 0; i <= 4; ++i) {
+    Scalar offset = 10 + (i / 4.0);
+    std::unique_ptr<impeller::testing::Screenshot> right =
+        MakeScreenshot(callback(offset));
+    if (!right) {
+      GTEST_SKIP() << "making screenshots not supported.";
+    }
+    intensity[i] = CalculateLeftmostIntensity(right.get());
+    ASSERT_NE(intensity[i].value, 0);
+  }
+  for (int i = 1; i < 5; ++i) {
+    EXPECT_TRUE(intensity[i].x - intensity[i - 1].x == 1 ||
+                intensity[i].value < intensity[i - 1].value)
+        << i;
+  }
+  EXPECT_EQ(intensity[4].x - intensity[0].x, 1);
+}
+
+// Checks that the left most edge of the glyph is fading out as we push
+// it to the right by fractional pixels.
+TEST_P(DlGoldenTest, SubpixelScaled) {
+  SetWindowSize(impeller::ISize(1024, 200));
+  impeller::Scalar font_size = 200;
+  Scalar scalar = 0.75;
+  auto callback = [&](Scalar offset_x) -> sk_sp<DisplayList> {
+    DisplayListBuilder builder;
+    builder.Scale(scalar, scalar);
+    DlPaint paint;
+    paint.setColor(DlColor::ARGB(1, 0, 0, 0));
+    builder.DrawPaint(paint);
+    RenderTextInCanvasSkia(&builder, "ui", "Roboto-Regular.ttf",
+                           DlPoint::MakeXY(offset_x, 180),
+                           TextRenderOptions{
+                               .font_size = font_size,
+                               .is_subpixel = true,
+                           });
+    return builder.Build();
+  };
+
+  LeftmostIntensity intensity[5];
+  Scalar offset_fraction = 0.25 / scalar;
+  for (int i = 0; i <= 4; ++i) {
+    Scalar offset = 10 + (offset_fraction * i);
+    std::unique_ptr<impeller::testing::Screenshot> right =
+        MakeScreenshot(callback(offset));
+    if (!right) {
+      GTEST_SKIP() << "making screenshots not supported.";
+    }
+    intensity[i] = CalculateLeftmostIntensity(right.get());
+    ASSERT_NE(intensity[i].value, 0);
+  }
+  for (int i = 1; i < 5; ++i) {
+    EXPECT_TRUE(intensity[i].x - intensity[i - 1].x == 1 ||
+                intensity[i].value < intensity[i - 1].value)
+        << i;
+  }
+  EXPECT_EQ(intensity[4].x - intensity[0].x, 1);
+}
+
+// Checks that the left most edge of the glyph is fading out as we push
+// it to the right by fractional pixels.
+TEST_P(DlGoldenTest, SubpixelScaledTranslated) {
+  SetWindowSize(impeller::ISize(1024, 200));
+  impeller::Scalar font_size = 200;
+  Scalar scalar = 0.75;
+  auto callback = [&](Scalar offset_x) -> sk_sp<DisplayList> {
+    DisplayListBuilder builder;
+    builder.Scale(scalar, scalar);
+    DlPaint paint;
+    paint.setColor(DlColor::ARGB(1, 0, 0, 0));
+    builder.DrawPaint(paint);
+    builder.Translate(offset_x, 180);
+    RenderTextInCanvasSkia(&builder, "ui", "Roboto-Regular.ttf",
+                           DlPoint::MakeXY(0, 0),
+                           TextRenderOptions{
+                               .font_size = font_size,
+                               .is_subpixel = true,
+                           });
+    return builder.Build();
+  };
+
+  LeftmostIntensity intensity[5];
+  Scalar offset_fraction = 0.25 / scalar;
+  for (int i = 0; i <= 4; ++i) {
+    Scalar offset = 10 + (offset_fraction * i);
+    std::unique_ptr<impeller::testing::Screenshot> right =
+        MakeScreenshot(callback(offset));
+    if (!right) {
+      GTEST_SKIP() << "making screenshots not supported.";
+    }
+    intensity[i] = CalculateLeftmostIntensity(right.get());
+    ASSERT_NE(intensity[i].value, 0);
+  }
+  for (int i = 1; i < 5; ++i) {
+    EXPECT_TRUE(intensity[i].x - intensity[i - 1].x == 1 ||
+                intensity[i].value < intensity[i - 1].value)
+        << i;
+  }
+  EXPECT_EQ(intensity[4].x - intensity[0].x, 1);
+}
+
 }  // namespace testing
 }  // namespace flutter
diff --git a/engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.cc b/engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.cc
@@ -22,6 +22,9 @@ bool RenderTextInCanvasSkia(DlCanvas* canvas,
   }
   sk_sp<SkFontMgr> font_mgr = txt::GetDefaultFontManager();
   SkFont sk_font(font_mgr->makeFromData(mapping), options.font_size);
+  if (options.is_subpixel) {
+    sk_font.setSubpixel(true);
+  }
   auto blob = SkTextBlob::MakeFromString(text.c_str(), sk_font);
   if (!blob) {
     return false;

diff --git a/engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.h b/engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.h
@@ -20,6 +20,7 @@ struct TextRenderOptions {
   SkScalar font_size = 50;
   DlColor color = DlColor::kYellow();
   std::shared_ptr<DlMaskFilter> mask_filter;
+  bool is_subpixel = false;
 };
 
 bool RenderTextInCanvasSkia(DlCanvas* canvas,

diff --git a/engine/src/flutter/impeller/entity/contents/text_contents.cc b/engine/src/flutter/impeller/entity/contents/text_contents.cc
@@ -149,7 +149,7 @@ void TextContents::ComputeVertexData(
           continue;
         }
         Point subpixel = TextFrame::ComputeSubpixelPosition(
-            glyph_position, font.GetAxisAlignment(), offset, rounded_scale);
+            glyph_position, font.GetAxisAlignment(), entity_transform);
 
         std::optional<FrameBounds> maybe_atlas_glyph_bounds =
             font_atlas->FindGlyphBounds(SubpixelGlyph{

diff --git a/engine/src/flutter/impeller/entity/contents/text_contents_unittests.cc b/engine/src/flutter/impeller/entity/contents/text_contents_unittests.cc
@@ -49,8 +49,8 @@ std::shared_ptr<GlyphAtlas> CreateGlyphAtlas(
     Scalar scale,
     const std::shared_ptr<GlyphAtlasContext>& atlas_context,
     const std::shared_ptr<TextFrame>& frame) {
-  frame->SetPerFrameData(scale, /*offset=*/{0, 0},
-                         /*properties=*/std::nullopt);
+  frame->SetPerFrameData(scale, /*offset=*/Point(0, 0),
+                         /*transform=*/Matrix(), /*properties=*/std::nullopt);
   return typographer_context->CreateGlyphAtlas(context, type, host_buffer,
                                                atlas_context, {frame});
 }

diff --git a/engine/src/flutter/impeller/typographer/backends/skia/typographer_context_skia.cc b/engine/src/flutter/impeller/typographer/backends/skia/typographer_context_skia.cc
@@ -452,7 +452,8 @@ TypographerContextSkia::CollectNewGlyphs(
       for (const auto& glyph_position : run.GetGlyphPositions()) {
         Point subpixel = TextFrame::ComputeSubpixelPosition(
             glyph_position, scaled_font.font.GetAxisAlignment(),
-            frame->GetOffset(), frame->GetScale());
+            frame->GetTransform() *
+                Matrix::MakeTranslation(frame->GetOffset()));
         SubpixelGlyph subpixel_glyph(glyph_position.glyph, subpixel,
                                      frame->GetProperties());
         const auto& font_glyph_bounds =

diff --git a/engine/src/flutter/impeller/typographer/lazy_glyph_atlas.cc b/engine/src/flutter/impeller/typographer/lazy_glyph_atlas.cc
@@ -34,8 +34,9 @@ LazyGlyphAtlas::~LazyGlyphAtlas() = default;
 void LazyGlyphAtlas::AddTextFrame(const std::shared_ptr<TextFrame>& frame,
                                   Scalar scale,
                                   Point offset,
+                                  const Matrix& transform,
                                   std::optional<GlyphProperties> properties) {
-  frame->SetPerFrameData(scale, offset, properties);
+  frame->SetPerFrameData(scale, offset, transform, properties);
   FML_DCHECK(alpha_atlas_ == nullptr && color_atlas_ == nullptr);
   if (frame->GetAtlasType() == GlyphAtlas::Type::kAlphaBitmap) {
     alpha_text_frames_.push_back(frame);

diff --git a/engine/src/flutter/impeller/typographer/lazy_glyph_atlas.h b/engine/src/flutter/impeller/typographer/lazy_glyph_atlas.h
@@ -22,6 +22,7 @@ class LazyGlyphAtlas {
   void AddTextFrame(const std::shared_ptr<TextFrame>& frame,
                     Scalar scale,
                     Point offset,
+                    const Matrix& transform,
                     std::optional<GlyphProperties> properties);
 
   void ResetTextFrames();

diff --git a/engine/src/flutter/impeller/typographer/text_frame.cc b/engine/src/flutter/impeller/typographer/text_frame.cc
@@ -82,26 +82,26 @@ static constexpr Scalar ComputeFractionalPosition(Scalar value) {
 Point TextFrame::ComputeSubpixelPosition(
     const TextRun::GlyphPosition& glyph_position,
     AxisAlignment alignment,
-    Point offset,
-    Scalar scale) {
-  Point pos = glyph_position.position + offset;
+    const Matrix& transform) {
+  Point pos = transform * glyph_position.position;
   switch (alignment) {
     case AxisAlignment::kNone:
       return Point(0, 0);
     case AxisAlignment::kX:
-      return Point(ComputeFractionalPosition(pos.x * scale), 0);
+      return Point(ComputeFractionalPosition(pos.x), 0);
     case AxisAlignment::kY:
-      return Point(0, ComputeFractionalPosition(pos.y * scale));
+      return Point(0, ComputeFractionalPosition(pos.y));
     case AxisAlignment::kAll:
-      return Point(ComputeFractionalPosition(pos.x * scale),
-                   ComputeFractionalPosition(pos.y * scale));
+      return Point(ComputeFractionalPosition(pos.x),
+                   ComputeFractionalPosition(pos.y));
   }
 }
 
 void TextFrame::SetPerFrameData(Scalar scale,
                                 Point offset,
+                                const Matrix& transform,
                                 std::optional<GlyphProperties> properties) {
-  if (!ScalarNearlyEqual(scale_, scale) ||
+  if (!transform_.Equals(transform) || !ScalarNearlyEqual(scale_, scale) ||
       !ScalarNearlyEqual(offset_.x, offset.x) ||
       !ScalarNearlyEqual(offset_.y, offset.y) ||
       !TextPropertiesEquals(properties_, properties)) {
@@ -110,6 +110,7 @@ void TextFrame::SetPerFrameData(Scalar scale,
   scale_ = scale;
   offset_ = offset;
   properties_ = properties;
+  transform_ = transform;
 }
 
 Scalar TextFrame::GetScale() const {

diff --git a/engine/src/flutter/impeller/typographer/text_frame.h b/engine/src/flutter/impeller/typographer/text_frame.h
@@ -30,8 +30,7 @@ class TextFrame {
   static Point ComputeSubpixelPosition(
       const TextRun::GlyphPosition& glyph_position,
       AxisAlignment alignment,
-      Point offset,
-      Scalar scale);
+      const Matrix& transform);
 
   static Scalar RoundScaledFontSize(Scalar scale);
 
@@ -83,6 +82,7 @@ class TextFrame {
   /// glyph atlas.
   void SetPerFrameData(Scalar scale,
                        Point offset,
+                       const Matrix& transform,
                        std::optional<GlyphProperties> properties);
 
   // A generation id for the glyph atlas this text run was associated
@@ -95,6 +95,8 @@ class TextFrame {
 
   TextFrame(const TextFrame& other) = default;
 
+  const Matrix& GetTransform() const { return transform_; }
+
  private:
   friend class TypographerContextSkia;
   friend class LazyGlyphAtlas;
@@ -123,6 +125,7 @@ class TextFrame {
   intptr_t atlas_id_ = 0;
   Point offset_;
   std::optional<GlyphProperties> properties_;
+  Matrix transform_;
 };
 
 }  // namespace impeller

diff --git a/engine/src/flutter/impeller/typographer/typographer_unittests.cc b/engine/src/flutter/impeller/typographer/typographer_unittests.cc
@@ -37,7 +37,7 @@ static std::shared_ptr<GlyphAtlas> CreateGlyphAtlas(
     Scalar scale,
     const std::shared_ptr<GlyphAtlasContext>& atlas_context,
     const std::shared_ptr<TextFrame>& frame) {
-  frame->SetPerFrameData(scale, {0, 0}, std::nullopt);
+  frame->SetPerFrameData(scale, {0, 0}, Matrix(), std::nullopt);
   return typographer_context->CreateGlyphAtlas(context, type, host_buffer,
                                                atlas_context, {frame});
 }
@@ -53,7 +53,7 @@ static std::shared_ptr<GlyphAtlas> CreateGlyphAtlas(
     const std::vector<std::optional<GlyphProperties>>& properties) {
   size_t offset = 0;
   for (auto& frame : frames) {
-    frame->SetPerFrameData(scale, {0, 0}, properties[offset++]);
+    frame->SetPerFrameData(scale, {0, 0}, Matrix(), properties[offset++]);
   }
   return typographer_context->CreateGlyphAtlas(context, type, host_buffer,
                                                atlas_context, frames);
@@ -137,14 +137,14 @@ TEST_P(TypographerTest, LazyAtlasTracksColor) {
 
   LazyGlyphAtlas lazy_atlas(TypographerContextSkia::Make());
 
-  lazy_atlas.AddTextFrame(frame, 1.0f, {0, 0}, {});
+  lazy_atlas.AddTextFrame(frame, 1.0f, {0, 0}, Matrix(), {});
 
   frame = MakeTextFrameFromTextBlobSkia(
       SkTextBlob::MakeFromString("😀 ", emoji_font));
 
   ASSERT_TRUE(frame->GetAtlasType() == GlyphAtlas::Type::kColorBitmap);
 
-  lazy_atlas.AddTextFrame(frame, 1.0f, {0, 0}, {});
+  lazy_atlas.AddTextFrame(frame, 1.0f, {0, 0}, Matrix(), {});
 
   // Creates different atlases for color and red bitmap.
   auto color_atlas = lazy_atlas.CreateOrGetGlyphAtlas(
@@ -227,7 +227,7 @@ TEST_P(TypographerTest, GlyphAtlasWithLotsOfdUniqueGlyphSize) {
   std::vector<std::shared_ptr<TextFrame>> frames;
   for (size_t index = 0; index < size_count; index += 1) {
     frames.push_back(MakeTextFrameFromTextBlobSkia(blob));
-    frames.back()->SetPerFrameData(0.6 * index, {0, 0}, {});
+    frames.back()->SetPerFrameData(0.6 * index, {0, 0}, Matrix(), {});
   };
   auto atlas =
       context->CreateGlyphAtlas(*GetContext(), GlyphAtlas::Type::kAlphaBitmap,