Implement toGpuImage, a synchronous, GPU-resident version of #33736

dnfield · 2022-06-01T04:26:43Z

Picture.toImage.

Fixes flutter/flutter#13498
Fixes flutter/flutter#77289
fixes flutter/flutter#106381

Some additional context at flutter/flutter#40990

This method kicks off asynchronous work on the raster task runner.
If it fails to rasterize, it will synchronously throw later when
the user attempts to draw to a canvas.

This supports several use cases:

Quickly snapping off an expensive-to-rasterize image for reuse
across multiple frames.
Applying multi-pass filters to a render target.

This patch amends flutter_tester so that it can produce an image
object, but that image will always be a four square checkerboard of white and light grey.

Adds support for CanvasKit on Web, which basically already used
this method for its Picture.toImage implementation.

Throws an UnsupportedError for HTML on Web, since any implementation
there would almost certainly be slower than drawPicture.

Starting as a draft to run tests.

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.

Picture.toImage. This method kicks off asynchronous work on the raster task runner. If it fails to rasterize, it will synchronously throw later when the user attempts to draw to a canvas. This supports several use cases: - Quickly snapping off an expensive-to-rasterize image for reuse across multiple frames. - Applying multi-pass filters to a render target. This patch amends flutter_tester so that it can produce an image object, but that image will always be transparent black pixels. Adds support for CanvasKit on Web, which basically already used this method for its Picture.toImage implementation. Throws an UnsupportedError for HTML on Web, since any implementation there would almost certainly be slower than drawPicture.

dnfield · 2022-06-01T04:49:28Z

This won't work because of the mock gr context. Need to fix that. Going to close this in the mean time.

dnfield · 2022-06-21T21:56:40Z

Ok. On flutter_tester toGpuImage now just emits a checkerboard image.

The following things remain around this:

Implement for Scene. I skipped this because Scene currently uses SkPicture directly and I'd rather avoid implementing API compatible with this, but would also rather avoid fixing Remove Engine flag for disabling the DisplayList flutter#86634 in this patch.
Implement for ImageShader. Should be doable but this patch is already relatively large.
Move image creation to the IO thread. Should be doable but requires some more synchronization and is probably fine as a follow up patch.

dnfield · 2022-06-21T22:08:11Z

lib/ui/painting.dart

