From dbbfa2ff3189aa0e0d1054b9502de3bda84b3b90 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 10 Mar 2025 19:10:33 -0700 Subject: [PATCH] [Impeller] Workaround for mismatched transform in preroll vs paint for text frames. (#164931) Fixes https://github.com/flutter/flutter/issues/164606 When we preroll the text frame, we record the scale/transform used to compute the size and subpixel position. Use this same size + transform for the subsequent paint so that it is not possible for it to mismatch. This is not really a fix for the underlying issue where the subpixel position may be mismatched. --- .../display_list/aiks_dl_text_unittests.cc | 69 +++++++++++++++++++ .../impeller/entity/contents/text_contents.cc | 5 +- .../contents/text_contents_unittests.cc | 7 +- .../impeller/entity/entity_unittests.cc | 4 -- .../backends/skia/typographer_context_skia.cc | 3 +- .../impeller/typographer/text_frame.cc | 24 ++----- .../flutter/impeller/typographer/text_frame.h | 10 +-- 7 files changed, 89 insertions(+), 33 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 e5c2cf42693..f12411811f4 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 @@ -13,9 +13,12 @@ #include "flutter/fml/build_config.h" #include "flutter/impeller/display_list/aiks_unittests.h" #include "flutter/testing/testing.h" +#include "impeller/entity/contents/text_contents.h" +#include "impeller/entity/entity.h" #include "impeller/geometry/matrix.h" #include "impeller/typographer/backends/skia/text_frame_skia.h" +#include "impeller/typographer/backends/skia/typographer_context_skia.h" #include "txt/platform.h" using namespace flutter; @@ -595,5 +598,71 @@ TEST_P(AiksTest, DifferenceClipsMustRenderIdenticallyAcrossBackends) { ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); } +TEST_P(AiksTest, TextContentsMismatchedTransformTest) { + AiksContext aiks_context(GetContext(), + std::make_shared()); + + // Verifies that TextContents only use the scale/transform that is + // computed during preroll. + constexpr const char* font_fixture = "Roboto-Regular.ttf"; + + // Construct the text blob. + auto c_font_fixture = std::string(font_fixture); + auto mapping = flutter::testing::OpenFixtureAsSkData(c_font_fixture.c_str()); + ASSERT_TRUE(mapping); + + sk_sp font_mgr = txt::GetDefaultFontManager(); + SkFont sk_font(font_mgr->makeFromData(mapping), 16); + + auto blob = SkTextBlob::MakeFromString("Hello World", sk_font); + ASSERT_TRUE(blob); + + auto text_frame = MakeTextFrameFromTextBlobSkia(blob); + + // Simulate recording the text frame during preroll. + Matrix preroll_matrix = + Matrix::MakeTranslateScale({1.5, 1.5, 1}, {100, 50, 0}); + Point preroll_point = Point{23, 45}; + { + auto scale = TextFrame::RoundScaledFontSize( + (preroll_matrix * Matrix::MakeTranslation(preroll_point)) + .GetMaxBasisLengthXY()); + + aiks_context.GetContentContext().GetLazyGlyphAtlas()->AddTextFrame( + text_frame, // + scale, // + preroll_point, // + preroll_matrix, + std::nullopt // + ); + } + + // Now simulate rendering with a slightly different scale factor. + RenderTarget render_target = + aiks_context.GetContentContext() + .GetRenderTargetCache() + ->CreateOffscreenMSAA(*aiks_context.GetContext(), {100, 100}, 1); + + TextContents text_contents; + text_contents.SetTextFrame(text_frame); + text_contents.SetOffset(preroll_point); + text_contents.SetScale(1.6); + text_contents.SetColor(Color::Aqua()); + + Matrix not_preroll_matrix = + Matrix::MakeTranslateScale({1.5, 1.5, 1}, {100, 50, 0}); + + Entity entity; + entity.SetTransform(not_preroll_matrix); + + std::shared_ptr command_buffer = + aiks_context.GetContext()->CreateCommandBuffer(); + std::shared_ptr render_pass = + command_buffer->CreateRenderPass(render_target); + + EXPECT_TRUE(text_contents.Render(aiks_context.GetContentContext(), entity, + *render_pass)); +} + } // namespace testing } // namespace impeller diff --git a/engine/src/flutter/impeller/entity/contents/text_contents.cc b/engine/src/flutter/impeller/entity/contents/text_contents.cc index d3d6f6e36ec..3ca08c4fe2e 100644 --- a/engine/src/flutter/impeller/entity/contents/text_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/text_contents.cc @@ -105,7 +105,8 @@ void TextContents::ComputeVertexData( size_t bounds_offset = 0u; for (const TextRun& run : frame->GetRuns()) { const Font& font = run.GetFont(); - Scalar rounded_scale = TextFrame::RoundScaledFontSize(scale); + Scalar rounded_scale = frame->GetScale(); + const Matrix transform = frame->GetOffsetTransform(); FontGlyphAtlas* font_atlas = nullptr; // Adjust glyph position based on the subpixel rounding @@ -149,7 +150,7 @@ void TextContents::ComputeVertexData( continue; } Point subpixel = TextFrame::ComputeSubpixelPosition( - glyph_position, font.GetAxisAlignment(), entity_transform); + glyph_position, font.GetAxisAlignment(), transform); std::optional 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 index 7eaa016e23d..d1eec4fc2b0 100644 --- a/engine/src/flutter/impeller/entity/contents/text_contents_unittests.cc +++ b/engine/src/flutter/impeller/entity/contents/text_contents_unittests.cc @@ -58,8 +58,10 @@ std::shared_ptr CreateGlyphAtlas( const std::shared_ptr& atlas_context, const std::shared_ptr& frame, Point offset) { - frame->SetPerFrameData(scale, /*offset=*/offset, /*transform=*/Matrix(), - /*properties=*/std::nullopt); + frame->SetPerFrameData( + TextFrame::RoundScaledFontSize(scale), /*offset=*/offset, + /*transform=*/Matrix::MakeScale(Vector3{scale, scale, 1}), + /*properties=*/std::nullopt); return typographer_context->CreateGlyphAtlas(context, type, host_buffer, atlas_context, {frame}); } @@ -183,6 +185,7 @@ TEST_P(TextContentsTest, MaintainsShape) { std::shared_ptr host_buffer = HostBuffer::Create( GetContext()->GetResourceAllocator(), GetContext()->GetIdleWaiter()); ASSERT_TRUE(context && context->IsValid()); + for (int i = 0; i <= 1000; ++i) { Scalar font_scale = 0.440 + (i / 1000.0); Rect position_rect[2]; diff --git a/engine/src/flutter/impeller/entity/entity_unittests.cc b/engine/src/flutter/impeller/entity/entity_unittests.cc index a14dd87d357..afd8e28fd2f 100644 --- a/engine/src/flutter/impeller/entity/entity_unittests.cc +++ b/engine/src/flutter/impeller/entity/entity_unittests.cc @@ -17,7 +17,6 @@ #include "impeller/core/host_buffer.h" #include "impeller/core/raw_ptr.h" #include "impeller/core/texture_descriptor.h" -#include "impeller/entity/contents/clip_contents.h" #include "impeller/entity/contents/conical_gradient_contents.h" #include "impeller/entity/contents/content_context.h" #include "impeller/entity/contents/contents.h" @@ -55,10 +54,7 @@ #include "impeller/renderer/render_target.h" #include "impeller/renderer/testing/mocks.h" #include "impeller/renderer/vertex_buffer_builder.h" -#include "impeller/typographer/backends/skia/text_frame_skia.h" -#include "impeller/typographer/backends/skia/typographer_context_skia.h" #include "third_party/imgui/imgui.h" -#include "third_party/skia/include/core/SkTextBlob.h" // TODO(zanderso): https://github.com/flutter/flutter/issues/127701 // NOLINTBEGIN(bugprone-unchecked-optional-access) 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 index 25b8b7d9f35..19374cac0ca 100644 --- a/engine/src/flutter/impeller/typographer/backends/skia/typographer_context_skia.cc +++ b/engine/src/flutter/impeller/typographer/backends/skia/typographer_context_skia.cc @@ -459,8 +459,7 @@ TypographerContextSkia::CollectNewGlyphs( for (const auto& glyph_position : run.GetGlyphPositions()) { Point subpixel = TextFrame::ComputeSubpixelPosition( glyph_position, scaled_font.font.GetAxisAlignment(), - frame->GetTransform() * - Matrix::MakeTranslation(frame->GetOffset())); + frame->GetOffsetTransform()); SubpixelGlyph subpixel_glyph(glyph_position.glyph, subpixel, frame->GetProperties()); const auto& font_glyph_bounds = diff --git a/engine/src/flutter/impeller/typographer/text_frame.cc b/engine/src/flutter/impeller/typographer/text_frame.cc index 870231a6043..a679e2c2201 100644 --- a/engine/src/flutter/impeller/typographer/text_frame.cc +++ b/engine/src/flutter/impeller/typographer/text_frame.cc @@ -9,19 +9,6 @@ namespace impeller { -namespace { -static bool TextPropertiesEquals(const std::optional& a, - const std::optional& b) { - if (!a.has_value() && !b.has_value()) { - return true; - } - if (a.has_value() && b.has_value()) { - return GlyphProperties::Equal{}(a.value(), b.value()); - } - return false; -} -} // namespace - TextFrame::TextFrame() = default; TextFrame::TextFrame(std::vector& runs, Rect bounds, bool has_color) @@ -97,16 +84,15 @@ Point TextFrame::ComputeSubpixelPosition( } } +Matrix TextFrame::GetOffsetTransform() const { + return transform_ * Matrix::MakeTranslation(offset_); +} + void TextFrame::SetPerFrameData(Scalar scale, Point offset, const Matrix& transform, std::optional properties) { - if (!transform_.Equals(transform) || !ScalarNearlyEqual(scale_, scale) || - !ScalarNearlyEqual(offset_.x, offset.x) || - !ScalarNearlyEqual(offset_.y, offset.y) || - !TextPropertiesEquals(properties_, properties)) { - bound_values_.clear(); - } + bound_values_.clear(); scale_ = scale; offset_ = offset; properties_ = properties; diff --git a/engine/src/flutter/impeller/typographer/text_frame.h b/engine/src/flutter/impeller/typographer/text_frame.h index 86b03d6480f..0a000109f1e 100644 --- a/engine/src/flutter/impeller/typographer/text_frame.h +++ b/engine/src/flutter/impeller/typographer/text_frame.h @@ -91,20 +91,22 @@ class TextFrame { // processed. std::pair GetAtlasGenerationAndID() const; + Scalar GetScale() const; + TextFrame& operator=(TextFrame&& other) = default; TextFrame(const TextFrame& other) = default; const Matrix& GetTransform() const { return transform_; } + Point GetOffset() const; + + Matrix GetOffsetTransform() const; + private: friend class TypographerContextSkia; friend class LazyGlyphAtlas; - Scalar GetScale() const; - - Point GetOffset() const; - std::optional GetProperties() const; void AppendFrameBounds(const FrameBounds& frame_bounds);