[Impeller] Reorganize the glyph atlas to improve efficiency when looking up runs of glyphs in the same font #45191
Conversation
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Hey @jason-simmons, were you able to get positive improvement in the macro benchmarks?
|
Building this now to take a look. |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Logic looks good. In most places I think we can clearly see this is faster. I called out 2 places where it might not be (one of them I think we can actually remove). It would be nice to have a measurement for this =)
| struct std::equal_to<impeller::ScaledFont> { | ||
| constexpr bool operator()(const impeller::ScaledFont& lhs, | ||
| const impeller::ScaledFont& rhs) const { | ||
| return lhs.font.IsEqual(rhs.font) && lhs.scale == rhs.scale; |
There was a problem hiding this comment.
No need to change this, but I'm just realizing that hashing on a floating value might finicky.
There was a problem hiding this comment.
We round the scale to two decimal places elsewhere so it should be okay. This definitely caused a problem before if there was animated text
| std::optional<Rect> GlyphAtlas::FindFontGlyphBounds( | ||
| const FontGlyphPair& pair) const { | ||
| const auto& found = positions_.find(pair); | ||
| if (found == positions_.end()) { | ||
| const auto& found = font_atlas_map_.find(pair.scaled_font); | ||
| if (found == font_atlas_map_.end()) { | ||
| return std::nullopt; | ||
| } | ||
| return found->second; | ||
| return found->second.FindGlyphBounds(pair.glyph); | ||
| } |
There was a problem hiding this comment.
This is one place where we are replacing one lookup with 2, so FindFontGlyphBounds may be slower. This is where having some benchmark improvement would be nice. Are we even using this anymore?
There was a problem hiding this comment.
This is called when adding new glyphs to the atlas.
The overall intent of the PR is to make rendering faster when no new glyphs are needed in the atlas. The additional overhead when new glyphs are added should not be significant.
| } | ||
|
|
||
| void TextFrame::CollectUniqueFontGlyphPairs(FontGlyphPair::Set& set, | ||
| void TextFrame::CollectUniqueFontGlyphPairs(FontGlyphMap& glyph_map, |
There was a problem hiding this comment.
It's hard to know if this is going to be faster. There are multiple inserts happening now.
|
The main goal here is minimizing the cost of the common case where no new glyphs need to be added to the atlas. I tested this by scrolling repeatedly through one of the license texts in the Gallery license viewer screen on a Pixel 3. Without this patch the Android profiler reported that With this patch |
…ing up runs of glyphs in the same font The atlas will now store a separate map of glyph positions for each font/scale pair. This avoids the cost of repeated hashing and copying of font objects for each glyph in a text run. See flutter/flutter#133201
jason-simmons
force-pushed
the
bug_133201_c
branch
from
2d506ff to
19a3660
Compare
jason-simmons
added
the
autosubmit
… when looking up runs of glyphs in the same font (flutter/engine#45191)
…133692) flutter/engine@31d5662...b63eee2 2023-08-30 [email protected] [Impeller] Reorganize the glyph atlas to improve efficiency when looking up runs of glyphs in the same font (flutter/engine#45191) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
3 participants
The atlas will now store a separate map of glyph positions for each font/scale pair. This avoids the cost of repeated hashing and copying of font objects for each glyph in a text run.
See flutter/flutter#133201