+  ///
+  /// In the flutter_tester, this will always created a light gray and white
+  /// checkerboard bitmap with the requested dimensions.
+  Image toGpuImage(int width, int height) {


Should this just be toTexture?

Is that term overloaded?

its all quite overloaded.

is it worth clarifying here (and elsewhere) that these are physical pixels and not the dp used in the framework?

We don't document this elsewhere - I think generally all the dart:ui API works in physical pixels, the framework introduces logical pixels.

jonahwilliams · 2022-06-21T22:14:02Z

lib/ui/painting.dart

@@ -5871,3 +5905,22 @@ Future<T> _futurize<T>(_Callbacker<T> callbacker) {
    throw Exception(error);
  return completer.future;
 }
+
+class PictureRasterizationException implements Exception {


Does this need to be its own exception type, or can use use Exception for this?

I want this to be something you can try to specifically catch, with the idea being that this API could potentially be grown to support more of an idea of what kind of exception was thrown (particularly if you tried to make a texture larger than the user's GPU supports)

where would you put the catch though?

Around calls to canvas.drawImage, or whatever else calls it.

I would give this a documentation comment at least describing the intent

skia-gold · 2022-06-21T22:24:36Z

Gold has detected about 1 new digest(s) on patchset 7.
View them at https://flutter-engine-gold.skia.org/cl/github/33736

skia-gold · 2022-06-22T00:39:36Z

Gold has detected about 1 new digest(s) on patchset 9.
View them at https://flutter-engine-gold.skia.org/cl/github/33736

flutter-dashboard · 2022-06-22T01:07:42Z

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 #33736 at sha f727e50

…promise_img

jonahwilliams · 2022-06-23T16:06:07Z

lib/ui/painting.dart

@@ -5871,3 +5907,30 @@ Future<T> _futurize<T>(_Callbacker<T> callbacker) {
    throw Exception(error);
  return completer.future;
 }
+
+/// An exception thrown by [Canvas.drawImage] and realted methods when drawing


Suggested change

/// An exception thrown by [Canvas.drawImage] and realted methods when drawing

/// An exception thrown by [Canvas.drawImage] and related methods when drawing

jonahwilliams · 2022-06-23T16:07:37Z

lib/ui/painting/image_shader.cc

+
+  if (image->image()->owning_context() != DlImage::OwningContext::kIO) {
+    matrix4.Release();
+    // TODO(dnfield): it should be possible to support this


I'm extremely excited about this

jonahwilliams · 2022-06-23T16:11:19Z

testing/dart/canvas_test.dart

+    // On a slower CI machine, the raster thread may get behind the UI thread
+    // here. However, once the image is in an error state it will immediately
+    // throw on subsequent attempts.
+    while (true) {


This makes me a bit uncomfortable, I'd be worried about the scenario where we break the error reporting somehow and this shard times out instead of fails.

If this had a max iteration count + perhaps a delay between drawImage attempts that might prevent that scenario, assuming there is no other way to stabilize it

I don't want to introduce a delay - the await null is just to turn the event loop. I'm fine with having a max iteration count though. I thought we had timeouts on these tests though - somewhere in litetest or something it should just eventually say the test failed due to timeout.

Yeah as long as it times out before the CI shard times out, that is fine. The await null will run the next iteration in another microtask which not be enough, you need a non-zero duration await to get a full event loop

TIL. I'll add a 1ms delay.

jonahwilliams

overall LGTM

skia-gold · 2022-06-23T16:34:36Z

Gold has detected about 2 new digest(s) on patchset 24.
View them at https://flutter-engine-gold.skia.org/cl/github/33736

flutter-dashboard · 2022-06-23T17:12:14Z

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

Changes reported for pull request #33736 at sha db97f79

jason-simmons · 2022-06-23T23:45:51Z

lib/ui/painting/display_list_deferred_image_gpu.cc

+}
+
+// |DlImage|
+size_t DlDeferredImageGPU::GetApproximateByteSize() const {


This will be invoked on the UI thread, and it's reading image_ (which is written on the raster thread).

Can this compute an estimate based on size_? That would avoid potential races.

jason-simmons · 2022-06-23T23:55:35Z

shell/common/rasterizer.cc

+  TRACE_EVENT0("flutter", "Rasterizer::MakeGpuImage");
+  FML_DCHECK(display_list);
+
+  switch (gpu_image_behavior_) {


This could be simplified to:

if (gpu_image_behavior_ == MakeGpuImageBehavior::kBitmap) { return MakeBitmapImage(picture_size); }

Prefer to avoid == for enums

skia-gold · 2022-06-24T16:34:36Z

Gold has detected about 1 new digest(s) on patchset 26.
View them at https://flutter-engine-gold.skia.org/cl/github/33736

skia-gold · 2022-06-24T22:19:36Z

Gold has detected about 1 new digest(s) on patchset 28.
View them at https://flutter-engine-gold.skia.org/cl/github/33736

fzyzcjy · 2022-06-24T23:21:17Z

🎉

…lutter/engine#33736)

flutter-dashboard bot added the platform-web Code specifically for the web engine label Jun 1, 2022

dnfield force-pushed the gpu_promise_img branch from 496a91d to 67325b1 Compare June 1, 2022 04:32

dnfield closed this Jun 1, 2022

dnfield added 3 commits June 6, 2022 13:10

merge

13a25bf

Merge remote-tracking branch 'upstream/main' into gpu_promise_img

76b7e8e

fix tests, make toGpuImage do something on flutter_tester

86f9f1f

dnfield reopened this Jun 21, 2022

dnfield added 2 commits June 21, 2022 14:53

test

3bf1247

Merge remote-tracking branch 'upstream/main' into gpu_promise_img

711479f

dnfield commented Jun 21, 2022

View reviewed changes

format

169de25

jonahwilliams reviewed Jun 21, 2022

View reviewed changes

dnfield added 2 commits June 21, 2022 17:07

missing docs and mirror API in web

7aa625a

simplify test for firefox

f727e50

dnfield marked this pull request as ready for review June 22, 2022 00:09

dnfield mentioned this pull request Jun 22, 2022

Implement toGpuImage for Scene flutter/flutter#106381

Closed

dnfield mentioned this pull request Jun 22, 2022

toGpuImage could be done on the IO thread flutter/flutter#106382

Closed

flutter-dashboard bot added the will affect goldens label Jun 22, 2022

dnfield added 2 commits June 22, 2022 00:36

tryAlloc

0f0cf9f

fix test

e4fbe83

dnfield mentioned this pull request Jun 22, 2022

[Proposal]Allow users to force a layer to be raster cached flutter/flutter#106006

Closed

dnfield and others added 3 commits June 22, 2022 11:29

Update canvas_test.dart

50793f3

merge

edaaeb8

Merge branch 'gpu_promise_img' of github.com:dnfield/engine into gpu_…

2864649

…promise_img

jonahwilliams reviewed Jun 23, 2022

View reviewed changes

review

dedd872

jonahwilliams reviewed Jun 23, 2022

View reviewed changes

jonahwilliams approved these changes Jun 23, 2022

View reviewed changes

dnfield added 2 commits June 23, 2022 09:17

max iterations on test

7c331a1

delay

db97f79

dnfield requested review from jason-simmons and chinmaygarde June 23, 2022 17:30

jason-simmons reviewed Jun 24, 2022

View reviewed changes

size

6fe3223

jason-simmons approved these changes Jun 24, 2022

View reviewed changes

safe

7e6e3be

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 24, 2022

jonahwilliams mentioned this pull request Jun 24, 2022

use toGPU image dnfield/vector_graphics#93

Merged

dnfield added 2 commits June 24, 2022 13:36

Work around for dart-lang/sdk#49332

fbc66d9

fix 32 bit builds

1e22e7c

dnfield merged commit e752095 into flutter:main Jun 24, 2022

dnfield deleted the gpu_promise_img branch June 24, 2022 23:14

engine-flutter-autoroll mentioned this pull request Jun 25, 2022

Roll Flutter Engine from 83b9a591a3d5 to 4446e3edda6a (5 revisions) flutter/flutter#106591

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2022

e752095 Implement toGpuImage, a synchronous, GPU-resident version of (f…

bf4c1d9

…lutter/engine#33736)

fzyzcjy mentioned this pull request Jun 25, 2022

GPU specific Scene.toImage and Picture.toImage flutter/flutter#40990

Closed

dnfield mentioned this pull request Jun 30, 2022

Use toPictureSync for faster zoom page transition flutter/flutter#106621

Merged

fzyzcjy mentioned this pull request Sep 15, 2022

[Proposal]Let Flutter run animations at 60fps even if there are heavy widgets, possibly using React Fiber-like or suspend-like algorithm? flutter/flutter#101227

Open

fzyzcjy mentioned this pull request Oct 29, 2022

Feature Request for a Synchronous way to Rasterize Images / Draw Pixels to Canvas flutter/flutter#37180

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement toGpuImage, a synchronous, GPU-resident version of #33736

Implement toGpuImage, a synchronous, GPU-resident version of #33736

dnfield commented Jun 1, 2022 •

edited

Loading

dnfield commented Jun 1, 2022

dnfield commented Jun 21, 2022

dnfield Jun 21, 2022

jonahwilliams Jun 21, 2022

jonahwilliams Jun 23, 2022

dnfield Jun 23, 2022

jonahwilliams Jun 21, 2022

dnfield Jun 21, 2022

jonahwilliams Jun 21, 2022

dnfield Jun 21, 2022

jonahwilliams Jun 22, 2022

dnfield Jun 22, 2022

skia-gold commented Jun 21, 2022

skia-gold commented Jun 22, 2022

flutter-dashboard bot commented Jun 22, 2022

jonahwilliams Jun 23, 2022

dnfield Jun 23, 2022

jonahwilliams Jun 23, 2022

jonahwilliams Jun 23, 2022

dnfield Jun 23, 2022

jonahwilliams Jun 23, 2022

dnfield Jun 23, 2022

jonahwilliams left a comment

skia-gold commented Jun 23, 2022

flutter-dashboard bot commented Jun 23, 2022

jason-simmons Jun 23, 2022

dnfield Jun 24, 2022

jason-simmons Jun 23, 2022

dnfield Jun 24, 2022

skia-gold commented Jun 24, 2022

skia-gold commented Jun 24, 2022

fzyzcjy commented Jun 24, 2022

	/// An exception thrown by [Canvas.drawImage] and realted methods when drawing
	/// An exception thrown by [Canvas.drawImage] and related methods when drawing

Implement toGpuImage, a synchronous, GPU-resident version of #33736

Implement toGpuImage, a synchronous, GPU-resident version of #33736

Conversation

dnfield commented Jun 1, 2022 • edited Loading

Pre-launch Checklist

dnfield commented Jun 1, 2022

dnfield commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skia-gold commented Jun 21, 2022

skia-gold commented Jun 22, 2022

flutter-dashboard bot commented Jun 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

skia-gold commented Jun 23, 2022

flutter-dashboard bot commented Jun 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skia-gold commented Jun 24, 2022

skia-gold commented Jun 24, 2022

fzyzcjy commented Jun 24, 2022

dnfield commented Jun 1, 2022 •

edited

Loading