-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[skwasm] Port to DisplayList objects
#172314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[skwasm] Port to DisplayList objects
#172314
Conversation
DisplayList objects
harryterkelsen
left a comment
There was a problem hiding this 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.
The |
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 |
flar
left a comment
There was a problem hiding this 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, |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_ |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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)), |
There was a problem hiding this comment.
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?
|
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... |
…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.
99ff77d to
868ad70
Compare
|
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. |
flar
left a comment
There was a problem hiding this 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...
flar
left a comment
There was a problem hiding this 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...
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
engine/src/flutter/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paint.dart
Show resolved
Hide resolved
…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.
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.
…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.
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.
…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.
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.
…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.
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.
This PR refactors the skwasm renderer to use
DisplayListobjects 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.DisplayListto compile via emscriptenDlRuntimeEffectand the text drawing system.SkPathandSkImageare still used as the main model objects in skwasm. As of right now,DisplayListjust 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.DisplayListitself. This includes shadow drawing, determining when to enable dithering, and determining the right clamp value for filters.