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

[Impeller] Add direct tesselation of circles for DrawCircle and Round end caps #48103

Merged
merged 9 commits into from
Nov 30, 2023

Conversation

flar
Copy link
Contributor

@flar flar commented Nov 16, 2023

Note: The GPU tesselation for drawPoints is disabled in this test PR, but will be re-enabled when I can run some benchmarks to see where a good cutoff exists.

Tessellation for circular or quarter circular paths is now handled by a single tessellator class that provides services for DrawCircle, DrawPoints (round caps), and DrawLines (round caps). Creating a general path is avoided along with the associated overhead of recognizing and using a generic tessellation step.

@flar flar added Work in progress (WIP) Not ready (yet) for review! e: impeller labels Nov 16, 2023
@flar flar force-pushed the impeller-direct-circle-tessellation branch from ee39a17 to fbe91b4 Compare November 16, 2023 03:00
@flar flar marked this pull request as ready for review November 16, 2023 06:05
@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #48103 at sha fbe91b4

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #48103 at sha 92b97fd

@flar flar force-pushed the impeller-direct-circle-tessellation branch from 573472e to 39f8a54 Compare November 22, 2023 07:34
@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #48103 at sha 39f8a54

@flar
Copy link
Contributor Author

flar commented Nov 22, 2023

I triaged the new golden for the brand new test created here, but left all of the others for others to look. We don't tessellate the circles exactly the same as the Path might have done it, instead now using the same technique that had been used in DrawPoints(PointMode), but hopefully with very similar fidelity. I wanted to leave those golden diffs un-triaged so that reviewers could look through them and evaluate any fidelity differences.

impeller/entity/geometry/circle_tessellator.h Outdated Show resolved Hide resolved
impeller/entity/geometry/circle_tessellator.h Outdated Show resolved Hide resolved
impeller/entity/geometry/circle_tessellator.cc Outdated Show resolved Hide resolved
impeller/entity/geometry/circle_tessellator.cc Outdated Show resolved Hide resolved
impeller/entity/geometry/ellipse_geometry.cc Outdated Show resolved Hide resolved
impeller/entity/geometry/ellipse_geometry.cc Outdated Show resolved Hide resolved
@flar
Copy link
Contributor Author

flar commented Nov 22, 2023

Here are 2 charts that demonstrate the performance of the new direct tessellation code to the previous mechanisms.

Note that the regressions in DrawPoints(PointMode) are due to the measurements being taken with the special point_field GPU tessellator disabled for comparison purposes. In other words, this comparison for PointMode demonstrates that the custom CPU tessellator is not much slower than the GPU tessellator even at 10k points being processed. The GPU tessellator is still used at the same performance levels, I just disabled it for testing and benchmarking these numbers.

DrawLine and DrawPoints on Pixel 6a

drawLine and drawPoints performance comparison on Pixel 6a (1)

DrawLine and DrawPoints on M1 MBPro

drawLine and drawPoints performance comparison on M1 Max MBP (1)

@flar
Copy link
Contributor Author

flar commented Nov 22, 2023

I will be looking at doing HostBuffer emplacement for PointField geometry as well. Not sure if I should do it now, or as a follow-on PR...

@flar
Copy link
Contributor Author

flar commented Nov 22, 2023

I reran the 10k line/point benchmarks again with the direct HostBuffer emplacement - all except the PointMode measurements since those continue to use GPU in the latest commit and so they haven't changed. The direct HostBuffer emplacement had a small but measurable improvement over the vertex builders.

The blue lines are prior to these changes and the red/orange lines are with these changes. The orange line improves over the red line due to the HostBuffer emplacement technique.

DrawLine and DrawPoints on Pixel 6a

drawLine and drawPoints performance comparison on Pixel 6a (2)

DrawLine and DrawPoints on M1 MBPro

drawLine and drawPoints performance comparison on M1 Max MBP (2)

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #48103 at sha 769d4af

@flar flar force-pushed the impeller-direct-circle-tessellation branch from 769d4af to 4ccdb46 Compare November 30, 2023 05:52
@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #48103 at sha 4ccdb46

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #48103 at sha aeef542

