Skip to content

Conversation

@eyebrowsoffire
Copy link
Contributor

@eyebrowsoffire eyebrowsoffire commented Jul 17, 2025

This PR refactors the skwasm renderer to use DisplayList objects as its main model objects instead of using Skia objects directly. Then, at render time, we dispatch the display list commands to the skia surface. This is a preparatory step for impeller on web.

  • Some build rules were reworked in order to allow DisplayList to compile via emscripten
  • Some pieces of the display list library were further refactored to allow us to compile it without actually building and linking the impeller shaders. The two major classes that needed to be separated out were DlRuntimeEffect and the text drawing system.
  • SkPath and SkImage are still used as the main model objects in skwasm. As of right now, DisplayList just thinly wraps these objects, so this is the minimal possible change for now. I will have to refactor this somewhat further when preparing for actual impeller adoption.
  • Several special cased code paths in skwasm were removed, as they are taken care of by DisplayList itself. This includes shadow drawing, determining when to enable dithering, and determining the right clamp value for filters.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems engine flutter/engine related. See also e: labels. platform-web Web applications specifically e: impeller Impeller rendering backend issues and features requests labels Jul 17, 2025
@eyebrowsoffire eyebrowsoffire changed the title Skwasm display list [skwasm] Port to DisplayList objects Aug 20, 2025
@eyebrowsoffire eyebrowsoffire marked this pull request as ready for review August 20, 2025 01:46
Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

The web_ui parts LGTM. I don't know enough about the native engine to comment on the display_list changes except to note that some of them seem orthogonal to the web_ui changes. I think the drawText and runtime effects changes could be a separate PR.

@eyebrowsoffire
Copy link
Contributor Author

The web_ui parts LGTM. I don't know enough about the native engine to comment on the display_list changes except to note that some of them seem orthogonal to the web_ui changes. I think the drawText and runtime effects changes could be a separate PR.

The drawText and runtime effect changes are there to decouple some impeller things from the main display list library. If I didn't do them in this change, depending on the display_list library would pull in a bunch of impeller stuff that doesn't yet compile on emscripten. I'd have to do a bunch more massaging to get that code working on the web build, and I'd rather do that in a subsequent PR when I'm actually adding the impeller backend.

@flar
Copy link
Contributor

flar commented Aug 20, 2025

The web_ui parts LGTM. I don't know enough about the native engine to comment on the display_list changes except to note that some of them seem orthogonal to the web_ui changes. I think the drawText and runtime effects changes could be a separate PR.

The drawText and runtime effect changes are there to decouple some impeller things from the main display list library. If I didn't do them in this change, depending on the display_list library would pull in a bunch of impeller stuff that doesn't yet compile on emscripten. I'd have to do a bunch more massaging to get that code working on the web build, and I'd rather do that in a subsequent PR when I'm actually adding the impeller backend.

I thought the same thing, but you are looking at it backwards. If you land DlText first then it works, but you'd have to hold off on the skwasm changes until that PR lands. Your call, but they can be separated...

@eyebrowsoffire
Copy link
Contributor Author

The web_ui parts LGTM. I don't know enough about the native engine to comment on the display_list changes except to note that some of them seem orthogonal to the web_ui changes. I think the drawText and runtime effects changes could be a separate PR.

The drawText and runtime effect changes are there to decouple some impeller things from the main display list library. If I didn't do them in this change, depending on the display_list library would pull in a bunch of impeller stuff that doesn't yet compile on emscripten. I'd have to do a bunch more massaging to get that code working on the web build, and I'd rather do that in a subsequent PR when I'm actually adding the impeller backend.

I thought the same thing, but you are looking at it backwards. If you land DlText first then it works, but you'd have to hold off on the skwasm changes until that PR lands. Your call, but they can be separated...

Actually yes, you're right. If you think it's important, I can try to split this PR out.

@flar
Copy link
Contributor

flar commented Aug 20, 2025

The web_ui parts LGTM. I don't know enough about the native engine to comment on the display_list changes except to note that some of them seem orthogonal to the web_ui changes. I think the drawText and runtime effects changes could be a separate PR.

The drawText and runtime effect changes are there to decouple some impeller things from the main display list library. If I didn't do them in this change, depending on the display_list library would pull in a bunch of impeller stuff that doesn't yet compile on emscripten. I'd have to do a bunch more massaging to get that code working on the web build, and I'd rather do that in a subsequent PR when I'm actually adding the impeller backend.

