From 26dd7e27cf679700138b82a0f51cf0f0fccb946f Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Tue, 26 Nov 2024 13:35:23 -0800 Subject: [PATCH] [Impeller] cache even more text frame data to skip lookups. (flutter/engine#56798) Every text frame must inform the glyph atlas about the glyphs + scale it contains. When this happens, the glyph atlas will populate the glyph and then tell teh text frame about the location in the atlas, so that the text contents shader can sample it correctly. Once this has been done once for a given text frame + scale + offset, we can actually just keep reusing the same data provided 1) the atlas itself hasn't changed and 2) the scale/offset/properties are the same. This constitutes a nice CPU usage reduction in places where there is a lot of text that isn't being invalidated, like scrolling on the flutter gallery. --- .../impeller/display_list/dl_dispatcher.cc | 5 +- .../impeller/entity/contents/text_contents.cc | 7 +- .../impeller/entity/entity_unittests.cc | 10 +- .../backends/skia/typographer_context_skia.cc | 17 ++-- .../impeller/typographer/font_glyph_pair.h | 19 ++-- .../impeller/typographer/glyph_atlas.cc | 14 ++- .../impeller/typographer/glyph_atlas.h | 16 +++- .../impeller/typographer/text_frame.cc | 32 ++++++- .../flutter/impeller/typographer/text_frame.h | 14 ++- .../typographer/typographer_unittests.cc | 93 ++++++++++++++++++- 10 files changed, 192 insertions(+), 35 deletions(-) diff --git a/engine/src/flutter/impeller/display_list/dl_dispatcher.cc b/engine/src/flutter/impeller/display_list/dl_dispatcher.cc index 1bdad62aff..b1ba5b7e35 100644 --- a/engine/src/flutter/impeller/display_list/dl_dispatcher.cc +++ b/engine/src/flutter/impeller/display_list/dl_dispatcher.cc @@ -1109,8 +1109,9 @@ void FirstPassDispatcher::drawTextFrame( // we do not double-apply the alpha. properties.color = paint_.color.WithAlpha(1.0); } - auto scale = - (matrix_ * Matrix::MakeTranslation(Point(x, y))).GetMaxBasisLengthXY(); + auto scale = TextFrame::RoundScaledFontSize( + (matrix_ * Matrix::MakeTranslation(Point(x, y))).GetMaxBasisLengthXY()); + renderer_.GetLazyGlyphAtlas()->AddTextFrame( text_frame, // scale, // diff --git a/engine/src/flutter/impeller/entity/contents/text_contents.cc b/engine/src/flutter/impeller/entity/contents/text_contents.cc index dfa4c42198..4d4a130480 100644 --- a/engine/src/flutter/impeller/entity/contents/text_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/text_contents.cc @@ -175,8 +175,7 @@ bool TextContents::Render(const ContentContext& renderer, size_t bounds_offset = 0u; for (const TextRun& run : frame_->GetRuns()) { const Font& font = run.GetFont(); - Scalar rounded_scale = TextFrame::RoundScaledFontSize( - scale_, font.GetMetrics().point_size); + Scalar rounded_scale = TextFrame::RoundScaledFontSize(scale_); FontGlyphAtlas* font_atlas = nullptr; // Adjust glyph position based on the subpixel rounding @@ -220,9 +219,9 @@ bool TextContents::Render(const ContentContext& renderer, VALIDATION_LOG << "Could not find font in the atlas."; continue; } - // Note: uses unrounded scale for more accurate subpixel position. Point subpixel = TextFrame::ComputeSubpixelPosition( - glyph_position, font.GetAxisAlignment(), offset_, scale_); + glyph_position, font.GetAxisAlignment(), offset_, + rounded_scale); std::optional maybe_atlas_glyph_bounds = font_atlas->FindGlyphBounds(SubpixelGlyph{ diff --git a/engine/src/flutter/impeller/entity/entity_unittests.cc b/engine/src/flutter/impeller/entity/entity_unittests.cc index 8fee465fab..756203ab2f 100644 --- a/engine/src/flutter/impeller/entity/entity_unittests.cc +++ b/engine/src/flutter/impeller/entity/entity_unittests.cc @@ -2120,11 +2120,11 @@ TEST_P(EntityTest, ColorFilterContentsWithLargeGeometry) { } TEST_P(EntityTest, TextContentsCeilsGlyphScaleToDecimal) { - ASSERT_EQ(TextFrame::RoundScaledFontSize(0.4321111f, 12), 0.43f); - ASSERT_EQ(TextFrame::RoundScaledFontSize(0.5321111f, 12), 0.53f); - ASSERT_EQ(TextFrame::RoundScaledFontSize(2.1f, 12), 2.1f); - ASSERT_EQ(TextFrame::RoundScaledFontSize(0.0f, 12), 0.0f); - ASSERT_EQ(TextFrame::RoundScaledFontSize(100000000.0f, 12), 48.0f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(0.4321111f), 0.43f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(0.5321111f), 0.53f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(2.1f), 2.1f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(0.0f), 0.0f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(100000000.0f), 48.0f); } TEST_P(EntityTest, SpecializationConstantsAreAppliedToVariants) { 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 80b499352b..13dc254e67 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 @@ -413,18 +413,20 @@ TypographerContextSkia::CollectNewGlyphs( const std::vector>& text_frames) { std::vector new_glyphs; std::vector glyph_sizes; + size_t generation_id = atlas->GetAtlasGeneration(); for (const auto& frame : text_frames) { - // TODO(jonahwilliams): unless we destroy the atlas (which we know about), - // we could probably guarantee that a text frame that is complete does not - // need to be processed unless the scale or properties changed. I'm leaving - // this as a future optimization. + if (frame->IsFrameComplete() && + frame->GetAtlasGeneration() == generation_id && + !frame->GetFrameBounds(0).is_placeholder) { + continue; + } frame->ClearFrameBounds(); + frame->SetAtlasGeneration(generation_id); for (const auto& run : frame->GetRuns()) { auto metrics = run.GetFont().GetMetrics(); - auto rounded_scale = - TextFrame::RoundScaledFontSize(frame->GetScale(), metrics.point_size); + auto rounded_scale = TextFrame::RoundScaledFontSize(frame->GetScale()); ScaledFont scaled_font{.font = run.GetFont(), .scale = rounded_scale}; FontGlyphAtlas* font_glyph_atlas = @@ -566,7 +568,8 @@ std::shared_ptr TypographerContextSkia::CreateGlyphAtlas( if (atlas_context->GetAtlasSize().height >= max_texture_height || context.GetBackendType() == Context::BackendType::kOpenGLES) { blit_old_atlas = false; - new_atlas = std::make_shared(type); + new_atlas = std::make_shared( + type, /*initial_generation=*/last_atlas->GetAtlasGeneration() + 1); auto [update_glyphs, update_sizes] = CollectNewGlyphs(new_atlas, text_frames); diff --git a/engine/src/flutter/impeller/typographer/font_glyph_pair.h b/engine/src/flutter/impeller/typographer/font_glyph_pair.h index e530f5b3ed..e3df2dae50 100644 --- a/engine/src/flutter/impeller/typographer/font_glyph_pair.h +++ b/engine/src/flutter/impeller/typographer/font_glyph_pair.h @@ -20,6 +20,17 @@ struct GlyphProperties { Join stroke_join = Join::kMiter; Scalar stroke_miter = 4.0; bool stroke = false; + + struct Equal { + constexpr bool operator()(const impeller::GlyphProperties& lhs, + const impeller::GlyphProperties& rhs) const { + return lhs.color.ToARGB() == rhs.color.ToARGB() && + lhs.stroke == rhs.stroke && lhs.stroke_cap == rhs.stroke_cap && + lhs.stroke_join == rhs.stroke_join && + lhs.stroke_miter == rhs.stroke_miter && + lhs.stroke_width == rhs.stroke_width; + } + }; }; //------------------------------------------------------------------------------ @@ -85,12 +96,8 @@ struct SubpixelGlyph { lhs.glyph.type == rhs.glyph.type && lhs.subpixel_offset == rhs.subpixel_offset && lhs.properties.has_value() && rhs.properties.has_value() && - lhs.properties->color.ToARGB() == rhs.properties->color.ToARGB() && - lhs.properties->stroke == rhs.properties->stroke && - lhs.properties->stroke_cap == rhs.properties->stroke_cap && - lhs.properties->stroke_join == rhs.properties->stroke_join && - lhs.properties->stroke_miter == rhs.properties->stroke_miter && - lhs.properties->stroke_width == rhs.properties->stroke_width; + GlyphProperties::Equal{}(lhs.properties.value(), + rhs.properties.value()); } }; }; diff --git a/engine/src/flutter/impeller/typographer/glyph_atlas.cc b/engine/src/flutter/impeller/typographer/glyph_atlas.cc index 093593804e..98038668fb 100644 --- a/engine/src/flutter/impeller/typographer/glyph_atlas.cc +++ b/engine/src/flutter/impeller/typographer/glyph_atlas.cc @@ -12,7 +12,8 @@ namespace impeller { GlyphAtlasContext::GlyphAtlasContext(GlyphAtlas::Type type) - : atlas_(std::make_shared(type)), atlas_size_(ISize(0, 0)) {} + : atlas_(std::make_shared(type, /*initial_generation=*/0)), + atlas_size_(ISize(0, 0)) {} GlyphAtlasContext::~GlyphAtlasContext() {} @@ -45,7 +46,8 @@ void GlyphAtlasContext::UpdateRectPacker( rect_packer_ = std::move(rect_packer); } -GlyphAtlas::GlyphAtlas(Type type) : type_(type) {} +GlyphAtlas::GlyphAtlas(Type type, size_t initial_generation) + : type_(type), generation_(initial_generation) {} GlyphAtlas::~GlyphAtlas() = default; @@ -65,6 +67,14 @@ void GlyphAtlas::SetTexture(std::shared_ptr texture) { texture_ = std::move(texture); } +size_t GlyphAtlas::GetAtlasGeneration() const { + return generation_; +} + +void GlyphAtlas::SetAtlasGeneration(size_t generation) { + generation_ = generation; +} + void GlyphAtlas::AddTypefaceGlyphPositionAndBounds(const FontGlyphPair& pair, Rect position, Rect bounds) { diff --git a/engine/src/flutter/impeller/typographer/glyph_atlas.h b/engine/src/flutter/impeller/typographer/glyph_atlas.h index 87c5490318..bf11ff056a 100644 --- a/engine/src/flutter/impeller/typographer/glyph_atlas.h +++ b/engine/src/flutter/impeller/typographer/glyph_atlas.h @@ -58,8 +58,9 @@ class GlyphAtlas { /// @brief Create an empty glyph atlas. /// /// @param[in] type How the glyphs are represented in the texture. + /// @param[in] initial_generation the atlas generation. /// - explicit GlyphAtlas(Type type); + GlyphAtlas(Type type, size_t initial_generation); ~GlyphAtlas(); @@ -142,9 +143,22 @@ class GlyphAtlas { /// FontGlyphAtlas* GetOrCreateFontGlyphAtlas(const ScaledFont& scaled_font); + //---------------------------------------------------------------------------- + /// @brief Retrieve the generation id for this glyph atlas. + /// + /// The generation id is used to match with a TextFrame to + /// determine if the frame is guaranteed to already be populated + /// in the atlas. + size_t GetAtlasGeneration() const; + + //---------------------------------------------------------------------------- + /// @brief Update the atlas generation. + void SetAtlasGeneration(size_t value); + private: const Type type_; std::shared_ptr texture_; + size_t generation_ = 0; std::unordered_map& 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) @@ -37,7 +51,7 @@ bool TextFrame::HasColor() const { } // static -Scalar TextFrame::RoundScaledFontSize(Scalar scale, Scalar point_size) { +Scalar TextFrame::RoundScaledFontSize(Scalar scale) { // An arbitrarily chosen maximum text scale to ensure that regardless of the // 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. @@ -87,6 +101,12 @@ Point TextFrame::ComputeSubpixelPosition( void TextFrame::SetPerFrameData(Scalar scale, Point offset, std::optional properties) { + if (!ScalarNearlyEqual(scale_, scale) || + !ScalarNearlyEqual(offset_.x, offset.x) || + !ScalarNearlyEqual(offset_.y, offset.y) || + !TextPropertiesEquals(properties_, properties)) { + bound_values_.clear(); + } scale_ = scale; offset_ = offset; properties_ = properties; @@ -121,7 +141,15 @@ bool TextFrame::IsFrameComplete() const { } const FrameBounds& TextFrame::GetFrameBounds(size_t index) const { - return bound_values_.at(index); + return bound_values_[index]; +} + +size_t TextFrame::GetAtlasGeneration() const { + return generation_; +} + +void TextFrame::SetAtlasGeneration(size_t value) { + generation_ = value; } } // namespace impeller diff --git a/engine/src/flutter/impeller/typographer/text_frame.h b/engine/src/flutter/impeller/typographer/text_frame.h index 0604d6cc1e..067c8ebd05 100644 --- a/engine/src/flutter/impeller/typographer/text_frame.h +++ b/engine/src/flutter/impeller/typographer/text_frame.h @@ -32,7 +32,7 @@ class TextFrame { Point offset, Scalar scale); - static Scalar RoundScaledFontSize(Scalar scale, Scalar point_size); + static Scalar RoundScaledFontSize(Scalar scale); //---------------------------------------------------------------------------- /// @brief The conservative bounding box for this text frame. @@ -84,13 +84,18 @@ class TextFrame { Point offset, std::optional properties); + // A generation id for the glyph atlas this text run was associated + // with. As long as the frame generation matches the atlas generation, + // the contents are guaranteed to be populated and do not need to be + // processed. + size_t GetAtlasGeneration() const; + TextFrame& operator=(TextFrame&& other) = default; TextFrame(const TextFrame& other) = default; private: friend class TypographerContextSkia; - friend class TypographerContextSTB; friend class LazyGlyphAtlas; Scalar GetScale() const; @@ -103,14 +108,17 @@ class TextFrame { void ClearFrameBounds(); + void SetAtlasGeneration(size_t value); + std::vector runs_; Rect bounds_; bool has_color_; // Data that is cached when rendering the text frame and is only - // valid for a single frame. + // valid for the current atlas generation. std::vector bound_values_; Scalar scale_ = 0; + size_t generation_ = 0; Point offset_; std::optional properties_; }; diff --git a/engine/src/flutter/impeller/typographer/typographer_unittests.cc b/engine/src/flutter/impeller/typographer/typographer_unittests.cc index 6367314d9b..4b8cc1cbc9 100644 --- a/engine/src/flutter/impeller/typographer/typographer_unittests.cc +++ b/engine/src/flutter/impeller/typographer/typographer_unittests.cc @@ -508,7 +508,7 @@ TEST_P(TypographerTest, TextFrameInitialBoundsArePlaceholder) { GetContext()->GetIdleWaiter()); auto atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, - GlyphAtlas::Type::kAlphaBitmap, 1.0f, + GlyphAtlas::Type::kAlphaBitmap, /*scale=*/1.0f, atlas_context, frame); // The glyph position in the atlas was not known when this value @@ -517,14 +517,101 @@ TEST_P(TypographerTest, TextFrameInitialBoundsArePlaceholder) { EXPECT_TRUE(frame->GetFrameBounds(0).is_placeholder); atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, - GlyphAtlas::Type::kAlphaBitmap, 1.0f, atlas_context, - frame); + GlyphAtlas::Type::kAlphaBitmap, /*scale=*/1.0f, + atlas_context, frame); // The second time the glyph is rendered, the bounds are correcly known. EXPECT_TRUE(frame->IsFrameComplete()); EXPECT_FALSE(frame->GetFrameBounds(0).is_placeholder); } +TEST_P(TypographerTest, TextFrameInvalidationWithScale) { + SkFont font = flutter::testing::CreateTestFontOfSize(12); + auto blob = SkTextBlob::MakeFromString( + "the quick brown fox jumped over the lazy dog.", font); + ASSERT_TRUE(blob); + auto frame = MakeTextFrameFromTextBlobSkia(blob); + + EXPECT_FALSE(frame->IsFrameComplete()); + + auto context = TypographerContextSkia::Make(); + auto atlas_context = + context->CreateGlyphAtlasContext(GlyphAtlas::Type::kAlphaBitmap); + auto host_buffer = HostBuffer::Create(GetContext()->GetResourceAllocator(), + GetContext()->GetIdleWaiter()); + + auto atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, + GlyphAtlas::Type::kAlphaBitmap, /*scale=*/1.0f, + atlas_context, frame); + + // The glyph position in the atlas was not known when this value + // was recorded. It is marked as a placeholder. + EXPECT_TRUE(frame->IsFrameComplete()); + EXPECT_TRUE(frame->GetFrameBounds(0).is_placeholder); + + // Change the scale and the glyph data will still be a placeholder, as the + // old data is no longer valid. + atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, + GlyphAtlas::Type::kAlphaBitmap, /*scale=*/2.0f, + atlas_context, frame); + + // The second time the glyph is rendered, the bounds are correcly known. + EXPECT_TRUE(frame->IsFrameComplete()); + EXPECT_TRUE(frame->GetFrameBounds(0).is_placeholder); +} + +TEST_P(TypographerTest, TextFrameAtlasGenerationTracksState) { + SkFont font = flutter::testing::CreateTestFontOfSize(12); + auto blob = SkTextBlob::MakeFromString( + "the quick brown fox jumped over the lazy dog.", font); + ASSERT_TRUE(blob); + auto frame = MakeTextFrameFromTextBlobSkia(blob); + + EXPECT_FALSE(frame->IsFrameComplete()); + + auto context = TypographerContextSkia::Make(); + auto atlas_context = + context->CreateGlyphAtlasContext(GlyphAtlas::Type::kAlphaBitmap); + auto host_buffer = HostBuffer::Create(GetContext()->GetResourceAllocator(), + GetContext()->GetIdleWaiter()); + + auto atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, + GlyphAtlas::Type::kAlphaBitmap, /*scale=*/1.0f, + atlas_context, frame); + + // The glyph position in the atlas was not known when this value + // was recorded. It is marked as a placeholder. + EXPECT_TRUE(frame->IsFrameComplete()); + EXPECT_TRUE(frame->GetFrameBounds(0).is_placeholder); + if (GetBackend() == PlaygroundBackend::kOpenGLES) { + // OpenGLES must always increase the atlas backend if the texture grows. + EXPECT_EQ(frame->GetAtlasGeneration(), 1u); + } else { + EXPECT_EQ(frame->GetAtlasGeneration(), 0u); + } + + atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, + GlyphAtlas::Type::kAlphaBitmap, /*scale=*/1.0f, + atlas_context, frame); + + // The second time the glyph is rendered, the bounds are correcly known. + EXPECT_TRUE(frame->IsFrameComplete()); + EXPECT_FALSE(frame->GetFrameBounds(0).is_placeholder); + if (GetBackend() == PlaygroundBackend::kOpenGLES) { + EXPECT_EQ(frame->GetAtlasGeneration(), 1u); + } else { + EXPECT_EQ(frame->GetAtlasGeneration(), 0u); + } + + // Force increase the generation. + atlas_context->GetGlyphAtlas()->SetAtlasGeneration(2u); + atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, + GlyphAtlas::Type::kAlphaBitmap, /*scale=*/1.0f, + atlas_context, frame); + + EXPECT_EQ(frame->GetAtlasGeneration(), 2u); +} + } // namespace testing } // namespace impeller