Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Build DisplayList directly from flutter::Canvas #29470

Merged

Conversation

flar
Copy link
Contributor

@flar flar commented Nov 2, 2021

Fixes flutter/flutter#93415

This is a big step along the way towards eventually deleting the SkPicture code and should provide a tiny performance boost for DL because it bypasses the current roundabout way of filling the DL records.

Current mechanism: flutter::Canvas => SkCanvas calls => SkDL adapter => DLBuilder => DL
After this patch: flutter::Canvas => DLBuilder calls => DL

There is a lot of reworking of some of the DL code which is mostly logistical. We used to have 2 or so different ways of describing what attributes and properties each rendering op used or exhibited. Because this PR was going to add yet another mechanism that needed to know mostly the same information, I took the opportunity to consolidate all of our various Ops-flags mechanisms into a common "DisplayListAttributeFlags" class. Now 4 different mechanisms use this data:

  • Bounds calculator to determine which attributes contribute to the bounds of an Op
  • Canvas Adapter to determine which attributes to sync from an SkPaint object
  • flutter::Canvas calls ask flutter::Paint to sync the Dart attributes for a given Op
  • DL canvas unit tests use the data to verify when to expect the pixels to change when we render with a given Op with a given attribute

I am marking this as a WIP only until I run some more tests manually (and get the test results from this PR) and look for some potentially dead code. The mechanism should be ready to go other than that.

@flar flar added the Work in progress (WIP) Not ready (yet) for review! label Nov 2, 2021
@google-cla google-cla bot added the cla: yes label Nov 2, 2021
@flar flar removed the Work in progress (WIP) Not ready (yet) for review! label Nov 2, 2021
@flar flar force-pushed the call-DL-directly-from-Dart-Canvas branch from 804653a to a00e245 Compare November 2, 2021 19:41
@flar flar requested review from chinmaygarde and gw280 November 2, 2021 19:51
@flar flar force-pushed the call-DL-directly-from-Dart-Canvas branch from e5404e6 to c43fd28 Compare November 3, 2021 00:17
@flar
Copy link
Contributor Author

flar commented Nov 3, 2021

I've rebased to a working engine commit so this PR is now ready for review.

if (SkScalarsAreFinite(mxx, myx) &&
SkScalarsAreFinite(mxy, myy) &&
SkScalarsAreFinite(mxt, myt) &&
!(mxx == 1 && mxy == 0 && mxt == 0 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider using SkMatrix here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. We use Skia objects in a few places in DL, but I was generally looking to start eliminating them where possible to make the packages somewhat independent. There are tasks in flutter/flutter#85737 that talk about removing or simplifying our dependence on Skia objects.

For matrices, we need to start tracking things with 4x4 matrices everywhere to eliminate the problems with perspective matrix concatenation, but that makes simple operations take more memory and compute time so I was toying with creating our own transform class that will track a 2x3 matrix 99% of the time, and expand to 3x3 or 4x4 only when needed. Then we can rethink issues like these.

@@ -1463,4 +1546,165 @@ void DisplayListBuilder::drawShadow(const SkPath& path,
: Push<DrawShadowOp>(0, 1, path, color, elevation, dpr);
}

// clang-format off
// Flags common to all primitives that apply colors
#define PAINT_FLAGS (kUsesDither_ | \
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we make these constexpr?

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 would have to place these in the class, but they are only used for initialization. The kConstants they refer to are private in the Flags class.

if (p.asBlendMode()) {
setBlendMode(p.asBlendMode().value());
} else {
(current_blender_ = blender) //
Copy link
Contributor

Choose a reason for hiding this comment

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

should we null out the current_blend_mode_ here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't null out an enum value, unfortunately. The rule here is that current_blender_ supersedes current_blend_mode_.

I added doc comments for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

would it be worth adding a Blender to the BlendMode enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a Skia enum. We'd have to ask them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One issue with that is that someone might call "setBlendMode(kCustom)" and what would we do with that request?

if (!canvas_) {
return;
if (display_list_recorder_) {
builder()->rotate(radians * 180.0 / M_PI);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the Builder API using degrees, and the Canvas API using radians?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Builder code is building from an SkPicture replacement so it matches SkCanvas rather than ui.Canvas. It is somewhat arbitrary, though.

@gw280
Copy link
Contributor

gw280 commented Nov 4, 2021

In general I really like a lot of the refactoring in this PR, especially the new DisplayListFlags class. I just find the way SkCanvas vs. DisplayListCanvasRecorder is handled right now to be very confusing.

@flar flar force-pushed the call-DL-directly-from-Dart-Canvas branch from c43fd28 to d998f8c Compare November 10, 2021 22:03
@flar flar requested a review from gw280 November 10, 2021 22:35
@flar flar added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 10, 2021
@fluttergithubbot fluttergithubbot merged commit d5cadd2 into flutter:master Nov 10, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 11, 2021
flar added a commit that referenced this pull request Nov 11, 2021
@flar flar deleted the call-DL-directly-from-Dart-Canvas branch November 12, 2021 19:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui.Canvas native methods should call DisplayListBuilder methods directly
3 participants