From 62031173dd1496d7dafee314a52a33ce2cfc7413 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Tue, 5 Sep 2023 17:50:48 -0700 Subject: [PATCH] [Impeller] construct text frames on UI thread. (flutter/engine#45418) Conversion of SkTextBlobs to impeller::TextFrame objects is one of the most expensive operations in display list dispatching. While the rest of the engine and framework makes a reasonable attempt to cache the SkTextBlobs generated during paragraph construction, the design of the dl dispatcher means that these the Impeller backend will always reconstruct all text frames on each frame - even if the display list/picture that contained those text frames was unchanged. Removing this overhead is one of the goals of https://github.com/flutter/engine/pull/45386 , however this patch is also fairly risky and will be difficult to land. As a more incremental solution, we can instead construct the impeller::TextFrame objects when performing paragraph painting and record them in the display list. This both moves the text frame construction to the UI thread and allows the framework/engine to cache unchanged text frames. This also does not conflict with the dl_aiks_canvas patch directly, and is fine to land before or after it does. (though I'd argue we should land this first). To compare the current performance levels, I ran the complex_layout_scroll perf test, since this is fairly text filled. On a Pixel 6 pro. Across several runs this is a fairly consistent ~1ms raster time improvement. ### Skia ``` "average_frame_build_time_millis": 1.497333333333333, "90th_percentile_frame_build_time_millis": 2.038, "99th_percentile_frame_build_time_millis": 17.686, "worst_frame_build_time_millis": 23.095, "missed_frame_build_budget_count": 3, "average_frame_rasterizer_time_millis": 5.5078589743589745, "stddev_frame_rasterizer_time_millis": 2.226343414420338, "90th_percentile_frame_rasterizer_time_millis": 7.481, "99th_percentile_frame_rasterizer_time_millis": 19.11, "worst_frame_rasterizer_time_millis": 79.799, "missed_frame_rasterizer_budget_count": 7, "frame_count": 234, "frame_rasterizer_count": 234, "new_gen_gc_count": 10, "old_gen_gc_count": 2, ``` ### Impeller (ToT) ``` "average_frame_build_time_millis": 1.431575000000001, "90th_percentile_frame_build_time_millis": 2.196, "99th_percentile_frame_build_time_millis": 14.486, "worst_frame_build_time_millis": 23.728, "missed_frame_build_budget_count": 2, "average_frame_rasterizer_time_millis": 6.536087499999999, "stddev_frame_rasterizer_time_millis": 1.9902712500000004, "90th_percentile_frame_rasterizer_time_millis": 9.705, "99th_percentile_frame_rasterizer_time_millis": 14.727, "worst_frame_rasterizer_time_millis": 17.838, "missed_frame_rasterizer_budget_count": 1, "frame_count": 240, "frame_rasterizer_count": 240, "new_gen_gc_count": 10, "old_gen_gc_count": 2, ``` ### Impeller (Patched) ``` "average_frame_build_time_millis": 1.4500167364016743, "90th_percentile_frame_build_time_millis": 2.478, "99th_percentile_frame_build_time_millis": 14.883, "worst_frame_build_time_millis": 18.782, "missed_frame_build_budget_count": 1, "average_frame_rasterizer_time_millis": 5.023033333333336, "stddev_frame_rasterizer_time_millis": 1.6445388888888894, "90th_percentile_frame_rasterizer_time_millis": 7.814, "99th_percentile_frame_rasterizer_time_millis": 13.497, "worst_frame_rasterizer_time_millis": 15.008, "missed_frame_rasterizer_budget_count": 0, "frame_count": 239, "frame_rasterizer_count": 240, "new_gen_gc_count": 8, "old_gen_gc_count": 0, ``` --- engine/src/flutter/display_list/BUILD.gn | 1 + .../benchmarking/dl_complexity_gl.cc | 5 + .../benchmarking/dl_complexity_gl.h | 3 + .../benchmarking/dl_complexity_metal.cc | 5 + .../benchmarking/dl_complexity_metal.h | 3 + .../src/flutter/display_list/display_list.h | 1 + engine/src/flutter/display_list/dl_builder.cc | 42 +++++ engine/src/flutter/display_list/dl_builder.h | 10 ++ engine/src/flutter/display_list/dl_canvas.h | 11 +- .../src/flutter/display_list/dl_op_receiver.h | 4 + .../src/flutter/display_list/dl_op_records.h | 20 +++ .../flutter/display_list/skia/dl_sk_canvas.cc | 8 + .../flutter/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 + engine/src/flutter/flow/BUILD.gn | 5 +- .../flow/layers/performance_overlay_layer.cc | 10 ++ .../flutter/impeller/aiks/aiks_unittests.cc | 16 +- engine/src/flutter/impeller/aiks/canvas.cc | 4 +- engine/src/flutter/impeller/aiks/canvas.h | 2 +- .../impeller/display_list/dl_dispatcher.cc | 21 +-- .../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 | 13 +- .../backends/skia/text_frame_skia.h | 3 +- .../backends/stb/text_frame_stb.cc | 10 +- .../typographer/backends/stb/text_frame_stb.h | 7 +- .../typographer/typographer_context.h | 1 - .../typographer/typographer_unittests.cc | 47 +++--- engine/src/flutter/shell/common/dl_op_spy.cc | 8 + engine/src/flutter/shell/common/dl_op_spy.h | 3 + .../flutter/testing/display_list_testing.cc | 9 ++ .../flutter/testing/display_list_testing.h | 3 + engine/src/flutter/testing/mock_canvas.cc | 8 + engine/src/flutter/testing/mock_canvas.h | 4 + engine/src/flutter/third_party/txt/BUILD.gn | 1 + .../txt/src/skia/paragraph_skia.cc | 46 +++++- .../txt/tests/paragraph_unittests.cc | 153 ++++++++++++++---- 41 files changed, 411 insertions(+), 125 deletions(-) diff --git a/engine/src/flutter/display_list/BUILD.gn b/engine/src/flutter/display_list/BUILD.gn index 97c9683511b..736e13dc5c7 100644 --- a/engine/src/flutter/display_list/BUILD.gn +++ b/engine/src/flutter/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/engine/src/flutter/display_list/benchmarking/dl_complexity_gl.cc b/engine/src/flutter/display_list/benchmarking/dl_complexity_gl.cc index f8dd1c52a12..53b70e2c24e 100644 --- a/engine/src/flutter/display_list/benchmarking/dl_complexity_gl.cc +++ b/engine/src/flutter/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/engine/src/flutter/display_list/benchmarking/dl_complexity_gl.h b/engine/src/flutter/display_list/benchmarking/dl_complexity_gl.h index 9fc75966870..5115bb4d6ec 100644 --- a/engine/src/flutter/display_list/benchmarking/dl_complexity_gl.h +++ b/engine/src/flutter/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/engine/src/flutter/display_list/benchmarking/dl_complexity_metal.cc b/engine/src/flutter/display_list/benchmarking/dl_complexity_metal.cc index 56d4f3b406a..c6b6547afee 100644 --- a/engine/src/flutter/display_list/benchmarking/dl_complexity_metal.cc +++ b/engine/src/flutter/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/engine/src/flutter/display_list/benchmarking/dl_complexity_metal.h b/engine/src/flutter/display_list/benchmarking/dl_complexity_metal.h index aa63863fa4d..dd068e2fa32 100644 --- a/engine/src/flutter/display_list/benchmarking/dl_complexity_metal.h +++ b/engine/src/flutter/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/engine/src/flutter/display_list/display_list.h b/engine/src/flutter/display_list/display_list.h index 668a247d559..3d4a7acff58 100644 --- a/engine/src/flutter/display_list/display_list.h +++ b/engine/src/flutter/display_list/display_list.h @@ -136,6 +136,7 @@ namespace flutter { \ V(DrawDisplayList) \ V(DrawTextBlob) \ + V(DrawTextFrame) \ \ V(DrawShadow) \ V(DrawShadowTransparentOccluder) diff --git a/engine/src/flutter/display_list/dl_builder.cc b/engine/src/flutter/display_list/dl_builder.cc index 2906f9a85fd..f20b8fe82ec 100644 --- a/engine/src/flutter/display_list/dl_builder.cc +++ b/engine/src/flutter/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/engine/src/flutter/display_list/dl_builder.h b/engine/src/flutter/display_list/dl_builder.h index ab0bce0db3c..3a65b8fad53 100644 --- a/engine/src/flutter/display_list/dl_builder.h +++ b/engine/src/flutter/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/engine/src/flutter/display_list/dl_canvas.h b/engine/src/flutter/display_list/dl_canvas.h index f04645c1577..5cd06146c78 100644 --- a/engine/src/flutter/display_list/dl_canvas.h +++ b/engine/src/flutter/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/engine/src/flutter/display_list/dl_op_receiver.h b/engine/src/flutter/display_list/dl_op_receiver.h index 23a0aaf6fd9..d9bbc7b7f4f 100644 --- a/engine/src/flutter/display_list/dl_op_receiver.h +++ b/engine/src/flutter/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/engine/src/flutter/display_list/dl_op_records.h b/engine/src/flutter/display_list/dl_op_records.h index 47cdeb9d0e6..a1c7835e43d 100644 --- a/engine/src/flutter/display_list/dl_op_records.h +++ b/engine/src/flutter/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/engine/src/flutter/display_list/skia/dl_sk_canvas.cc b/engine/src/flutter/display_list/skia/dl_sk_canvas.cc index 9299c9a6ff9..34270bd8801 100644 --- a/engine/src/flutter/display_list/skia/dl_sk_canvas.cc +++ b/engine/src/flutter/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/engine/src/flutter/display_list/skia/dl_sk_canvas.h b/engine/src/flutter/display_list/skia/dl_sk_canvas.h index 0377b7de75e..f085d35ae8c 100644 --- a/engine/src/flutter/display_list/skia/dl_sk_canvas.h +++ b/engine/src/flutter/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/engine/src/flutter/display_list/skia/dl_sk_dispatcher.cc b/engine/src/flutter/display_list/skia/dl_sk_dispatcher.cc index def08fa8886..f8cc16d1151 100644 --- a/engine/src/flutter/display_list/skia/dl_sk_dispatcher.cc +++ b/engine/src/flutter/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/engine/src/flutter/display_list/skia/dl_sk_dispatcher.h b/engine/src/flutter/display_list/skia/dl_sk_dispatcher.h index a7fc73bbe2a..ee6101291e7 100644 --- a/engine/src/flutter/display_list/skia/dl_sk_dispatcher.h +++ b/engine/src/flutter/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/engine/src/flutter/display_list/utils/dl_receiver_utils.h b/engine/src/flutter/display_list/utils/dl_receiver_utils.h index e2d07310368..2a1f9cb7eff 100644 --- a/engine/src/flutter/display_list/utils/dl_receiver_utils.h +++ b/engine/src/flutter/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/engine/src/flutter/flow/BUILD.gn b/engine/src/flutter/flow/BUILD.gn index 53de13474e3..a75605749c7 100644 --- a/engine/src/flutter/flow/BUILD.gn +++ b/engine/src/flutter/flow/BUILD.gn @@ -99,7 +99,10 @@ source_set("flow") { deps = [ "//third_party/skia" ] if (impeller_supports_rendering) { - deps += [ "//flutter/impeller" ] + deps += [ + "//flutter/impeller", + "//flutter/impeller/typographer/backends/skia:typographer_skia_backend", + ] } } diff --git a/engine/src/flutter/flow/layers/performance_overlay_layer.cc b/engine/src/flutter/flow/layers/performance_overlay_layer.cc index 911131fd0cf..1c1ebb45667 100644 --- a/engine/src/flutter/flow/layers/performance_overlay_layer.cc +++ b/engine/src/flutter/flow/layers/performance_overlay_layer.cc @@ -14,6 +14,9 @@ #include "flow/stopwatch_sk.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" // nogncheck +#endif // IMPELLER_SUPPORTS_RENDERING namespace flutter { namespace { @@ -50,7 +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); + } +#endif // IMPELLER_SUPPORTS_RENDERING canvas->DrawTextBlob(text, x + label_x, y + height + label_y, paint); + return; } } diff --git a/engine/src/flutter/impeller/aiks/aiks_unittests.cc b/engine/src/flutter/impeller/aiks/aiks_unittests.cc index 5245c43da3a..02388334f45 100644 --- a/engine/src/flutter/impeller/aiks/aiks_unittests.cc +++ b/engine/src/flutter/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); 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); 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); canvas.DrawTextFrame(frame, Point(), text_paint); ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); diff --git a/engine/src/flutter/impeller/aiks/canvas.cc b/engine/src/flutter/impeller/aiks/canvas.cc index a5ae1c71cc1..d2bab026df2 100644 --- a/engine/src/flutter/impeller/aiks/canvas.cc +++ b/engine/src/flutter/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/engine/src/flutter/impeller/aiks/canvas.h b/engine/src/flutter/impeller/aiks/canvas.h index 84f1c486bc3..d36bc3ff589 100644 --- a/engine/src/flutter/impeller/aiks/canvas.h +++ b/engine/src/flutter/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/engine/src/flutter/impeller/display_list/dl_dispatcher.cc b/engine/src/flutter/impeller/display_list/dl_dispatcher.cc index 1ae6e492062..2ba40a16463 100644 --- a/engine/src/flutter/impeller/display_list/dl_dispatcher.cc +++ b/engine/src/flutter/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,14 @@ 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, Point(x + bounds.left(), y + bounds.top())); - canvas_.DrawPath(path, paint_); - return; - } + // When running with Impeller enabled Skia text blobs are converted to + // Impeller text frames in paragraph_skia.cc + 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/engine/src/flutter/impeller/display_list/dl_dispatcher.h b/engine/src/flutter/impeller/display_list/dl_dispatcher.h index d9a36e22399..0dc3b7ce724 100644 --- a/engine/src/flutter/impeller/display_list/dl_dispatcher.h +++ b/engine/src/flutter/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/engine/src/flutter/impeller/entity/contents/text_contents.cc b/engine/src/flutter/impeller/entity/contents/text_contents.cc index fceab90a6ed..c17c47ae20b 100644 --- a/engine/src/flutter/impeller/entity/contents/text_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/text_contents.cc @@ -24,8 +24,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( @@ -49,7 +49,7 @@ Color TextContents::GetColor() const { } bool TextContents::CanInheritOpacity(const Entity& entity) const { - return !frame_.MaybeHasOverlapping(); + return !frame_->MaybeHasOverlapping(); } void TextContents::SetInheritedOpacity(Scalar opacity) { @@ -61,13 +61,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; } @@ -79,7 +79,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()); @@ -152,7 +152,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 +163,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/engine/src/flutter/impeller/entity/contents/text_contents.h b/engine/src/flutter/impeller/entity/contents/text_contents.h index 6e3b89d558e..07d191ef3ca 100644 --- a/engine/src/flutter/impeller/entity/contents/text_contents.h +++ b/engine/src/flutter/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/engine/src/flutter/impeller/entity/entity_unittests.cc b/engine/src/flutter/impeller/entity/entity_unittests.cc index 5af2bd9d23b..127d1c0bf9a 100644 --- a/engine/src/flutter/impeller/entity/entity_unittests.cc +++ b/engine/src/flutter/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); 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/engine/src/flutter/impeller/typographer/backends/skia/text_frame_skia.cc b/engine/src/flutter/impeller/typographer/backends/skia/text_frame_skia.cc index 11c256a8af9..b5d7ee8c468 100644 --- a/engine/src/flutter/impeller/typographer/backends/skia/text_frame_skia.cc +++ b/engine/src/flutter/impeller/typographer/backends/skia/text_frame_skia.cc @@ -8,7 +8,6 @@ #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" @@ -39,16 +38,10 @@ static Rect ToRect(const SkRect& rect) { static constexpr Scalar kScaleSize = 100000.0f; -std::optional MakeTextFrameFromTextBlobSkia( +std::shared_ptr 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::vector runs; 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 @@ -95,7 +88,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/engine/src/flutter/impeller/typographer/backends/skia/text_frame_skia.h b/engine/src/flutter/impeller/typographer/backends/skia/text_frame_skia.h index 53170a248d3..e7721a0c6e9 100644 --- a/engine/src/flutter/impeller/typographer/backends/skia/text_frame_skia.h +++ b/engine/src/flutter/impeller/typographer/backends/skia/text_frame_skia.h @@ -4,13 +4,12 @@ #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( +std::shared_ptr MakeTextFrameFromTextBlobSkia( const sk_sp& blob); } // namespace impeller diff --git a/engine/src/flutter/impeller/typographer/backends/stb/text_frame_stb.cc b/engine/src/flutter/impeller/typographer/backends/stb/text_frame_stb.cc index 5e6060ba325..e1d1dd0d1c6 100644 --- a/engine/src/flutter/impeller/typographer/backends/stb/text_frame_stb.cc +++ b/engine/src/flutter/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/engine/src/flutter/impeller/typographer/backends/stb/text_frame_stb.h b/engine/src/flutter/impeller/typographer/backends/stb/text_frame_stb.h index 622cb1f3b72..39f8cccc768 100644 --- a/engine/src/flutter/impeller/typographer/backends/stb/text_frame_stb.h +++ b/engine/src/flutter/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/engine/src/flutter/impeller/typographer/typographer_context.h b/engine/src/flutter/impeller/typographer/typographer_context.h index 1ca4d4a7687..3714dd80407 100644 --- a/engine/src/flutter/impeller/typographer/typographer_context.h +++ b/engine/src/flutter/impeller/typographer/typographer_context.h @@ -4,7 +4,6 @@ #pragma once -#include #include #include "flutter/fml/macros.h" diff --git a/engine/src/flutter/impeller/typographer/typographer_unittests.cc b/engine/src/flutter/impeller/typographer/typographer_unittests.cc index a996bdf98d9..eb3581d750f 100644 --- a/engine/src/flutter/impeller/typographer/typographer_unittests.cc +++ b/engine/src/flutter/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); + 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)); 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); - 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)); - 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)); 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)); 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)); ASSERT_EQ(atlas, next_atlas); ASSERT_EQ(atlas_context->GetGlyphAtlas(), atlas); } @@ -197,7 +196,7 @@ 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( + MakeTextFrameFromTextBlobSkia(blob)->CollectUniqueFontGlyphPairs( font_glyph_map, 0.6 * index); }; auto atlas = @@ -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)); 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)); 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)); 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)); ASSERT_NE(atlas, next_atlas); auto* second_texture = next_atlas->GetTexture().get(); @@ -297,15 +296,13 @@ TEST_P(TypographerTest, MaybeHasOverlapping) { SkFont sk_font(typeface, 0.5f); auto frame = - MakeTextFrameFromTextBlobSkia(SkTextBlob::MakeFromString("1", sk_font)) - .value(); + MakeTextFrameFromTextBlobSkia(SkTextBlob::MakeFromString("1", sk_font)); // 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)); + ASSERT_FALSE(frame_2->MaybeHasOverlapping()); } TEST_P(TypographerTest, RectanglePackerAddsNonoverlapingRectangles) { diff --git a/engine/src/flutter/shell/common/dl_op_spy.cc b/engine/src/flutter/shell/common/dl_op_spy.cc index 15910fc2e85..bcf213bffa8 100644 --- a/engine/src/flutter/shell/common/dl_op_spy.cc +++ b/engine/src/flutter/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/engine/src/flutter/shell/common/dl_op_spy.h b/engine/src/flutter/shell/common/dl_op_spy.h index 1cb29ff7219..1bcf08c7a84 100644 --- a/engine/src/flutter/shell/common/dl_op_spy.h +++ b/engine/src/flutter/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/engine/src/flutter/testing/display_list_testing.cc b/engine/src/flutter/testing/display_list_testing.cc index f54a90c192d..3ddeeac6cd9 100644 --- a/engine/src/flutter/testing/display_list_testing.cc +++ b/engine/src/flutter/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/engine/src/flutter/testing/display_list_testing.h b/engine/src/flutter/testing/display_list_testing.h index 4f3830a204b..a468deda878 100644 --- a/engine/src/flutter/testing/display_list_testing.h +++ b/engine/src/flutter/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/engine/src/flutter/testing/mock_canvas.cc b/engine/src/flutter/testing/mock_canvas.cc index 75b03107e2a..8d5a51fd1b6 100644 --- a/engine/src/flutter/testing/mock_canvas.cc +++ b/engine/src/flutter/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/engine/src/flutter/testing/mock_canvas.h b/engine/src/flutter/testing/mock_canvas.h index 71260d5b875..591f1c908e9 100644 --- a/engine/src/flutter/testing/mock_canvas.h +++ b/engine/src/flutter/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/engine/src/flutter/third_party/txt/BUILD.gn b/engine/src/flutter/third_party/txt/BUILD.gn index b799bc5dd46..a61438b72c5 100644 --- a/engine/src/flutter/third_party/txt/BUILD.gn +++ b/engine/src/flutter/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/engine/src/flutter/third_party/txt/src/skia/paragraph_skia.cc b/engine/src/flutter/third_party/txt/src/skia/paragraph_skia.cc index 76d737224aa..a688720cfce 100644 --- a/engine/src/flutter/third_party/txt/src/skia/paragraph_skia.cc +++ b/engine/src/flutter/third_party/txt/src/skia/paragraph_skia.cc @@ -18,7 +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 { @@ -65,10 +68,10 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { /// decision (i.e. with `#ifdef`) instead of a runtime option. DisplayListParagraphPainter(DisplayListBuilder* builder, const std::vector& dl_paints, - bool draw_path_effect) + bool impeller_enabled) : builder_(builder), dl_paints_(dl_paints), - draw_path_effect_(draw_path_effect) {} + impeller_enabled_(impeller_enabled) {} void drawTextBlob(const sk_sp& blob, SkScalar x, @@ -79,6 +82,22 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { } size_t paint_id = std::get(paint); FML_DCHECK(paint_id < dl_paints_.size()); + +#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]); } @@ -96,6 +115,11 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { DlBlurMaskFilter filter(DlBlurStyle::kNormal, blur_sigma, false); paint.setMaskFilter(&filter); } + if (impeller_enabled_) { + builder_->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(blob), x, + y, paint); + return; + } builder_->DrawTextBlob(blob, x, y, paint); } @@ -129,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(); - if (draw_path_effect_ && dash_path_effect) { +#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) { @@ -180,6 +206,16 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { return path; } + 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 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); + } + DlPaint toDlPaint(const DecorationStyle& decor_style, DlDrawStyle draw_style = DlDrawStyle::kStroke) { DlPaint paint; @@ -192,7 +228,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 +238,7 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { DisplayListBuilder* builder_; const std::vector& dl_paints_; - bool draw_path_effect_; + const bool impeller_enabled_; }; } // anonymous namespace diff --git a/engine/src/flutter/third_party/txt/tests/paragraph_unittests.cc b/engine/src/flutter/third_party/txt/tests/paragraph_unittests.cc index ec06ce70251..7eaf771c6cd 100644 --- a/engine/src/flutter/third_party/txt/tests/paragraph_unittests.cc +++ b/engine/src/flutter/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" @@ -23,6 +28,8 @@ class DlOpRecorder final : public virtual DlOpReceiver, 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: @@ -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,19 +135,8 @@ TEST_F(PainterTest, DrawsSolidLineSkia) { PretendImpellerIsEnabled(false); auto recorder = DlOpRecorder(); - draw(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 - EXPECT_EQ(recorder.rectCount(), 1); - EXPECT_FALSE(recorder.hasPathEffect()); -} - -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,23 +148,98 @@ 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); EXPECT_TRUE(recorder.hasPathEffect()); } +#ifdef IMPELLER_SUPPORTS_RENDERING +TEST_F(PainterTest, DrawsSolidLineImpeller) { + PretendImpellerIsEnabled(true); + + auto recorder = DlOpRecorder(); + 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 + EXPECT_EQ(recorder.rectCount(), 1); + EXPECT_FALSE(recorder.hasPathEffect()); +} + 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); +} +#endif // IMPELLER_SUPPORTS_RENDERING + } // namespace testing } // namespace flutter