From 83a7f9cad03e1d35094a795bce16fc0387457cf6 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 2 Sep 2023 21:07:47 -0700 Subject: [PATCH 01/13] [Impeller] construct text frames on UI thread. --- display_list/BUILD.gn | 1 + display_list/benchmarking/dl_complexity_gl.cc | 5 ++ display_list/benchmarking/dl_complexity_gl.h | 3 ++ .../benchmarking/dl_complexity_metal.cc | 5 ++ .../benchmarking/dl_complexity_metal.h | 3 ++ display_list/display_list.h | 1 + display_list/dl_builder.cc | 42 +++++++++++++++ display_list/dl_builder.h | 10 ++++ display_list/dl_canvas.h | 11 +++- display_list/dl_op_receiver.h | 4 ++ display_list/dl_op_records.h | 20 ++++++++ display_list/skia/dl_sk_canvas.cc | 8 +++ display_list/skia/dl_sk_canvas.h | 5 ++ display_list/skia/dl_sk_dispatcher.cc | 7 +++ display_list/skia/dl_sk_dispatcher.h | 3 ++ display_list/utils/dl_receiver_utils.h | 3 ++ impeller/aiks/aiks_unittests.cc | 16 ++---- impeller/aiks/canvas.cc | 4 +- impeller/aiks/canvas.h | 2 +- impeller/display_list/dl_dispatcher.cc | 19 ++----- impeller/display_list/dl_dispatcher.h | 5 ++ impeller/entity/contents/text_contents.cc | 16 +++--- impeller/entity/contents/text_contents.h | 4 +- impeller/entity/entity_unittests.cc | 6 +-- .../backends/skia/text_frame_skia.cc | 28 ++-------- .../backends/skia/text_frame_skia.h | 6 +-- .../backends/stb/text_frame_stb.cc | 10 ++-- .../typographer/backends/stb/text_frame_stb.h | 7 +-- impeller/typographer/typographer_context.h | 1 - impeller/typographer/typographer_unittests.cc | 51 +++++++++---------- shell/common/dl_op_spy.cc | 8 +++ shell/common/dl_op_spy.h | 3 ++ testing/display_list_testing.cc | 9 ++++ testing/display_list_testing.h | 3 ++ testing/mock_canvas.cc | 8 +++ testing/mock_canvas.h | 4 ++ third_party/txt/BUILD.gn | 1 + third_party/txt/src/skia/paragraph_skia.cc | 37 +++++++++++--- third_party/txt/src/skia/paragraph_skia.h | 2 +- 39 files changed, 267 insertions(+), 114 deletions(-) diff --git a/display_list/BUILD.gn b/display_list/BUILD.gn index 97c9683511b61..736e13dc5c7bb 100644 --- a/display_list/BUILD.gn +++ b/display_list/BUILD.gn @@ -87,6 +87,7 @@ source_set("display_list") { public_deps = [ "//flutter/fml", "//flutter/impeller/runtime_stage", + "//flutter/impeller/typographer", "//third_party/skia", ] diff --git a/display_list/benchmarking/dl_complexity_gl.cc b/display_list/benchmarking/dl_complexity_gl.cc index f8dd1c52a1260..53b70e2c24e47 100644 --- a/display_list/benchmarking/dl_complexity_gl.cc +++ b/display_list/benchmarking/dl_complexity_gl.cc @@ -637,6 +637,11 @@ void DisplayListGLComplexityCalculator::GLHelper::drawTextBlob( draw_text_blob_count_++; } +void DisplayListGLComplexityCalculator::GLHelper::drawTextFrame( + const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) {} + void DisplayListGLComplexityCalculator::GLHelper::drawShadow( const SkPath& path, const DlColor color, diff --git a/display_list/benchmarking/dl_complexity_gl.h b/display_list/benchmarking/dl_complexity_gl.h index 9fc7596687051..5115bb4d6ecd6 100644 --- a/display_list/benchmarking/dl_complexity_gl.h +++ b/display_list/benchmarking/dl_complexity_gl.h @@ -70,6 +70,9 @@ class DisplayListGLComplexityCalculator void drawTextBlob(const sk_sp blob, SkScalar x, SkScalar y) override; + void drawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) override; void drawShadow(const SkPath& path, const DlColor color, const SkScalar elevation, diff --git a/display_list/benchmarking/dl_complexity_metal.cc b/display_list/benchmarking/dl_complexity_metal.cc index 56d4f3b406ad9..c6b6547afee56 100644 --- a/display_list/benchmarking/dl_complexity_metal.cc +++ b/display_list/benchmarking/dl_complexity_metal.cc @@ -581,6 +581,11 @@ void DisplayListMetalComplexityCalculator::MetalHelper::drawTextBlob( draw_text_blob_count_++; } +void DisplayListMetalComplexityCalculator::MetalHelper::drawTextFrame( + const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) {} + void DisplayListMetalComplexityCalculator::MetalHelper::drawShadow( const SkPath& path, const DlColor color, diff --git a/display_list/benchmarking/dl_complexity_metal.h b/display_list/benchmarking/dl_complexity_metal.h index aa63863fa4d05..dd068e2fa3243 100644 --- a/display_list/benchmarking/dl_complexity_metal.h +++ b/display_list/benchmarking/dl_complexity_metal.h @@ -70,6 +70,9 @@ class DisplayListMetalComplexityCalculator void drawTextBlob(const sk_sp blob, SkScalar x, SkScalar y) override; + void drawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) override; void drawShadow(const SkPath& path, const DlColor color, const SkScalar elevation, diff --git a/display_list/display_list.h b/display_list/display_list.h index 668a247d5596f..3d4a7acff5896 100644 --- a/display_list/display_list.h +++ b/display_list/display_list.h @@ -136,6 +136,7 @@ namespace flutter { \ V(DrawDisplayList) \ V(DrawTextBlob) \ + V(DrawTextFrame) \ \ V(DrawShadow) \ V(DrawShadowTransparentOccluder) diff --git a/display_list/dl_builder.cc b/display_list/dl_builder.cc index b78145c124f42..40932db1c9dc8 100644 --- a/display_list/dl_builder.cc +++ b/display_list/dl_builder.cc @@ -1302,6 +1302,48 @@ void DisplayListBuilder::DrawTextBlob(const sk_sp& blob, SetAttributesFromPaint(paint, DisplayListOpFlags::kDrawTextBlobFlags); drawTextBlob(blob, x, y); } + +void DisplayListBuilder::drawTextFrame( + const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) { + DisplayListAttributeFlags flags = kDrawTextBlobFlags; + OpResult result = PaintResult(current_, flags); + if (result == OpResult::kNoEffect) { + return; + } + impeller::Rect bounds = text_frame->GetBounds(); + SkRect sk_bounds = SkRect::MakeLTRB(bounds.GetLeft(), bounds.GetTop(), + bounds.GetRight(), bounds.GetBottom()); + bool unclipped = AccumulateOpBounds(sk_bounds.makeOffset(x, y), flags); + // TODO(https://github.com/flutter/flutter/issues/82202): Remove once the + // unit tests can use Fuchsia's font manager instead of the empty default. + // Until then we might encounter empty bounds for otherwise valid text and + // thus we ignore the results from AccumulateOpBounds. +#if defined(OS_FUCHSIA) + unclipped = true; +#endif // OS_FUCHSIA + if (unclipped) { + Push(0, 1, text_frame, x, y); + // There is no way to query if the glyphs of a text blob overlap and + // there are no current guarantees from either Skia or Impeller that + // they will protect overlapping glyphs from the effects of overdraw + // so we must make the conservative assessment that this DL layer is + // not compatible with group opacity inheritance. + UpdateLayerOpacityCompatibility(false); + UpdateLayerResult(result); + } +} + +void DisplayListBuilder::DrawTextFrame( + const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y, + const DlPaint& paint) { + SetAttributesFromPaint(paint, DisplayListOpFlags::kDrawTextBlobFlags); + drawTextFrame(text_frame, x, y); +} + void DisplayListBuilder::DrawShadow(const SkPath& path, const DlColor color, const SkScalar elevation, diff --git a/display_list/dl_builder.h b/display_list/dl_builder.h index ab0bce0db3c38..3a65b8fad5328 100644 --- a/display_list/dl_builder.h +++ b/display_list/dl_builder.h @@ -224,6 +224,16 @@ class DisplayListBuilder final : public virtual DlCanvas, SkScalar x, SkScalar y, const DlPaint& paint) override; + + void drawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) override; + + void DrawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y, + const DlPaint& paint) override; + // |DlCanvas| void DrawShadow(const SkPath& path, const DlColor color, diff --git a/display_list/dl_canvas.h b/display_list/dl_canvas.h index f04645c157788..5cd06146c78c3 100644 --- a/display_list/dl_canvas.h +++ b/display_list/dl_canvas.h @@ -18,6 +18,8 @@ #include "third_party/skia/include/core/SkRect.h" #include "third_party/skia/include/core/SkTextBlob.h" +#include "impeller/typographer/text_frame.h" + namespace flutter { //------------------------------------------------------------------------------ @@ -152,7 +154,7 @@ class DlCanvas { virtual void DrawVertices(const DlVertices* vertices, DlBlendMode mode, const DlPaint& paint) = 0; - void DrawVertices(const std::shared_ptr vertices, + void DrawVertices(const std::shared_ptr& vertices, DlBlendMode mode, const DlPaint& paint) { DrawVertices(vertices.get(), mode, paint); @@ -201,6 +203,13 @@ class DlCanvas { const DlPaint* paint = nullptr) = 0; virtual void DrawDisplayList(const sk_sp display_list, SkScalar opacity = SK_Scalar1) = 0; + + virtual void DrawTextFrame( + const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y, + const DlPaint& paint) = 0; + virtual void DrawTextBlob(const sk_sp& blob, SkScalar x, SkScalar y, diff --git a/display_list/dl_op_receiver.h b/display_list/dl_op_receiver.h index 23a0aaf6fd97c..d9bbc7b7f4f3f 100644 --- a/display_list/dl_op_receiver.h +++ b/display_list/dl_op_receiver.h @@ -257,6 +257,10 @@ class DlOpReceiver { virtual void drawTextBlob(const sk_sp blob, SkScalar x, SkScalar y) = 0; + virtual void drawTextFrame( + const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) = 0; virtual void drawShadow(const SkPath& path, const DlColor color, const SkScalar elevation, diff --git a/display_list/dl_op_records.h b/display_list/dl_op_records.h index 47cdeb9d0e6fb..a1c7835e43df9 100644 --- a/display_list/dl_op_records.h +++ b/display_list/dl_op_records.h @@ -12,6 +12,7 @@ #include "flutter/display_list/effects/dl_color_source.h" #include "flutter/fml/macros.h" +#include "impeller/typographer/text_frame.h" #include "third_party/skia/include/core/SkRSXform.h" namespace flutter { @@ -1084,6 +1085,25 @@ struct DrawTextBlobOp final : DrawOpBase { } }; +struct DrawTextFrameOp final : DrawOpBase { + static const auto kType = DisplayListOpType::kDrawTextFrame; + + DrawTextFrameOp(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) + : x(x), y(y), text_frame(text_frame) {} + + const SkScalar x; + const SkScalar y; + const std::shared_ptr text_frame; + + void dispatch(DispatchContext& ctx) const { + if (op_needed(ctx)) { + ctx.receiver.drawTextFrame(text_frame, x, y); + } + } +}; + // 4 byte header + 28 byte payload packs evenly into 32 bytes #define DEFINE_DRAW_SHADOW_OP(name, transparent_occluder) \ struct Draw##name##Op final : DrawOpBase { \ diff --git a/display_list/skia/dl_sk_canvas.cc b/display_list/skia/dl_sk_canvas.cc index 9299c9a6ff9d7..34270bd88018d 100644 --- a/display_list/skia/dl_sk_canvas.cc +++ b/display_list/skia/dl_sk_canvas.cc @@ -325,6 +325,14 @@ void DlSkCanvasAdapter::DrawTextBlob(const sk_sp& blob, delegate_->drawTextBlob(blob, x, y, ToSk(paint)); } +void DlSkCanvasAdapter::DrawTextFrame( + const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y, + const DlPaint& paint) { + FML_CHECK(false); +} + void DlSkCanvasAdapter::DrawShadow(const SkPath& path, const DlColor color, const SkScalar elevation, diff --git a/display_list/skia/dl_sk_canvas.h b/display_list/skia/dl_sk_canvas.h index 0377b7de75e30..f085d35ae8c9d 100644 --- a/display_list/skia/dl_sk_canvas.h +++ b/display_list/skia/dl_sk_canvas.h @@ -7,6 +7,7 @@ #include "flutter/display_list/dl_canvas.h" #include "flutter/display_list/skia/dl_sk_types.h" +#include "impeller/typographer/text_frame.h" namespace flutter { @@ -144,6 +145,10 @@ class DlSkCanvasAdapter final : public virtual DlCanvas { SkScalar x, SkScalar y, const DlPaint& paint) override; + void DrawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y, + const DlPaint& paint) override; void DrawShadow(const SkPath& path, const DlColor color, const SkScalar elevation, diff --git a/display_list/skia/dl_sk_dispatcher.cc b/display_list/skia/dl_sk_dispatcher.cc index def08fa8886a3..f8cc16d1151c1 100644 --- a/display_list/skia/dl_sk_dispatcher.cc +++ b/display_list/skia/dl_sk_dispatcher.cc @@ -270,6 +270,13 @@ void DlSkCanvasDispatcher::drawTextBlob(const sk_sp blob, canvas_->drawTextBlob(blob, x, y, paint()); } +void DlSkCanvasDispatcher::drawTextFrame( + const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) { + FML_CHECK(false); +} + void DlSkCanvasDispatcher::DrawShadow(SkCanvas* canvas, const SkPath& path, DlColor color, diff --git a/display_list/skia/dl_sk_dispatcher.h b/display_list/skia/dl_sk_dispatcher.h index a7fc73bbe2a54..ee6101291e76a 100644 --- a/display_list/skia/dl_sk_dispatcher.h +++ b/display_list/skia/dl_sk_dispatcher.h @@ -98,6 +98,9 @@ class DlSkCanvasDispatcher : public virtual DlOpReceiver, void drawTextBlob(const sk_sp blob, SkScalar x, SkScalar y) override; + void drawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) override; void drawShadow(const SkPath& path, const DlColor color, const SkScalar elevation, diff --git a/display_list/utils/dl_receiver_utils.h b/display_list/utils/dl_receiver_utils.h index e2d07310368ae..2a1f9cb7efff4 100644 --- a/display_list/utils/dl_receiver_utils.h +++ b/display_list/utils/dl_receiver_utils.h @@ -129,6 +129,9 @@ class IgnoreDrawDispatchHelper : public virtual DlOpReceiver { void drawTextBlob(const sk_sp blob, SkScalar x, SkScalar y) override {} + void drawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) override {} void drawShadow(const SkPath& path, const DlColor color, const SkScalar elevation, diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 9788c09cd08c1..d229008a59144 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -5,7 +5,6 @@ #include #include #include -#include #include #include #include @@ -1278,11 +1277,7 @@ bool RenderTextInCanvasSkia(const std::shared_ptr& context, } // Create the Impeller text frame and draw it at the designated baseline. - auto maybe_frame = MakeTextFrameFromTextBlobSkia(blob); - if (!maybe_frame.has_value()) { - return false; - } - auto frame = maybe_frame.value(); + auto frame = MakeTextFrameFromTextBlobSkia(blob, false); Paint text_paint; text_paint.color = Color::Yellow().WithAlpha(options.alpha); @@ -1471,7 +1466,7 @@ TEST_P(AiksTest, CanRenderTextOutsideBoundaries) { { auto blob = SkTextBlob::MakeFromString(t.text, sk_font); ASSERT_NE(blob, nullptr); - auto frame = MakeTextFrameFromTextBlobSkia(blob).value(); + auto frame = MakeTextFrameFromTextBlobSkia(blob, false); canvas.DrawTextFrame(frame, Point(), text_paint); } canvas.Restore(); @@ -3094,12 +3089,7 @@ TEST_P(AiksTest, TextForegroundShaderWithTransform) { auto blob = SkTextBlob::MakeFromString("Hello", sk_font); ASSERT_NE(blob, nullptr); - auto maybe_frame = MakeTextFrameFromTextBlobSkia(blob); - ASSERT_TRUE(maybe_frame.has_value()); - if (!maybe_frame.has_value()) { - return; - } - auto frame = maybe_frame.value(); + auto frame = MakeTextFrameFromTextBlobSkia(blob, false); canvas.DrawTextFrame(frame, Point(), text_paint); ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); diff --git a/impeller/aiks/canvas.cc b/impeller/aiks/canvas.cc index a5ae1c71cc147..d2bab026df210 100644 --- a/impeller/aiks/canvas.cc +++ b/impeller/aiks/canvas.cc @@ -533,7 +533,7 @@ void Canvas::SaveLayer(const Paint& paint, } } -void Canvas::DrawTextFrame(const TextFrame& text_frame, +void Canvas::DrawTextFrame(const std::shared_ptr& text_frame, Point position, const Paint& paint) { Entity entity; @@ -541,7 +541,7 @@ void Canvas::DrawTextFrame(const TextFrame& text_frame, entity.SetBlendMode(paint.blend_mode); auto text_contents = std::make_shared(); - text_contents->SetTextFrame(TextFrame(text_frame)); + text_contents->SetTextFrame(text_frame); text_contents->SetColor(paint.color); entity.SetTransformation(GetCurrentTransformation() * diff --git a/impeller/aiks/canvas.h b/impeller/aiks/canvas.h index 84f1c486bc30f..d36bc3ff58940 100644 --- a/impeller/aiks/canvas.h +++ b/impeller/aiks/canvas.h @@ -139,7 +139,7 @@ class Canvas { void DrawPicture(const Picture& picture); - void DrawTextFrame(const TextFrame& text_frame, + void DrawTextFrame(const std::shared_ptr& text_frame, Point position, const Paint& paint); diff --git a/impeller/display_list/dl_dispatcher.cc b/impeller/display_list/dl_dispatcher.cc index 5e4205465850d..b4d0a7f0a4df0 100644 --- a/impeller/display_list/dl_dispatcher.cc +++ b/impeller/display_list/dl_dispatcher.cc @@ -34,7 +34,6 @@ #include "impeller/geometry/path_builder.h" #include "impeller/geometry/scalar.h" #include "impeller/geometry/sigma.h" -#include "impeller/typographer/backends/skia/text_frame_skia.h" #if IMPELLER_ENABLE_3D #include "impeller/entity/contents/scene_contents.h" @@ -1110,20 +1109,12 @@ void DlDispatcher::drawDisplayList( void DlDispatcher::drawTextBlob(const sk_sp blob, SkScalar x, SkScalar y) { - 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 bounds = blob->bounds(); - auto path = skia_conversions::PathDataFromTextBlob(blob); - path.Shift(Point(x + bounds.left(), y + bounds.top())); - canvas_.DrawPath(path, paint_); - return; - } + UNIMPLEMENTED; +} +void DlDispatcher::drawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) { canvas_.DrawTextFrame(text_frame, // impeller::Point{x, y}, // paint_ // diff --git a/impeller/display_list/dl_dispatcher.h b/impeller/display_list/dl_dispatcher.h index d9a36e2239980..0dc3b7ce72461 100644 --- a/impeller/display_list/dl_dispatcher.h +++ b/impeller/display_list/dl_dispatcher.h @@ -212,6 +212,11 @@ class DlDispatcher final : public flutter::DlOpReceiver { SkScalar x, SkScalar y) override; + // |flutter::DlOpReceiver| + void drawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) override; + // |flutter::DlOpReceiver| void drawShadow(const SkPath& path, const flutter::DlColor color, diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index 9495556bef6f5..e7b631f6a65b2 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/impeller/entity/contents/text_contents.cc @@ -23,8 +23,8 @@ TextContents::TextContents() = default; TextContents::~TextContents() = default; -void TextContents::SetTextFrame(TextFrame&& frame) { - frame_ = std::move(frame); +void TextContents::SetTextFrame(const std::shared_ptr& frame) { + frame_ = frame; } std::shared_ptr TextContents::ResolveAtlas( @@ -48,7 +48,7 @@ Color TextContents::GetColor() const { } bool TextContents::CanInheritOpacity(const Entity& entity) const { - return !frame_.MaybeHasOverlapping(); + return !frame_->MaybeHasOverlapping(); } void TextContents::SetInheritedOpacity(Scalar opacity) { @@ -60,13 +60,13 @@ void TextContents::SetOffset(Vector2 offset) { } std::optional TextContents::GetCoverage(const Entity& entity) const { - return frame_.GetBounds().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); + lazy_glyph_atlas->AddTextFrame(*frame_, scale); scale_ = scale; } @@ -78,7 +78,7 @@ bool TextContents::Render(const ContentContext& renderer, return true; } - auto type = frame_.GetAtlasType(); + auto type = frame_->GetAtlasType(); auto atlas = ResolveAtlas(*renderer.GetContext(), type, renderer.GetLazyGlyphAtlas()); @@ -151,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; @@ -162,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); diff --git a/impeller/entity/contents/text_contents.h b/impeller/entity/contents/text_contents.h index 6e3b89d558e61..07d191ef3caad 100644 --- a/impeller/entity/contents/text_contents.h +++ b/impeller/entity/contents/text_contents.h @@ -26,7 +26,7 @@ class TextContents final : public Contents { ~TextContents(); - void SetTextFrame(TextFrame&& frame); + void SetTextFrame(const std::shared_ptr& frame); void SetColor(Color color); @@ -56,7 +56,7 @@ class TextContents final : public Contents { RenderPass& pass) const override; private: - TextFrame frame_; + std::shared_ptr 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 5af2bd9d23b33..af9242660b9d6 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -2174,13 +2174,13 @@ TEST_P(EntityTest, InheritOpacityTest) { SkFont font; font.setSize(30); auto blob = SkTextBlob::MakeFromString("A", font); - auto frame = MakeTextFrameFromTextBlobSkia(blob).value(); + auto frame = MakeTextFrameFromTextBlobSkia(blob, false); auto lazy_glyph_atlas = std::make_shared(TypographerContextSkia::Make()); - lazy_glyph_atlas->AddTextFrame(frame, 1.0f); + lazy_glyph_atlas->AddTextFrame(*frame, 1.0f); auto text_contents = std::make_shared(); - text_contents->SetTextFrame(std::move(frame)); + text_contents->SetTextFrame(frame); text_contents->SetColor(Color::Blue().WithAlpha(0.5)); ASSERT_TRUE(text_contents->CanInheritOpacity(entity)); diff --git a/impeller/typographer/backends/skia/text_frame_skia.cc b/impeller/typographer/backends/skia/text_frame_skia.cc index 11c256a8af918..4d436dc96a906 100644 --- a/impeller/typographer/backends/skia/text_frame_skia.cc +++ b/impeller/typographer/backends/skia/text_frame_skia.cc @@ -8,11 +8,7 @@ #include "flutter/fml/logging.h" #include "impeller/typographer/backends/skia/typeface_skia.h" -#include "include/core/SkFontTypes.h" -#include "include/core/SkRect.h" -#include "third_party/skia/include/core/SkFont.h" #include "third_party/skia/include/core/SkFontMetrics.h" -#include "third_party/skia/src/core/SkStrikeSpec.h" // nogncheck #include "third_party/skia/src/core/SkTextBlobPriv.h" // nogncheck namespace impeller { @@ -39,22 +35,12 @@ static Rect ToRect(const SkRect& rect) { 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 {}; - } - +std::shared_ptr MakeTextFrameFromTextBlobSkia( + const sk_sp& blob, + bool has_color) { + const auto type = has_color ? Glyph::Type::kBitmap : Glyph::Type::kPath; std::vector runs; - bool has_color = false; for (SkTextBlobRunIterator run(blob.get()); !run.done(); run.next()) { - // 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()); - SkBulkGlyphMetricsAndPaths paths{strikeSpec}; - const auto glyph_count = run.glyphCount(); const auto* glyphs = run.glyphs(); switch (run.positioning()) { @@ -76,10 +62,6 @@ std::optional MakeTextFrameFromTextBlobSkia( // 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; positions.emplace_back(TextRun::GlyphPosition{ Glyph{glyphs[i], type, @@ -95,7 +77,7 @@ std::optional MakeTextFrameFromTextBlobSkia( continue; } } - return TextFrame(runs, ToRect(blob->bounds()), has_color); + return std::make_shared(runs, ToRect(blob->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 53170a248d3a1..1a9d0b9c3a98c 100644 --- a/impeller/typographer/backends/skia/text_frame_skia.h +++ b/impeller/typographer/backends/skia/text_frame_skia.h @@ -4,13 +4,13 @@ #pragma once -#include "flutter/fml/macros.h" #include "impeller/typographer/text_frame.h" #include "third_party/skia/include/core/SkTextBlob.h" namespace impeller { -std::optional MakeTextFrameFromTextBlobSkia( - const sk_sp& blob); +std::shared_ptr MakeTextFrameFromTextBlobSkia( + const sk_sp& blob, + bool 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 5e6060ba32551..e1d1dd0d1c662 100644 --- a/impeller/typographer/backends/stb/text_frame_stb.cc +++ b/impeller/typographer/backends/stb/text_frame_stb.cc @@ -8,9 +8,10 @@ namespace impeller { -TextFrame MakeTextFrameSTB(const std::shared_ptr& typeface_stb, - Font::Metrics metrics, - const std::string& text) { +std::shared_ptr MakeTextFrameSTB( + const std::shared_ptr& typeface_stb, + Font::Metrics metrics, + const std::string& text) { TextRun run(Font(typeface_stb, metrics)); // Shape the text run using STB. The glyph positions could also be resolved @@ -61,7 +62,8 @@ TextFrame MakeTextFrameSTB(const std::shared_ptr& typeface_stb, } std::vector runs = {run}; - return TextFrame(runs, result.value_or(Rect::MakeLTRB(0, 0, 0, 0)), false); + return std::make_shared( + runs, result.value_or(Rect::MakeLTRB(0, 0, 0, 0)), false); } } // namespace impeller diff --git a/impeller/typographer/backends/stb/text_frame_stb.h b/impeller/typographer/backends/stb/text_frame_stb.h index 622cb1f3b72fd..39f8cccc7688a 100644 --- a/impeller/typographer/backends/stb/text_frame_stb.h +++ b/impeller/typographer/backends/stb/text_frame_stb.h @@ -10,8 +10,9 @@ namespace impeller { -TextFrame MakeTextFrameSTB(const std::shared_ptr& typeface_stb, - Font::Metrics metrics, - const std::string& text); +std::shared_ptr MakeTextFrameSTB( + const std::shared_ptr& typeface_stb, + Font::Metrics metrics, + const std::string& text); } // namespace impeller diff --git a/impeller/typographer/typographer_context.h b/impeller/typographer/typographer_context.h index 1ca4d4a7687c3..3714dd804070e 100644 --- a/impeller/typographer/typographer_context.h +++ b/impeller/typographer/typographer_context.h @@ -4,7 +4,6 @@ #pragma once -#include #include #include "flutter/fml/macros.h" diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc index a996bdf98d98d..61c73e813a65d 100644 --- a/impeller/typographer/typographer_unittests.cc +++ b/impeller/typographer/typographer_unittests.cc @@ -40,9 +40,9 @@ 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).value(); - ASSERT_EQ(frame.GetRunCount(), 1u); - for (const auto& run : frame.GetRuns()) { + auto frame = MakeTextFrameFromTextBlobSkia(blob, false); + ASSERT_EQ(frame->GetRunCount(), 1u); + for (const auto& run : frame->GetRuns()) { ASSERT_TRUE(run.IsValid()); ASSERT_EQ(run.GetGlyphCount(), 45u); } @@ -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).value()); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob, false)); ASSERT_NE(atlas, nullptr); ASSERT_NE(atlas->GetTexture(), nullptr); ASSERT_EQ(atlas->GetType(), GlyphAtlas::Type::kAlphaBitmap); @@ -113,21 +113,20 @@ TEST_P(TypographerTest, LazyAtlasTracksColor) { auto blob = SkTextBlob::MakeFromString("hello", sk_font); ASSERT_TRUE(blob); - auto frame = MakeTextFrameFromTextBlobSkia(blob).value(); + auto frame = MakeTextFrameFromTextBlobSkia(blob, false); - ASSERT_FALSE(frame.GetAtlasType() == GlyphAtlas::Type::kColorBitmap); + ASSERT_FALSE(frame->GetAtlasType() == GlyphAtlas::Type::kColorBitmap); LazyGlyphAtlas lazy_atlas(TypographerContextSkia::Make()); - lazy_atlas.AddTextFrame(frame, 1.0f); + lazy_atlas.AddTextFrame(*frame, 1.0f); frame = MakeTextFrameFromTextBlobSkia( - SkTextBlob::MakeFromString("😀 ", emoji_font)) - .value(); + SkTextBlob::MakeFromString("😀 ", emoji_font), true); - ASSERT_TRUE(frame.GetAtlasType() == GlyphAtlas::Type::kColorBitmap); + ASSERT_TRUE(frame->GetAtlasType() == GlyphAtlas::Type::kColorBitmap); - lazy_atlas.AddTextFrame(frame, 1.0f); + lazy_atlas.AddTextFrame(*frame, 1.0f); // Creates different atlases for color and alpha bitmap. auto color_atlas = lazy_atlas.CreateOrGetGlyphAtlas( @@ -148,7 +147,7 @@ TEST_P(TypographerTest, GlyphAtlasWithOddUniqueGlyphSize) { ASSERT_TRUE(blob); auto atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, MakeTextFrameFromTextBlobSkia(blob).value()); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob, false)); ASSERT_NE(atlas, nullptr); ASSERT_NE(atlas->GetTexture(), nullptr); @@ -165,7 +164,7 @@ TEST_P(TypographerTest, GlyphAtlasIsRecycledIfUnchanged) { ASSERT_TRUE(blob); auto atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, MakeTextFrameFromTextBlobSkia(blob).value()); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob, false)); ASSERT_NE(atlas, nullptr); ASSERT_NE(atlas->GetTexture(), nullptr); ASSERT_EQ(atlas, atlas_context->GetGlyphAtlas()); @@ -174,7 +173,7 @@ TEST_P(TypographerTest, GlyphAtlasIsRecycledIfUnchanged) { auto next_atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, MakeTextFrameFromTextBlobSkia(blob).value()); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob, false)); ASSERT_EQ(atlas, next_atlas); ASSERT_EQ(atlas_context->GetGlyphAtlas(), atlas); } @@ -197,8 +196,8 @@ TEST_P(TypographerTest, GlyphAtlasWithLotsOfdUniqueGlyphSize) { FontGlyphMap font_glyph_map; size_t size_count = 8; for (size_t index = 0; index < size_count; index += 1) { - MakeTextFrameFromTextBlobSkia(blob).value().CollectUniqueFontGlyphPairs( - font_glyph_map, 0.6 * index); + MakeTextFrameFromTextBlobSkia(blob, false) + ->CollectUniqueFontGlyphPairs(font_glyph_map, 0.6 * index); }; auto atlas = context->CreateGlyphAtlas(*GetContext(), GlyphAtlas::Type::kAlphaBitmap, @@ -232,7 +231,7 @@ TEST_P(TypographerTest, GlyphAtlasTextureIsRecycledIfUnchanged) { ASSERT_TRUE(blob); auto atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, MakeTextFrameFromTextBlobSkia(blob).value()); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob, false)); auto old_packer = atlas_context->GetRectPacker(); ASSERT_NE(atlas, nullptr); @@ -246,7 +245,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).value()); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob2, false)); ASSERT_EQ(atlas, next_atlas); auto* second_texture = next_atlas->GetTexture().get(); @@ -265,7 +264,7 @@ TEST_P(TypographerTest, GlyphAtlasTextureIsRecreatedIfTypeChanges) { ASSERT_TRUE(blob); auto atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, MakeTextFrameFromTextBlobSkia(blob).value()); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob, false)); auto old_packer = atlas_context->GetRectPacker(); ASSERT_NE(atlas, nullptr); @@ -280,7 +279,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).value()); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob2, false)); ASSERT_NE(atlas, next_atlas); auto* second_texture = next_atlas->GetTexture().get(); @@ -296,16 +295,14 @@ TEST_P(TypographerTest, MaybeHasOverlapping) { font_mgr->matchFamilyStyle("Arial", SkFontStyle::Normal()); SkFont sk_font(typeface, 0.5f); - auto frame = - MakeTextFrameFromTextBlobSkia(SkTextBlob::MakeFromString("1", sk_font)) - .value(); + auto frame = MakeTextFrameFromTextBlobSkia( + SkTextBlob::MakeFromString("1", sk_font), false); // Single character has no overlapping - ASSERT_FALSE(frame.MaybeHasOverlapping()); + ASSERT_FALSE(frame->MaybeHasOverlapping()); auto frame_2 = MakeTextFrameFromTextBlobSkia( - SkTextBlob::MakeFromString("123456789", sk_font)) - .value(); - ASSERT_FALSE(frame_2.MaybeHasOverlapping()); + SkTextBlob::MakeFromString("123456789", sk_font), false); + ASSERT_FALSE(frame_2->MaybeHasOverlapping()); } TEST_P(TypographerTest, RectanglePackerAddsNonoverlapingRectangles) { diff --git a/shell/common/dl_op_spy.cc b/shell/common/dl_op_spy.cc index 15910fc2e85de..bcf213bffa8e9 100644 --- a/shell/common/dl_op_spy.cc +++ b/shell/common/dl_op_spy.cc @@ -127,6 +127,14 @@ void DlOpSpy::drawTextBlob(const sk_sp blob, SkScalar y) { did_draw_ |= will_draw_; } + +void DlOpSpy::drawTextFrame( + const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) { + did_draw_ |= will_draw_; +} + void DlOpSpy::drawShadow(const SkPath& path, const DlColor color, const SkScalar elevation, diff --git a/shell/common/dl_op_spy.h b/shell/common/dl_op_spy.h index 1cb29ff7219aa..1bcf08c7a84c5 100644 --- a/shell/common/dl_op_spy.h +++ b/shell/common/dl_op_spy.h @@ -90,6 +90,9 @@ class DlOpSpy final : public virtual DlOpReceiver, void drawTextBlob(const sk_sp blob, SkScalar x, SkScalar y) override; + void drawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) override; void drawShadow(const SkPath& path, const DlColor color, const SkScalar elevation, diff --git a/testing/display_list_testing.cc b/testing/display_list_testing.cc index f54a90c192d19..3ddeeac6cd92c 100644 --- a/testing/display_list_testing.cc +++ b/testing/display_list_testing.cc @@ -859,6 +859,15 @@ void DisplayListStreamDispatcher::drawTextBlob(const sk_sp blob, << blob.get() << ", " << x << ", " << y << ");" << std::endl; } + +void DisplayListStreamDispatcher::drawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) { + startl() << "drawTextFrame(" + << text_frame.get() << ", " + << x << ", " << y << ");" << std::endl; +} + void DisplayListStreamDispatcher::drawShadow(const SkPath& path, const DlColor color, const SkScalar elevation, diff --git a/testing/display_list_testing.h b/testing/display_list_testing.h index 4f3830a204b44..a468deda878e2 100644 --- a/testing/display_list_testing.h +++ b/testing/display_list_testing.h @@ -144,6 +144,9 @@ class DisplayListStreamDispatcher final : public DlOpReceiver { void drawTextBlob(const sk_sp blob, SkScalar x, SkScalar y) override; + void drawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) override; void drawShadow(const SkPath& path, const DlColor color, const SkScalar elevation, diff --git a/testing/mock_canvas.cc b/testing/mock_canvas.cc index 75b03107e2a75..8d5a51fd1b6c6 100644 --- a/testing/mock_canvas.cc +++ b/testing/mock_canvas.cc @@ -160,6 +160,14 @@ void MockCanvas::DrawTextBlob(const sk_sp& text, paint, SkPoint::Make(x, y)}}); } +void MockCanvas::DrawTextFrame( + const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y, + const DlPaint& paint) { + FML_DCHECK(false); +} + void MockCanvas::DrawRect(const SkRect& rect, const DlPaint& paint) { draw_calls_.emplace_back(DrawCall{current_layer_, DrawRectData{rect, paint}}); } diff --git a/testing/mock_canvas.h b/testing/mock_canvas.h index 71260d5b8755f..591f1c908e9fb 100644 --- a/testing/mock_canvas.h +++ b/testing/mock_canvas.h @@ -273,6 +273,10 @@ class MockCanvas final : public DlCanvas { SkScalar x, SkScalar y, const DlPaint& paint) override; + void DrawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y, + const DlPaint& paint) override; void DrawShadow(const SkPath& path, const DlColor color, const SkScalar elevation, diff --git a/third_party/txt/BUILD.gn b/third_party/txt/BUILD.gn index b799bc5dd46d5..a61438b72c5eb 100644 --- a/third_party/txt/BUILD.gn +++ b/third_party/txt/BUILD.gn @@ -81,6 +81,7 @@ source_set("txt") { public_deps = [ "//flutter/display_list", "//flutter/fml", + "//flutter/impeller/typographer/backends/skia:typographer_skia_backend", "//third_party/harfbuzz", "//third_party/icu", "//third_party/skia", diff --git a/third_party/txt/src/skia/paragraph_skia.cc b/third_party/txt/src/skia/paragraph_skia.cc index 76d737224aa79..f48a0ff042d62 100644 --- a/third_party/txt/src/skia/paragraph_skia.cc +++ b/third_party/txt/src/skia/paragraph_skia.cc @@ -19,6 +19,7 @@ #include #include #include "fml/logging.h" +#include "impeller/typographer/backends/skia/text_frame_skia.h" namespace txt { @@ -63,12 +64,15 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { /// See https://github.com/flutter/flutter/issues/126673. It /// probably makes sense to eventually make this a compile-time /// decision (i.e. with `#ifdef`) instead of a runtime option. - DisplayListParagraphPainter(DisplayListBuilder* builder, - const std::vector& dl_paints, - bool draw_path_effect) + DisplayListParagraphPainter( + DisplayListBuilder* builder, + const std::vector& dl_paints, + bool impeller_enabled, + const std::shared_ptr& paragraph) : builder_(builder), dl_paints_(dl_paints), - draw_path_effect_(draw_path_effect) {} + impeller_enabled_(impeller_enabled), + paragraph_(paragraph) {} void drawTextBlob(const sk_sp& blob, SkScalar x, @@ -79,6 +83,14 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { } size_t paint_id = std::get(paint); FML_DCHECK(paint_id < dl_paints_.size()); + + if (impeller_enabled_) { + auto has_color = paragraph_->containsColorFontOrBitmap(blob.get()); + builder_->DrawTextFrame( + impeller::MakeTextFrameFromTextBlobSkia(blob, has_color), x, y, + dl_paints_[paint_id]); + return; + } builder_->DrawTextBlob(blob, x, y, dl_paints_[paint_id]); } @@ -96,6 +108,13 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { DlBlurMaskFilter filter(DlBlurStyle::kNormal, blur_sigma, false); paint.setMaskFilter(&filter); } + if (impeller_enabled_) { + auto has_color = paragraph_->containsColorFontOrBitmap(blob.get()); + builder_->DrawTextFrame( + impeller::MakeTextFrameFromTextBlobSkia(blob, has_color), x, y, + paint); + return; + } builder_->DrawTextBlob(blob, x, y, paint); } @@ -129,7 +148,7 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { // the line directly using the `drawLine` API instead of using a path effect // (because Impeller does not support path effects). auto dash_path_effect = decor_style.getDashPathEffect(); - if (draw_path_effect_ && dash_path_effect) { + if (impeller_enabled_ && dash_path_effect) { auto path = dashedLine(x0, x1, y0, *dash_path_effect); builder_->DrawPath(path, toDlPaint(decor_style)); return; @@ -192,7 +211,7 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { void setPathEffect(DlPaint& paint, const DashPathEffect& dash_path_effect) { // Impeller does not support path effects, so we should never be setting. - FML_DCHECK(!draw_path_effect_); + FML_DCHECK(!impeller_enabled_); std::array intervals{dash_path_effect.fOnLength, dash_path_effect.fOffLength}; @@ -202,7 +221,8 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { DisplayListBuilder* builder_; const std::vector& dl_paints_; - bool draw_path_effect_; + const bool impeller_enabled_; + const std::shared_ptr paragraph_; }; } // anonymous namespace @@ -292,7 +312,8 @@ void ParagraphSkia::Layout(double width) { } bool ParagraphSkia::Paint(DisplayListBuilder* builder, double x, double y) { - DisplayListParagraphPainter painter(builder, dl_paints_, impeller_enabled_); + DisplayListParagraphPainter painter(builder, dl_paints_, impeller_enabled_, + paragraph_); paragraph_->paint(&painter, x, y); return true; } diff --git a/third_party/txt/src/skia/paragraph_skia.h b/third_party/txt/src/skia/paragraph_skia.h index 5779445e09a71..c909405fc94e6 100644 --- a/third_party/txt/src/skia/paragraph_skia.h +++ b/third_party/txt/src/skia/paragraph_skia.h @@ -72,7 +72,7 @@ class ParagraphSkia : public Paragraph { private: TextStyle SkiaToTxt(const skia::textlayout::TextStyle& skia); - std::unique_ptr paragraph_; + std::shared_ptr paragraph_; std::vector dl_paints_; std::optional> line_metrics_; std::vector line_metrics_styles_; From cf17b4da48fba89a3430902cee9cfb0763ac6e6a Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 3 Sep 2023 08:35:24 -0700 Subject: [PATCH 02/13] revert optional changes to color font checking. --- impeller/aiks/aiks_unittests.cc | 6 ++-- impeller/entity/entity_unittests.cc | 2 +- .../backends/skia/text_frame_skia.cc | 17 ++++++++-- .../backends/skia/text_frame_skia.h | 3 +- impeller/typographer/typographer_unittests.cc | 32 +++++++++---------- third_party/txt/src/skia/paragraph_skia.cc | 27 ++++++---------- third_party/txt/src/skia/paragraph_skia.h | 2 +- 7 files changed, 45 insertions(+), 44 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index d229008a59144..d6ff63078816b 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -1277,7 +1277,7 @@ bool RenderTextInCanvasSkia(const std::shared_ptr& context, } // Create the Impeller text frame and draw it at the designated baseline. - auto frame = MakeTextFrameFromTextBlobSkia(blob, false); + auto frame = MakeTextFrameFromTextBlobSkia(blob); Paint text_paint; text_paint.color = Color::Yellow().WithAlpha(options.alpha); @@ -1466,7 +1466,7 @@ TEST_P(AiksTest, CanRenderTextOutsideBoundaries) { { auto blob = SkTextBlob::MakeFromString(t.text, sk_font); ASSERT_NE(blob, nullptr); - auto frame = MakeTextFrameFromTextBlobSkia(blob, false); + auto frame = MakeTextFrameFromTextBlobSkia(blob); canvas.DrawTextFrame(frame, Point(), text_paint); } canvas.Restore(); @@ -3089,7 +3089,7 @@ TEST_P(AiksTest, TextForegroundShaderWithTransform) { auto blob = SkTextBlob::MakeFromString("Hello", sk_font); ASSERT_NE(blob, nullptr); - auto frame = MakeTextFrameFromTextBlobSkia(blob, false); + auto frame = MakeTextFrameFromTextBlobSkia(blob); canvas.DrawTextFrame(frame, Point(), text_paint); ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index af9242660b9d6..127d1c0bf9a2a 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, false); + auto frame = MakeTextFrameFromTextBlobSkia(blob); 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 4d436dc96a906..b5d7ee8c468e5 100644 --- a/impeller/typographer/backends/skia/text_frame_skia.cc +++ b/impeller/typographer/backends/skia/text_frame_skia.cc @@ -8,7 +8,10 @@ #include "flutter/fml/logging.h" #include "impeller/typographer/backends/skia/typeface_skia.h" +#include "include/core/SkRect.h" +#include "third_party/skia/include/core/SkFont.h" #include "third_party/skia/include/core/SkFontMetrics.h" +#include "third_party/skia/src/core/SkStrikeSpec.h" // nogncheck #include "third_party/skia/src/core/SkTextBlobPriv.h" // nogncheck namespace impeller { @@ -36,11 +39,15 @@ static Rect ToRect(const SkRect& rect) { static constexpr Scalar kScaleSize = 100000.0f; std::shared_ptr MakeTextFrameFromTextBlobSkia( - const sk_sp& blob, - bool has_color) { - const auto type = has_color ? Glyph::Type::kBitmap : Glyph::Type::kPath; + const sk_sp& blob) { + bool has_color = false; std::vector runs; for (SkTextBlobRunIterator run(blob.get()); !run.done(); run.next()) { + // 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()); + SkBulkGlyphMetricsAndPaths paths{strikeSpec}; + const auto glyph_count = run.glyphCount(); const auto* glyphs = run.glyphs(); switch (run.positioning()) { @@ -62,6 +69,10 @@ std::shared_ptr MakeTextFrameFromTextBlobSkia( // 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; positions.emplace_back(TextRun::GlyphPosition{ Glyph{glyphs[i], type, diff --git a/impeller/typographer/backends/skia/text_frame_skia.h b/impeller/typographer/backends/skia/text_frame_skia.h index 1a9d0b9c3a98c..e7721a0c6e9c7 100644 --- a/impeller/typographer/backends/skia/text_frame_skia.h +++ b/impeller/typographer/backends/skia/text_frame_skia.h @@ -10,7 +10,6 @@ namespace impeller { std::shared_ptr MakeTextFrameFromTextBlobSkia( - const sk_sp& blob, - bool has_color); + const sk_sp& blob); } // namespace impeller diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc index 61c73e813a65d..eb3581d750f0b 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, false); + auto frame = MakeTextFrameFromTextBlobSkia(blob); 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, false)); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob)); ASSERT_NE(atlas, nullptr); ASSERT_NE(atlas->GetTexture(), nullptr); ASSERT_EQ(atlas->GetType(), GlyphAtlas::Type::kAlphaBitmap); @@ -113,7 +113,7 @@ TEST_P(TypographerTest, LazyAtlasTracksColor) { auto blob = SkTextBlob::MakeFromString("hello", sk_font); ASSERT_TRUE(blob); - auto frame = MakeTextFrameFromTextBlobSkia(blob, false); + auto frame = MakeTextFrameFromTextBlobSkia(blob); ASSERT_FALSE(frame->GetAtlasType() == GlyphAtlas::Type::kColorBitmap); @@ -122,7 +122,7 @@ TEST_P(TypographerTest, LazyAtlasTracksColor) { lazy_atlas.AddTextFrame(*frame, 1.0f); frame = MakeTextFrameFromTextBlobSkia( - SkTextBlob::MakeFromString("😀 ", emoji_font), true); + SkTextBlob::MakeFromString("😀 ", emoji_font)); ASSERT_TRUE(frame->GetAtlasType() == GlyphAtlas::Type::kColorBitmap); @@ -147,7 +147,7 @@ TEST_P(TypographerTest, GlyphAtlasWithOddUniqueGlyphSize) { ASSERT_TRUE(blob); auto atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, *MakeTextFrameFromTextBlobSkia(blob, false)); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob)); ASSERT_NE(atlas, nullptr); ASSERT_NE(atlas->GetTexture(), nullptr); @@ -164,7 +164,7 @@ TEST_P(TypographerTest, GlyphAtlasIsRecycledIfUnchanged) { ASSERT_TRUE(blob); auto atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, *MakeTextFrameFromTextBlobSkia(blob, false)); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob)); ASSERT_NE(atlas, nullptr); ASSERT_NE(atlas->GetTexture(), nullptr); ASSERT_EQ(atlas, atlas_context->GetGlyphAtlas()); @@ -173,7 +173,7 @@ TEST_P(TypographerTest, GlyphAtlasIsRecycledIfUnchanged) { auto next_atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, *MakeTextFrameFromTextBlobSkia(blob, false)); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob)); ASSERT_EQ(atlas, next_atlas); ASSERT_EQ(atlas_context->GetGlyphAtlas(), atlas); } @@ -196,8 +196,8 @@ TEST_P(TypographerTest, GlyphAtlasWithLotsOfdUniqueGlyphSize) { FontGlyphMap font_glyph_map; size_t size_count = 8; for (size_t index = 0; index < size_count; index += 1) { - MakeTextFrameFromTextBlobSkia(blob, false) - ->CollectUniqueFontGlyphPairs(font_glyph_map, 0.6 * index); + MakeTextFrameFromTextBlobSkia(blob)->CollectUniqueFontGlyphPairs( + font_glyph_map, 0.6 * index); }; auto atlas = context->CreateGlyphAtlas(*GetContext(), GlyphAtlas::Type::kAlphaBitmap, @@ -231,7 +231,7 @@ TEST_P(TypographerTest, GlyphAtlasTextureIsRecycledIfUnchanged) { ASSERT_TRUE(blob); auto atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, *MakeTextFrameFromTextBlobSkia(blob, false)); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob)); auto old_packer = atlas_context->GetRectPacker(); ASSERT_NE(atlas, nullptr); @@ -245,7 +245,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, false)); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob2)); ASSERT_EQ(atlas, next_atlas); auto* second_texture = next_atlas->GetTexture().get(); @@ -264,7 +264,7 @@ TEST_P(TypographerTest, GlyphAtlasTextureIsRecreatedIfTypeChanges) { ASSERT_TRUE(blob); auto atlas = CreateGlyphAtlas( *GetContext(), context.get(), GlyphAtlas::Type::kAlphaBitmap, 1.0f, - atlas_context, *MakeTextFrameFromTextBlobSkia(blob, false)); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob)); auto old_packer = atlas_context->GetRectPacker(); ASSERT_NE(atlas, nullptr); @@ -279,7 +279,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, false)); + atlas_context, *MakeTextFrameFromTextBlobSkia(blob2)); ASSERT_NE(atlas, next_atlas); auto* second_texture = next_atlas->GetTexture().get(); @@ -295,13 +295,13 @@ TEST_P(TypographerTest, MaybeHasOverlapping) { font_mgr->matchFamilyStyle("Arial", SkFontStyle::Normal()); SkFont sk_font(typeface, 0.5f); - auto frame = MakeTextFrameFromTextBlobSkia( - SkTextBlob::MakeFromString("1", sk_font), false); + auto frame = + MakeTextFrameFromTextBlobSkia(SkTextBlob::MakeFromString("1", sk_font)); // Single character has no overlapping ASSERT_FALSE(frame->MaybeHasOverlapping()); auto frame_2 = MakeTextFrameFromTextBlobSkia( - SkTextBlob::MakeFromString("123456789", sk_font), false); + SkTextBlob::MakeFromString("123456789", sk_font)); ASSERT_FALSE(frame_2->MaybeHasOverlapping()); } diff --git a/third_party/txt/src/skia/paragraph_skia.cc b/third_party/txt/src/skia/paragraph_skia.cc index f48a0ff042d62..2e8f8739e54ad 100644 --- a/third_party/txt/src/skia/paragraph_skia.cc +++ b/third_party/txt/src/skia/paragraph_skia.cc @@ -64,15 +64,12 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { /// See https://github.com/flutter/flutter/issues/126673. It /// probably makes sense to eventually make this a compile-time /// decision (i.e. with `#ifdef`) instead of a runtime option. - DisplayListParagraphPainter( - DisplayListBuilder* builder, - const std::vector& dl_paints, - bool impeller_enabled, - const std::shared_ptr& paragraph) + DisplayListParagraphPainter(DisplayListBuilder* builder, + const std::vector& dl_paints, + bool impeller_enabled) : builder_(builder), dl_paints_(dl_paints), - impeller_enabled_(impeller_enabled), - paragraph_(paragraph) {} + impeller_enabled_(impeller_enabled) {} void drawTextBlob(const sk_sp& blob, SkScalar x, @@ -85,10 +82,8 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { FML_DCHECK(paint_id < dl_paints_.size()); if (impeller_enabled_) { - auto has_color = paragraph_->containsColorFontOrBitmap(blob.get()); - builder_->DrawTextFrame( - impeller::MakeTextFrameFromTextBlobSkia(blob, has_color), x, y, - dl_paints_[paint_id]); + builder_->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(blob), x, + y, dl_paints_[paint_id]); return; } builder_->DrawTextBlob(blob, x, y, dl_paints_[paint_id]); @@ -109,10 +104,8 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { paint.setMaskFilter(&filter); } if (impeller_enabled_) { - auto has_color = paragraph_->containsColorFontOrBitmap(blob.get()); - builder_->DrawTextFrame( - impeller::MakeTextFrameFromTextBlobSkia(blob, has_color), x, y, - paint); + builder_->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(blob), x, + y, paint); return; } builder_->DrawTextBlob(blob, x, y, paint); @@ -222,7 +215,6 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { DisplayListBuilder* builder_; const std::vector& dl_paints_; const bool impeller_enabled_; - const std::shared_ptr paragraph_; }; } // anonymous namespace @@ -312,8 +304,7 @@ void ParagraphSkia::Layout(double width) { } bool ParagraphSkia::Paint(DisplayListBuilder* builder, double x, double y) { - DisplayListParagraphPainter painter(builder, dl_paints_, impeller_enabled_, - paragraph_); + DisplayListParagraphPainter painter(builder, dl_paints_, impeller_enabled_); paragraph_->paint(&painter, x, y); return true; } diff --git a/third_party/txt/src/skia/paragraph_skia.h b/third_party/txt/src/skia/paragraph_skia.h index c909405fc94e6..5779445e09a71 100644 --- a/third_party/txt/src/skia/paragraph_skia.h +++ b/third_party/txt/src/skia/paragraph_skia.h @@ -72,7 +72,7 @@ class ParagraphSkia : public Paragraph { private: TextStyle SkiaToTxt(const skia::textlayout::TextStyle& skia); - std::shared_ptr paragraph_; + std::unique_ptr paragraph_; std::vector dl_paints_; std::optional> line_metrics_; std::vector line_metrics_styles_; From e8ee793718cf251dc2aa49a50c13572fa3e05d3f Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 3 Sep 2023 10:42:59 -0700 Subject: [PATCH 03/13] make stroked/gradient text work and add unit tests. --- third_party/txt/src/skia/paragraph_skia.cc | 17 +++ third_party/txt/tests/paragraph_unittests.cc | 135 ++++++++++++++++--- 2 files changed, 130 insertions(+), 22 deletions(-) diff --git a/third_party/txt/src/skia/paragraph_skia.cc b/third_party/txt/src/skia/paragraph_skia.cc index 2e8f8739e54ad..f5ba5b7dfd4b4 100644 --- a/third_party/txt/src/skia/paragraph_skia.cc +++ b/third_party/txt/src/skia/paragraph_skia.cc @@ -18,8 +18,10 @@ #include #include +#include "display_list/dl_paint.h" #include "fml/logging.h" #include "impeller/typographer/backends/skia/text_frame_skia.h" +#include "include/core/SkMatrix.h" namespace txt { @@ -81,6 +83,14 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { size_t paint_id = std::get(paint); FML_DCHECK(paint_id < dl_paints_.size()); + if (ShouldRenderAsPath(dl_paints_[paint_id])) { + auto path = skia::textlayout::Paragraph::GetPath(blob.get()); + auto transformed = path.makeTransform(SkMatrix::Translate( + x + blob->bounds().left(), y + blob->bounds().top())); + builder_->DrawPath(transformed, dl_paints_[paint_id]); + return; + } + if (impeller_enabled_) { builder_->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(blob), x, y, dl_paints_[paint_id]); @@ -192,6 +202,13 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { return path; } + bool ShouldRenderAsPath(const DlPaint& paint) const { + // Text with non-trivial color sources or stroke paint mode should be + // rendered as a path when running on Impeller. + return impeller_enabled_ && (!paint.getColorSource()->asColor() || + paint.getDrawStyle() == DlDrawStyle::kStroke); + } + DlPaint toDlPaint(const DecorationStyle& decor_style, DlDrawStyle draw_style = DlDrawStyle::kStroke) { DlPaint paint; diff --git a/third_party/txt/tests/paragraph_unittests.cc b/third_party/txt/tests/paragraph_unittests.cc index ec06ce702512e..1ffbc7cd1751f 100644 --- a/third_party/txt/tests/paragraph_unittests.cc +++ b/third_party/txt/tests/paragraph_unittests.cc @@ -3,8 +3,13 @@ // found in the LICENSE file. #include +#include "display_list/dl_color.h" +#include "display_list/dl_paint.h" +#include "display_list/dl_tile_mode.h" +#include "display_list/effects/dl_color_source.h" #include "display_list/utils/dl_receiver_utils.h" #include "gtest/gtest.h" +#include "include/core/SkScalar.h" #include "runtime/test_font_data.h" #include "skia/paragraph_builder_skia.h" #include "testing/canvas_test.h" @@ -20,9 +25,11 @@ class DlOpRecorder final : public virtual DlOpReceiver, private IgnoreDrawDispatchHelper, private IgnoreTransformDispatchHelper { public: - int lineCount() const { return lines_.size(); } - int rectCount() const { return rects_.size(); } - int pathCount() const { return paths_.size(); } + size_t lineCount() const { return lines_.size(); } + size_t rectCount() const { return rects_.size(); } + size_t pathCount() const { return paths_.size(); } + size_t textFrameCount() const { return text_frames_.size(); } + size_t blobCount() const { return blobs_.size(); } bool hasPathEffect() const { return path_effect_ != nullptr; } private: @@ -30,6 +37,18 @@ class DlOpRecorder final : public virtual DlOpReceiver, lines_.emplace_back(p0, p1); } + void drawTextFrame(const std::shared_ptr& text_frame, + SkScalar x, + SkScalar y) override { + text_frames_.push_back(text_frame); + } + + void drawTextBlob(const sk_sp blob, + SkScalar x, + SkScalar y) override { + blobs_.push_back(blob); + } + void drawRect(const SkRect& rect) override { rects_.push_back(rect); } void drawPath(const SkPath& path) override { paths_.push_back(path); } @@ -38,6 +57,8 @@ class DlOpRecorder final : public virtual DlOpReceiver, path_effect_ = effect; } + std::vector> text_frames_; + std::vector> blobs_; std::vector> lines_; std::vector rects_; std::vector paths_; @@ -52,10 +73,30 @@ class PainterTestBase : public CanvasTestBase { void PretendImpellerIsEnabled(bool impeller) { impeller_ = impeller; } protected: - sk_sp draw(txt::TextDecorationStyle style) const { - auto t_style = makeDecoratedStyle(style); + txt::TextStyle makeDecoratedStyle(txt::TextDecorationStyle style) { + auto t_style = txt::TextStyle(); + t_style.color = SK_ColorBLACK; // default + t_style.font_weight = txt::FontWeight::w400; // normal + t_style.font_size = 14; // default + t_style.decoration = txt::TextDecoration::kUnderline; + t_style.decoration_style = style; + t_style.decoration_color = SK_ColorBLACK; + t_style.font_families.push_back("ahem"); + return t_style; + } + + txt::TextStyle makeStyle() { + auto t_style = txt::TextStyle(); + t_style.color = SK_ColorBLACK; // default + t_style.font_weight = txt::FontWeight::w400; // normal + t_style.font_size = 14; // default + t_style.font_families.push_back("ahem"); + return t_style; + } + + sk_sp draw(txt::TextStyle style) const { auto pb_skia = makeParagraphBuilder(); - pb_skia.PushStyle(t_style); + pb_skia.PushStyle(style); pb_skia.AddText(u"Hello World!"); pb_skia.Pop(); @@ -85,18 +126,6 @@ class PainterTestBase : public CanvasTestBase { return txt::ParagraphBuilderSkia(p_style, f_collection, impeller_); } - txt::TextStyle makeDecoratedStyle(txt::TextDecorationStyle style) const { - auto t_style = txt::TextStyle(); - t_style.color = SK_ColorBLACK; // default - t_style.font_weight = txt::FontWeight::w400; // normal - t_style.font_size = 14; // default - t_style.decoration = txt::TextDecoration::kUnderline; - t_style.decoration_style = style; - t_style.decoration_color = SK_ColorBLACK; - t_style.font_families.push_back("ahem"); - return t_style; - } - bool impeller_ = false; }; @@ -106,7 +135,8 @@ TEST_F(PainterTest, DrawsSolidLineSkia) { PretendImpellerIsEnabled(false); auto recorder = DlOpRecorder(); - draw(txt::TextDecorationStyle::kSolid)->Dispatch(recorder); + draw(makeDecoratedStyle(txt::TextDecorationStyle::kSolid)) + ->Dispatch(recorder); // Skia may draw a solid underline as a filled rectangle: // https://skia.googlesource.com/skia/+/refs/heads/main/modules/skparagraph/src/Decorations.cpp#91 @@ -118,7 +148,8 @@ TEST_F(PainterTest, DrawsSolidLineImpeller) { PretendImpellerIsEnabled(true); auto recorder = DlOpRecorder(); - draw(txt::TextDecorationStyle::kSolid)->Dispatch(recorder); + draw(makeDecoratedStyle(txt::TextDecorationStyle::kSolid)) + ->Dispatch(recorder); // Skia may draw a solid underline as a filled rectangle: // https://skia.googlesource.com/skia/+/refs/heads/main/modules/skparagraph/src/Decorations.cpp#91 @@ -130,7 +161,8 @@ TEST_F(PainterTest, DrawDashedLineSkia) { PretendImpellerIsEnabled(false); auto recorder = DlOpRecorder(); - draw(txt::TextDecorationStyle::kDashed)->Dispatch(recorder); + draw(makeDecoratedStyle(txt::TextDecorationStyle::kDashed)) + ->Dispatch(recorder); // Skia draws a dashed underline as a filled rectangle with a path effect. EXPECT_EQ(recorder.lineCount(), 1); @@ -141,12 +173,71 @@ TEST_F(PainterTest, DrawDashedLineImpeller) { PretendImpellerIsEnabled(true); auto recorder = DlOpRecorder(); - draw(txt::TextDecorationStyle::kDashed)->Dispatch(recorder); + draw(makeDecoratedStyle(txt::TextDecorationStyle::kDashed)) + ->Dispatch(recorder); // Impeller draws a dashed underline as a path. EXPECT_EQ(recorder.pathCount(), 1); EXPECT_FALSE(recorder.hasPathEffect()); } +TEST_F(PainterTest, DrawTextFrameImpeller) { + PretendImpellerIsEnabled(true); + + auto recorder = DlOpRecorder(); + draw(makeStyle())->Dispatch(recorder); + + EXPECT_EQ(recorder.textFrameCount(), 1); + EXPECT_EQ(recorder.blobCount(), 0); +} + +TEST_F(PainterTest, DrawStrokedTextImpeller) { + PretendImpellerIsEnabled(true); + + auto style = makeStyle(); + // What is your shtyle? + DlPaint foreground; + foreground.setDrawStyle(DlDrawStyle::kStroke); + style.foreground = foreground; + + auto recorder = DlOpRecorder(); + draw(style)->Dispatch(recorder); + + EXPECT_EQ(recorder.textFrameCount(), 0); + EXPECT_EQ(recorder.blobCount(), 0); + EXPECT_EQ(recorder.pathCount(), 1); +} + +TEST_F(PainterTest, DrawTextWithGradientImpeller) { + PretendImpellerIsEnabled(true); + + auto style = makeStyle(); + // how do you like my shtyle? + DlPaint foreground; + std::vector colors = {DlColor::kRed(), DlColor::kCyan()}; + std::vector stops = {0.0, 1.0}; + foreground.setColorSource(DlColorSource::MakeLinear( + SkPoint::Make(0, 0), SkPoint::Make(100, 100), 2, colors.data(), + stops.data(), DlTileMode::kClamp)); + style.foreground = foreground; + + auto recorder = DlOpRecorder(); + draw(style)->Dispatch(recorder); + + EXPECT_EQ(recorder.textFrameCount(), 0); + EXPECT_EQ(recorder.blobCount(), 0); + EXPECT_EQ(recorder.pathCount(), 1); +} + +TEST_F(PainterTest, DrawTextBlobNoImpeller) { + PretendImpellerIsEnabled(false); + + auto recorder = DlOpRecorder(); + draw(makeStyle())->Dispatch(recorder); + + EXPECT_EQ(recorder.textFrameCount(), 0); + EXPECT_EQ(recorder.blobCount(), 1); +} + } // namespace testing } // namespace flutter From a0cf7eeaf5d224d2608186aae168926d1edde1ce Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 3 Sep 2023 11:17:42 -0700 Subject: [PATCH 04/13] ++ --- third_party/txt/tests/paragraph_unittests.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/third_party/txt/tests/paragraph_unittests.cc b/third_party/txt/tests/paragraph_unittests.cc index 1ffbc7cd1751f..0c0a37984ea5b 100644 --- a/third_party/txt/tests/paragraph_unittests.cc +++ b/third_party/txt/tests/paragraph_unittests.cc @@ -25,11 +25,11 @@ class DlOpRecorder final : public virtual DlOpReceiver, private IgnoreDrawDispatchHelper, private IgnoreTransformDispatchHelper { public: - size_t lineCount() const { return lines_.size(); } - size_t rectCount() const { return rects_.size(); } - size_t pathCount() const { return paths_.size(); } - size_t textFrameCount() const { return text_frames_.size(); } - size_t blobCount() const { return blobs_.size(); } + int lineCount() const { return lines_.size(); } + int rectCount() const { return rects_.size(); } + int pathCount() const { return paths_.size(); } + int textFrameCount() const { return text_frames_.size(); } + int blobCount() const { return blobs_.size(); } bool hasPathEffect() const { return path_effect_ != nullptr; } private: From d1bc27ef3959828c1762df5a59a135657792b6f6 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 3 Sep 2023 11:43:58 -0700 Subject: [PATCH 05/13] ++ --- third_party/txt/src/skia/paragraph_skia.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/third_party/txt/src/skia/paragraph_skia.cc b/third_party/txt/src/skia/paragraph_skia.cc index f5ba5b7dfd4b4..0ccd378e16852 100644 --- a/third_party/txt/src/skia/paragraph_skia.cc +++ b/third_party/txt/src/skia/paragraph_skia.cc @@ -205,8 +205,9 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { bool ShouldRenderAsPath(const DlPaint& paint) const { // Text with non-trivial color sources or stroke paint mode should be // rendered as a path when running on Impeller. - return impeller_enabled_ && (!paint.getColorSource()->asColor() || - paint.getDrawStyle() == DlDrawStyle::kStroke); + return impeller_enabled_ && + ((paint.getColorSource() && !paint.getColorSource()->asColor()) || + paint.getDrawStyle() == DlDrawStyle::kStroke); } DlPaint toDlPaint(const DecorationStyle& decor_style, From 8e91ad1b561326d7000a1f9f00785a1b00ff290b Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 3 Sep 2023 12:29:06 -0700 Subject: [PATCH 06/13] fix perf overlay text. --- flow/layers/performance_overlay_layer.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/flow/layers/performance_overlay_layer.cc b/flow/layers/performance_overlay_layer.cc index 911131fd0cfbc..67ff9c5827c4d 100644 --- a/flow/layers/performance_overlay_layer.cc +++ b/flow/layers/performance_overlay_layer.cc @@ -12,6 +12,7 @@ #include "flow/stopwatch.h" #include "flow/stopwatch_dl.h" #include "flow/stopwatch_sk.h" +#include "impeller/typographer/backends/skia/text_frame_skia.h" #include "third_party/skia/include/core/SkFont.h" #include "third_party/skia/include/core/SkTextBlob.h" @@ -50,7 +51,12 @@ void VisualizeStopWatch(DlCanvas* canvas, stopwatch, label_prefix, font_path); // Historically SK_ColorGRAY (== 0xFF888888) was used here DlPaint paint(0xFF888888); - canvas->DrawTextBlob(text, x + label_x, y + height + label_y, paint); + if (impeller_enabled) { + canvas->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(text), + x + label_x, y + height + label_y, paint); + } else { + canvas->DrawTextBlob(text, x + label_x, y + height + label_y, paint); + } } } From a2f9ad8aa9b3a85eacdc3ff5d80b49a58ff75357 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 3 Sep 2023 13:33:37 -0700 Subject: [PATCH 07/13] fix fuchsia compilation --- flow/layers/performance_overlay_layer.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/flow/layers/performance_overlay_layer.cc b/flow/layers/performance_overlay_layer.cc index 67ff9c5827c4d..7223393ed07aa 100644 --- a/flow/layers/performance_overlay_layer.cc +++ b/flow/layers/performance_overlay_layer.cc @@ -12,9 +12,11 @@ #include "flow/stopwatch.h" #include "flow/stopwatch_dl.h" #include "flow/stopwatch_sk.h" -#include "impeller/typographer/backends/skia/text_frame_skia.h" #include "third_party/skia/include/core/SkFont.h" #include "third_party/skia/include/core/SkTextBlob.h" +#ifdef IMPELLER_SUPPORTS_RENDERING +#include "impeller/typographer/backends/skia/text_frame_skia.h" +#endif // IMPELLER_SUPPORTS_RENDERING namespace flutter { namespace { @@ -51,12 +53,14 @@ void VisualizeStopWatch(DlCanvas* canvas, stopwatch, label_prefix, font_path); // Historically SK_ColorGRAY (== 0xFF888888) was used here DlPaint paint(0xFF888888); +#ifdef IMPELLER_SUPPORTS_RENDERING if (impeller_enabled) { canvas->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(text), x + label_x, y + height + label_y, paint); - } else { - canvas->DrawTextBlob(text, x + label_x, y + height + label_y, paint); } +#endif // IMPELLER_SUPPORTS_RENDERING + canvas->DrawTextBlob(text, x + label_x, y + height + label_y, paint); + return; } } From 42da42a7dfc43d893292b71659641d922f0bf909 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 3 Sep 2023 13:51:01 -0700 Subject: [PATCH 08/13] update build file --- flow/BUILD.gn | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flow/BUILD.gn b/flow/BUILD.gn index 53de13474e316..9e360169b8903 100644 --- a/flow/BUILD.gn +++ b/flow/BUILD.gn @@ -99,7 +99,7 @@ source_set("flow") { deps = [ "//third_party/skia" ] if (impeller_supports_rendering) { - deps += [ "//flutter/impeller" ] + deps += [ "//flutter/impeller", "//flutter/impeller/typographer/backends/skia" ] } } From 95c7f23c2aeea39e4f2afc00b4be7625f219925c Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 3 Sep 2023 13:51:23 -0700 Subject: [PATCH 09/13] ++ --- flow/BUILD.gn | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/flow/BUILD.gn b/flow/BUILD.gn index 9e360169b8903..78674fafd3237 100644 --- a/flow/BUILD.gn +++ b/flow/BUILD.gn @@ -99,7 +99,10 @@ source_set("flow") { deps = [ "//third_party/skia" ] if (impeller_supports_rendering) { - deps += [ "//flutter/impeller", "//flutter/impeller/typographer/backends/skia" ] + deps += [ + "//flutter/impeller", + "//flutter/impeller/typographer/backends/skia", + ] } } From 9454209bdbf5dcb4b5fb6ce4112204aaf5c2c9ef Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 3 Sep 2023 14:50:32 -0700 Subject: [PATCH 10/13] ++ --- flow/BUILD.gn | 2 +- flow/layers/performance_overlay_layer.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/flow/BUILD.gn b/flow/BUILD.gn index 78674fafd3237..a75605749c7aa 100644 --- a/flow/BUILD.gn +++ b/flow/BUILD.gn @@ -101,7 +101,7 @@ source_set("flow") { if (impeller_supports_rendering) { deps += [ "//flutter/impeller", - "//flutter/impeller/typographer/backends/skia", + "//flutter/impeller/typographer/backends/skia:typographer_skia_backend", ] } } diff --git a/flow/layers/performance_overlay_layer.cc b/flow/layers/performance_overlay_layer.cc index 7223393ed07aa..e8d11adc7cf36 100644 --- a/flow/layers/performance_overlay_layer.cc +++ b/flow/layers/performance_overlay_layer.cc @@ -15,7 +15,7 @@ #include "third_party/skia/include/core/SkFont.h" #include "third_party/skia/include/core/SkTextBlob.h" #ifdef IMPELLER_SUPPORTS_RENDERING -#include "impeller/typographer/backends/skia/text_frame_skia.h" +#include "impeller/typographer/backends/skia/text_frame_skia.h" // nogncheck #endif // IMPELLER_SUPPORTS_RENDERING namespace flutter { From bea2189ebe4e4dcc1de592284772a3a7bec8246e Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 3 Sep 2023 14:58:44 -0700 Subject: [PATCH 11/13] ++ --- flow/layers/performance_overlay_layer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flow/layers/performance_overlay_layer.cc b/flow/layers/performance_overlay_layer.cc index e8d11adc7cf36..1c1ebb4566798 100644 --- a/flow/layers/performance_overlay_layer.cc +++ b/flow/layers/performance_overlay_layer.cc @@ -15,7 +15,7 @@ #include "third_party/skia/include/core/SkFont.h" #include "third_party/skia/include/core/SkTextBlob.h" #ifdef IMPELLER_SUPPORTS_RENDERING -#include "impeller/typographer/backends/skia/text_frame_skia.h" // nogncheck +#include "impeller/typographer/backends/skia/text_frame_skia.h" // nogncheck #endif // IMPELLER_SUPPORTS_RENDERING namespace flutter { From bb4452fa25a919e53c014fc7b11539249fad9ffe Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 5 Sep 2023 14:04:00 -0700 Subject: [PATCH 12/13] add comment and use IMPELLER_SUPPORTS_RENDERING. --- third_party/txt/src/skia/paragraph_skia.cc | 28 +++++++++++++--------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/third_party/txt/src/skia/paragraph_skia.cc b/third_party/txt/src/skia/paragraph_skia.cc index 0ccd378e16852..a688720cfced9 100644 --- a/third_party/txt/src/skia/paragraph_skia.cc +++ b/third_party/txt/src/skia/paragraph_skia.cc @@ -83,19 +83,21 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { size_t paint_id = std::get(paint); FML_DCHECK(paint_id < dl_paints_.size()); - if (ShouldRenderAsPath(dl_paints_[paint_id])) { - auto path = skia::textlayout::Paragraph::GetPath(blob.get()); - auto transformed = path.makeTransform(SkMatrix::Translate( - x + blob->bounds().left(), y + blob->bounds().top())); - builder_->DrawPath(transformed, dl_paints_[paint_id]); - return; - } - +#ifdef IMPELLER_SUPPORTS_RENDERING if (impeller_enabled_) { + if (ShouldRenderAsPath(dl_paints_[paint_id])) { + auto path = skia::textlayout::Paragraph::GetPath(blob.get()); + auto transformed = path.makeTransform(SkMatrix::Translate( + x + blob->bounds().left(), y + blob->bounds().top())); + builder_->DrawPath(transformed, dl_paints_[paint_id]); + return; + } + builder_->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(blob), x, y, dl_paints_[paint_id]); return; } +#endif // IMPELLER_SUPPORTS_RENDERING builder_->DrawTextBlob(blob, x, y, dl_paints_[paint_id]); } @@ -151,11 +153,13 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { // the line directly using the `drawLine` API instead of using a path effect // (because Impeller does not support path effects). auto dash_path_effect = decor_style.getDashPathEffect(); +#ifdef IMPELLER_SUPPORTS_RENDERING if (impeller_enabled_ && dash_path_effect) { auto path = dashedLine(x0, x1, y0, *dash_path_effect); builder_->DrawPath(path, toDlPaint(decor_style)); return; } +#endif // IMPELLER_SUPPORTS_RENDERING auto paint = toDlPaint(decor_style); if (dash_path_effect) { @@ -203,10 +207,12 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { } bool ShouldRenderAsPath(const DlPaint& paint) const { + FML_DCHECK(impeller_enabled_); // Text with non-trivial color sources or stroke paint mode should be - // rendered as a path when running on Impeller. - return impeller_enabled_ && - ((paint.getColorSource() && !paint.getColorSource()->asColor()) || + // rendered as a path when running on Impeller for correctness. These + // filters rely on having the glyph coverage, whereas regular text is + // drawn as rectangular texture samples. + return ((paint.getColorSource() && !paint.getColorSource()->asColor()) || paint.getDrawStyle() == DlDrawStyle::kStroke); } From 91e4d3e86c1ee9b790ac05cc4702a92c830ea6e4 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 5 Sep 2023 15:13:01 -0700 Subject: [PATCH 13/13] ++ --- third_party/txt/tests/paragraph_unittests.cc | 26 +++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/third_party/txt/tests/paragraph_unittests.cc b/third_party/txt/tests/paragraph_unittests.cc index 0c0a37984ea5b..7eaf771c6cde8 100644 --- a/third_party/txt/tests/paragraph_unittests.cc +++ b/third_party/txt/tests/paragraph_unittests.cc @@ -144,6 +144,19 @@ TEST_F(PainterTest, DrawsSolidLineSkia) { EXPECT_FALSE(recorder.hasPathEffect()); } +TEST_F(PainterTest, DrawDashedLineSkia) { + PretendImpellerIsEnabled(false); + + auto recorder = DlOpRecorder(); + draw(makeDecoratedStyle(txt::TextDecorationStyle::kDashed)) + ->Dispatch(recorder); + + // Skia draws a dashed underline as a filled rectangle with a path effect. + EXPECT_EQ(recorder.lineCount(), 1); + EXPECT_TRUE(recorder.hasPathEffect()); +} + +#ifdef IMPELLER_SUPPORTS_RENDERING TEST_F(PainterTest, DrawsSolidLineImpeller) { PretendImpellerIsEnabled(true); @@ -157,18 +170,6 @@ TEST_F(PainterTest, DrawsSolidLineImpeller) { EXPECT_FALSE(recorder.hasPathEffect()); } -TEST_F(PainterTest, DrawDashedLineSkia) { - PretendImpellerIsEnabled(false); - - auto recorder = DlOpRecorder(); - draw(makeDecoratedStyle(txt::TextDecorationStyle::kDashed)) - ->Dispatch(recorder); - - // Skia draws a dashed underline as a filled rectangle with a path effect. - EXPECT_EQ(recorder.lineCount(), 1); - EXPECT_TRUE(recorder.hasPathEffect()); -} - TEST_F(PainterTest, DrawDashedLineImpeller) { PretendImpellerIsEnabled(true); @@ -238,6 +239,7 @@ TEST_F(PainterTest, DrawTextBlobNoImpeller) { EXPECT_EQ(recorder.textFrameCount(), 0); EXPECT_EQ(recorder.blobCount(), 1); } +#endif // IMPELLER_SUPPORTS_RENDERING } // namespace testing } // namespace flutter