I thought the same thing, but you are looking at it backwards. If you land DlText first then it works, but you'd have to hold off on the skwasm changes until that PR lands. Your call, but they can be separated...

Actually yes, you're right. If you think it's important, I can try to split this PR out.

I think it was a missed opportunity, but given the logistics of having them baked independently when they are both ready to go right now is so constrained that there isn't much point of doing it at this stage.

@harryterkelsen
Copy link
Contributor

The web_ui parts LGTM. I don't know enough about the native engine to comment on the display_list changes except to note that some of them seem orthogonal to the web_ui changes. I think the drawText and runtime effects changes could be a separate PR.

The drawText and runtime effect changes are there to decouple some impeller things from the main display list library. If I didn't do them in this change, depending on the display_list library would pull in a bunch of impeller stuff that doesn't yet compile on emscripten. I'd have to do a bunch more massaging to get that code working on the web build, and I'd rather do that in a subsequent PR when I'm actually adding the impeller backend.

I thought the same thing, but you are looking at it backwards. If you land DlText first then it works, but you'd have to hold off on the skwasm changes until that PR lands. Your call, but they can be separated...

Actually yes, you're right. If you think it's important, I can try to split this PR out.

I think it was a missed opportunity, but given the logistics of having them baked independently when they are both ready to go right now is so constrained that there isn't much point of doing it at this stage.

Agreed

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

I think I reviewed all of the DL code here. Lots of suggestions.

void transformReset() override;

