Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Cache Skia text bounds computation. #45150

Merged
merged 7 commits into from
Aug 28, 2023
Merged

[Impeller] Cache Skia text bounds computation. #45150

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c669dc2
[Impeller] cache Skia text bounds computation.
Aug 26, 2023
e7d4907
[Impeller] use Skia computed glyph bounds.
Aug 27, 2023
642fde4
++
Aug 27, 2023
bae1005
Merge branch 'main' of github.com:flutter/engine into cache_bounds
Aug 28, 2023
8b6a21a
Merge branch 'main' of github.com:flutter/engine into cache_bounds
Aug 28, 2023
273f9a0
Merge branch 'main' of github.com:flutter/engine into cache_bounds
Aug 28, 2023
dd50c18
++
Aug 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions impeller/aiks/aiks_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1278,7 +1278,11 @@ bool RenderTextInCanvasSkia(const std::shared_ptr<Context>& context,
}

// Create the Impeller text frame and draw it at the designated baseline.
auto frame = MakeTextFrameFromTextBlobSkia(blob);
auto maybe_frame = MakeTextFrameFromTextBlobSkia(blob);
if (!maybe_frame.has_value()) {
return false;
}
auto frame = maybe_frame.value();

Paint text_paint;
text_paint.color = Color::Yellow().WithAlpha(options.alpha);
Expand Down Expand Up @@ -1467,7 +1471,7 @@ TEST_P(AiksTest, CanRenderTextOutsideBoundaries) {
{
auto blob = SkTextBlob::MakeFromString(t.text, sk_font);
ASSERT_NE(blob, nullptr);
auto frame = MakeTextFrameFromTextBlobSkia(blob);
auto frame = MakeTextFrameFromTextBlobSkia(blob).value();
canvas.DrawTextFrame(frame, Point(), text_paint);
}
canvas.Restore();
Expand Down Expand Up @@ -3060,7 +3064,12 @@ TEST_P(AiksTest, TextForegroundShaderWithTransform) {

auto blob = SkTextBlob::MakeFromString("Hello", sk_font);
ASSERT_NE(blob, nullptr);
auto frame = MakeTextFrameFromTextBlobSkia(blob);
auto maybe_frame = MakeTextFrameFromTextBlobSkia(blob);
ASSERT_TRUE(maybe_frame.has_value());
if (!maybe_frame.has_value()) {
return;
}
auto frame = maybe_frame.value();
canvas.DrawTextFrame(frame, Point(), text_paint);

ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
Expand Down
6 changes: 5 additions & 1 deletion impeller/display_list/dl_dispatcher.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1111,7 +1111,11 @@ void DlDispatcher::drawDisplayList(
void DlDispatcher::drawTextBlob(const sk_sp<SkTextBlob>& blob,
SkScalar x,
SkScalar y) {
const auto text_frame = MakeTextFrameFromTextBlobSkia(blob);
const auto maybe_text_frame = MakeTextFrameFromTextBlobSkia(blob);
if (!maybe_text_frame.has_value()) {
return;
}
const auto text_frame = maybe_text_frame.value();
if (paint_.style == Paint::Style::kStroke ||
paint_.color_source.GetType() != ColorSource::Type::kColor) {
auto path = skia_conversions::PathDataFromTextBlob(blob);
Expand Down
10 changes: 1 addition & 9 deletions impeller/entity/contents/text_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,8 @@ void TextContents::SetOffset(Vector2 offset) {
offset_ = offset;
}

std::optional<Rect> TextContents::GetTextFrameBounds() const {
return frame_.GetBounds();
}

std::optional<Rect> TextContents::GetCoverage(const Entity& entity) const {
auto bounds = frame_.GetBounds();
if (!bounds.has_value()) {
return std::nullopt;
}
return bounds->TransformBounds(entity.GetTransformation());
return frame_.GetBounds().TransformBounds(entity.GetTransformation());
}

void TextContents::PopulateGlyphAtlas(
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/entity_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2174,7 +2174,7 @@ TEST_P(EntityTest, InheritOpacityTest) {
SkFont font;
font.setSize(30);
auto blob = SkTextBlob::MakeFromString("A", font);
auto frame = MakeTextFrameFromTextBlobSkia(blob);
auto frame = MakeTextFrameFromTextBlobSkia(blob).value();
auto lazy_glyph_atlas =
std::make_shared<LazyGlyphAtlas>(TypographerContextSkia::Make());
lazy_glyph_atlas->AddTextFrame(frame, 1.0f);
Expand Down
31 changes: 12 additions & 19 deletions impeller/typographer/backends/skia/text_frame_skia.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,15 @@ static Rect ToRect(const SkRect& rect) {

static constexpr Scalar kScaleSize = 100000.0f;

TextFrame MakeTextFrameFromTextBlobSkia(const sk_sp<SkTextBlob>& blob) {
std::optional<TextFrame> MakeTextFrameFromTextBlobSkia(
const sk_sp<SkTextBlob>& blob) {
if (!blob) {
return {};
}

TextFrame frame;

std::vector<TextRun> runs;
bool has_color = false;
for (SkTextBlobRunIterator run(blob.get()); !run.done(); run.next()) {
TextRun text_run(ToFont(run));

// TODO(jonahwilliams): ask Skia for a public API to look this up.
// https://github.com/flutter/flutter/issues/112005
SkStrikeSpec strikeSpec = SkStrikeSpec::MakeWithNoDevice(run.font());
Expand All @@ -57,12 +56,6 @@ TextFrame MakeTextFrameFromTextBlobSkia(const sk_sp<SkTextBlob>& blob) {
const auto glyph_count = run.glyphCount();
const auto* glyphs = run.glyphs();
switch (run.positioning()) {
case SkTextBlobRunIterator::kDefault_Positioning:
FML_DLOG(ERROR) << "Unimplemented.";
break;
case SkTextBlobRunIterator::kHorizontal_Positioning:
FML_DLOG(ERROR) << "Unimplemented.";
break;
Comment on lines -60 to -65
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're never going to use these, I deleted them and left it for the default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case SkTextBlobRunIterator::kFull_Positioning: {
std::vector<SkRect> glyph_bounds;
glyph_bounds.resize(glyph_count);
Expand All @@ -75,32 +68,32 @@ TextFrame MakeTextFrameFromTextBlobSkia(const sk_sp<SkTextBlob>& blob) {
font.setSize(kScaleSize);
font.getBounds(glyphs, glyph_count, glyph_bounds.data(), nullptr);

std::vector<TextRun::GlyphPosition> positions;
positions.reserve(glyph_count);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allocate the right amount of storage once rather than appending.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put this as a comment in the code instead of in the PR review

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels like

/// the int
int a = 0;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷🏼. 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.

for (auto i = 0u; i < glyph_count; i++) {
// kFull_Positioning has two scalars per glyph.
const SkPoint* glyph_points = run.points();
const auto* point = glyph_points + i;
Glyph::Type type = paths.glyph(glyphs[i])->isColor()
? Glyph::Type::kBitmap
: Glyph::Type::kPath;
has_color |= type == Glyph::Type::kBitmap;

text_run.AddGlyph(
positions.emplace_back(TextRun::GlyphPosition{
Glyph{glyphs[i], type,
ToRect(glyph_bounds[i]).Scale(font_size / kScaleSize)},
Point{point->x(), point->y()});
Point{point->x(), point->y()}});
}
TextRun text_run(ToFont(run), positions);
runs.emplace_back(text_run);
break;
}
case SkTextBlobRunIterator::kRSXform_Positioning:
FML_DLOG(ERROR) << "Unimplemented.";
break;
default:
FML_DLOG(ERROR) << "Unimplemented.";
continue;
}
frame.AddTextRun(std::move(text_run));
}

return frame;
return TextFrame(runs, ToRect(blob->bounds()), has_color);
}

} // namespace impeller
3 changes: 2 additions & 1 deletion impeller/typographer/backends/skia/text_frame_skia.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

namespace impeller {

TextFrame MakeTextFrameFromTextBlobSkia(const sk_sp<SkTextBlob>& blob);
std::optional<TextFrame> MakeTextFrameFromTextBlobSkia(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this optional so that we can avoid creating TextContents at all if the textblob is nullptr for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto above, comment in the code :)

const sk_sp<SkTextBlob>& blob);

} // namespace impeller
12 changes: 9 additions & 3 deletions impeller/typographer/backends/stb/text_frame_stb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,16 @@ TextFrame MakeTextFrameSTB(const std::shared_ptr<TypefaceSTB>& typeface_stb,
}
}

TextFrame frame;
frame.AddTextRun(std::move(run));
std::optional<Rect> result;
for (const auto& glyph_position : run.GetGlyphPositions()) {
Rect glyph_rect =
Rect(glyph_position.position + glyph_position.glyph.bounds.origin,
glyph_position.glyph.bounds.size);
result = result.has_value() ? result->Union(glyph_rect) : glyph_rect;
}

return frame;
std::vector<TextRun> runs = {run};
return TextFrame(runs, result.value_or(Rect::MakeLTRB(0, 0, 0, 0)), false);
}

} // namespace impeller
27 changes: 5 additions & 22 deletions impeller/typographer/text_frame.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,13 @@ namespace impeller {

TextFrame::TextFrame() = default;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default constructor is no glyphs, empty bounds, no color. Which should be perfectly safe.


TextFrame::~TextFrame() = default;

std::optional<Rect> TextFrame::GetBounds() const {
std::optional<Rect> result;

for (const auto& run : runs_) {
for (const auto& glyph_position : run.GetGlyphPositions()) {
Rect glyph_rect =
Rect(glyph_position.position + glyph_position.glyph.bounds.origin,
glyph_position.glyph.bounds.size);
result = result.has_value() ? result->Union(glyph_rect) : glyph_rect;
}
}
TextFrame::TextFrame(std::vector<TextRun>& runs, Rect bounds, bool has_color)
: runs_(std::move(runs)), bounds_(bounds), has_color_(has_color) {}

return result;
}
TextFrame::~TextFrame() = default;

bool TextFrame::AddTextRun(TextRun&& run) {
if (!run.IsValid()) {
return false;
}
has_color_ |= run.HasColor();
runs_.emplace_back(std::move(run));
return true;
Rect TextFrame::GetBounds() const {
return bounds_;
}

size_t TextFrame::GetRunCount() const {
Expand Down
16 changes: 5 additions & 11 deletions impeller/typographer/text_frame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ class TextFrame {
public:
TextFrame();

TextFrame(std::vector<TextRun>& runs, Rect bounds, bool has_color);

~TextFrame();

void CollectUniqueFontGlyphPairs(FontGlyphPair::Set& set, Scalar scale) const;
Expand All @@ -30,9 +32,9 @@ class TextFrame {
/// @brief The conservative bounding box for this text frame.
///
/// @return The bounds rectangle. If there are no glyphs in this text
/// frame, std::nullopt is returned.
/// frame and empty Rectangle is returned instead.
///
std::optional<Rect> GetBounds() const;
Rect GetBounds() const;

//----------------------------------------------------------------------------
/// @brief The number of runs in this text frame.
Expand All @@ -41,15 +43,6 @@ class TextFrame {
///
size_t GetRunCount() const;

//----------------------------------------------------------------------------
/// @brief Adds a new text run to the text frame.
///
/// @param[in] run The run
///
/// @return If the text run could be added to this frame.
///
bool AddTextRun(TextRun&& run);

//----------------------------------------------------------------------------
/// @brief Returns a reference to all the text runs in this frame.
///
Expand Down Expand Up @@ -77,6 +70,7 @@ class TextFrame {

private:
std::vector<TextRun> runs_;
Rect bounds_;
bool has_color_ = false;
};

Expand Down
13 changes: 8 additions & 5 deletions impeller/typographer/text_run.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,18 @@ TextRun::TextRun(const Font& font) : font_(font) {
is_valid_ = true;
}

TextRun::TextRun(const Font& font, std::vector<GlyphPosition>& glyphs)
: font_(font), glyphs_(std::move(glyphs)) {
if (!font_.IsValid()) {
return;
}
is_valid_ = true;
}

TextRun::~TextRun() = default;

bool TextRun::AddGlyph(Glyph glyph, Point position) {
glyphs_.emplace_back(GlyphPosition{glyph, position});
has_color_ |= glyph.type == Glyph::Type::kBitmap;
return true;
}

Expand All @@ -37,8 +44,4 @@ const Font& TextRun::GetFont() const {
return font_;
}

bool TextRun::HasColor() const {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to track this per run.

return has_color_;
}

} // namespace impeller
9 changes: 3 additions & 6 deletions impeller/typographer/text_run.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ class TextRun {
///
/// @param[in] font The font
///
TextRun(const Font& font);
explicit TextRun(const Font& font);

TextRun(const Font& font, std::vector<GlyphPosition>& glyphs);

~TextRun();

Expand Down Expand Up @@ -68,15 +70,10 @@ class TextRun {
///
const Font& GetFont() const;

//----------------------------------------------------------------------------
/// @brief Whether any glyph in this run has color.
bool HasColor() const;

private:
Font font_;
std::vector<GlyphPosition> glyphs_;
bool is_valid_ = false;
bool has_color_ = false;
};

} // namespace impeller
Loading