From d7752e30c532c45fd6e0fc1e133d3300a6c09a0c Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Tue, 18 Apr 2023 13:07:58 -0700 Subject: [PATCH] [Impeller] faster glyph atlas generation by removing data copies (flutter/engine#41290) fixes https://github.com/flutter/flutter/issues/124996 This is the best I could come up with easily. It doesn't give as good of a result as I'd hoped. I measured it with a simple microbenchmark derived from one of the unit tests and it gave a 6% decrease in time ( 241.314us vs 257.626us) on macos release builds (run with rosetta). Improvements: 1) Removed the copying of the std::set to an std::vector 1) Uses references instead of copying FontGlyphPairs out of collections in a few places 1) Holds new glyphs as a vector of references to the set instead of copying all the FontGlyphPairs 1) Deletes more lines of code than it adds ## the benchmark ```diff diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc index 01a11d494c..1b99afa699 100644 --- a/impeller/typographer/typographer_unittests.cc +++ b/impeller/typographer/typographer_unittests.cc @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include #include "flutter/testing/testing.h" #include "impeller/playground/playground_test.h" #include "impeller/typographer/backends/skia/text_frame_skia.h" @@ -149,23 +150,29 @@ TEST_P(TypographerTest, GlyphAtlasWithLotsOfdUniqueGlyphSize) { sk_font); ASSERT_TRUE(blob); - TextFrame frame; - size_t count = 0; - TextRenderContext::FrameIterator iterator = [&]() -> const TextFrame* { - if (count < 8) { - count++; - frame = TextFrameFromTextBlob(blob, 0.6 * count); - return &frame; - } - return nullptr; - }; - auto atlas = context->CreateGlyphAtlas(GlyphAtlas::Type::kAlphaBitmap, - atlas_context, iterator); - ASSERT_NE(atlas, nullptr); - ASSERT_NE(atlas->GetTexture(), nullptr); - - ASSERT_EQ(atlas->GetTexture()->GetSize().width * 2, - atlas->GetTexture()->GetSize().height); + auto beg = std::chrono::high_resolution_clock::now(); + int count = 10000; + for (int i = 0; i < count; ++i) { + TextFrame frame; + size_t count = 0; + TextRenderContext::FrameIterator iterator = [&]() -> const TextFrame* { + if (count < 8) { + count++; + frame = TextFrameFromTextBlob(blob, 0.6 * count); + return &frame; + } + return nullptr; + }; + auto atlas = context->CreateGlyphAtlas(GlyphAtlas::Type::kAlphaBitmap, + atlas_context, iterator); + ASSERT_NE(atlas, nullptr); + ASSERT_NE(atlas->GetTexture(), nullptr); + ASSERT_EQ(atlas->GetTexture()->GetSize().width * 2, + atlas->GetTexture()->GetSize().height); + } + auto end = std::chrono::high_resolution_clock::now(); + auto duration = std::chrono::duration_cast(end - beg); + FML_LOG(ERROR) << "Elapsed Time: " << static_cast(duration.count())/count << "us"; } ``` [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../impeller/entity/contents/text_contents.cc | 2 +- .../backends/skia/text_render_context_skia.cc | 70 ++++++++++--------- .../impeller/typographer/glyph_atlas.cc | 13 +--- .../impeller/typographer/glyph_atlas.h | 11 --- .../typographer/typographer_unittests.cc | 16 ++++- 5 files changed, 54 insertions(+), 58 deletions(-) diff --git a/engine/src/flutter/impeller/entity/contents/text_contents.cc b/engine/src/flutter/impeller/entity/contents/text_contents.cc index d3071702728..6a4f0be4753 100644 --- a/engine/src/flutter/impeller/entity/contents/text_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/text_contents.cc @@ -157,7 +157,7 @@ static bool CommonRender( Vector2 screen_offset = (entity.GetTransformation() * offset).Round(); for (const auto& run : frame.GetRuns()) { - auto font = run.GetFont(); + const Font& font = run.GetFont(); for (const auto& glyph_position : run.GetGlyphPositions()) { FontGlyphPair font_glyph_pair{font, glyph_position.glyph}; diff --git a/engine/src/flutter/impeller/typographer/backends/skia/text_render_context_skia.cc b/engine/src/flutter/impeller/typographer/backends/skia/text_render_context_skia.cc index 313a7d8b456..51ec87b712d 100644 --- a/engine/src/flutter/impeller/typographer/backends/skia/text_render_context_skia.cc +++ b/engine/src/flutter/impeller/typographer/backends/skia/text_render_context_skia.cc @@ -22,6 +22,9 @@ namespace impeller { +using FontGlyphPairRefVector = + std::vector>; + std::unique_ptr TextRenderContext::Create( std::shared_ptr context) { // There is only one backend today. @@ -38,17 +41,19 @@ TextRenderContextSkia::TextRenderContextSkia(std::shared_ptr context) TextRenderContextSkia::~TextRenderContextSkia() = default; -static FontGlyphPair::Set CollectUniqueFontGlyphPairsSet( +static FontGlyphPair::Set CollectUniqueFontGlyphPairs( GlyphAtlas::Type type, const TextRenderContext::FrameIterator& frame_iterator) { + TRACE_EVENT0("impeller", __FUNCTION__); FontGlyphPair::Set set; - while (auto frame = frame_iterator()) { - for (const auto& run : frame->GetRuns()) { - auto font = run.GetFont(); + while (const TextFrame* frame = frame_iterator()) { + for (const TextRun& run : frame->GetRuns()) { + const Font& font = run.GetFont(); // TODO(dnfield): If we're doing SDF here, we should be using a consistent // point size. // https://github.com/flutter/flutter/issues/112016 - for (const auto& glyph_position : run.GetGlyphPositions()) { + for (const TextRun::GlyphPosition& glyph_position : + run.GetGlyphPositions()) { set.insert({font, glyph_position.glyph}); } } @@ -56,21 +61,8 @@ static FontGlyphPair::Set CollectUniqueFontGlyphPairsSet( return set; } -static FontGlyphPair::Vector CollectUniqueFontGlyphPairs( - GlyphAtlas::Type type, - const TextRenderContext::FrameIterator& frame_iterator) { - TRACE_EVENT0("impeller", __FUNCTION__); - FontGlyphPair::Vector vector; - auto set = CollectUniqueFontGlyphPairsSet(type, frame_iterator); - vector.reserve(set.size()); - for (const auto& item : set) { - vector.emplace_back(item); - } - return vector; -} - static size_t PairsFitInAtlasOfSize( - const FontGlyphPair::Vector& pairs, + const FontGlyphPair::Set& pairs, const ISize& atlas_size, std::vector& glyph_positions, const std::shared_ptr& rect_packer) { @@ -81,8 +73,9 @@ static size_t PairsFitInAtlasOfSize( glyph_positions.clear(); glyph_positions.reserve(pairs.size()); - for (size_t i = 0; i < pairs.size(); i++) { - const auto& pair = pairs[i]; + size_t i = 0; + for (auto it = pairs.begin(); it != pairs.end(); ++i, ++it) { + const auto& pair = *it; const auto glyph_size = ISize::Ceil((pair.glyph.bounds * pair.font.GetMetrics().scale).size); @@ -105,7 +98,7 @@ static size_t PairsFitInAtlasOfSize( static bool CanAppendToExistingAtlas( const std::shared_ptr& atlas, - const FontGlyphPair::Vector& extra_pairs, + const FontGlyphPairRefVector& extra_pairs, std::vector& glyph_positions, ISize atlas_size, const std::shared_ptr& rect_packer) { @@ -120,7 +113,7 @@ static bool CanAppendToExistingAtlas( FML_DCHECK(glyph_positions.size() == 0); glyph_positions.reserve(extra_pairs.size()); for (size_t i = 0; i < extra_pairs.size(); i++) { - const auto& pair = extra_pairs[i]; + const FontGlyphPair& pair = extra_pairs[i]; const auto glyph_size = ISize::Ceil((pair.glyph.bounds * pair.font.GetMetrics().scale).size); @@ -141,8 +134,9 @@ static bool CanAppendToExistingAtlas( return true; } -static ISize OptimumAtlasSizeForFontGlyphPairs( - const FontGlyphPair::Vector& pairs, +namespace { +ISize OptimumAtlasSizeForFontGlyphPairs( + const FontGlyphPair::Set& pairs, std::vector& glyph_positions, const std::shared_ptr& atlas_context) { static constexpr auto kMinAtlasSize = 8u; @@ -175,6 +169,7 @@ static ISize OptimumAtlasSizeForFontGlyphPairs( current_size.height <= kMaxAtlasSize); return ISize{0, 0}; } +} // namespace /// Compute signed-distance field for an 8-bpp grayscale image (values greater /// than 127 are considered "on") For details of this algorithm, see "The 'dead @@ -326,7 +321,7 @@ static void DrawGlyph(SkCanvas* canvas, static bool UpdateAtlasBitmap(const GlyphAtlas& atlas, const std::shared_ptr& bitmap, - const FontGlyphPair::Vector& new_pairs) { + const FontGlyphPairRefVector& new_pairs) { TRACE_EVENT0("impeller", __FUNCTION__); FML_DCHECK(bitmap != nullptr); @@ -341,7 +336,7 @@ static bool UpdateAtlasBitmap(const GlyphAtlas& atlas, bool has_color = atlas.GetType() == GlyphAtlas::Type::kColorBitmap; - for (const auto& pair : new_pairs) { + for (const FontGlyphPair& pair : new_pairs) { auto pos = atlas.FindFontGlyphBounds(pair); if (!pos.has_value()) { continue; @@ -457,13 +452,14 @@ std::shared_ptr TextRenderContextSkia::CreateGlyphAtlas( if (!IsValid()) { return nullptr; } - auto last_atlas = atlas_context->GetGlyphAtlas(); + std::shared_ptr last_atlas = atlas_context->GetGlyphAtlas(); // --------------------------------------------------------------------------- // Step 1: Collect unique font-glyph pairs in the frame. // --------------------------------------------------------------------------- - auto font_glyph_pairs = CollectUniqueFontGlyphPairs(type, frame_iterator); + FontGlyphPair::Set font_glyph_pairs = + CollectUniqueFontGlyphPairs(type, frame_iterator); if (font_glyph_pairs.empty()) { return last_atlas; } @@ -472,7 +468,12 @@ std::shared_ptr TextRenderContextSkia::CreateGlyphAtlas( // Step 2: Determine if the atlas type and font glyph pairs are compatible // with the current atlas and reuse if possible. // --------------------------------------------------------------------------- - auto new_glyphs = last_atlas->HasSamePairs(font_glyph_pairs); + FontGlyphPairRefVector new_glyphs; + for (const FontGlyphPair& pair : font_glyph_pairs) { + if (!last_atlas->FindFontGlyphBounds(pair).has_value()) { + new_glyphs.push_back(pair); + } + } if (last_atlas->GetType() == type && new_glyphs.size() == 0) { return last_atlas; } @@ -540,9 +541,12 @@ std::shared_ptr TextRenderContextSkia::CreateGlyphAtlas( // --------------------------------------------------------------------------- // Step 6: Record the positions in the glyph atlas. // --------------------------------------------------------------------------- - for (size_t i = 0, count = glyph_positions.size(); i < count; i++) { - glyph_atlas->AddTypefaceGlyphPosition(font_glyph_pairs[i], - glyph_positions[i]); + { + size_t i = 0; + for (auto it = font_glyph_pairs.begin(); it != font_glyph_pairs.end(); + ++i, ++it) { + glyph_atlas->AddTypefaceGlyphPosition(*it, glyph_positions[i]); + } } // --------------------------------------------------------------------------- diff --git a/engine/src/flutter/impeller/typographer/glyph_atlas.cc b/engine/src/flutter/impeller/typographer/glyph_atlas.cc index 5ad577322e9..6908b9eaa20 100644 --- a/engine/src/flutter/impeller/typographer/glyph_atlas.cc +++ b/engine/src/flutter/impeller/typographer/glyph_atlas.cc @@ -72,7 +72,7 @@ void GlyphAtlas::AddTypefaceGlyphPosition(const FontGlyphPair& pair, std::optional GlyphAtlas::FindFontGlyphBounds( const FontGlyphPair& pair) const { - auto found = positions_.find(pair); + const auto& found = positions_.find(pair); if (found == positions_.end()) { return std::nullopt; } @@ -100,15 +100,4 @@ size_t GlyphAtlas::IterateGlyphs( return count; } -FontGlyphPair::Vector GlyphAtlas::HasSamePairs( - const FontGlyphPair::Vector& new_glyphs) { - std::vector new_pairs; - for (auto pair : new_glyphs) { - if (positions_.find(pair) == positions_.end()) { - new_pairs.push_back(pair); - } - } - return new_pairs; -} - } // namespace impeller diff --git a/engine/src/flutter/impeller/typographer/glyph_atlas.h b/engine/src/flutter/impeller/typographer/glyph_atlas.h index d12cc44daf6..e7282a0cdb6 100644 --- a/engine/src/flutter/impeller/typographer/glyph_atlas.h +++ b/engine/src/flutter/impeller/typographer/glyph_atlas.h @@ -121,17 +121,6 @@ class GlyphAtlas { /// std::optional FindFontGlyphBounds(const FontGlyphPair& pair) const; - //---------------------------------------------------------------------------- - /// @brief whether this atlas contains all of the same font-glyph pairs - /// as the vector. - /// - /// @param[in] new_glyphs The full set of new glyphs - /// - /// @return A vector containing the glyphs from new_glyphs that are not - /// present in the existing atlas. May be empty of there are none. - /// - FontGlyphPair::Vector HasSamePairs(const FontGlyphPair::Vector& new_glyphs); - private: const Type type_; std::shared_ptr texture_; diff --git a/engine/src/flutter/impeller/typographer/typographer_unittests.cc b/engine/src/flutter/impeller/typographer/typographer_unittests.cc index 01a11d494c8..445cc283fe6 100644 --- a/engine/src/flutter/impeller/typographer/typographer_unittests.cc +++ b/engine/src/flutter/impeller/typographer/typographer_unittests.cc @@ -45,7 +45,21 @@ TEST_P(TypographerTest, CanCreateGlyphAtlas) { context->CreateGlyphAtlas(GlyphAtlas::Type::kAlphaBitmap, atlas_context, TextFrameFromTextBlob(blob)); ASSERT_NE(atlas, nullptr); - OpenPlaygroundHere([](RenderTarget&) { return true; }); + ASSERT_NE(atlas->GetTexture(), nullptr); + ASSERT_EQ(atlas->GetType(), GlyphAtlas::Type::kAlphaBitmap); + ASSERT_EQ(atlas->GetGlyphCount(), 4llu); + + std::optional first_pair; + Rect first_rect; + atlas->IterateGlyphs( + [&](const FontGlyphPair& pair, const Rect& rect) -> bool { + first_pair = pair; + first_rect = rect; + return false; + }); + + ASSERT_TRUE(first_pair.has_value()); + ASSERT_TRUE(atlas->FindFontGlyphBounds(first_pair.value()).has_value()); } static sk_sp OpenFixtureAsSkData(const char* fixture_name) {