[Impeller] Cache Skia text bounds computation. #45150
Conversation
|
Its easier for this to land after #45090 |
| case SkTextBlobRunIterator::kDefault_Positioning: | ||
| FML_DLOG(ERROR) << "Unimplemented."; | ||
| break; | ||
| case SkTextBlobRunIterator::kHorizontal_Positioning: | ||
| FML_DLOG(ERROR) << "Unimplemented."; | ||
| break; |
There was a problem hiding this comment.
Since we're never going to use these, I deleted them and left it for the default.
There was a problem hiding this comment.
I have a slight preference for removing the default so that we know if Skia adds some other value to this that we could be handling.
| font.getBounds(glyphs, glyph_count, glyph_bounds.data(), nullptr); | ||
|
|
||
| std::vector<TextRun::GlyphPosition> positions; | ||
| positions.reserve(glyph_count); |
There was a problem hiding this comment.
Allocate the right amount of storage once rather than appending.
There was a problem hiding this comment.
Put this as a comment in the code instead of in the PR review
There was a problem hiding this comment.
feels like
/// the int
int a = 0;
There was a problem hiding this comment.
🤷🏼. My point is if you think it's important enough to call out in the review, it's probably better as a comment :)
Defer to you, definitely a micro-nit.
|
|
||
| namespace impeller { | ||
|
|
||
| TextFrame::TextFrame() = default; |
There was a problem hiding this comment.
The default constructor is no glyphs, empty bounds, no color. Which should be perfectly safe.
| return font_; | ||
| } | ||
|
|
||
| bool TextRun::HasColor() const { |
There was a problem hiding this comment.
We don't need to track this per run.
| namespace impeller { | ||
|
|
||
| TextFrame MakeTextFrameFromTextBlobSkia(const sk_sp<SkTextBlob>& blob); | ||
| std::optional<TextFrame> MakeTextFrameFromTextBlobSkia( |
There was a problem hiding this comment.
I made this optional so that we can avoid creating TextContents at all if the textblob is nullptr for some reason.
There was a problem hiding this comment.
Ditto above, comment in the code :)
chinmaygarde
changed the title
| font.getBounds(glyphs, glyph_count, glyph_bounds.data(), nullptr); | ||
|
|
||
| std::vector<TextRun::GlyphPosition> positions; | ||
| positions.reserve(glyph_count); |
There was a problem hiding this comment.
Put this as a comment in the code instead of in the PR review
| namespace impeller { | ||
|
|
||
| TextFrame MakeTextFrameFromTextBlobSkia(const sk_sp<SkTextBlob>& blob); | ||
| std::optional<TextFrame> MakeTextFrameFromTextBlobSkia( |
There was a problem hiding this comment.
Ditto above, comment in the code :)
jonahwilliams
added
the
autosubmit
…133516) flutter/engine@a7a4c1c...bd2132a 2023-08-28 [email protected] Roll Skia from 335f748463db to 66e367b12e96 (1 revision) (flutter/engine#45189) 2023-08-28 [email protected] [Impeller] Cache Skia text bounds computation. (flutter/engine#45150) 2023-08-28 [email protected] Roll Skia from 83f6fbad8a38 to 335f748463db (1 revision) (flutter/engine#45186) 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
Fixes flutter/flutter#133388 Just use the TextBounds that Skia gives us. These are slightly larger but already computed. If the larger bounds are a problem we can go back to Impeller computed bounds, but using a cached computation. ### Skia  ### Impeller 
Fixes flutter/flutter#133388
Just use the TextBounds that Skia gives us. These are slightly larger but already computed. If the larger bounds are a problem we can go back to Impeller computed bounds, but using a cached computation.
Skia
Impeller