void drawTextFrame(const std::shared_ptr<impeller::TextFrame>& text_frame,
void drawText(const std::shared_ptr<flutter::DlText>& text,
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't the following lines fail the format check?

Oh, we turn off formatting for the transform methods (which have a strong preference for how the arguments line up) at line 340-ish and never turn it back on... D'oh!

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just fix the formatting on these 3 lines and leave the "uh oh, we need to turn formatting back on" for a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and fixed the formatting in this file (in the other PR).

// found in the LICENSE file.

#ifndef FLUTTER_DISPLAY_LIST_DL_TEXT_H_
#define FLUTTER_DISPLAY_LIST_DL_TEXT_H_
Copy link
Contributor

Choose a reason for hiding this comment

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

We've been making .cc files for every .h file even if they have no code in them even if just to verify that they can compile without any issues.

See dl_types.h/cc for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, your call as to whether to move the impl into the .cc file. I think it is a decent gesture unless templating requires all of the code to go into the .h file. Though, if it is just getters then it feels like an empty gesture. But we still create the empty .cc file...

// found in the LICENSE file.

#ifndef FLUTTER_DISPLAY_LIST_DL_TEXT_SKIA_H_
#define FLUTTER_DISPLAY_LIST_DL_TEXT_SKIA_H_
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, a .cc file...

DlScalar y,
const DlPaint& paint) override;

void drawTextFrame(const std::shared_ptr<impeller::TextFrame>& text_frame,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does a double take...

Oops, this was supposed to be in the private section anyway. D'oh!


#include "impeller/typographer/text_frame.h"
namespace impeller {
class TextFrame;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this in the .h file?

And do we still need to include SkTextBlob either?

DlBlendMode::kSrcOver, DlImageSampling::kLinear,
nullptr, &paint);
builder.DrawTextBlob(GetTestTextBlob(1), 10, 10, paint);
builder.DrawText(DlTextSkia::Make(GetTestTextBlob(1)), 10, 10, paint);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, DlTextImpeller case if it is easy to make the appropriate test frame.

DlScalar y,
const DlPaint& paint) {

void DisplayListBuilder::DrawText(const std::shared_ptr<DlText>& text,
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit. All of the pairs in this file are "internal op method first, then public canvas method". In the surgery here you've flipped that ordering. (OCD triggering)

DlScalar x,
DlScalar y,
const DlPaint& paint) {
SetAttributesFromPaint(paint, DisplayListOpFlags::kDrawTextBlobFlags);
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename kDrawTextBlobFlags now maybe?

{1, 32, 1,
[](DlOpReceiver& r) {
r.drawTextBlob(GetTestTextBlob(2), 10, 10);
r.drawText(DlTextSkia::Make(GetTestTextBlob(2)), 10, 10);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a (set of) DlTextImpeller case(s). Should it/they be != to all TextSkia cases?

canvas->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(text),
x + label_x, y + height + label_y, paint);
canvas->DrawText(
DlTextImpeller::Make(impeller::MakeTextFrameFromTextBlobSkia(text)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any logistical value for a suggestion to have DlTextImpeller have a "MakeBlob" factory to do the conversion for the caller?

@eyebrowsoffire
Copy link
Contributor Author

Since this is a big PR, and there's a nonzero chance we'll need to revert if something goes wrong, I think it actually is worthwhile to separate the display list stuff out, so I did so in this PR: #174219

I'll address Jim's comments over in that one, and then once we have that one landed I'll rebase this PR and the diffs should be smaller.

@flar
Copy link
Contributor

flar commented Aug 21, 2025

Since this is a big PR, and there's a nonzero chance we'll need to revert if something goes wrong, I think it actually is worthwhile to separate the display list stuff out, so I did so in this PR: #174219

I'll address Jim's comments over in that one, and then once we have that one landed I'll rebase this PR and the diffs should be smaller.

Well the DlText part was 77 of these 104 files which hopefully means the skwasm changes will be much simpler...

github-merge-queue bot pushed a commit that referenced this pull request Aug 22, 2025
…entations. (#174219)

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
#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.
This PR refactors the skwasm renderer to use DisplayList objects as its main model objects instead of using Skia objects directly. Then, at render time, we dispatch the display list commands to the skia surface. This is a preparatory step for impeller on web.

* Some build rules were reworked in order to allow DisplayList to compile via emscripten
* SkPath and SkImage are still used as the main model objects in skwasm. As of right now, DisplayList just thinly wraps these objects, so this is the minimal possible change for now. I will have to refactor this somewhat further when preparing for actual impeller adoption.
* Several special cased code paths in skwasm were removed, as they are taken care of by DisplayList itself. This includes shadow drawing, determining when to enable dithering, and determining the right clamp value for filters.
@eyebrowsoffire
Copy link
Contributor Author

I landed the non-skwasm related changes which do some refactors to DisplayList here: #174219

And I reconfigured this PR to only have the skwasm changes in it now.

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

Deferring to @harryterkelsen now...

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

Can't "unrequest changes" - must approve.

For the record, I didn't really review any of the code that's left, assuming that @harryterkelsen took care of that...

@eyebrowsoffire eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 25, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Aug 25, 2025
Merged via the queue into flutter:master with commit 3340cae Aug 25, 2025
186 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 25, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 25, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 25, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 25, 2025
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Aug 25, 2025
flutter/flutter@edd434a...a4cb00a

2025-08-25 [email protected] Clarify how Gemini should handle conflicting guidelines (flutter/flutter#174294)
2025-08-25 [email protected] [skwasm] Port to `DisplayList` objects (flutter/flutter#172314)
2025-08-25 [email protected] Roll Packages from 092d832 to fe66130 (4 revisions) (flutter/flutter#174373)
2025-08-25 [email protected] Roll Dart SDK from 4f9623f024ab to e283a9e88242 (8 revisions) (flutter/flutter#174358)
2025-08-25 [email protected] Roll Skia from 3bcd0a1d8c48 to da724d312e65 (1 revision) (flutter/flutter#174357)
2025-08-25 [email protected] Roll Skia from 8689a1169a32 to 3bcd0a1d8c48 (4 revisions) (flutter/flutter#174353)
2025-08-25 [email protected] Roll Fuchsia Linux SDK from 21v1vYTYWmyEHu-eP... to UiY8gj468PZUj6QTm... (flutter/flutter#174349)
2025-08-24 [email protected] Roll Skia from 61169c1f6f7c to 8689a1169a32 (2 revisions) (flutter/flutter#174343)
2025-08-24 [email protected] Roll Fuchsia Linux SDK from XLSNQCsY1VkIthSjt... to 21v1vYTYWmyEHu-eP... (flutter/flutter#174332)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
mboetger pushed a commit to mboetger/flutter that referenced this pull request Sep 18, 2025
…entations. (flutter#174219)

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
flutter#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.
mboetger pushed a commit to mboetger/flutter that referenced this pull request Sep 18, 2025
This PR refactors the skwasm renderer to use `DisplayList` objects as
its main model objects instead of using Skia objects directly. Then, at
render time, we dispatch the display list commands to the skia surface.
This is a preparatory step for impeller on web.
* Some build rules were reworked in order to allow `DisplayList` to
compile via emscripten
* Some pieces of the display list library were further refactored to
allow us to compile it without actually building and linking the
impeller shaders. The two major classes that needed to be separated out
were `DlRuntimeEffect` and the text drawing system.
* `SkPath` and `SkImage` are still used as the main model objects in
skwasm. As of right now, `DisplayList` just thinly wraps these objects,
so this is the minimal possible change for now. I will have to refactor
this somewhat further when preparing for actual impeller adoption.
* Several special cased code paths in skwasm were removed, as they are
taken care of by `DisplayList` itself. This includes shadow drawing,
determining when to enable dithering, and determining the right clamp
value for filters.
korca0220 pushed a commit to korca0220/flutter that referenced this pull request Sep 22, 2025
…entations. (flutter#174219)

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
flutter#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.
korca0220 pushed a commit to korca0220/flutter that referenced this pull request Sep 22, 2025
This PR refactors the skwasm renderer to use `DisplayList` objects as
its main model objects instead of using Skia objects directly. Then, at
render time, we dispatch the display list commands to the skia surface.
This is a preparatory step for impeller on web.
* Some build rules were reworked in order to allow `DisplayList` to
compile via emscripten
* Some pieces of the display list library were further refactored to
allow us to compile it without actually building and linking the
impeller shaders. The two major classes that needed to be separated out
were `DlRuntimeEffect` and the text drawing system.
* `SkPath` and `SkImage` are still used as the main model objects in
skwasm. As of right now, `DisplayList` just thinly wraps these objects,
so this is the minimal possible change for now. I will have to refactor
this somewhat further when preparing for actual impeller adoption.
* Several special cased code paths in skwasm were removed, as they are
taken care of by `DisplayList` itself. This includes shadow drawing,
determining when to enable dithering, and determining the right clamp
value for filters.
Jaineel-Mamtora pushed a commit to Jaineel-Mamtora/flutter_forked that referenced this pull request Sep 24, 2025
…entations. (flutter#174219)

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
flutter#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.
Jaineel-Mamtora pushed a commit to Jaineel-Mamtora/flutter_forked that referenced this pull request Sep 24, 2025
This PR refactors the skwasm renderer to use `DisplayList` objects as
its main model objects instead of using Skia objects directly. Then, at
render time, we dispatch the display list commands to the skia surface.
This is a preparatory step for impeller on web.
* Some build rules were reworked in order to allow `DisplayList` to
compile via emscripten
* Some pieces of the display list library were further refactored to
allow us to compile it without actually building and linking the
impeller shaders. The two major classes that needed to be separated out
were `DlRuntimeEffect` and the text drawing system.
* `SkPath` and `SkImage` are still used as the main model objects in
skwasm. As of right now, `DisplayList` just thinly wraps these objects,
so this is the minimal possible change for now. I will have to refactor
this somewhat further when preparing for actual impeller adoption.
* Several special cased code paths in skwasm were removed, as they are
taken care of by `DisplayList` itself. This includes shadow drawing,
determining when to enable dithering, and determining the right clamp
value for filters.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 12, 2025
lucaantonelli pushed a commit to lucaantonelli/flutter that referenced this pull request Nov 21, 2025
…entations. (flutter#174219)

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
flutter#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.
lucaantonelli pushed a commit to lucaantonelli/flutter that referenced this pull request Nov 21, 2025
This PR refactors the skwasm renderer to use `DisplayList` objects as
its main model objects instead of using Skia objects directly. Then, at
render time, we dispatch the display list commands to the skia surface.
This is a preparatory step for impeller on web.
* Some build rules were reworked in order to allow `DisplayList` to
compile via emscripten
* Some pieces of the display list library were further refactored to
allow us to compile it without actually building and linking the
impeller shaders. The two major classes that needed to be separated out
were `DlRuntimeEffect` and the text drawing system.
* `SkPath` and `SkImage` are still used as the main model objects in
skwasm. As of right now, `DisplayList` just thinly wraps these objects,
so this is the minimal possible change for now. I will have to refactor
this somewhat further when preparing for actual impeller adoption.
* Several special cased code paths in skwasm were removed, as they are
taken care of by `DisplayList` itself. This includes shadow drawing,
determining when to enable dithering, and determining the right clamp
value for filters.
@eyebrowsoffire eyebrowsoffire deleted the skwasm_display_list branch December 12, 2025 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems e: impeller Impeller rendering backend issues and features requests engine flutter/engine related. See also e: labels. platform-web Web applications specifically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants