From cebc34c92c8f62711c3a5bf9a5d303e43e97b386 Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Mon, 3 Feb 2025 13:29:37 -0800 Subject: [PATCH] Increased the glyph atlas resolution 2x (#162555) issue: https://github.com/flutter/flutter/issues/149652 This makes https://github.com/flutter/flutter/issues/149652 better. It doesn't completely it better though, there is still a shrinking of whitespace at larger scales. Without this patch though the test will fail with many 3 pixel jumps between glyphs. I think scaling in the thousands is reasonable so this is an adequate test. ## 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 --- .../display_list/aiks_dl_text_unittests.cc | 9 +++- .../display_list/dl_golden_unittests.cc | 53 +++++++++++++++++++ .../impeller/entity/contents/text_contents.cc | 1 - .../impeller/typographer/text_frame.cc | 2 +- 4 files changed, 62 insertions(+), 3 deletions(-) diff --git a/engine/src/flutter/impeller/display_list/aiks_dl_text_unittests.cc b/engine/src/flutter/impeller/display_list/aiks_dl_text_unittests.cc index 0d03982a35..4f11e10f29 100644 --- a/engine/src/flutter/impeller/display_list/aiks_dl_text_unittests.cc +++ b/engine/src/flutter/impeller/display_list/aiks_dl_text_unittests.cc @@ -31,6 +31,7 @@ struct TextRenderOptions { DlColor color = DlColor::kYellow(); DlPoint position = DlPoint(100, 200); std::shared_ptr filter; + bool is_subpixel = false; }; bool RenderTextInCanvasSkia(const std::shared_ptr& context, @@ -57,6 +58,9 @@ bool RenderTextInCanvasSkia(const std::shared_ptr& context, } sk_sp 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; @@ -153,10 +157,12 @@ TEST_P(AiksTest, CanRenderTextFrameWithHalfScaling) { TEST_P(AiksTest, CanRenderTextFrameWithFractionScaling) { Scalar fine_scale = 0.f; + bool is_subpixel = false; auto callback = [&]() -> sk_sp { if (AiksTest::ImGuiBegin("Controls", nullptr, ImGuiWindowFlags_AlwaysAutoResize)) { ImGui::SliderFloat("Fine Scale", &fine_scale, -1, 1); + ImGui::Checkbox("subpixel", &is_subpixel); ImGui::End(); } @@ -168,7 +174,8 @@ TEST_P(AiksTest, CanRenderTextFrameWithFractionScaling) { builder.Scale(scale, scale); RenderTextInCanvasSkia(GetContext(), builder, "the quick brown fox jumped over the lazy dog!.?", - "Roboto-Regular.ttf"); + "Roboto-Regular.ttf", + TextRenderOptions{.is_subpixel = is_subpixel}); return builder.Build(); }; diff --git a/engine/src/flutter/impeller/display_list/dl_golden_unittests.cc b/engine/src/flutter/impeller/display_list/dl_golden_unittests.cc index 5236a01ce0..918eae778e 100644 --- a/engine/src/flutter/impeller/display_list/dl_golden_unittests.cc +++ b/engine/src/flutter/impeller/display_list/dl_golden_unittests.cc @@ -366,6 +366,22 @@ int32_t CalculateMaxY(const impeller::testing::Screenshot* img) { } return max_y; } + +int32_t CalculateSpaceBetweenUI(const impeller::testing::Screenshot* img) { + const uint32_t* ptr = reinterpret_cast(img->GetBytes()); + ptr += img->GetWidth() * static_cast(img->GetHeight() / 2.0); + std::vector boundaries; + uint32_t value = *ptr++; + for (size_t i = 1; i < img->GetWidth(); ++i) { + if (((*ptr & 0x00ffffff) != 0) != ((value & 0x00ffffff) != 0)) { + boundaries.push_back(i); + } + value = *ptr++; + } + + assert(boundaries.size() == 6); + return boundaries[4] - boundaries[3]; +} } // namespace TEST_P(DlGoldenTest, BaselineHE) { @@ -399,5 +415,42 @@ TEST_P(DlGoldenTest, BaselineHE) { int32_t y_diff = std::abs(left_max_y - right_max_y); EXPECT_TRUE(y_diff <= 2) << "y diff: " << y_diff; } + +TEST_P(DlGoldenTest, MaintainsSpace) { + SetWindowSize(impeller::ISize(1024, 200)); + impeller::Scalar font_size = 300; + auto callback = [&](const char* text, + impeller::Scalar scale) -> sk_sp { + DisplayListBuilder builder; + DlPaint paint; + paint.setColor(DlColor::ARGB(1, 0, 0, 0)); + builder.DrawPaint(paint); + builder.Scale(scale, scale); + RenderTextInCanvasSkia(&builder, text, "Roboto-Regular.ttf", + DlPoint::MakeXY(10, 300), + TextRenderOptions{ + .font_size = font_size, + }); + return builder.Build(); + }; + + std::optional last_space; + for (int i = 0; i <= 100; ++i) { + Scalar scale = 0.440 + i / 1000.0; + std::unique_ptr right = + MakeScreenshot(callback("ui", scale)); + if (!right) { + GTEST_SKIP() << "making screenshots not supported."; + } + + int32_t space = CalculateSpaceBetweenUI(right.get()); + if (last_space.has_value()) { + int32_t diff = abs(space - *last_space); + EXPECT_TRUE(diff <= 1) + << "i:" << i << " space:" << space << " last_space:" << *last_space; + } + last_space = space; + } +} } // namespace testing } // namespace flutter diff --git a/engine/src/flutter/impeller/entity/contents/text_contents.cc b/engine/src/flutter/impeller/entity/contents/text_contents.cc index b24bd8a4ee..db111f2ec5 100644 --- a/engine/src/flutter/impeller/entity/contents/text_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/text_contents.cc @@ -183,7 +183,6 @@ void TextContents::ComputeVertexData( Point screen_glyph_position = (screen_offset + unrounded_glyph_position + subpixel_adjustment) .Floor(); - for (const Point& point : unit_points) { Point position; if (is_translation_scale) { diff --git a/engine/src/flutter/impeller/typographer/text_frame.cc b/engine/src/flutter/impeller/typographer/text_frame.cc index b8395fd074..4bdc6e22f9 100644 --- a/engine/src/flutter/impeller/typographer/text_frame.cc +++ b/engine/src/flutter/impeller/typographer/text_frame.cc @@ -56,7 +56,7 @@ Scalar TextFrame::RoundScaledFontSize(Scalar scale) { // CTM, a glyph will fit in the atlas. If we clamp significantly, this may // reduce fidelity but is preferable to the alternative of failing to render. constexpr Scalar kMaximumTextScale = 48; - Scalar result = std::round(scale * 100) / 100; + Scalar result = std::round(scale * 200) / 200; return std::clamp(result, 0.0f, kMaximumTextScale); }