From b204b53f24659768b94c54bcbe0aefd4211f7209 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 25 Nov 2024 14:03:17 -0800 Subject: [PATCH 1/6] [Impeller] cache even more text frame data to skip lookups. --- .../backends/skia/typographer_context_skia.cc | 12 +++++ impeller/typographer/glyph_atlas.cc | 8 +++ impeller/typographer/glyph_atlas.h | 13 +++++ impeller/typographer/text_frame.cc | 10 +++- impeller/typographer/text_frame.h | 12 ++++- impeller/typographer/typographer_unittests.cc | 52 +++++++++++++++++++ 6 files changed, 104 insertions(+), 3 deletions(-) diff --git a/impeller/typographer/backends/skia/typographer_context_skia.cc b/impeller/typographer/backends/skia/typographer_context_skia.cc index 80b499352b557..d9370ec6db126 100644 --- a/impeller/typographer/backends/skia/typographer_context_skia.cc +++ b/impeller/typographer/backends/skia/typographer_context_skia.cc @@ -413,12 +413,19 @@ 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(); @@ -567,6 +574,7 @@ std::shared_ptr TypographerContextSkia::CreateGlyphAtlas( context.GetBackendType() == Context::BackendType::kOpenGLES) { blit_old_atlas = false; new_atlas = std::make_shared(type); + new_atlas->SetAtlasGeneration(last_atlas->GetAtlasGeneration() + 1); auto [update_glyphs, update_sizes] = CollectNewGlyphs(new_atlas, text_frames); @@ -580,6 +588,10 @@ std::shared_ptr TypographerContextSkia::CreateGlyphAtlas( height_adjustment = 0; atlas_context->UpdateRectPacker(nullptr); atlas_context->UpdateGlyphAtlas(new_atlas, {0, 0}, 0); + } else { + // If any part of the old atlas is reused the atlas generation can be + // treated the same as existing glyphs will not be moved. + new_atlas->SetAtlasGeneration(last_atlas->GetAtlasGeneration()); } // A new glyph atlas must be created. diff --git a/impeller/typographer/glyph_atlas.cc b/impeller/typographer/glyph_atlas.cc index 093593804ea5f..a1a6863195219 100644 --- a/impeller/typographer/glyph_atlas.cc +++ b/impeller/typographer/glyph_atlas.cc @@ -65,6 +65,14 @@ void GlyphAtlas::SetTexture(std::shared_ptr texture) { texture_ = std::move(texture); } +size_t GlyphAtlas::GetAtlasGeneration() const { + return generation_; +} + +void GlyphAtlas::SetAtlasGeneration(size_t value) { + generation_ = value; +} + void GlyphAtlas::AddTypefaceGlyphPositionAndBounds(const FontGlyphPair& pair, Rect position, Rect bounds) { diff --git a/impeller/typographer/glyph_atlas.h b/impeller/typographer/glyph_atlas.h index 87c5490318387..3017d12c415c7 100644 --- a/impeller/typographer/glyph_atlas.h +++ b/impeller/typographer/glyph_atlas.h @@ -142,9 +142,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 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/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc index 6367314d9b397..5023a3682a7b2 100644 --- a/impeller/typographer/typographer_unittests.cc +++ b/impeller/typographer/typographer_unittests.cc @@ -525,6 +525,58 @@ TEST_P(TypographerTest, TextFrameInitialBoundsArePlaceholder) { EXPECT_FALSE(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, 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, 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, 1.0f, atlas_context, + frame); + + EXPECT_EQ(frame->GetAtlasGeneration(), 2u); +} + } // namespace testing } // namespace impeller From 3c61e53bd62b4e1bac846f5f274954ff10055781 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 25 Nov 2024 14:05:17 -0800 Subject: [PATCH 2/6] remove TODO that I did. --- .../typographer/backends/skia/typographer_context_skia.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/impeller/typographer/backends/skia/typographer_context_skia.cc b/impeller/typographer/backends/skia/typographer_context_skia.cc index d9370ec6db126..9a77d3b2b7158 100644 --- a/impeller/typographer/backends/skia/typographer_context_skia.cc +++ b/impeller/typographer/backends/skia/typographer_context_skia.cc @@ -415,10 +415,6 @@ TypographerContextSkia::CollectNewGlyphs( 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) { From 02d97be628d3c0b1733efe78fb6b3b418e129520 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 25 Nov 2024 14:36:12 -0800 Subject: [PATCH 3/6] correct scale invalidation. --- impeller/display_list/dl_dispatcher.cc | 5 +-- impeller/entity/contents/text_contents.cc | 7 ++-- .../backends/skia/typographer_context_skia.cc | 3 +- impeller/typographer/font_glyph_pair.h | 19 ++++++---- impeller/typographer/text_frame.cc | 22 +++++++++++- impeller/typographer/text_frame.h | 2 +- impeller/typographer/typographer_unittests.cc | 35 +++++++++++++++++++ 7 files changed, 77 insertions(+), 16 deletions(-) diff --git a/impeller/display_list/dl_dispatcher.cc b/impeller/display_list/dl_dispatcher.cc index 1bdad62aff584..b1ba5b7e35365 100644 --- a/impeller/display_list/dl_dispatcher.cc +++ b/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/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index dfa4c42198a94..4d4a130480157 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/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/impeller/typographer/backends/skia/typographer_context_skia.cc b/impeller/typographer/backends/skia/typographer_context_skia.cc index 9a77d3b2b7158..dbae05c7fc124 100644 --- a/impeller/typographer/backends/skia/typographer_context_skia.cc +++ b/impeller/typographer/backends/skia/typographer_context_skia.cc @@ -426,8 +426,7 @@ TypographerContextSkia::CollectNewGlyphs( 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 = diff --git a/impeller/typographer/font_glyph_pair.h b/impeller/typographer/font_glyph_pair.h index e530f5b3edcda..e3df2dae50c11 100644 --- a/impeller/typographer/font_glyph_pair.h +++ b/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/impeller/typographer/text_frame.cc b/impeller/typographer/text_frame.cc index 98db25151e261..3535fb207f2b0 100644 --- a/impeller/typographer/text_frame.cc +++ b/impeller/typographer/text_frame.cc @@ -3,11 +3,25 @@ // found in the LICENSE file. #include "impeller/typographer/text_frame.h" +#include "impeller/geometry/scalar.h" #include "impeller/typographer/font.h" #include "impeller/typographer/font_glyph_pair.h" 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) @@ -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; diff --git a/impeller/typographer/text_frame.h b/impeller/typographer/text_frame.h index d2a1a0309611c..067c8ebd05486 100644 --- a/impeller/typographer/text_frame.h +++ b/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. diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc index 5023a3682a7b2..4cc71643553ba 100644 --- a/impeller/typographer/typographer_unittests.cc +++ b/impeller/typographer/typographer_unittests.cc @@ -525,6 +525,41 @@ TEST_P(TypographerTest, TextFrameInitialBoundsArePlaceholder) { 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, 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, 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( From 9577eb11ba279157a31140ff5c76e8c40aa70e15 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 25 Nov 2024 15:02:57 -0800 Subject: [PATCH 4/6] ++ --- impeller/entity/entity_unittests.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 8fee465fabe9c..756203ab2f04a 100644 --- a/impeller/entity/entity_unittests.cc +++ b/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) { From b1a1670e0d4659e2e855658d985de08038e60e96 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 26 Nov 2024 09:47:34 -0800 Subject: [PATCH 5/6] aaron feedback. --- .../backends/skia/typographer_context_skia.cc | 4 ---- impeller/typographer/typographer_unittests.cc | 22 +++++++++---------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/impeller/typographer/backends/skia/typographer_context_skia.cc b/impeller/typographer/backends/skia/typographer_context_skia.cc index dbae05c7fc124..7c466e43a1e59 100644 --- a/impeller/typographer/backends/skia/typographer_context_skia.cc +++ b/impeller/typographer/backends/skia/typographer_context_skia.cc @@ -583,10 +583,6 @@ std::shared_ptr TypographerContextSkia::CreateGlyphAtlas( height_adjustment = 0; atlas_context->UpdateRectPacker(nullptr); atlas_context->UpdateGlyphAtlas(new_atlas, {0, 0}, 0); - } else { - // If any part of the old atlas is reused the atlas generation can be - // treated the same as existing glyphs will not be moved. - new_atlas->SetAtlasGeneration(last_atlas->GetAtlasGeneration()); } // A new glyph atlas must be created. diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc index 4cc71643553ba..4b8cc1cbc9606 100644 --- a/impeller/typographer/typographer_unittests.cc +++ b/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,8 +517,8 @@ 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()); @@ -541,7 +541,7 @@ TEST_P(TypographerTest, TextFrameInvalidationWithScale) { 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 @@ -552,8 +552,8 @@ TEST_P(TypographerTest, TextFrameInvalidationWithScale) { // 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, 2.0f, atlas_context, - frame); + 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()); @@ -576,7 +576,7 @@ TEST_P(TypographerTest, TextFrameAtlasGenerationTracksState) { 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 @@ -591,8 +591,8 @@ TEST_P(TypographerTest, TextFrameAtlasGenerationTracksState) { } 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()); @@ -606,8 +606,8 @@ TEST_P(TypographerTest, TextFrameAtlasGenerationTracksState) { // Force increase the generation. atlas_context->GetGlyphAtlas()->SetAtlasGeneration(2u); atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, - GlyphAtlas::Type::kAlphaBitmap, 1.0f, atlas_context, - frame); + GlyphAtlas::Type::kAlphaBitmap, /*scale=*/1.0f, + atlas_context, frame); EXPECT_EQ(frame->GetAtlasGeneration(), 2u); } From 7e93ae6ebf9b936264d6fd54faef5e90bdf39772 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 26 Nov 2024 12:55:01 -0800 Subject: [PATCH 6/6] Add generation id to constructor. --- .../backends/skia/typographer_context_skia.cc | 4 ++-- impeller/typographer/glyph_atlas.cc | 10 ++++++---- impeller/typographer/glyph_atlas.h | 3 ++- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/impeller/typographer/backends/skia/typographer_context_skia.cc b/impeller/typographer/backends/skia/typographer_context_skia.cc index 7c466e43a1e59..13dc254e67dce 100644 --- a/impeller/typographer/backends/skia/typographer_context_skia.cc +++ b/impeller/typographer/backends/skia/typographer_context_skia.cc @@ -568,8 +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->SetAtlasGeneration(last_atlas->GetAtlasGeneration() + 1); + 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/impeller/typographer/glyph_atlas.cc b/impeller/typographer/glyph_atlas.cc index a1a6863195219..98038668fb501 100644 --- a/impeller/typographer/glyph_atlas.cc +++ b/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; @@ -69,8 +71,8 @@ size_t GlyphAtlas::GetAtlasGeneration() const { return generation_; } -void GlyphAtlas::SetAtlasGeneration(size_t value) { - generation_ = value; +void GlyphAtlas::SetAtlasGeneration(size_t generation) { + generation_ = generation; } void GlyphAtlas::AddTypefaceGlyphPositionAndBounds(const FontGlyphPair& pair, diff --git a/impeller/typographer/glyph_atlas.h b/impeller/typographer/glyph_atlas.h index 3017d12c415c7..bf11ff056ae24 100644 --- a/impeller/typographer/glyph_atlas.h +++ b/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();