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

Conversation

@flar
Copy link
Contributor

@flar flar commented Nov 12, 2021

This is a resubmit of #29470 with fixes for the golden image failures down the line. This is a WIP until I can confirm that it fixes the failures in the downstream tests.

The following description is copied from the original PR:

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

@flar flar added the Work in progress (WIP) Not ready (yet) for review! label Nov 12, 2021
@google-cla google-cla bot added the cla: yes label Nov 12, 2021
@flar flar requested review from chinmaygarde and gw280 November 12, 2021 20:07
if (mask_sigma_valid(sigma) &&
(current_mask_style_ != style || current_mask_sigma_ != sigma)) {
if (!mask_sigma_valid(sigma)) {
setMaskFilter(nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the behaviour we actually want? It seems more intuitive to me that if we pass an invalid sigma we error out and don't alter the state of the DisplayList. If we're doing this, please can we explicitly document this behaviour somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the behavior of SkMaskFilter::MakeBlur(). If the sigma fails the tests there, they return nullptr and setting that into the paint resets the mask filter to null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that using an SkMaskFilter(..., 0.0) filter has always been equivalent to turning the filter off. That's what was happening in the downstream tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, but please add an API comment here detailing this behaviour.

@flar flar requested a review from gw280 November 13, 2021 01:42
@CaseyHillers CaseyHillers changed the base branch from master to main November 15, 2021 18:11
@flar flar changed the base branch from master to main November 15, 2021 21:37
@flar flar added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. and removed Work in progress (WIP) Not ready (yet) for review! labels Nov 16, 2021
@fluttergithubbot fluttergithubbot merged commit 1a816e8 into flutter:main Nov 16, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 17, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 17, 2021
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