@flar
Copy link
Contributor Author

flar commented Nov 30, 2023

This should be in its (hopefully) final form ready for review.

I left the goldens untriaged for review, but I've done a cursory inspection and they look like the kind of diffs one would expect for the change in the math.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM with nit

impeller/entity/geometry/ellipse_geometry.cc Outdated Show resolved Hide resolved
impeller/entity/geometry/ellipse_geometry.cc Outdated Show resolved Hide resolved
@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #48103 at sha 16fa52d

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 30, 2023
@auto-submit auto-submit bot merged commit 2420fb0 into flutter:main Nov 30, 2023
28 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 1, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 1, 2023
…139342)

flutter/engine@74d2df5...c26e6ce

2023-12-01 [email protected] Roll a new version of googletest (2021->2023). (flutter/engine#48285)
2023-12-01 [email protected] [canvaskit] Add ImageFilter.compose (flutter/engine#48546)
2023-12-01 [email protected] Avoid reloading the kernel snapshot when spawning an isolate in the same group (flutter/engine#48478)
2023-12-01 [email protected] [Android] Check for text to paste before trying to retrieve data from URI (flutter/engine#48166)
2023-11-30 [email protected] Roll Skia from 88a1bcb9e43e to 0a90c366ff87 (3 revisions) (flutter/engine#48549)
2023-11-30 [email protected] Manual roll Dart SDK to 3.3.0-174.2.beta (4 revisions) (flutter/engine#48538)
2023-11-30 [email protected] Roll Skia from db3399a541f3 to 88a1bcb9e43e (1 revision) (flutter/engine#48545)
2023-11-30 [email protected] [Windows] Begin decoupling text input plugin from the view (flutter/engine#47833)
2023-11-30 [email protected] Roll Skia from 2d236de89898 to db3399a541f3 (3 revisions) (flutter/engine#48543)
2023-11-30 [email protected] [Impeller] Add direct tesselation of circles for DrawCircle and Round end caps (flutter/engine#48103)
2023-11-30 [email protected] [canvaskit] Revert to `drawImage` rendering on Chrome 110 or earlier (flutter/engine#48515)
2023-11-30 [email protected] Roll Skia from 6b4bdebaab88 to 2d236de89898 (1 revision) (flutter/engine#48537)
2023-11-30 [email protected] [Impeller] Started expanding the blur clip region (flutter/engine#48535)
2023-11-30 [email protected] Roll Skia from 0e479728cc1f to 6b4bdebaab88 (1 revision) (flutter/engine#48536)
2023-11-30 [email protected] Roll Skia from 0968fe18ff75 to 0e479728cc1f (1 revision) (flutter/engine#48534)
2023-11-30 [email protected] Use Chromium mirror for archive dependency (flutter/engine#48509)
2023-11-30 [email protected] Roll Skia from 2f01d500a352 to 0968fe18ff75 (2 revisions) (flutter/engine#48531)
2023-11-30 [email protected] Roll Skia from 11a15444a3aa to 2f01d500a352 (1 revision) (flutter/engine#48524)
2023-11-30 [email protected] Roll Skia from 84fdd36b1eea to 11a15444a3aa (1 revision) (flutter/engine#48523)
2023-11-30 [email protected] [Android] Add support for the PlatformChannel  "Share.invoke" command (flutter/engine#48265)
2023-11-30 [email protected] Roll Skia from 5d64b1322879 to 84fdd36b1eea (1 revision) (flutter/engine#48521)
2023-11-30 [email protected] Roll Skia from 23721750e433 to 5d64b1322879 (1 revision) (flutter/engine#48520)
2023-11-30 [email protected] Roll Fuchsia Linux SDK from 8wu5EgBh1yJPNOd5W... to Bb2k375udWIltCEAx... (flutter/engine#48519)
2023-11-30 [email protected] Roll Skia from 5a635f2211ce to 23721750e433 (1 revision) (flutter/engine#48514)
2023-11-29 [email protected] Roll Skia from 928e8950e8e3 to 5a635f2211ce (1 revision) (flutter/engine#48511)
2023-11-29 [email protected] [web] No implicit view in multi-view mode (flutter/engine#48505)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 8wu5EgBh1yJP to Bb2k375udWIl

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

To file a bug in Flutter: 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
auto-submit bot pushed a commit that referenced this pull request Dec 2, 2023
Similar work as done for filled circles in #48103, stroked circles can also be directly tessellated using the same internal data structures used for filled circles.
caseycrogers pushed a commit to caseycrogers/flutter that referenced this pull request Dec 29, 2023
…lutter#139342)

flutter/engine@74d2df5...c26e6ce

2023-12-01 [email protected] Roll a new version of googletest (2021->2023). (flutter/engine#48285)
2023-12-01 [email protected] [canvaskit] Add ImageFilter.compose (flutter/engine#48546)
2023-12-01 [email protected] Avoid reloading the kernel snapshot when spawning an isolate in the same group (flutter/engine#48478)
2023-12-01 [email protected] [Android] Check for text to paste before trying to retrieve data from URI (flutter/engine#48166)
2023-11-30 [email protected] Roll Skia from 88a1bcb9e43e to 0a90c366ff87 (3 revisions) (flutter/engine#48549)
2023-11-30 [email protected] Manual roll Dart SDK to 3.3.0-174.2.beta (4 revisions) (flutter/engine#48538)
2023-11-30 [email protected] Roll Skia from db3399a541f3 to 88a1bcb9e43e (1 revision) (flutter/engine#48545)
2023-11-30 [email protected] [Windows] Begin decoupling text input plugin from the view (flutter/engine#47833)
2023-11-30 [email protected] Roll Skia from 2d236de89898 to db3399a541f3 (3 revisions) (flutter/engine#48543)
2023-11-30 [email protected] [Impeller] Add direct tesselation of circles for DrawCircle and Round end caps (flutter/engine#48103)
2023-11-30 [email protected] [canvaskit] Revert to `drawImage` rendering on Chrome 110 or earlier (flutter/engine#48515)
2023-11-30 [email protected] Roll Skia from 6b4bdebaab88 to 2d236de89898 (1 revision) (flutter/engine#48537)
2023-11-30 [email protected] [Impeller] Started expanding the blur clip region (flutter/engine#48535)
2023-11-30 [email protected] Roll Skia from 0e479728cc1f to 6b4bdebaab88 (1 revision) (flutter/engine#48536)
2023-11-30 [email protected] Roll Skia from 0968fe18ff75 to 0e479728cc1f (1 revision) (flutter/engine#48534)
2023-11-30 [email protected] Use Chromium mirror for archive dependency (flutter/engine#48509)
2023-11-30 [email protected] Roll Skia from 2f01d500a352 to 0968fe18ff75 (2 revisions) (flutter/engine#48531)
2023-11-30 [email protected] Roll Skia from 11a15444a3aa to 2f01d500a352 (1 revision) (flutter/engine#48524)
2023-11-30 [email protected] Roll Skia from 84fdd36b1eea to 11a15444a3aa (1 revision) (flutter/engine#48523)
2023-11-30 [email protected] [Android] Add support for the PlatformChannel  "Share.invoke" command (flutter/engine#48265)
2023-11-30 [email protected] Roll Skia from 5d64b1322879 to 84fdd36b1eea (1 revision) (flutter/engine#48521)
2023-11-30 [email protected] Roll Skia from 23721750e433 to 5d64b1322879 (1 revision) (flutter/engine#48520)
2023-11-30 [email protected] Roll Fuchsia Linux SDK from 8wu5EgBh1yJPNOd5W... to Bb2k375udWIltCEAx... (flutter/engine#48519)
2023-11-30 [email protected] Roll Skia from 5a635f2211ce to 23721750e433 (1 revision) (flutter/engine#48514)
2023-11-29 [email protected] Roll Skia from 928e8950e8e3 to 5a635f2211ce (1 revision) (flutter/engine#48511)
2023-11-29 [email protected] [web] No implicit view in multi-view mode (flutter/engine#48505)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 8wu5EgBh1yJP to Bb2k375udWIl

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

To file a bug in Flutter: 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
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 e: impeller will affect goldens
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants