Skip to content

Commit

Permalink
[Impeller] faster glyph atlas generation by removing data copies (flu…
Browse files Browse the repository at this point in the history 
…tter#41290)

fixes flutter/flutter#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 <chrono>
 #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<std::chrono::microseconds>(end - beg);
+  FML_LOG(ERROR) << "Elapsed Time: " << static_cast<double>(duration.count())/count << "us";
 }

```

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
  • Loading branch information
@gaaclarke
gaaclarke authored Apr 18, 2023
1 parent c42fbb1 commit b71dd7e
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 58 deletions.
2 changes: 1 addition & 1 deletion impeller/entity/contents/text_contents.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
70 changes: 37 additions & 33 deletions impeller/typographer/backends/skia/text_render_context_skia.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@

namespace impeller {

using FontGlyphPairRefVector =
std::vector<std::reference_wrapper<const FontGlyphPair>>;

std::unique_ptr<TextRenderContext> TextRenderContext::Create(
std::shared_ptr<Context> context) {
// There is only one backend today.
Expand All @@ -38,39 +41,28 @@ TextRenderContextSkia::TextRenderContextSkia(std::shared_ptr<Context> 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});
}
}
}
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<Rect>& glyph_positions,
const std::shared_ptr<GrRectanizer>& rect_packer) {
Expand All @@ -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);
Expand All @@ -105,7 +98,7 @@ static size_t PairsFitInAtlasOfSize(

static bool CanAppendToExistingAtlas(
const std::shared_ptr<GlyphAtlas>& atlas,
const FontGlyphPair::Vector& extra_pairs,
const FontGlyphPairRefVector& extra_pairs,
std::vector<Rect>& glyph_positions,
ISize atlas_size,
const std::shared_ptr<GrRectanizer>& rect_packer) {
Expand All @@ -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);
Expand All @@ -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<Rect>& glyph_positions,
const std::shared_ptr<GlyphAtlasContext>& atlas_context) {
static constexpr auto kMinAtlasSize = 8u;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -326,7 +321,7 @@ static void DrawGlyph(SkCanvas* canvas,

static bool UpdateAtlasBitmap(const GlyphAtlas& atlas,
const std::shared_ptr<SkBitmap>& bitmap,
const FontGlyphPair::Vector& new_pairs) {
const FontGlyphPairRefVector& new_pairs) {
TRACE_EVENT0("impeller", __FUNCTION__);
FML_DCHECK(bitmap != nullptr);

Expand All @@ -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;
Expand Down Expand Up @@ -457,13 +452,14 @@ std::shared_ptr<GlyphAtlas> TextRenderContextSkia::CreateGlyphAtlas(
if (!IsValid()) {
return nullptr;
}
auto last_atlas = atlas_context->GetGlyphAtlas();
std::shared_ptr<GlyphAtlas> 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;
}
Expand All @@ -472,7 +468,12 @@ std::shared_ptr<GlyphAtlas> 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;
}
Expand Down Expand Up @@ -540,9 +541,12 @@ std::shared_ptr<GlyphAtlas> 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]);
}
}

// ---------------------------------------------------------------------------
Expand Down
13 changes: 1 addition & 12 deletions impeller/typographer/glyph_atlas.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ void GlyphAtlas::AddTypefaceGlyphPosition(const FontGlyphPair& pair,

std::optional<Rect> GlyphAtlas::FindFontGlyphBounds(
const FontGlyphPair& pair) const {
auto found = positions_.find(pair);
const auto& found = positions_.find(pair);
if (found == positions_.end()) {
return std::nullopt;
}
Expand Down Expand Up @@ -100,15 +100,4 @@ size_t GlyphAtlas::IterateGlyphs(
return count;
}

FontGlyphPair::Vector GlyphAtlas::HasSamePairs(
const FontGlyphPair::Vector& new_glyphs) {
std::vector<FontGlyphPair> new_pairs;
for (auto pair : new_glyphs) {
if (positions_.find(pair) == positions_.end()) {
new_pairs.push_back(pair);
}
}
return new_pairs;
}

} // namespace impeller
11 changes: 0 additions & 11 deletions impeller/typographer/glyph_atlas.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,17 +121,6 @@ class GlyphAtlas {
///
std::optional<Rect> 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> texture_;
Expand Down
16 changes: 15 additions & 1 deletion impeller/typographer/typographer_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<FontGlyphPair> 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<SkData> OpenFixtureAsSkData(const char* fixture_name) {
Expand Down

0 comments on commit b71dd7e

Please sign in to comment.