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

[canvaskit] Enable CanvasKit to compute tight SkPicture bounds #43361

Merged
merged 215 commits into from
Aug 1, 2023

Conversation

harryterkelsen
Copy link
Contributor

Passes the computeBounds bool argument to PictureRecorder.beginRecording. This will allow us to get tight bounds of the draw commands in the Picture, which enables us to optimize away Pictures which don't paint anything, or optimize away overlays that don't overlap with Platform views.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Jun 30, 2023
@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 #43361 at sha 79d36ff

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

LGTM but an extra LGTM from @eyebrowsoffire would be nice, especially from the WasmGC point of view.

Copy link
Contributor

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

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

One other bounds calculation gotcha I had to deal with when I was implementing Skwasm was the ImageFilterEngineLayer. The filter itself might actually change the bounds of the content it is filtering. Skia has an API called filterBounds on their image filters that takes an input cull rect and gives the output cull rect. They do this on the native side as well. See https://github.com/flutter/engine/blame/40a8732a5de0e63f2903bf0e565383f9671ce93c/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/layers.dart#L164 for how I did this in Skwasm, I think you'll need a similar adjustment on the CanvasKit side (or else it cuts off some of the content of blurred images, for example)

@@ -15,17 +15,18 @@ import 'surface_factory.dart';

