Skip to content
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

add DlCanvas(SkCanvas) vs DlCanvas(Builder) variants to DL rendertests #39944

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

flar
Copy link
Contributor

@flar flar commented Feb 28, 2023

Just a note that this PR may have a slight negative impact on the "display list builder" benchmarks as it adds a couple of new op variants to the list of DL test snippets which are used to measure the builder performance.

This PR adds new testing variants to the display_list_rendertests test suite. Previously we tested that the following graphics flows produced the same results graphically:

  1. SkCanvas rendering to an SkSurface
  2. DisplayListBuilder rendering, dispatched to an SkCanvas (replaced by # 6 below)
  3. SkCanvas rendering captured into a DL (no longer represents an active Flutter work-flow)
  4. SkPicture from # 1 and DL from # 2 (now from # 6 below) that were recorded at 1x, rendered at 2x

The new rendering variations that are now added to the above are:

  1. DlCanvas talking to SkCanvas rendering to an SkSurface
  2. DlCanvas talking to a DLBuilder and then rendered onto SkSurface
  3. DL produced by # 4 identical to DL produced by # 6 (# 4 no longer produces a DL so no pair of DL's to compare)

Note that when all of the steps 1-7 were in place some bugs were discovered in DlSkCanvasAdapter not synchronizing all attributes and the image variants of DLOp not doing precise comparisons.

This PR is a draft because now that we are testing that all of the above agree with each other, some of the comparisons may be eliminated as obsolete. In particular, # 2 (replaced by # 6) and # 3. # 4's DL replaced by # 7's DL if # 2 is eliminated. (All adjustments to removing the old steps and replacing them with the new steps have been implemented so this PR is moving from draft status to ready for review.)

@flar flar added the Work in progress (WIP) Not ready (yet) for review! label Feb 28, 2023
@flar flar marked this pull request as ready for review February 28, 2023 23:58
@flar flar removed the Work in progress (WIP) Not ready (yet) for review! label Feb 28, 2023
@flar flar requested a review from chinmaygarde March 1, 2023 00:52
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

🎉

static const CvSetup kEmptyCvSetup = [](SkCanvas*, SkPaint&) {};
static const CvRenderer kEmptyCvRenderer = [](SkCanvas*, const SkPaint&) {};
static const DlRenderer kEmptyDlRenderer = [](DisplayListBuilder&) {};
typedef const std::function<void(SkCanvas*, SkPaint&)> SkSetup;
Copy link
Member

Choose a reason for hiding this comment

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

I realize this was just renaming existing stuff, but prefer using to typedef. It usually doesn't matter except you need to use using if you need template parameters. So its better (IMO) to be consistent and it seems to be the C++ way.

@flar flar force-pushed the DLrendertests-on-DlCanvas branch from aee3de6 to bb96142 Compare March 1, 2023 20:40
@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 1, 2023
@auto-submit auto-submit bot merged commit 03c9a9f into flutter:main Mar 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 1, 2023
chinmaygarde pushed a commit to chinmaygarde/flutter_engine that referenced this pull request Mar 3, 2023
…rations (flutter#39944)

add DlCanvas(SkCanvas) vs DlCanvas(Builder) variants to DL rendertests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants