Refactor text and runtime effect to separate skia and impeller implementations. #174219

eyebrowsoffire · 2025-08-21T15:39:20Z

This makes two refactors to the display list architecture:

The concrete implementations of DlRuntimeEffect for skia and impeller are separated, and the impeller implementation put into the impeller/display_list target. This makes sure that a client can link against the main display_list library without actually pulling in all of impeller. (This is needed for [skwasm] Port to DisplayList objects #172314)
The DrawTextBlob and DrawTextFrame methods are consolidated into one DrawText call, and that takes a DlText object. The DlText object has two implementations, one for skia and one for impeller, and the impeller one is moved into impeller/display_list for the same reason mentioned above.

…entations.

flar

Some things I should have noted before, but most of them are style and better testing related...

flar · 2025-08-21T19:57:27Z

.vscode/settings.json

@@ -5,5 +5,57 @@
    "githubPullRequests.ignoredPullRequestBranches": [
        "master"
    ],
-    "files.trimTrailingWhitespace": true
+    "files.trimTrailingWhitespace": true,
+    "files.associations": {


Is this part of the other PR? It doesn't make sense as part of this PR unless I've missed something...

Gahhh this freaking file keeps changing itself and I accidentally added it. I will unstage this change.

flar · 2025-08-21T20:07:57Z

engine/src/flutter/display_list/dl_op_records.h

-    receiver.drawTextFrame(text_frame, x, y);
+  DisplayListCompare equals(const DrawTextOp* other) const {
+    return text->getTextBlob() == other->text->getTextBlob() &&
+                   text->getTextFrame() == other->text->getTextFrame() &&


Perhaps put this pointer comparison inside a DlText::== operator to hide the details (and in case we eventually have some better way to compare them than by (pair of) subsumed pointer(s)?)

flar · 2025-08-21T20:12:15Z

engine/src/flutter/display_list/dl_text.h

+namespace flutter {
+class DlText {
+ public:
+  virtual DlRect getBounds() const = 0;


I know there is a mixed style in DL objects, but I think we are moving towards capitalized getters like GetBounds, GetTextFrame, etc. I wasn't doing that until I started integrating with Impeller, but I think it is better matched to the C++ style guidelines so I'd like to move in that direction.

Technically you can have a strict getter that is lower case, but I don't think either of getTextFoo() are "strict getters" since they are only defined for specific subclasses...

flar · 2025-08-21T20:13:36Z

engine/src/flutter/display_list/dl_text.h

+#define FLUTTER_DISPLAY_LIST_DL_TEXT_H_
+
+#include "flutter/display_list/geometry/dl_geometry_types.h"
+#include "third_party/skia/include/core/SkRefCnt.h"


I think we can change the getter to SkTextBlob* and we won't need SkRefCnt included here...?

flar · 2025-08-21T20:15:00Z

engine/src/flutter/display_list/dl_text.h

+ public:
+  virtual DlRect getBounds() const = 0;
+  virtual std::shared_ptr<impeller::TextFrame> getTextFrame() const = 0;
+  virtual sk_sp<SkTextBlob> getTextBlob() const = 0;


One of the magic things about sk_sp is that you can just use a raw pointer in cases where you are just passing it along (because the ref count is built-in and there is free "shared from this"). So, this could return SkTextBlob*. Indeed, if you look at SkCanvas::drawTextBlob, the real method takes the ptr and the version that takes an sk_sp does a get() on it and passes it to the pointer version...

flar · 2025-08-21T20:39:19Z

engine/src/flutter/flow/surface.h

@@ -12,6 +12,8 @@
 #include "flutter/flow/surface_frame.h"
 #include "flutter/fml/macros.h"

+#include "third_party/skia/include/core/SkData.h"


Do we need this for the DlText changes?

SurfaceData below uses an SkData. I think we previously got away with it through some kind of transitive include.

flar · 2025-08-21T20:42:02Z

engine/src/flutter/impeller/display_list/dl_dispatcher.cc

-  }
+// 2x3 2D affine subset of a 4x4 transform in row major order
+void FirstPassDispatcher::transform2DAffine(
+  DlScalar mxx, DlScalar mxy, DlScalar mxt,


I think your indent is off by 2 for these. Argument continuations are indented 4 spots, not 2. Same for FullPerspective below...

These still look off by 2. You got the ones inside the method body, but these argument methods should also be indented by 4 as well.

flar · 2025-08-21T20:49:57Z

engine/src/flutter/display_list/display_list_unittests.cc

@@ -1224,7 +1230,8 @@ TEST_F(DisplayListTest, SingleOpsMightSupportGroupOpacityBlendMode) {
    static auto display_list = builder.Build();
    RUN_TESTS2(canvas.DrawDisplayList(display_list);, false);
  }
-  RUN_TESTS2(canvas.DrawTextBlob(GetTestTextBlob(1), 0, 0, paint);, false);
+  RUN_TESTS2(canvas.DrawText(DlTextSkia::Make(GetTestTextBlob(1)), 0, 0, paint);


There is no test for Impeller Text here (and the one test for it down below is testing a different condition). While the builder code is the same for both, it couldn't hurt to have both tested here so that when we eventually delete Skia we aren't left without any "text" case for this test.

flar · 2025-08-21T20:50:36Z

engine/src/flutter/display_list/display_list_unittests.cc

@@ -3516,7 +3523,7 @@ TEST_F(DisplayListTest, NopOperationsOmittedFromRecords) {
          builder.DrawAtlas(kTestImage1, xforms, rects, nullptr, 2,
                            DlBlendMode::kSrcOver, DlImageSampling::kLinear,
                            nullptr, &paint);
-          builder.DrawTextBlob(GetTestTextBlob(1), 10, 10, paint);
+          builder.DrawText(DlTextSkia::Make(GetTestTextBlob(1)), 10, 10, paint);


Also need a test to make sure that DrawText(Impeller) is also nop'd.

flar · 2025-08-21T20:51:21Z

engine/src/flutter/display_list/display_list_unittests.cc

@@ -5505,13 +5514,16 @@ TEST_F(DisplayListTest, BoundedRenderOpsDoNotReportUnbounded) {
    ASSERT_LT(blob->bounds().width(), draw_rect.GetWidth());
    ASSERT_LT(blob->bounds().height(), draw_rect.GetHeight());

+    auto text = DlTextImpeller::Make(frame);


Or, makefromblob above instead of blob->frame->text.

flar · 2025-08-21T20:59:24Z

engine/src/flutter/txt/src/skia/paragraph_skia.cc

@@ -120,8 +120,10 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter {
    }
    std::shared_ptr<DlText> text;
    if (impeller_enabled_) {
+#if IMPELLER_SUPPORTS_RENDERING


Should this have the if inside it as well? Otherwise we might have a mismatch? Or is that possible?

I don't think it's possible for that to happen, but I can switch this up to cover the whole if statement.

flar · 2025-08-21T23:24:51Z

engine/src/flutter/display_list/dl_op_records.h

-    return text->GetTextBlob() == other->text->GetTextBlob() &&
-                   text->GetTextFrame() == other->text->GetTextFrame() &&
-                   x == other->x && y == other->y
+    return text == other->text && x == other->x && y == other->y


I think this only compares the shared pointers. You can use the Equals mechanism (see SetSharedImageFilterOp) which does pointer comparisons and then also *foo == *bar comparisons if needed. You could also do *text == *(other->text), but that gets dangerous if one of them is null. The Equals family of methods protects against that.

Equals(text, other->text)

flar · 2025-08-21T23:30:36Z

engine/src/flutter/impeller/display_list/dl_dispatcher.cc

+// clang-format off
+// full 4x4 transform in row major order
+void FirstPassDispatcher::transformFullPerspective(
+  DlScalar mxx, DlScalar mxy, DlScalar mxz, DlScalar mxt,


These argument methods are also indented off by 2.

flar · 2025-08-21T23:40:10Z

engine/src/flutter/display_list/dl_op_records.h

@@ -1013,7 +1013,7 @@ struct DrawTextOp final : DrawOpBase {
  void dispatch(DlOpReceiver& receiver) const { receiver.drawText(text, x, y); }

  DisplayListCompare equals(const DrawTextOp* other) const {
-    return text == other->text && x == other->x && y == other->y
+    return *text == *other->text && x == other->x && y == other->y


I'd prefer Equals(text, other->text), but this is likely safe in practice.

Yeah I just reread your comment, I switched it to use Equals() instead

Refactor text and runtime effect to separate skia and impeller implem…

95c4bca

…entations.

github-actions bot added a: text input Entering text in a text field or keyboard related problems engine flutter/engine repository. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Aug 21, 2025

eyebrowsoffire mentioned this pull request Aug 21, 2025

[skwasm] Port to DisplayList objects #172314

Open

This comment was marked as outdated.

Sign in to view

eyebrowsoffire mentioned this pull request Aug 20, 2025

DL/Geometry engine migration tracking issue #161456

Open

37 tasks

eyebrowsoffire requested a review from flar August 21, 2025 16:19

eyebrowsoffire added 2 commits August 21, 2025 12:20

Addressed some review comments from Jim and Gemini.

c5b5a32

Hide DlTextImpeller behind IMPELLER_SUPPORTS_RENDERING

91b6ece

flar requested changes Aug 21, 2025

View reviewed changes

flar reviewed Aug 21, 2025

View reviewed changes

eyebrowsoffire added 2 commits August 21, 2025 16:07

Address Jim's comments.

8b07d66

Implement DlText::==

4313054

flar reviewed Aug 21, 2025

View reviewed changes

eyebrowsoffire added 2 commits August 21, 2025 16:37

Some fixes to DlText equality stuff.

f9a2550

Fix indentation.

240dd87

flar approved these changes Aug 21, 2025

View reviewed changes

eyebrowsoffire added 4 commits August 21, 2025 16:40

Use Equals to protect against nullptr.

3e20588

Hide text frame tests behind IMPELLER_SUPPORTS_RENDERING.

4151a0a

Merge branch 'master' into dl_text_runtimeeffect

451955a

Merge branch 'master' into dl_text_runtimeeffect

18768a7

eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 22, 2025

auto-submit bot added this pull request to the merge queue Aug 22, 2025

Merged via the queue into flutter:master with commit a24dbd5 Aug 22, 2025
186 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 22, 2025

Refactor text and runtime effect to separate skia and impeller implementations. #174219

Refactor text and runtime effect to separate skia and impeller implementations. #174219

Conversation

eyebrowsoffire commented Aug 21, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!