/// Implements [ui.Picture] on top of [SkPicture].
class CkPicture implements ui.Picture {
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually made this abstract class that you might want to conform to here: https://github.com/flutter/engine/blame/893ab3bf7bb9cf104846ec50b79baf4864089253/lib/web_ui/lib/src/engine/scene_painting.dart#L15C3-L15C3

I actually wrote Skwasm's scene builder in an agnostic way where it could theoretically be reused for any renderer that implements the normal dart:ui interfaces plus a few odds and ends that aren't exposed (such as this cullRect function). It might be interesting to see if CanvasKit could be migrated to that scene builder and do some perf measurements at some point, I think it does a little less work in some cases than the CanvasKit one (although it doesn't support platform views yet, so we wouldn't want to switch over to that until the platform views are implemented).

I also think a getter for cullRect makes more sense than a function, but up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I'm getting started on the filterBounds CanvasKit change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@harryterkelsen
Copy link
Contributor Author

Filed flutter/flutter#130453 to track the work of adjusting the image filter bounds using filterBounds

@flutter-dashboard
Copy link

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

Changes reported for pull request #43361 at sha 2da248b

jonahwilliams and others added 10 commits July 20, 2023 16:35
…ter#43564)

flutter/flutter#129392

Buffer allocations on worker threads can interefere with allocations on the main thread. Since all buffers that we allocate on the main thread are discarded after a frame, we can use a ring buffer of VmaPool objects with a linear allocate bit to get faster allocations.

This works for buffers because they are likely to be suballocated, so we can continue to reuse the same memory over and over. Whereas most of our textures require dedicated allocations.

In order to support this, we need the allocator to track an incrementing frame counter (actually this would probably be useful for the context to do in general, but we'll get there eventually).

![image](https://github.com/flutter/engine/assets/8975114/4a485e2b-c557-4567-9266-a725fc1448db)
…indows). (flutter#43388)

Actually works now, though there is some issue with the default fbo stencil so I've filled flutter/flutter#130048

Other issues:
* ~~Rendering looks wrong~~
* ~~Resizing window hangs~~
* ~~Reactor isn't set up correctly and all blit passes are currently failing.~~
* ~~Needs to handle falling back to sample count of 1 like we do on Android~~.
)

https://skia.googlesource.com/skia.git/+log/bedc92598644..6ed93436d57c

2023-07-12 skia-autoroll@skia-public.iam.gserviceaccount.com Roll vulkan-deps from 4ba3255697ef to 3b2c55a1bc2b (5 revisions)
2023-07-12 jvanverth@google.com [graphite] Add wait/signal semaphore support to InsertRecordingInfo.
2023-07-12 herb@google.com Move sk_float_nextlog2 to WangsFormula.h and cleanup

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/skia-flutter-autoroll
Please CC brianosman@google.com,kjlubick@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Skia: https://bugs.chromium.org/p/skia/issues/entry
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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
flutter#43622)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/fuchsia-mac-sdk-flutter-engine
Please CC rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r#43623)

https://dart.googlesource.com/sdk.git/+log/8f8f281ccdc6..f499e91e8cb2

2023-07-13 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.1.0-305.0.dev
2023-07-12 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.1.0-304.0.dev
2023-07-12 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.1.0-303.0.dev

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/dart-sdk-flutter-engine
Please CC dart-vm-team@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter Engine: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
https://skia.googlesource.com/skia.git/+log/6ed93436d57c..7f391ea9164e

2023-07-13 michaelludwig@google.com [skif] Replace SkTileImageFilter with nested crops

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/skia-flutter-autoroll
Please CC brianosman@google.com,kjlubick@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Skia: https://bugs.chromium.org/p/skia/issues/entry
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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…... (flutter#43627)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/fuchsia-linux-sdk-flutter-engine
Please CC rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
https://skia.googlesource.com/skia.git/+log/7f391ea9164e..c8da0c657c4e

2023-07-13 skia-autoroll@skia-public.iam.gserviceaccount.com Roll SwiftShader from dda70a3ef9fe to 151fa797ee3e (1 revision)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/skia-flutter-autoroll
Please CC brianosman@google.com,kjlubick@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Skia: https://bugs.chromium.org/p/skia/issues/entry
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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r instance creation. (flutter#43629)

I didn't know this when I wrote it initially but structure chains will throw a compile time error if a chain member violates the Vulkan spec. pNext chaining is easy to mess up otherwise and we should use structure chains where possible. We are already doing this during pipeline construction.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 2, 2023
…sions) (#131785)

Manual roll requested by jacksongardner@google.com

flutter/engine@9dae7b7...d6b962d

2023-08-02 zanderso@users.noreply.github.com Revert "[Impeller] Support for rendering Android Platform Views into a HardwareBuffer backed texture." (flutter/engine#44262)
2023-08-02 skia-flutter-autoroll@skia.org Roll Skia from c6577d328585 to 6009cc6d7d80 (2 revisions) (flutter/engine#44264)
2023-08-02 skia-flutter-autoroll@skia.org Roll ANGLE from 11cef17b53ac to d61a50c155bd (1 revision) (flutter/engine#44261)
2023-08-02 skia-flutter-autoroll@skia.org Roll Skia from 1c0bba7c1053 to c6577d328585 (2 revisions) (flutter/engine#44260)
2023-08-02 skia-flutter-autoroll@skia.org Roll Dart SDK from 8ff03ebf7eaa to afbaf4216fc8 (1 revision) (flutter/engine#44259)
2023-08-02 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 3JSF6hXLRdAK1wUN1... to Hx7ap5qcoqRIknnnG... (flutter/engine#44258)
2023-08-02 skia-flutter-autoroll@skia.org Roll ANGLE from a21631c02e45 to 11cef17b53ac (1 revision) (flutter/engine#44256)
2023-08-02 skia-flutter-autoroll@skia.org Roll Skia from 6807d8b8a9d3 to 1c0bba7c1053 (1 revision) (flutter/engine#44255)
2023-08-02 skia-flutter-autoroll@skia.org Roll ANGLE from 5d4b3645d0dc to a21631c02e45 (1 revision) (flutter/engine#44252)
2023-08-02 skia-flutter-autoroll@skia.org Roll Skia from 6087a5224c6f to 6807d8b8a9d3 (2 revisions) (flutter/engine#44250)
2023-08-02 skia-flutter-autoroll@skia.org Roll Skia from 6cb888889ad8 to 6087a5224c6f (1 revision) (flutter/engine#44249)
2023-08-02 jonahwilliams@google.com [Impeller] Give a fixed timeout for acquireNextImageKHR. (flutter/engine#44241)
2023-08-02 skia-flutter-autoroll@skia.org Roll Dart SDK from 7c03426705da to 8ff03ebf7eaa (2 revisions) (flutter/engine#44243)
2023-08-02 skia-flutter-autoroll@skia.org Roll Skia from 4deae93198f9 to 6cb888889ad8 (1 revision) (flutter/engine#44240)
2023-08-01 hterkelsen@users.noreply.github.com [canvaskit] Enable CanvasKit to compute tight SkPicture bounds (flutter/engine#43361)
2023-08-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 197fc0d7fea2 to 7c03426705da (2 revisions) (flutter/engine#44237)
2023-08-01 john@johnmccutchan.com [Impeller] Support for rendering Android Platform Views into a HardwareBuffer backed texture. (flutter/engine#44087)
2023-08-01 yjbanov@google.com [web:a11y] add platform view role (flutter/engine#44188)
2023-08-01 skia-flutter-autoroll@skia.org Roll Skia from 18cf818e044f to 4deae93198f9 (1 revision) (flutter/engine#44236)
2023-08-01 skia-flutter-autoroll@skia.org Roll Skia from 58c031441cbb to 18cf818e044f (6 revisions) (flutter/engine#44234)
2023-08-01 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from g0dgOL2IlZQJCK4El... to 3JSF6hXLRdAK1wUN1... (flutter/engine#44233)
2023-08-01 skia-flutter-autoroll@skia.org Roll ANGLE from b53d99d87e6a to 5d4b3645d0dc (1 revision) (flutter/engine#44231)
2023-08-01 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from lwCo6le6r0X-Srvx3... to KPSWBhOvG6piddBQG... (flutter/engine#44230)
2023-08-01 skia-flutter-autoroll@skia.org Roll Skia from d53f7b880651 to 58c031441cbb (2 revisions) (flutter/engine#44229)
2023-08-01 58529443+srujzs@users.noreply.github.com Remove extends JSTypedArray from JSUint8Array1 (flutter/engine#44175)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from lwCo6le6r0X- to KPSWBhOvG6pi
  fuchsia/sdk/core/mac-amd64 from g0dgOL2IlZQJ to Hx7ap5qcoqRI

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 jacksongardner@google.com,rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2023
vashworth pushed a commit to vashworth/flutter that referenced this pull request Aug 2, 2023
…sions) (flutter#131785)

Manual roll requested by jacksongardner@google.com

flutter/engine@9dae7b7...d6b962d

2023-08-02 zanderso@users.noreply.github.com Revert "[Impeller] Support for rendering Android Platform Views into a HardwareBuffer backed texture." (flutter/engine#44262)
2023-08-02 skia-flutter-autoroll@skia.org Roll Skia from c6577d328585 to 6009cc6d7d80 (2 revisions) (flutter/engine#44264)
2023-08-02 skia-flutter-autoroll@skia.org Roll ANGLE from 11cef17b53ac to d61a50c155bd (1 revision) (flutter/engine#44261)
2023-08-02 skia-flutter-autoroll@skia.org Roll Skia from 1c0bba7c1053 to c6577d328585 (2 revisions) (flutter/engine#44260)
2023-08-02 skia-flutter-autoroll@skia.org Roll Dart SDK from 8ff03ebf7eaa to afbaf4216fc8 (1 revision) (flutter/engine#44259)
2023-08-02 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 3JSF6hXLRdAK1wUN1... to Hx7ap5qcoqRIknnnG... (flutter/engine#44258)
2023-08-02 skia-flutter-autoroll@skia.org Roll ANGLE from a21631c02e45 to 11cef17b53ac (1 revision) (flutter/engine#44256)
2023-08-02 skia-flutter-autoroll@skia.org Roll Skia from 6807d8b8a9d3 to 1c0bba7c1053 (1 revision) (flutter/engine#44255)
2023-08-02 skia-flutter-autoroll@skia.org Roll ANGLE from 5d4b3645d0dc to a21631c02e45 (1 revision) (flutter/engine#44252)
2023-08-02 skia-flutter-autoroll@skia.org Roll Skia from 6087a5224c6f to 6807d8b8a9d3 (2 revisions) (flutter/engine#44250)
2023-08-02 skia-flutter-autoroll@skia.org Roll Skia from 6cb888889ad8 to 6087a5224c6f (1 revision) (flutter/engine#44249)
2023-08-02 jonahwilliams@google.com [Impeller] Give a fixed timeout for acquireNextImageKHR. (flutter/engine#44241)
2023-08-02 skia-flutter-autoroll@skia.org Roll Dart SDK from 7c03426705da to 8ff03ebf7eaa (2 revisions) (flutter/engine#44243)
2023-08-02 skia-flutter-autoroll@skia.org Roll Skia from 4deae93198f9 to 6cb888889ad8 (1 revision) (flutter/engine#44240)
2023-08-01 hterkelsen@users.noreply.github.com [canvaskit] Enable CanvasKit to compute tight SkPicture bounds (flutter/engine#43361)
2023-08-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 197fc0d7fea2 to 7c03426705da (2 revisions) (flutter/engine#44237)
2023-08-01 john@johnmccutchan.com [Impeller] Support for rendering Android Platform Views into a HardwareBuffer backed texture. (flutter/engine#44087)
2023-08-01 yjbanov@google.com [web:a11y] add platform view role (flutter/engine#44188)
2023-08-01 skia-flutter-autoroll@skia.org Roll Skia from 18cf818e044f to 4deae93198f9 (1 revision) (flutter/engine#44236)
2023-08-01 skia-flutter-autoroll@skia.org Roll Skia from 58c031441cbb to 18cf818e044f (6 revisions) (flutter/engine#44234)
2023-08-01 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from g0dgOL2IlZQJCK4El... to 3JSF6hXLRdAK1wUN1... (flutter/engine#44233)
2023-08-01 skia-flutter-autoroll@skia.org Roll ANGLE from b53d99d87e6a to 5d4b3645d0dc (1 revision) (flutter/engine#44231)
2023-08-01 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from lwCo6le6r0X-Srvx3... to KPSWBhOvG6piddBQG... (flutter/engine#44230)
2023-08-01 skia-flutter-autoroll@skia.org Roll Skia from d53f7b880651 to 58c031441cbb (2 revisions) (flutter/engine#44229)
2023-08-01 58529443+srujzs@users.noreply.github.com Remove extends JSTypedArray from JSUint8Array1 (flutter/engine#44175)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from lwCo6le6r0X- to KPSWBhOvG6pi
  fuchsia/sdk/core/mac-amd64 from g0dgOL2IlZQJ to Hx7ap5qcoqRI

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 jacksongardner@google.com,rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform-web Code specifically for the web engine will affect goldens
Projects
None yet
Development

Successfully merging this pull request may close these issues.