Skip to content

Commit dbbfa2f

Browse files
author
Jonah Williams
authored
[Impeller] Workaround for mismatched transform in preroll vs paint for text frames. (#164931)
Fixes flutter/flutter#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.
1 parent 9e5906f commit dbbfa2f

File tree

7 files changed

+89
-33
lines changed

7 files changed

+89
-33
lines changed

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

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,12 @@
1313
#include "flutter/fml/build_config.h"
1414
#include "flutter/impeller/display_list/aiks_unittests.h"
1515
#include "flutter/testing/testing.h"
16+
#include "impeller/entity/contents/text_contents.h"
17+
#include "impeller/entity/entity.h"
1618
#include "impeller/geometry/matrix.h"
1719
#include "impeller/typographer/backends/skia/text_frame_skia.h"
1820

21+
#include "impeller/typographer/backends/skia/typographer_context_skia.h"
1922
#include "txt/platform.h"
2023

2124
using namespace flutter;
@@ -595,5 +598,71 @@ TEST_P(AiksTest, DifferenceClipsMustRenderIdenticallyAcrossBackends) {
595598
ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
596599
}
597600

601+
TEST_P(AiksTest, TextContentsMismatchedTransformTest) {
602+
AiksContext aiks_context(GetContext(),
603+
std::make_shared<TypographerContextSkia>());
604+
605+
// Verifies that TextContents only use the scale/transform that is
606+
// computed during preroll.
607+
constexpr const char* font_fixture = "Roboto-Regular.ttf";
608+
609+
// Construct the text blob.
610+
auto c_font_fixture = std::string(font_fixture);
611+
auto mapping = flutter::testing::OpenFixtureAsSkData(c_font_fixture.c_str());
612+
ASSERT_TRUE(mapping);
613+
614+
sk_sp<SkFontMgr> font_mgr = txt::GetDefaultFontManager();
615+
SkFont sk_font(font_mgr->makeFromData(mapping), 16);
616+
617+
auto blob = SkTextBlob::MakeFromString("Hello World", sk_font);
618+
ASSERT_TRUE(blob);
619+
620+
auto text_frame = MakeTextFrameFromTextBlobSkia(blob);
621+
622+
// Simulate recording the text frame during preroll.
623+
Matrix preroll_matrix =
624+
Matrix::MakeTranslateScale({1.5, 1.5, 1}, {100, 50, 0});
625+
Point preroll_point = Point{23, 45};
626+
{
627+
auto scale = TextFrame::RoundScaledFontSize(
628+
(preroll_matrix * Matrix::MakeTranslation(preroll_point))
629+
.GetMaxBasisLengthXY());
630+
631+
aiks_context.GetContentContext().GetLazyGlyphAtlas()->AddTextFrame(
632+
text_frame, //
633+
scale, //
634+
preroll_point, //
635+
preroll_matrix,
636+
std::nullopt //
637+
);
638+
}
639+
640+
// Now simulate rendering with a slightly different scale factor.
641+
RenderTarget render_target =
642+
aiks_context.GetContentContext()
643+
.GetRenderTargetCache()
644+
->CreateOffscreenMSAA(*aiks_context.GetContext(), {100, 100}, 1);
645+
646+
TextContents text_contents;
647+
text_contents.SetTextFrame(text_frame);
648+
text_contents.SetOffset(preroll_point);
649+
text_contents.SetScale(1.6);
650+
text_contents.SetColor(Color::Aqua());
651+
652+
Matrix not_preroll_matrix =
653+
Matrix::MakeTranslateScale({1.5, 1.5, 1}, {100, 50, 0});
654+
655+
Entity entity;
656+
entity.SetTransform(not_preroll_matrix);
657+
658+
std::shared_ptr<CommandBuffer> command_buffer =
659+
aiks_context.GetContext()->CreateCommandBuffer();
660+
std::shared_ptr<RenderPass> render_pass =
661+
command_buffer->CreateRenderPass(render_target);
662+
663+
EXPECT_TRUE(text_contents.Render(aiks_context.GetContentContext(), entity,
664+
*render_pass));
665+
}
666+
598667
} // namespace testing
599668
} // namespace impeller

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,8 @@ void TextContents::ComputeVertexData(
105105
size_t bounds_offset = 0u;
106106
for (const TextRun& run : frame->GetRuns()) {
107107
const Font& font = run.GetFont();
108-
Scalar rounded_scale = TextFrame::RoundScaledFontSize(scale);
108+
Scalar rounded_scale = frame->GetScale();
109+
const Matrix transform = frame->GetOffsetTransform();
109110
FontGlyphAtlas* font_atlas = nullptr;
110111

111112
// Adjust glyph position based on the subpixel rounding
@@ -149,7 +150,7 @@ void TextContents::ComputeVertexData(
149150
continue;
150151
}
151152
Point subpixel = TextFrame::ComputeSubpixelPosition(
152-
glyph_position, font.GetAxisAlignment(), entity_transform);
153+
glyph_position, font.GetAxisAlignment(), transform);
153154

154155
std::optional<FrameBounds> maybe_atlas_glyph_bounds =
155156
font_atlas->FindGlyphBounds(SubpixelGlyph{

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,10 @@ std::shared_ptr<GlyphAtlas> CreateGlyphAtlas(
5858
const std::shared_ptr<GlyphAtlasContext>& atlas_context,
5959
const std::shared_ptr<TextFrame>& frame,
6060
Point offset) {
61-
frame->SetPerFrameData(scale, /*offset=*/offset, /*transform=*/Matrix(),
62-
/*properties=*/std::nullopt);
61+
frame->SetPerFrameData(
62+
TextFrame::RoundScaledFontSize(scale), /*offset=*/offset,
63+
/*transform=*/Matrix::MakeScale(Vector3{scale, scale, 1}),
64+
/*properties=*/std::nullopt);
6365
return typographer_context->CreateGlyphAtlas(context, type, host_buffer,
6466
atlas_context, {frame});
6567
}
@@ -183,6 +185,7 @@ TEST_P(TextContentsTest, MaintainsShape) {
183185
std::shared_ptr<HostBuffer> host_buffer = HostBuffer::Create(
184186
GetContext()->GetResourceAllocator(), GetContext()->GetIdleWaiter());
185187
ASSERT_TRUE(context && context->IsValid());
188+
186189
for (int i = 0; i <= 1000; ++i) {
187190
Scalar font_scale = 0.440 + (i / 1000.0);
188191
Rect position_rect[2];

engine/src/flutter/impeller/entity/entity_unittests.cc

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
#include "impeller/core/host_buffer.h"
1818
#include "impeller/core/raw_ptr.h"
1919
#include "impeller/core/texture_descriptor.h"
20-
#include "impeller/entity/contents/clip_contents.h"
2120
#include "impeller/entity/contents/conical_gradient_contents.h"
2221
#include "impeller/entity/contents/content_context.h"
2322
#include "impeller/entity/contents/contents.h"
@@ -55,10 +54,7 @@
5554
#include "impeller/renderer/render_target.h"
5655
#include "impeller/renderer/testing/mocks.h"
5756
#include "impeller/renderer/vertex_buffer_builder.h"
58-
#include "impeller/typographer/backends/skia/text_frame_skia.h"
59-
#include "impeller/typographer/backends/skia/typographer_context_skia.h"
6057
#include "third_party/imgui/imgui.h"
61-
#include "third_party/skia/include/core/SkTextBlob.h"
6258

6359
// TODO(zanderso): https://github.com/flutter/flutter/issues/127701
6460
// NOLINTBEGIN(bugprone-unchecked-optional-access)

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -459,8 +459,7 @@ TypographerContextSkia::CollectNewGlyphs(
459459
for (const auto& glyph_position : run.GetGlyphPositions()) {
460460
Point subpixel = TextFrame::ComputeSubpixelPosition(
461461
glyph_position, scaled_font.font.GetAxisAlignment(),
462-
frame->GetTransform() *
463-
Matrix::MakeTranslation(frame->GetOffset()));
462+
frame->GetOffsetTransform());
464463
SubpixelGlyph subpixel_glyph(glyph_position.glyph, subpixel,
465464
frame->GetProperties());
466465
const auto& font_glyph_bounds =

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

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -9,19 +9,6 @@
99

1010
namespace impeller {
1111

12-
namespace {
13-
static bool TextPropertiesEquals(const std::optional<GlyphProperties>& a,
14-
const std::optional<GlyphProperties>& b) {
15-
if (!a.has_value() && !b.has_value()) {
16-
return true;
17-
}
18-
if (a.has_value() && b.has_value()) {
19-
return GlyphProperties::Equal{}(a.value(), b.value());
20-
}
21-
return false;
22-
}
23-
} // namespace
24-
2512
TextFrame::TextFrame() = default;
2613

2714
TextFrame::TextFrame(std::vector<TextRun>& runs, Rect bounds, bool has_color)
@@ -97,16 +84,15 @@ Point TextFrame::ComputeSubpixelPosition(
9784
}
9885
}
9986

87+
Matrix TextFrame::GetOffsetTransform() const {
88+
return transform_ * Matrix::MakeTranslation(offset_);
89+
}
90+
10091
void TextFrame::SetPerFrameData(Scalar scale,
10192
Point offset,
10293
const Matrix& transform,
10394
std::optional<GlyphProperties> properties) {
104-
if (!transform_.Equals(transform) || !ScalarNearlyEqual(scale_, scale) ||
105-
!ScalarNearlyEqual(offset_.x, offset.x) ||
106-
!ScalarNearlyEqual(offset_.y, offset.y) ||
107-
!TextPropertiesEquals(properties_, properties)) {
108-
bound_values_.clear();
109-
}
95+
bound_values_.clear();
11096
scale_ = scale;
11197
offset_ = offset;
11298
properties_ = properties;

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,20 +91,22 @@ class TextFrame {
9191
// processed.
9292
std::pair<size_t, intptr_t> GetAtlasGenerationAndID() const;
9393

94+
Scalar GetScale() const;
95+
9496
TextFrame& operator=(TextFrame&& other) = default;
9597

9698
TextFrame(const TextFrame& other) = default;
9799

98100
const Matrix& GetTransform() const { return transform_; }
99101

102+
Point GetOffset() const;
103+
104+
Matrix GetOffsetTransform() const;
105+
100106
private:
101107
friend class TypographerContextSkia;
102108
friend class LazyGlyphAtlas;
103109

104-
Scalar GetScale() const;
105-
106-
Point GetOffset() const;
107-
108110
std::optional<GlyphProperties> GetProperties() const;
109111

110112
void AppendFrameBounds(const FrameBounds& frame_bounds);

0 commit comments

Comments
 (0)