From 71ac81f9b881a7865e3fdc54bb77912b37ea116c Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Fri, 30 Aug 2024 16:05:53 -0700 Subject: [PATCH 1/2] [Impeller] Add all requested glyphs if TypographerContextSkia needs to create a new atlas TypographerContextSkia::CreateGlyphAtlas calculates the set of requested glyphs that are not already in the current atlas and then tries to add those glyphs to that atlas. But if that is not possible and a new atlas must be allocated, then CreateGlyphAtlas must populate the new atlas with every glyph. Fixes https://github.com/flutter/flutter/issues/153392 --- .../backends/skia/typographer_context_skia.cc | 67 +++++++++++-------- impeller/typographer/typographer_unittests.cc | 20 +++++- 2 files changed, 59 insertions(+), 28 deletions(-) diff --git a/impeller/typographer/backends/skia/typographer_context_skia.cc b/impeller/typographer/backends/skia/typographer_context_skia.cc index 2d143f4a23129..653377315c5c4 100644 --- a/impeller/typographer/backends/skia/typographer_context_skia.cc +++ b/impeller/typographer/backends/skia/typographer_context_skia.cc @@ -403,34 +403,14 @@ static Rect ComputeGlyphSize(const SkFont& font, scaled_bounds.fBottom); }; -std::shared_ptr TypographerContextSkia::CreateGlyphAtlas( - Context& context, - GlyphAtlas::Type type, - HostBuffer& host_buffer, - const std::shared_ptr& atlas_context, - const FontGlyphMap& font_glyph_map) const { - TRACE_EVENT0("impeller", __FUNCTION__); - if (!IsValid()) { - return nullptr; - } - std::shared_ptr last_atlas = atlas_context->GetGlyphAtlas(); - FML_DCHECK(last_atlas->GetType() == type); - - if (font_glyph_map.empty()) { - return last_atlas; - } - - // --------------------------------------------------------------------------- - // Step 1: Determine if the atlas type and font glyph pairs are compatible - // with the current atlas and reuse if possible. For each new font and - // glyph pair, compute the glyph size at scale. - // --------------------------------------------------------------------------- - std::vector glyph_sizes; - std::vector new_glyphs; +static void CollectNewGlyphs(const std::shared_ptr& atlas, + const FontGlyphMap& font_glyph_map, + std::vector& new_glyphs, + std::vector& glyph_sizes) { for (const auto& font_value : font_glyph_map) { const ScaledFont& scaled_font = font_value.first; const FontGlyphAtlas* font_glyph_atlas = - last_atlas->GetFontGlyphAtlas(scaled_font.font, scaled_font.scale); + atlas->GetFontGlyphAtlas(scaled_font.font, scaled_font.scale); auto metrics = scaled_font.font.GetMetrics(); @@ -462,6 +442,33 @@ std::shared_ptr TypographerContextSkia::CreateGlyphAtlas( } } } +} + +std::shared_ptr TypographerContextSkia::CreateGlyphAtlas( + Context& context, + GlyphAtlas::Type type, + HostBuffer& host_buffer, + const std::shared_ptr& atlas_context, + const FontGlyphMap& font_glyph_map) const { + TRACE_EVENT0("impeller", __FUNCTION__); + if (!IsValid()) { + return nullptr; + } + std::shared_ptr last_atlas = atlas_context->GetGlyphAtlas(); + FML_DCHECK(last_atlas->GetType() == type); + + if (font_glyph_map.empty()) { + return last_atlas; + } + + // --------------------------------------------------------------------------- + // Step 1: Determine if the atlas type and font glyph pairs are compatible + // with the current atlas and reuse if possible. For each new font and + // glyph pair, compute the glyph size at scale. + // --------------------------------------------------------------------------- + std::vector new_glyphs; + std::vector glyph_sizes; + CollectNewGlyphs(last_atlas, font_glyph_map, new_glyphs, glyph_sizes); if (new_glyphs.size() == 0) { return last_atlas; } @@ -528,10 +535,16 @@ std::shared_ptr TypographerContextSkia::CreateGlyphAtlas( if (atlas_context->GetAtlasSize().height >= max_texture_height || context.GetBackendType() == Context::BackendType::kOpenGLES) { blit_old_atlas = false; - first_missing_index = 0; + new_atlas = std::make_shared(type); + + new_glyphs.clear(); + glyph_sizes.clear(); + CollectNewGlyphs(new_atlas, font_glyph_map, new_glyphs, glyph_sizes); glyph_positions.clear(); + glyph_positions.reserve(new_glyphs.size()); + first_missing_index = 0; + height_adjustment = 0; - new_atlas = std::make_shared(type); atlas_context->UpdateRectPacker(nullptr); atlas_context->UpdateGlyphAtlas(new_atlas, {0, 0}, 0); } diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc index 928cba18f0c7e..509f94c8bda4e 100644 --- a/impeller/typographer/typographer_unittests.cc +++ b/impeller/typographer/typographer_unittests.cc @@ -453,9 +453,22 @@ TEST_P(TypographerTest, GlyphAtlasTextureWillGrowTilMaxTextureSize) { {4096, 4096} // Shrinks! }; + SkFont sk_font_small = flutter::testing::CreateTestFontOfSize(10); + for (int i = 0; i < 13; i++) { + SkTextBlobBuilder builder; + + auto add_char = [&](SkFont sk_font, char c) { + int count = sk_font.countText(&c, 1, SkTextEncoding::kUTF8); + auto buffer = builder.allocRunPos(sk_font, count); + sk_font.textToGlyphs(&c, 1, SkTextEncoding::kUTF8, buffer.glyphs, count); + sk_font.getPos(buffer.glyphs, count, buffer.points(), {0, 0}); + }; + SkFont sk_font = flutter::testing::CreateTestFontOfSize(50 + i); - auto blob = SkTextBlob::MakeFromString("A", sk_font); + add_char(sk_font, 'A'); + add_char(sk_font_small, 'B'); + auto blob = builder.make(); atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, @@ -465,6 +478,11 @@ TEST_P(TypographerTest, GlyphAtlasTextureWillGrowTilMaxTextureSize) { EXPECT_EQ(atlas->GetTexture()->GetTextureDescriptor().size, expected_sizes[i]); } + + // The final atlas should contain both the "A" glyph (which was not present + // in the previous atlas) and the "B" glyph (which existed in the previous + // atlas). + ASSERT_EQ(atlas->GetGlyphCount(), 2u); } } // namespace testing From f050045d8d0c97005f5cf2b4b3df6d8cc0cd3c9b Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Tue, 3 Sep 2024 12:12:44 -0700 Subject: [PATCH 2/2] clang_tidy --- impeller/typographer/typographer_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc index 509f94c8bda4e..7c8e9cd08852c 100644 --- a/impeller/typographer/typographer_unittests.cc +++ b/impeller/typographer/typographer_unittests.cc @@ -458,7 +458,7 @@ TEST_P(TypographerTest, GlyphAtlasTextureWillGrowTilMaxTextureSize) { for (int i = 0; i < 13; i++) { SkTextBlobBuilder builder; - auto add_char = [&](SkFont sk_font, char c) { + auto add_char = [&](const SkFont& sk_font, char c) { int count = sk_font.countText(&c, 1, SkTextEncoding::kUTF8); auto buffer = builder.allocRunPos(sk_font, count); sk_font.textToGlyphs(&c, 1, SkTextEncoding::kUTF8, buffer.glyphs, count);