From f00d387cb88b0422d20baa381cccfa8ca8d51edc Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Fri, 4 Apr 2025 13:28:25 -0700 Subject: [PATCH] [Impeller] if drawTextFrame scale is massive, convert to Path. (#166234) Fixes https://github.com/flutter/flutter/issues/165583 Fixes https://github.com/flutter/flutter/issues/136112 If the text scale gets too big, its faster (and higher res) to switch to path rendering than to keep using the glyph atlas cache. --- .../testing/dl_rendering_unittests.cc | 1 - .../display_list/aiks_dl_text_unittests.cc | 55 +++++++++++++++++++ .../flutter/impeller/display_list/canvas.cc | 21 ++++++- .../src/flutter/impeller/typographer/BUILD.gn | 5 +- .../backends/skia/text_frame_skia.cc | 15 ++++- .../impeller/typographer/text_frame.cc | 19 ++++++- .../flutter/impeller/typographer/text_frame.h | 22 ++++++-- .../flutter/txt/src/skia/paragraph_skia.cc | 2 +- 8 files changed, 128 insertions(+), 12 deletions(-) diff --git a/engine/src/flutter/display_list/testing/dl_rendering_unittests.cc b/engine/src/flutter/display_list/testing/dl_rendering_unittests.cc index 0965265d01..af996ab970 100644 --- a/engine/src/flutter/display_list/testing/dl_rendering_unittests.cc +++ b/engine/src/flutter/display_list/testing/dl_rendering_unittests.cc @@ -3900,7 +3900,6 @@ TEST_F(DisplayListRendering, DrawTextBlob) { // ~12 on top and ~8 on the bottom, so we add 33h & 13v allowed // padding to the tolerance CanvasCompareTester::DefaultTolerance.addBoundsPadding(33, 13)); - EXPECT_TRUE(blob->unique()); #endif // OS_FUCHSIA } 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 6d23191a0e..393743ecf9 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 @@ -190,6 +190,34 @@ TEST_P(AiksTest, ScaledK) { ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); } +// This is a test that looks for glyph artifacts we've see. +TEST_P(AiksTest, MassiveScaleConvertToPath) { + Scalar scale = 16.0; + auto callback = [&]() -> sk_sp { + if (AiksTest::ImGuiBegin("Controls", nullptr, + ImGuiWindowFlags_AlwaysAutoResize)) { + ImGui::SliderFloat("Scale", &scale, 4, 20); + ImGui::End(); + } + + DisplayListBuilder builder; + DlPaint paint; + paint.setColor(DlColor::ARGB(1, 0.1, 0.1, 0.1)); + builder.DrawPaint(paint); + + builder.Scale(scale, scale); + RenderTextInCanvasSkia( + GetContext(), builder, "HELLO", "Roboto-Regular.ttf", + TextRenderOptions{.font_size = 16, + .color = (16 * scale >= 250) ? DlColor::kYellow() + : DlColor::kOrange(), + .position = DlPoint(0, 20)}); + return builder.Build(); + }; + + ASSERT_TRUE(OpenPlaygroundHere(callback)); +} + TEST_P(AiksTest, CanRenderTextFrameWithScalingOverflow) { Scalar scale = 60.0; Scalar offsetx = -500.0; @@ -844,5 +872,32 @@ TEST_P(AiksTest, SingleIconShadowTest) { 1u); } +TEST_P(AiksTest, VarietyOfTextScalesShowingRasterAndPath) { + DisplayListBuilder builder; + DlPaint paint; + paint.setColor(DlColor::ARGB(1, 0.1, 0.1, 0.1)); + builder.DrawPaint(paint); + builder.Scale(GetContentScale().x, GetContentScale().y); + + std::vector scales = {4, 8, 16, 24, 32}; + std::vector spacing = {8, 8, 8, 8, 8}; + Scalar space = 16; + Scalar x = 0; + for (auto i = 0u; i < scales.size(); i++) { + builder.Save(); + builder.Scale(scales[i], scales[i]); + RenderTextInCanvasSkia( + GetContext(), builder, "lo", "Roboto-Regular.ttf", + TextRenderOptions{.font_size = 16, .position = DlPoint(x, space)}); + space += spacing[i]; + if (i == 3) { + x = 10; + space = 16; + } + builder.Restore(); + } + ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); +} + } // namespace testing } // namespace impeller diff --git a/engine/src/flutter/impeller/display_list/canvas.cc b/engine/src/flutter/impeller/display_list/canvas.cc index 435855dd61..cec0f277f2 100644 --- a/engine/src/flutter/impeller/display_list/canvas.cc +++ b/engine/src/flutter/impeller/display_list/canvas.cc @@ -14,6 +14,7 @@ #include "display_list/effects/dl_color_filter.h" #include "display_list/effects/dl_color_source.h" #include "display_list/effects/dl_image_filter.h" +#include "display_list/geometry/dl_path.h" #include "display_list/image/dl_image.h" #include "flutter/fml/logging.h" #include "flutter/fml/trace_event.h" @@ -1511,9 +1512,27 @@ bool Canvas::AttemptBlurredTextOptimization( } } +// If the text point size * max basis XY is larger than this value, +// render the text as paths (if available) for faster and higher +// fidelity rendering. This is a somewhat arbitrary cutoff +static constexpr Scalar kMaxTextScale = 250; + void Canvas::DrawTextFrame(const std::shared_ptr& text_frame, Point position, const Paint& paint) { + Scalar max_scale = GetCurrentTransform().GetMaxBasisLengthXY(); + if (max_scale * text_frame->GetFont().GetMetrics().point_size > + kMaxTextScale) { + fml::StatusOr path = text_frame->GetPath(); + if (path.ok()) { + Save(1); + Concat(Matrix::MakeTranslation(position)); + DrawPath(path.value().GetPath(), paint); + Restore(); + return; + } + } + Entity entity; entity.SetClipDepth(GetClipHeight()); entity.SetBlendMode(paint.blend_mode); @@ -1521,7 +1540,7 @@ void Canvas::DrawTextFrame(const std::shared_ptr& text_frame, auto text_contents = std::make_shared(); text_contents->SetTextFrame(text_frame); text_contents->SetForceTextColor(paint.mask_blur_descriptor.has_value()); - text_contents->SetScale(GetCurrentTransform().GetMaxBasisLengthXY()); + text_contents->SetScale(max_scale); text_contents->SetColor(paint.color); text_contents->SetOffset(position); text_contents->SetTextProperties(paint.color, // diff --git a/engine/src/flutter/impeller/typographer/BUILD.gn b/engine/src/flutter/impeller/typographer/BUILD.gn index 7279b6aabe..d3d53c3320 100644 --- a/engine/src/flutter/impeller/typographer/BUILD.gn +++ b/engine/src/flutter/impeller/typographer/BUILD.gn @@ -39,7 +39,10 @@ impeller_component("typographer") { [ "//flutter/third_party/abseil-cpp/absl/container:flat_hash_map" ] } - deps = [ "//flutter/fml" ] + deps = [ + "//flutter/fml", + "//flutter/skia", + ] } impeller_component("typographer_unittests") { 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 38e81acd04..db0f04b7a3 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 @@ -6,7 +6,9 @@ #include +#include "flutter/display_list/geometry/dl_path.h" #include "flutter/fml/logging.h" +#include "fml/status.h" #include "impeller/typographer/backends/skia/typeface_skia.h" #include "impeller/typographer/font.h" #include "impeller/typographer/glyph.h" @@ -14,6 +16,7 @@ #include "third_party/skia/include/core/SkFontMetrics.h" #include "third_party/skia/include/core/SkPaint.h" #include "third_party/skia/include/core/SkRect.h" +#include "third_party/skia/modules/skparagraph/include/Paragraph.h" // nogncheck #include "third_party/skia/src/core/SkStrikeSpec.h" // nogncheck #include "third_party/skia/src/core/SkTextBlobPriv.h" // nogncheck @@ -83,7 +86,17 @@ std::shared_ptr MakeTextFrameFromTextBlobSkia( continue; } } - return std::make_shared(runs, ToRect(blob->bounds()), has_color); + return std::make_shared( + runs, ToRect(blob->bounds()), has_color, + [blob]() -> fml::StatusOr { + SkPath path = skia::textlayout::Paragraph::GetPath(blob.get()); + if (path.isEmpty()) { + return fml::Status(fml::StatusCode::kCancelled, "No path available"); + } + SkPath transformed = path.makeTransform( + SkMatrix::Translate(blob->bounds().left(), blob->bounds().top())); + return flutter::DlPath(transformed); + }); } } // namespace impeller diff --git a/engine/src/flutter/impeller/typographer/text_frame.cc b/engine/src/flutter/impeller/typographer/text_frame.cc index 633a403610..073b34e7d1 100644 --- a/engine/src/flutter/impeller/typographer/text_frame.cc +++ b/engine/src/flutter/impeller/typographer/text_frame.cc @@ -3,6 +3,8 @@ // found in the LICENSE file. #include "impeller/typographer/text_frame.h" +#include "flutter/display_list/geometry/dl_path.h" // nogncheck +#include "fml/status.h" #include "impeller/geometry/scalar.h" #include "impeller/typographer/font.h" #include "impeller/typographer/font_glyph_pair.h" @@ -11,8 +13,14 @@ namespace impeller { TextFrame::TextFrame() = default; -TextFrame::TextFrame(std::vector& runs, Rect bounds, bool has_color) - : runs_(std::move(runs)), bounds_(bounds), has_color_(has_color) {} +TextFrame::TextFrame(std::vector& runs, + Rect bounds, + bool has_color, + const PathCreator& path_creator) + : runs_(std::move(runs)), + bounds_(bounds), + has_color_(has_color), + path_creator_(path_creator) {} TextFrame::~TextFrame() = default; @@ -138,6 +146,13 @@ void TextFrame::ClearFrameBounds() { bound_values_.clear(); } +fml::StatusOr TextFrame::GetPath() const { + if (path_creator_) { + return path_creator_(); + } + return fml::Status(fml::StatusCode::kCancelled, "no path creator specified."); +} + bool TextFrame::IsFrameComplete() const { size_t run_size = 0; for (const auto& x : runs_) { diff --git a/engine/src/flutter/impeller/typographer/text_frame.h b/engine/src/flutter/impeller/typographer/text_frame.h index 431edf0aa6..24ddc24e11 100644 --- a/engine/src/flutter/impeller/typographer/text_frame.h +++ b/engine/src/flutter/impeller/typographer/text_frame.h @@ -6,13 +6,23 @@ #define FLUTTER_IMPELLER_TYPOGRAPHER_TEXT_FRAME_H_ #include + +#include "fml/status_or.h" #include "impeller/geometry/rational.h" #include "impeller/typographer/glyph.h" #include "impeller/typographer/glyph_atlas.h" #include "impeller/typographer/text_run.h" +// TODO(https://github.com/flutter/flutter/issues/166593): This is required to +// break a cyclical dependency between display list, impeller, and typographer. +namespace flutter { +class DlPath; +} + namespace impeller { +using PathCreator = std::function()>; + //------------------------------------------------------------------------------ /// @brief Represents a collection of shaped text runs. /// @@ -25,7 +35,10 @@ class TextFrame { public: TextFrame(); - TextFrame(std::vector& runs, Rect bounds, bool has_color); + TextFrame(std::vector& runs, + Rect bounds, + bool has_color, + const PathCreator& path_creator = {}); ~TextFrame(); @@ -103,12 +116,10 @@ class TextFrame { Rational GetScale() const; - TextFrame& operator=(TextFrame&& other) = default; - - TextFrame(const TextFrame& other) = default; - const Matrix& GetTransform() const { return transform_; } + fml::StatusOr GetPath() const; + Point GetOffset() const; Matrix GetOffsetTransform() const; @@ -128,6 +139,7 @@ class TextFrame { std::vector runs_; Rect bounds_; bool has_color_; + const PathCreator path_creator_; // Data that is cached when rendering the text frame and is only // valid for the current atlas generation. diff --git a/engine/src/flutter/txt/src/skia/paragraph_skia.cc b/engine/src/flutter/txt/src/skia/paragraph_skia.cc index 7ba0c79bb7..bfe605b657 100644 --- a/engine/src/flutter/txt/src/skia/paragraph_skia.cc +++ b/engine/src/flutter/txt/src/skia/paragraph_skia.cc @@ -76,7 +76,7 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { if (impeller_enabled_) { SkTextBlobRunIterator run(blob.get()); if (ShouldRenderAsPath(dl_paints_[paint_id])) { - auto path = skia::textlayout::Paragraph::GetPath(blob.get()); + SkPath path = skia::textlayout::Paragraph::GetPath(blob.get()); // If there is no path, this is an emoji and should be drawn as is, // ignoring the color source. if (path.isEmpty()) {