From c669dc21149fc410175327425ae9634c9aea592c Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 26 Aug 2023 12:14:39 -0700 Subject: [PATCH 1/4] [Impeller] cache Skia text bounds computation. --- impeller/display_list/dl_aiks_canvas.cc | 11 +++--- impeller/display_list/dl_dispatcher.cc | 11 +++--- impeller/entity/contents/text_contents.cc | 27 +++++++++------ impeller/entity/contents/text_contents.h | 2 +- .../backends/skia/text_frame_skia.cc | 16 +++++---- .../backends/skia/text_frame_skia.h | 3 +- impeller/typographer/text_frame.cc | 34 +++++++------------ impeller/typographer/text_frame.h | 16 +++------ 8 files changed, 58 insertions(+), 62 deletions(-) diff --git a/impeller/display_list/dl_aiks_canvas.cc b/impeller/display_list/dl_aiks_canvas.cc index d916a5eb28ba8..ee5671c395894 100644 --- a/impeller/display_list/dl_aiks_canvas.cc +++ b/impeller/display_list/dl_aiks_canvas.cc @@ -1067,15 +1067,16 @@ void DlAiksCanvas::DrawTextBlob(const sk_sp& blob, SkScalar x, SkScalar y, const flutter::DlPaint& paint) { - 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.getDrawStyle() == flutter::DlDrawStyle::kStroke) { auto path = skia_conversions::PathDataFromTextBlob(blob); auto bounds = text_frame.GetBounds(); - if (!bounds.has_value()) { - return; - } canvas_.Save(); - canvas_.Translate({x + bounds->origin.x, y + bounds->origin.y, 0.0}); + canvas_.Translate({x + bounds.origin.x, y + bounds.origin.y, 0.0}); canvas_.DrawPath(path, ToPaint(paint)); canvas_.Restore(); return; diff --git a/impeller/display_list/dl_dispatcher.cc b/impeller/display_list/dl_dispatcher.cc index e5031eefd18d1..bf1a078df78ad 100644 --- a/impeller/display_list/dl_dispatcher.cc +++ b/impeller/display_list/dl_dispatcher.cc @@ -1111,15 +1111,16 @@ void DlDispatcher::drawDisplayList( void DlDispatcher::drawTextBlob(const sk_sp& 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) { auto path = skia_conversions::PathDataFromTextBlob(blob); auto bounds = text_frame.GetBounds(); - if (!bounds.has_value()) { - return; - } canvas_.Save(); - canvas_.Translate({x + bounds->origin.x, y + bounds->origin.y, 0.0}); + canvas_.Translate({x + bounds.origin.x, y + bounds.origin.y, 0.0}); canvas_.DrawPath(path, paint_); canvas_.Restore(); return; diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index d8e68b2929e65..eb1bbcfed09bc 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/impeller/entity/contents/text_contents.cc @@ -48,7 +48,7 @@ Color TextContents::GetColor() const { } bool TextContents::CanInheritOpacity(const Entity& entity) const { - return !frame_.MaybeHasOverlapping(); + return !frame_.has_value() || !frame_->MaybeHasOverlapping(); } void TextContents::SetInheritedOpacity(Scalar opacity) { @@ -60,33 +60,40 @@ void TextContents::SetOffset(Vector2 offset) { } std::optional TextContents::GetTextFrameBounds() const { - return frame_.GetBounds(); + // TODO: delete. + return std::nullopt; } std::optional TextContents::GetCoverage(const Entity& entity) const { - auto bounds = frame_.GetBounds(); - if (!bounds.has_value()) { + if (!frame_.has_value()) { return std::nullopt; } - return bounds->TransformBounds(entity.GetTransformation()); + return frame_->GetBounds().TransformBounds(entity.GetTransformation()); } void TextContents::PopulateGlyphAtlas( const std::shared_ptr& lazy_glyph_atlas, Scalar scale) { - lazy_glyph_atlas->AddTextFrame(frame_, scale); - scale_ = scale; + if (frame_.has_value()) { + lazy_glyph_atlas->AddTextFrame(frame_.value(), scale); + scale_ = scale; + } } bool TextContents::Render(const ContentContext& renderer, const Entity& entity, RenderPass& pass) const { + const auto maybe_text_frame = frame_; + if (!maybe_text_frame.has_value()) { + return true; + } + const auto frame = maybe_text_frame.value(); auto color = GetColor(); if (color.IsTransparent()) { return true; } - auto type = frame_.GetAtlasType(); + auto type = frame.GetAtlasType(); auto atlas = ResolveAtlas(*renderer.GetContext(), type, renderer.GetLazyGlyphAtlas()); @@ -159,7 +166,7 @@ bool TextContents::Render(const ContentContext& renderer, auto& host_buffer = pass.GetTransientsBuffer(); size_t vertex_count = 0; - for (const auto& run : frame_.GetRuns()) { + for (const auto& run : frame.GetRuns()) { vertex_count += run.GetGlyphPositions().size(); } vertex_count *= 6; @@ -170,7 +177,7 @@ bool TextContents::Render(const ContentContext& renderer, VS::PerVertexData vtx; VS::PerVertexData* vtx_contents = reinterpret_cast(contents); - for (const TextRun& run : frame_.GetRuns()) { + for (const TextRun& run : frame.GetRuns()) { const Font& font = run.GetFont(); Scalar rounded_scale = TextFrame::RoundScaledFontSize( scale_, font.GetMetrics().point_size); diff --git a/impeller/entity/contents/text_contents.h b/impeller/entity/contents/text_contents.h index 6e3b89d558e61..82abe617611f4 100644 --- a/impeller/entity/contents/text_contents.h +++ b/impeller/entity/contents/text_contents.h @@ -56,7 +56,7 @@ class TextContents final : public Contents { RenderPass& pass) const override; private: - TextFrame frame_; + std::optional frame_ = std::nullopt; Scalar scale_ = 1.0; Color color_; Scalar inherited_opacity_ = 1.0; diff --git a/impeller/typographer/backends/skia/text_frame_skia.cc b/impeller/typographer/backends/skia/text_frame_skia.cc index 008e965d14cd5..87071f2396d69 100644 --- a/impeller/typographer/backends/skia/text_frame_skia.cc +++ b/impeller/typographer/backends/skia/text_frame_skia.cc @@ -39,13 +39,14 @@ static Rect ToRect(const SkRect& rect) { static constexpr Scalar kScaleSize = 100000.0f; -TextFrame MakeTextFrameFromTextBlobSkia(const sk_sp& blob) { +std::optional MakeTextFrameFromTextBlobSkia( + const sk_sp& blob) { if (!blob) { return {}; } - TextFrame frame; - + std::vector runs; + bool has_color = false; for (SkTextBlobRunIterator run(blob.get()); !run.done(); run.next()) { TextRun text_run(ToFont(run)); @@ -82,6 +83,7 @@ TextFrame MakeTextFrameFromTextBlobSkia(const sk_sp& blob) { Glyph::Type type = paths.glyph(glyphs[i])->isColor() ? Glyph::Type::kBitmap : Glyph::Type::kPath; + has_color |= type == Glyph::Type::kBitmap; text_run.AddGlyph( Glyph{glyphs[i], type, @@ -97,10 +99,12 @@ TextFrame MakeTextFrameFromTextBlobSkia(const sk_sp& blob) { FML_DLOG(ERROR) << "Unimplemented."; continue; } - frame.AddTextRun(std::move(text_run)); + runs.emplace_back(text_run); } - - return frame; + auto sk_bounds = blob->bounds(); + auto bounds = Rect::MakeLTRB(sk_bounds.left(), sk_bounds.top(), + sk_bounds.right(), sk_bounds.bottom()); + return TextFrame(runs, bounds, has_color); } } // namespace impeller diff --git a/impeller/typographer/backends/skia/text_frame_skia.h b/impeller/typographer/backends/skia/text_frame_skia.h index 93431446afe32..53170a248d3a1 100644 --- a/impeller/typographer/backends/skia/text_frame_skia.h +++ b/impeller/typographer/backends/skia/text_frame_skia.h @@ -10,6 +10,7 @@ namespace impeller { -TextFrame MakeTextFrameFromTextBlobSkia(const sk_sp& blob); +std::optional MakeTextFrameFromTextBlobSkia( + const sk_sp& blob); } // namespace impeller diff --git a/impeller/typographer/text_frame.cc b/impeller/typographer/text_frame.cc index 898f69bdfe5e1..1c4a759560acd 100644 --- a/impeller/typographer/text_frame.cc +++ b/impeller/typographer/text_frame.cc @@ -6,33 +6,23 @@ namespace impeller { -TextFrame::TextFrame() = default; +TextFrame::TextFrame(std::vector& runs, Rect bounds, bool has_color) + : runs_(std::move(runs)), bounds_(bounds), has_color_(has_color) {} TextFrame::~TextFrame() = default; -std::optional TextFrame::GetBounds() const { - std::optional 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; - } - } - - return result; +Rect TextFrame::GetBounds() const { + return bounds_; } -bool TextFrame::AddTextRun(TextRun&& run) { - if (!run.IsValid()) { - return false; - } - has_color_ |= run.HasColor(); - runs_.emplace_back(std::move(run)); - return true; -} +// bool TextFrame::AddTextRun(TextRun&& run) { +// if (!run.IsValid()) { +// return false; +// } +// has_color_ |= run.HasColor(); +// runs_.emplace_back(std::move(run)); +// return true; +// } size_t TextFrame::GetRunCount() const { return runs_.size(); diff --git a/impeller/typographer/text_frame.h b/impeller/typographer/text_frame.h index 54ea12df30f6b..e29015f46af64 100644 --- a/impeller/typographer/text_frame.h +++ b/impeller/typographer/text_frame.h @@ -18,7 +18,7 @@ namespace impeller { /// class TextFrame { public: - TextFrame(); + TextFrame(std::vector& runs, Rect bounds, bool has_color); ~TextFrame(); @@ -30,9 +30,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 GetBounds() const; + Rect GetBounds() const; //---------------------------------------------------------------------------- /// @brief The number of runs in this text frame. @@ -41,15 +41,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. /// @@ -77,6 +68,7 @@ class TextFrame { private: std::vector runs_; + Rect bounds_; bool has_color_ = false; }; From e7d4907d75d2895bce7fc22982157b53f4b6de2f Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 26 Aug 2023 22:44:30 -0700 Subject: [PATCH 2/4] [Impeller] use Skia computed glyph bounds. --- impeller/aiks/aiks_unittests.cc | 15 +++++++-- .../contents/color_source_text_contents.cc | 3 +- impeller/entity/contents/text_contents.cc | 24 +++----------- impeller/entity/contents/text_contents.h | 2 +- impeller/entity/entity_unittests.cc | 2 +- .../backends/skia/text_frame_skia.cc | 25 +++++---------- .../backends/stb/text_frame_stb.cc | 12 +++++-- impeller/typographer/text_frame.cc | 11 ++----- impeller/typographer/text_frame.h | 2 ++ impeller/typographer/text_run.cc | 13 +++++--- impeller/typographer/text_run.h | 9 ++---- impeller/typographer/typographer_unittests.cc | 31 ++++++++++--------- 12 files changed, 69 insertions(+), 80 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index cec91e8ed0ee8..c070f1fc54635 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -1278,7 +1278,11 @@ bool RenderTextInCanvasSkia(const std::shared_ptr& 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); @@ -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(); @@ -3057,7 +3061,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())); diff --git a/impeller/entity/contents/color_source_text_contents.cc b/impeller/entity/contents/color_source_text_contents.cc index 8032bbb629d55..fc8fb19544c83 100644 --- a/impeller/entity/contents/color_source_text_contents.cc +++ b/impeller/entity/contents/color_source_text_contents.cc @@ -43,7 +43,8 @@ void ColorSourceTextContents::PopulateGlyphAtlas( bool ColorSourceTextContents::Render(const ContentContext& renderer, const Entity& entity, RenderPass& pass) const { - auto text_bounds = text_contents_->GetTextFrameBounds(); + // TODO(jonahwilliams): delete this. + std::optional text_bounds = std::nullopt; if (!text_bounds.has_value()) { return true; } diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index eb1bbcfed09bc..7259515d28862 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/impeller/entity/contents/text_contents.cc @@ -48,7 +48,7 @@ Color TextContents::GetColor() const { } bool TextContents::CanInheritOpacity(const Entity& entity) const { - return !frame_.has_value() || !frame_->MaybeHasOverlapping(); + return !frame_.MaybeHasOverlapping(); } void TextContents::SetInheritedOpacity(Scalar opacity) { @@ -59,35 +59,21 @@ void TextContents::SetOffset(Vector2 offset) { offset_ = offset; } -std::optional TextContents::GetTextFrameBounds() const { - // TODO: delete. - return std::nullopt; -} - std::optional TextContents::GetCoverage(const Entity& entity) const { - if (!frame_.has_value()) { - return std::nullopt; - } - return frame_->GetBounds().TransformBounds(entity.GetTransformation()); + return frame_.GetBounds().TransformBounds(entity.GetTransformation()); } void TextContents::PopulateGlyphAtlas( const std::shared_ptr& lazy_glyph_atlas, Scalar scale) { - if (frame_.has_value()) { - lazy_glyph_atlas->AddTextFrame(frame_.value(), scale); - scale_ = scale; - } + lazy_glyph_atlas->AddTextFrame(frame_, scale); + scale_ = scale; } bool TextContents::Render(const ContentContext& renderer, const Entity& entity, RenderPass& pass) const { - const auto maybe_text_frame = frame_; - if (!maybe_text_frame.has_value()) { - return true; - } - const auto frame = maybe_text_frame.value(); + const auto frame = frame_; auto color = GetColor(); if (color.IsTransparent()) { return true; diff --git a/impeller/entity/contents/text_contents.h b/impeller/entity/contents/text_contents.h index 82abe617611f4..6e3b89d558e61 100644 --- a/impeller/entity/contents/text_contents.h +++ b/impeller/entity/contents/text_contents.h @@ -56,7 +56,7 @@ class TextContents final : public Contents { RenderPass& pass) const override; private: - std::optional frame_ = std::nullopt; + TextFrame frame_; Scalar scale_ = 1.0; Color color_; Scalar inherited_opacity_ = 1.0; diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 7523efb5a4b14..5af2bd9d23b33 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -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(TypographerContextSkia::Make()); lazy_glyph_atlas->AddTextFrame(frame, 1.0f); diff --git a/impeller/typographer/backends/skia/text_frame_skia.cc b/impeller/typographer/backends/skia/text_frame_skia.cc index 87071f2396d69..9011ed64e1446 100644 --- a/impeller/typographer/backends/skia/text_frame_skia.cc +++ b/impeller/typographer/backends/skia/text_frame_skia.cc @@ -48,8 +48,6 @@ std::optional MakeTextFrameFromTextBlobSkia( std::vector 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()); @@ -58,12 +56,6 @@ std::optional MakeTextFrameFromTextBlobSkia( 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; case SkTextBlobRunIterator::kFull_Positioning: { std::vector glyph_bounds; glyph_bounds.resize(glyph_count); @@ -76,6 +68,8 @@ std::optional MakeTextFrameFromTextBlobSkia( font.setSize(kScaleSize); font.getBounds(glyphs, glyph_count, glyph_bounds.data(), nullptr); + std::vector positions; + positions.reserve(glyph_count); for (auto i = 0u; i < glyph_count; i++) { // kFull_Positioning has two scalars per glyph. const SkPoint* glyph_points = run.points(); @@ -85,26 +79,21 @@ std::optional MakeTextFrameFromTextBlobSkia( : 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; } - runs.emplace_back(text_run); } - auto sk_bounds = blob->bounds(); - auto bounds = Rect::MakeLTRB(sk_bounds.left(), sk_bounds.top(), - sk_bounds.right(), sk_bounds.bottom()); - return TextFrame(runs, bounds, has_color); + return TextFrame(runs, ToRect(blob->bounds()), has_color); } } // namespace impeller diff --git a/impeller/typographer/backends/stb/text_frame_stb.cc b/impeller/typographer/backends/stb/text_frame_stb.cc index f2d9e9177509e..5e6060ba32551 100644 --- a/impeller/typographer/backends/stb/text_frame_stb.cc +++ b/impeller/typographer/backends/stb/text_frame_stb.cc @@ -52,10 +52,16 @@ TextFrame MakeTextFrameSTB(const std::shared_ptr& typeface_stb, } } - TextFrame frame; - frame.AddTextRun(std::move(run)); + std::optional 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 runs = {run}; + return TextFrame(runs, result.value_or(Rect::MakeLTRB(0, 0, 0, 0)), false); } } // namespace impeller diff --git a/impeller/typographer/text_frame.cc b/impeller/typographer/text_frame.cc index 1c4a759560acd..9c02a7393bd62 100644 --- a/impeller/typographer/text_frame.cc +++ b/impeller/typographer/text_frame.cc @@ -6,6 +6,8 @@ namespace impeller { +TextFrame::TextFrame() = default; + TextFrame::TextFrame(std::vector& runs, Rect bounds, bool has_color) : runs_(std::move(runs)), bounds_(bounds), has_color_(has_color) {} @@ -15,15 +17,6 @@ Rect TextFrame::GetBounds() const { return bounds_; } -// bool TextFrame::AddTextRun(TextRun&& run) { -// if (!run.IsValid()) { -// return false; -// } -// has_color_ |= run.HasColor(); -// runs_.emplace_back(std::move(run)); -// return true; -// } - size_t TextFrame::GetRunCount() const { return runs_.size(); } diff --git a/impeller/typographer/text_frame.h b/impeller/typographer/text_frame.h index e29015f46af64..35316351e66f0 100644 --- a/impeller/typographer/text_frame.h +++ b/impeller/typographer/text_frame.h @@ -18,6 +18,8 @@ namespace impeller { /// class TextFrame { public: + TextFrame(); + TextFrame(std::vector& runs, Rect bounds, bool has_color); ~TextFrame(); diff --git a/impeller/typographer/text_run.cc b/impeller/typographer/text_run.cc index ee0af62b5029f..6fc3a3ba261c4 100644 --- a/impeller/typographer/text_run.cc +++ b/impeller/typographer/text_run.cc @@ -13,11 +13,18 @@ TextRun::TextRun(const Font& font) : font_(font) { is_valid_ = true; } +TextRun::TextRun(const Font& font, std::vector& 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; } @@ -37,8 +44,4 @@ const Font& TextRun::GetFont() const { return font_; } -bool TextRun::HasColor() const { - return has_color_; -} - } // namespace impeller diff --git a/impeller/typographer/text_run.h b/impeller/typographer/text_run.h index 2bf433afca2f8..340db94d9f40d 100644 --- a/impeller/typographer/text_run.h +++ b/impeller/typographer/text_run.h @@ -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& glyphs); ~TextRun(); @@ -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 glyphs_; bool is_valid_ = false; - bool has_color_ = false; }; } // namespace impeller diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc index 4a2bed4dc52cc..fd412297b4941 100644 --- a/impeller/typographer/typographer_unittests.cc +++ b/impeller/typographer/typographer_unittests.cc @@ -40,7 +40,7 @@ TEST_P(TypographerTest, CanConvertTextBlob) { auto blob = SkTextBlob::MakeFromString( "the quick brown fox jumped over the lazy dog.", font); ASSERT_TRUE(blob); - auto frame = MakeTextFrameFromTextBlobSkia(blob); + auto frame = MakeTextFrameFromTextBlobSkia(blob).value(); ASSERT_EQ(frame.GetRunCount(), 1u); for (const auto& run : frame.GetRuns()) { ASSERT_TRUE(run.IsValid()); @@ -62,7 +62,7 @@ TEST_P(TypographerTest, CanCreateGlyphAtlas) { ASSERT_TRUE(blob); auto atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, MakeTextFrameFromTextBlobSkia(blob)); + atlas_context, MakeTextFrameFromTextBlobSkia(blob).value()); ASSERT_NE(atlas, nullptr); ASSERT_NE(atlas->GetTexture(), nullptr); ASSERT_EQ(atlas->GetType(), GlyphAtlas::Type::kAlphaBitmap); @@ -108,7 +108,7 @@ TEST_P(TypographerTest, LazyAtlasTracksColor) { auto blob = SkTextBlob::MakeFromString("hello", sk_font); ASSERT_TRUE(blob); - auto frame = MakeTextFrameFromTextBlobSkia(blob); + auto frame = MakeTextFrameFromTextBlobSkia(blob).value(); ASSERT_FALSE(frame.GetAtlasType() == GlyphAtlas::Type::kColorBitmap); @@ -117,7 +117,8 @@ TEST_P(TypographerTest, LazyAtlasTracksColor) { lazy_atlas.AddTextFrame(frame, 1.0f); frame = MakeTextFrameFromTextBlobSkia( - SkTextBlob::MakeFromString("😀 ", emoji_font)); + SkTextBlob::MakeFromString("😀 ", emoji_font)) + .value(); ASSERT_TRUE(frame.GetAtlasType() == GlyphAtlas::Type::kColorBitmap); @@ -142,7 +143,7 @@ TEST_P(TypographerTest, GlyphAtlasWithOddUniqueGlyphSize) { ASSERT_TRUE(blob); auto atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, MakeTextFrameFromTextBlobSkia(blob)); + atlas_context, MakeTextFrameFromTextBlobSkia(blob).value()); ASSERT_NE(atlas, nullptr); ASSERT_NE(atlas->GetTexture(), nullptr); @@ -159,7 +160,7 @@ TEST_P(TypographerTest, GlyphAtlasIsRecycledIfUnchanged) { ASSERT_TRUE(blob); auto atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, MakeTextFrameFromTextBlobSkia(blob)); + atlas_context, MakeTextFrameFromTextBlobSkia(blob).value()); ASSERT_NE(atlas, nullptr); ASSERT_NE(atlas->GetTexture(), nullptr); ASSERT_EQ(atlas, atlas_context->GetGlyphAtlas()); @@ -168,7 +169,7 @@ TEST_P(TypographerTest, GlyphAtlasIsRecycledIfUnchanged) { auto next_atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, MakeTextFrameFromTextBlobSkia(blob)); + atlas_context, MakeTextFrameFromTextBlobSkia(blob).value()); ASSERT_EQ(atlas, next_atlas); ASSERT_EQ(atlas_context->GetGlyphAtlas(), atlas); } @@ -191,7 +192,7 @@ TEST_P(TypographerTest, GlyphAtlasWithLotsOfdUniqueGlyphSize) { FontGlyphPair::Set set; size_t size_count = 8; for (size_t index = 0; index < size_count; index += 1) { - MakeTextFrameFromTextBlobSkia(blob).CollectUniqueFontGlyphPairs( + MakeTextFrameFromTextBlobSkia(blob).value().CollectUniqueFontGlyphPairs( set, 0.6 * index); }; auto atlas = @@ -225,7 +226,7 @@ TEST_P(TypographerTest, GlyphAtlasTextureIsRecycledIfUnchanged) { ASSERT_TRUE(blob); auto atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, MakeTextFrameFromTextBlobSkia(blob)); + atlas_context, MakeTextFrameFromTextBlobSkia(blob).value()); auto old_packer = atlas_context->GetRectPacker(); ASSERT_NE(atlas, nullptr); @@ -239,7 +240,7 @@ TEST_P(TypographerTest, GlyphAtlasTextureIsRecycledIfUnchanged) { auto blob2 = SkTextBlob::MakeFromString("spooky 2", sk_font); auto next_atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, MakeTextFrameFromTextBlobSkia(blob2)); + atlas_context, MakeTextFrameFromTextBlobSkia(blob2).value()); ASSERT_EQ(atlas, next_atlas); auto* second_texture = next_atlas->GetTexture().get(); @@ -258,7 +259,7 @@ TEST_P(TypographerTest, GlyphAtlasTextureIsRecreatedIfTypeChanges) { ASSERT_TRUE(blob); auto atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, MakeTextFrameFromTextBlobSkia(blob)); + atlas_context, MakeTextFrameFromTextBlobSkia(blob).value()); auto old_packer = atlas_context->GetRectPacker(); ASSERT_NE(atlas, nullptr); @@ -273,7 +274,7 @@ TEST_P(TypographerTest, GlyphAtlasTextureIsRecreatedIfTypeChanges) { auto blob2 = SkTextBlob::MakeFromString("spooky 1", sk_font); auto next_atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kColorBitmap, 1.0f, - atlas_context, MakeTextFrameFromTextBlobSkia(blob2)); + atlas_context, MakeTextFrameFromTextBlobSkia(blob2).value()); ASSERT_NE(atlas, next_atlas); auto* second_texture = next_atlas->GetTexture().get(); @@ -308,12 +309,14 @@ TEST_P(TypographerTest, MaybeHasOverlapping) { SkFont sk_font(typeface, 0.5f); auto frame = - MakeTextFrameFromTextBlobSkia(SkTextBlob::MakeFromString("1", sk_font)); + MakeTextFrameFromTextBlobSkia(SkTextBlob::MakeFromString("1", sk_font)) + .value(); // Single character has no overlapping ASSERT_FALSE(frame.MaybeHasOverlapping()); auto frame_2 = MakeTextFrameFromTextBlobSkia( - SkTextBlob::MakeFromString("123456789", sk_font)); + SkTextBlob::MakeFromString("123456789", sk_font)) + .value(); ASSERT_FALSE(frame_2.MaybeHasOverlapping()); } From 642fde41fb3ad0f28db4a72ce8ed74b9d516f945 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 27 Aug 2023 12:08:50 -0700 Subject: [PATCH 3/4] ++ --- impeller/entity/contents/text_contents.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index 7259515d28862..a93967c938061 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/impeller/entity/contents/text_contents.cc @@ -73,13 +73,12 @@ void TextContents::PopulateGlyphAtlas( bool TextContents::Render(const ContentContext& renderer, const Entity& entity, RenderPass& pass) const { - const auto frame = frame_; auto color = GetColor(); if (color.IsTransparent()) { return true; } - auto type = frame.GetAtlasType(); + auto type = frame_.GetAtlasType(); auto atlas = ResolveAtlas(*renderer.GetContext(), type, renderer.GetLazyGlyphAtlas()); @@ -152,7 +151,7 @@ bool TextContents::Render(const ContentContext& renderer, auto& host_buffer = pass.GetTransientsBuffer(); size_t vertex_count = 0; - for (const auto& run : frame.GetRuns()) { + for (const auto& run : frame_.GetRuns()) { vertex_count += run.GetGlyphPositions().size(); } vertex_count *= 6; @@ -163,7 +162,7 @@ bool TextContents::Render(const ContentContext& renderer, VS::PerVertexData vtx; VS::PerVertexData* vtx_contents = reinterpret_cast(contents); - for (const TextRun& run : frame.GetRuns()) { + for (const TextRun& run : frame_.GetRuns()) { const Font& font = run.GetFont(); Scalar rounded_scale = TextFrame::RoundScaledFontSize( scale_, font.GetMetrics().point_size); From dd50c1854b4b96004bc547e74135687f23b40203 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 28 Aug 2023 13:56:41 -0700 Subject: [PATCH 4/4] ++ --- impeller/typographer/backends/skia/text_frame_skia.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/impeller/typographer/backends/skia/text_frame_skia.cc b/impeller/typographer/backends/skia/text_frame_skia.cc index 9011ed64e1446..11c256a8af918 100644 --- a/impeller/typographer/backends/skia/text_frame_skia.cc +++ b/impeller/typographer/backends/skia/text_frame_skia.cc @@ -41,6 +41,8 @@ static constexpr Scalar kScaleSize = 100000.0f; std::optional MakeTextFrameFromTextBlobSkia( const sk_sp& blob) { + // Handling nullptr text blobs feels overly defensive here, as I don't + // actually know if this happens. if (!blob) { return {}; }