Skip to content

Commit 68e785e

Browse files
authored
Fixed translated text's subpixel alignment. (#162824)
fixes flutter/flutter#162776 fixes flutter/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/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]. <!-- Links --> [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
1 parent 44203b6 commit 68e785e

File tree

12 files changed

+188
-23
lines changed

12 files changed

+188
-23
lines changed

engine/src/flutter/impeller/display_list/dl_dispatcher.cc

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1129,9 +1129,10 @@ void FirstPassDispatcher::drawTextFrame(
11291129
(matrix_ * Matrix::MakeTranslation(Point(x, y))).GetMaxBasisLengthXY());
11301130

11311131
renderer_.GetLazyGlyphAtlas()->AddTextFrame(
1132-
text_frame, //
1133-
scale, //
1134-
Point(x, y), //
1132+
text_frame, //
1133+
scale, //
1134+
Point(x, y), //
1135+
matrix_,
11351136
(properties.stroke || text_frame->HasColor()) //
11361137
? std::optional<GlyphProperties>(properties) //
11371138
: std::nullopt //

engine/src/flutter/impeller/display_list/dl_golden_unittests.cc

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -452,5 +452,158 @@ TEST_P(DlGoldenTest, MaintainsSpace) {
452452
last_space = space;
453453
}
454454
}
455+
456+
namespace {
457+
struct LeftmostIntensity {
458+
int32_t x;
459+
int32_t value;
460+
};
461+
462+
/// Returns the highest value in the green channel for leftmost column that
463+
/// isn't all black.
464+
LeftmostIntensity CalculateLeftmostIntensity(
465+
const impeller::testing::Screenshot* img) {
466+
LeftmostIntensity result = {.x = static_cast<int32_t>(img->GetWidth()),
467+
.value = 0};
468+
const uint32_t* ptr = reinterpret_cast<const uint32_t*>(img->GetBytes());
469+
for (size_t i = 0; i < img->GetHeight(); ++i) {
470+
for (int32_t j = 0; j < static_cast<int32_t>(img->GetWidth()); ++j) {
471+
if (((*ptr & 0x00ffffff) != 0)) {
472+
if (j < result.x) {
473+
result.x = j;
474+
result.value = (*ptr & 0xff00) >> 8;
475+
} else if (j == result.x) {
476+
result.value =
477+
std::max(static_cast<int32_t>(*ptr & 0xff), result.value);
478+
}
479+
}
480+
ptr++;
481+
}
482+
}
483+
return result;
484+
}
485+
} // namespace
486+
487+
// Checks that the left most edge of the glyph is fading out as we push
488+
// it to the right by fractional pixels.
489+
TEST_P(DlGoldenTest, Subpixel) {
490+
SetWindowSize(impeller::ISize(1024, 200));
491+
impeller::Scalar font_size = 200;
492+
auto callback = [&](Scalar offset_x) -> sk_sp<DisplayList> {
493+
DisplayListBuilder builder;
494+
DlPaint paint;
495+
paint.setColor(DlColor::ARGB(1, 0, 0, 0));
496+
builder.DrawPaint(paint);
497+
RenderTextInCanvasSkia(&builder, "ui", "Roboto-Regular.ttf",
498+
DlPoint::MakeXY(offset_x, 180),
499+
TextRenderOptions{
500+
.font_size = font_size,
501+
.is_subpixel = true,
502+
});
503+
return builder.Build();
504+
};
505+
506+
LeftmostIntensity intensity[5];
507+
for (int i = 0; i <= 4; ++i) {
508+
Scalar offset = 10 + (i / 4.0);
509+
std::unique_ptr<impeller::testing::Screenshot> right =
510+
MakeScreenshot(callback(offset));
511+
if (!right) {
512+
GTEST_SKIP() << "making screenshots not supported.";
513+
}
514+
intensity[i] = CalculateLeftmostIntensity(right.get());
515+
ASSERT_NE(intensity[i].value, 0);
516+
}
517+
for (int i = 1; i < 5; ++i) {
518+
EXPECT_TRUE(intensity[i].x - intensity[i - 1].x == 1 ||
519+
intensity[i].value < intensity[i - 1].value)
520+
<< i;
521+
}
522+
EXPECT_EQ(intensity[4].x - intensity[0].x, 1);
523+
}
524+
525+
// Checks that the left most edge of the glyph is fading out as we push
526+
// it to the right by fractional pixels.
527+
TEST_P(DlGoldenTest, SubpixelScaled) {
528+
SetWindowSize(impeller::ISize(1024, 200));
529+
impeller::Scalar font_size = 200;
530+
Scalar scalar = 0.75;
531+
auto callback = [&](Scalar offset_x) -> sk_sp<DisplayList> {
532+
DisplayListBuilder builder;
533+
builder.Scale(scalar, scalar);
534+
DlPaint paint;
535+
paint.setColor(DlColor::ARGB(1, 0, 0, 0));
536+
builder.DrawPaint(paint);
537+
RenderTextInCanvasSkia(&builder, "ui", "Roboto-Regular.ttf",
538+
DlPoint::MakeXY(offset_x, 180),
539+
TextRenderOptions{
540+
.font_size = font_size,
541+
.is_subpixel = true,
542+
});
543+
return builder.Build();
544+
};
545+
546+
LeftmostIntensity intensity[5];
547+
Scalar offset_fraction = 0.25 / scalar;
548+
for (int i = 0; i <= 4; ++i) {
549+
Scalar offset = 10 + (offset_fraction * i);
550+
std::unique_ptr<impeller::testing::Screenshot> right =
551+
MakeScreenshot(callback(offset));
552+
if (!right) {
553+
GTEST_SKIP() << "making screenshots not supported.";
554+
}
555+
intensity[i] = CalculateLeftmostIntensity(right.get());
556+
ASSERT_NE(intensity[i].value, 0);
557+
}
558+
for (int i = 1; i < 5; ++i) {
559+
EXPECT_TRUE(intensity[i].x - intensity[i - 1].x == 1 ||
560+
intensity[i].value < intensity[i - 1].value)
561+
<< i;
562+
}
563+
EXPECT_EQ(intensity[4].x - intensity[0].x, 1);
564+
}
565+
566+
// Checks that the left most edge of the glyph is fading out as we push
567+
// it to the right by fractional pixels.
568+
TEST_P(DlGoldenTest, SubpixelScaledTranslated) {
569+
SetWindowSize(impeller::ISize(1024, 200));
570+
impeller::Scalar font_size = 200;
571+
Scalar scalar = 0.75;
572+
auto callback = [&](Scalar offset_x) -> sk_sp<DisplayList> {
573+
DisplayListBuilder builder;
574+
builder.Scale(scalar, scalar);
575+
DlPaint paint;
576+
paint.setColor(DlColor::ARGB(1, 0, 0, 0));
577+
builder.DrawPaint(paint);
578+
builder.Translate(offset_x, 180);
579+
RenderTextInCanvasSkia(&builder, "ui", "Roboto-Regular.ttf",
580+
DlPoint::MakeXY(0, 0),
581+
TextRenderOptions{
582+
.font_size = font_size,
583+
.is_subpixel = true,
584+
});
585+
return builder.Build();
586+
};
587+
588+
LeftmostIntensity intensity[5];
589+
Scalar offset_fraction = 0.25 / scalar;
590+
for (int i = 0; i <= 4; ++i) {
591+
Scalar offset = 10 + (offset_fraction * i);
592+
std::unique_ptr<impeller::testing::Screenshot> right =
593+
MakeScreenshot(callback(offset));
594+
if (!right) {
595+
GTEST_SKIP() << "making screenshots not supported.";
596+
}
597+
intensity[i] = CalculateLeftmostIntensity(right.get());
598+
ASSERT_NE(intensity[i].value, 0);
599+
}
600+
for (int i = 1; i < 5; ++i) {
601+
EXPECT_TRUE(intensity[i].x - intensity[i - 1].x == 1 ||
602+
intensity[i].value < intensity[i - 1].value)
603+
<< i;
604+
}
605+
EXPECT_EQ(intensity[4].x - intensity[0].x, 1);
606+
}
607+
455608
} // namespace testing
456609
} // namespace flutter

engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.cc

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ bool RenderTextInCanvasSkia(DlCanvas* canvas,
2222
}
2323
sk_sp<SkFontMgr> font_mgr = txt::GetDefaultFontManager();
2424
SkFont sk_font(font_mgr->makeFromData(mapping), options.font_size);
25+
if (options.is_subpixel) {
26+
sk_font.setSubpixel(true);
27+
}
2528
auto blob = SkTextBlob::MakeFromString(text.c_str(), sk_font);
2629
if (!blob) {
2730
return false;

engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ struct TextRenderOptions {
2020
SkScalar font_size = 50;
2121
DlColor color = DlColor::kYellow();
2222
std::shared_ptr<DlMaskFilter> mask_filter;
23+
bool is_subpixel = false;
2324
};
2425

2526
bool RenderTextInCanvasSkia(DlCanvas* canvas,

engine/src/flutter/impeller/entity/contents/text_contents.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ void TextContents::ComputeVertexData(
149149
continue;
150150
}
151151
Point subpixel = TextFrame::ComputeSubpixelPosition(
152-
glyph_position, font.GetAxisAlignment(), offset, rounded_scale);
152+
glyph_position, font.GetAxisAlignment(), entity_transform);
153153

154154
std::optional<FrameBounds> maybe_atlas_glyph_bounds =
155155
font_atlas->FindGlyphBounds(SubpixelGlyph{

engine/src/flutter/impeller/entity/contents/text_contents_unittests.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,8 @@ std::shared_ptr<GlyphAtlas> CreateGlyphAtlas(
4949
Scalar scale,
5050
const std::shared_ptr<GlyphAtlasContext>& atlas_context,
5151
const std::shared_ptr<TextFrame>& frame) {
52-
frame->SetPerFrameData(scale, /*offset=*/{0, 0},
53-
/*properties=*/std::nullopt);
52+
frame->SetPerFrameData(scale, /*offset=*/Point(0, 0),
53+
/*transform=*/Matrix(), /*properties=*/std::nullopt);
5454
return typographer_context->CreateGlyphAtlas(context, type, host_buffer,
5555
atlas_context, {frame});
5656
}

engine/src/flutter/impeller/typographer/backends/skia/typographer_context_skia.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -452,7 +452,8 @@ TypographerContextSkia::CollectNewGlyphs(
452452
for (const auto& glyph_position : run.GetGlyphPositions()) {
453453
Point subpixel = TextFrame::ComputeSubpixelPosition(
454454
glyph_position, scaled_font.font.GetAxisAlignment(),
455-
frame->GetOffset(), frame->GetScale());
455+
frame->GetTransform() *
456+
Matrix::MakeTranslation(frame->GetOffset()));
456457
SubpixelGlyph subpixel_glyph(glyph_position.glyph, subpixel,
457458
frame->GetProperties());
458459
const auto& font_glyph_bounds =

engine/src/flutter/impeller/typographer/lazy_glyph_atlas.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,9 @@ LazyGlyphAtlas::~LazyGlyphAtlas() = default;
3434
void LazyGlyphAtlas::AddTextFrame(const std::shared_ptr<TextFrame>& frame,
3535
Scalar scale,
3636
Point offset,
37+
const Matrix& transform,
3738
std::optional<GlyphProperties> properties) {
38-
frame->SetPerFrameData(scale, offset, properties);
39+
frame->SetPerFrameData(scale, offset, transform, properties);
3940
FML_DCHECK(alpha_atlas_ == nullptr && color_atlas_ == nullptr);
4041
if (frame->GetAtlasType() == GlyphAtlas::Type::kAlphaBitmap) {
4142
alpha_text_frames_.push_back(frame);

engine/src/flutter/impeller/typographer/lazy_glyph_atlas.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ class LazyGlyphAtlas {
2222
void AddTextFrame(const std::shared_ptr<TextFrame>& frame,
2323
Scalar scale,
2424
Point offset,
25+
const Matrix& transform,
2526
std::optional<GlyphProperties> properties);
2627

2728
void ResetTextFrames();

engine/src/flutter/impeller/typographer/text_frame.cc

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -82,26 +82,26 @@ static constexpr Scalar ComputeFractionalPosition(Scalar value) {
8282
Point TextFrame::ComputeSubpixelPosition(
8383
const TextRun::GlyphPosition& glyph_position,
8484
AxisAlignment alignment,
85-
Point offset,
86-
Scalar scale) {
87-
Point pos = glyph_position.position + offset;
85+
const Matrix& transform) {
86+
Point pos = transform * glyph_position.position;
8887
switch (alignment) {
8988
case AxisAlignment::kNone:
9089
return Point(0, 0);
9190
case AxisAlignment::kX:
92-
return Point(ComputeFractionalPosition(pos.x * scale), 0);
91+
return Point(ComputeFractionalPosition(pos.x), 0);
9392
case AxisAlignment::kY:
94-
return Point(0, ComputeFractionalPosition(pos.y * scale));
93+
return Point(0, ComputeFractionalPosition(pos.y));
9594
case AxisAlignment::kAll:
96-
return Point(ComputeFractionalPosition(pos.x * scale),
97-
ComputeFractionalPosition(pos.y * scale));
95+
return Point(ComputeFractionalPosition(pos.x),
96+
ComputeFractionalPosition(pos.y));
9897
}
9998
}
10099

101100
void TextFrame::SetPerFrameData(Scalar scale,
102101
Point offset,
102+
const Matrix& transform,
103103
std::optional<GlyphProperties> properties) {
104-
if (!ScalarNearlyEqual(scale_, scale) ||
104+
if (!transform_.Equals(transform) || !ScalarNearlyEqual(scale_, scale) ||
105105
!ScalarNearlyEqual(offset_.x, offset.x) ||
106106
!ScalarNearlyEqual(offset_.y, offset.y) ||
107107
!TextPropertiesEquals(properties_, properties)) {
@@ -110,6 +110,7 @@ void TextFrame::SetPerFrameData(Scalar scale,
110110
scale_ = scale;
111111
offset_ = offset;
112112
properties_ = properties;
113+
transform_ = transform;
113114
}
114115

115116
Scalar TextFrame::GetScale() const {

0 commit comments

Comments
 (0)