Initial support for images in Skwasm #42019

eyebrowsoffire · 2023-05-13T17:44:25Z

This partially implements flutter/flutter#126341

It does not implement image codecs, because they are going to get complicated with transferring video frames to the web worker and so on. I am going to deal with image codecs in a subsequent change.

flutter-dashboard · 2023-05-13T19:14:44Z

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 #42019 at sha 7009a83

yjbanov · 2023-05-15T19:52:06Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/image.dart

+      dataHandle,
+      width,
+      height,
+      format == ui.PixelFormat.bgra8888,


Is a bool sufficient to express all pixel formats we have? https://api.flutter.dev/flutter/dart-ui/PixelFormat.html

Well, we just have the two formats in web:

enum PixelFormat { rgba8888, bgra8888, }

So I figured it was simpler to pass a bool for now. We can change it to an enum later if we have more than two.

Oh, that looks like a bug. PixelFormat is part of dart:ui, which is expected to be identical across all platforms.

I added support for it in Skwasm, but it's still not supported in all the renderers and the bitmap creation code path. I filed flutter/flutter#126895 as a follow-on.

yjbanov · 2023-05-15T20:03:20Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/renderer.dart

+        // Not scaled.
+        return null;
+      }
+      targetWidth = (width * targetHeight! / height).round();


Looks like the compiler failed to figure out that targetHeight is not null nere. Or am I missing something? Should we file an issue?

I think expecting the compiler to figure out that targetHeight is non-null here is a little unrealistic. I might be able to restructure the code a little to make it more obvious to the compiler though... Hmmm...

yjbanov · 2023-05-15T20:05:56Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/renderer.dart

+    );
+    if (!allowUpscaling && scaledSize != null &&
+      (scaledSize.width > width || scaledSize.height > height)) {
+        domWindow.console.warn('Cannot apply targetWidth/targetHeight when allowUpscaling is false.');


I'm not sure we need to issue a warning. This seems to be the expected behavior. Sometimes you want to shrink the image to save memory, but upscaling can be done in the GPU so you don't want to waste CPU and memory on upscaling prior to rendering.

I was doing this to be consistent with skiaDecodeImageFromPixels, which issues this warning under this condition.

Heh, benefits of revisiting old code! :) Yeah, I don't think skiaDecodeImageFromPixels should warn either.

yjbanov · 2023-05-15T20:10:56Z

lib/web_ui/skwasm/shaders.cpp

+                                               SkTileMode tileModeY,
+                                               FilterQuality quality,
+                                               SkScalar* matrix33) {
+  if (matrix33) {


Do we need to check? We seem to always pass a non-null matrix.

I think I'm going to go the other direction and bubble the nullability up to the higher level APIs. There is at least one place I could avoid passing this data altogether.

yjbanov · 2023-05-15T20:26:31Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/shaders.dart

+    Float64List matrix4,
+    ui.FilterQuality? filterQuality,
+  ) => withStackScope((StackScope scope) {
+    final RawMatrix33 localMatrix = scope.convertMatrix4toSkMatrix(matrix4);


Since the matrix is never null, can we allocate on the heap to avoid copying the matrix twice, once here and a second time in shader_createFromImage?

Two stack allocations is likely cheaper than one heap allocation. Also, I'm not entirely sure it really wins anything to do things on the heap. SkMatrix is actually not a POD, so you have to actually call the constructor on it (we can't just try to fake construction of it through memory accesses). I wouldn't worry about it much, this is likely to be very fast.

Gotcha. I thought the second one was on the heap.

yjbanov · 2023-05-15T20:28:29Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/shaders.dart

  }

  RuntimeEffectHandle handle;
  String name;
+  int floatUniformCount;
+  int childShaderCount;


Do all of the fields need to be mutable?

Definitely not. I should change this.

yjbanov · 2023-05-15T20:32:01Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/shaders.dart

+      image as SkwasmImage,
+      ui.TileMode.clamp,
+      ui.TileMode.clamp,
+      toMatrix64(Matrix4.identity().storage),


We should probably cache this.

Actually, I think I should just make this optional and not pass the data at all for the identity case.

yjbanov · 2023-05-15T20:42:16Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/surface.dart

+    final int byteCount = skDataGetSize(dataHandle);
+    final Pointer<Uint8> dataPointer = skDataGetConstPointer(dataHandle).cast<Uint8>();
+    final Uint8List output = Uint8List(byteCount);
+    for (int i = 0; i < byteCount; i++) {


Is there any equivalent to TypedArray.set for copying one array into another in WasmGC? Then we could create a Uint8List view into the linear memory and copy it into the GC space as one bulk operation.

You want WebAssembly/gc#367, which sadly won't make WasmGC MVP.

yjbanov · 2023-05-15T20:50:29Z

lib/web_ui/skwasm/surface.cpp

+    emscripten_dispatch_to_thread(_thread, EM_FUNC_SIG_VIIII,
+                                  reinterpret_cast<void*>(fRasterizeImage),
+                                  nullptr, this, image, format, callbackId);
+    return callbackId;
  }

  // Main thread only


Is there a way to validate this property at runtime?

Yes, I think I found one. I can make it an assert so that it only executes in debug mode.

yjbanov · 2023-05-15T20:51:44Z

lib/web_ui/skwasm/surface.cpp

+                       uint32_t callbackId) {
+    sk_sp<SkData> data;
+    if (format == ImageByteFormat::png) {
+      data = SkPngEncoder::Encode(_grContext.get(), image, {});


A-ha! I bet removing the dependency on SkPngEncoder will take us below 1MB :)

I'm not really sure we can get rid of this though. It might be possible... maybe draw the image to a canvas and then do toDataUrl() on it? I don't know.

Yep, exactly that. I'm not requesting this change in this PR. Just spotted an opportunity for later.

flutter-dashboard · 2023-05-16T05:44:07Z

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

Changes reported for pull request #42019 at sha fb6c2a6

flutter-dashboard · 2023-05-16T17:26:40Z

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

Changes reported for pull request #42019 at sha 36f988b

yjbanov

…sions) (#126961) Manual roll requested by zra@google.com flutter/engine@fe24767...1c775e3 2023-05-16 zanderso@users.noreply.github.com Revert "[ios_platform_view] only recycle maskView when the view is applying mutators" (flutter/engine#42080) 2023-05-16 gspencergoog@users.noreply.github.com [macOS] Wait for binding to be ready before requesting exits from framework (flutter/engine#41753) 2023-05-16 gspencergoog@users.noreply.github.com [linux] Wait for binding to be ready before requesting exits from framework (flutter/engine#41782) 2023-05-16 jacksongardner@google.com Initial support for images in Skwasm (flutter/engine#42019) 2023-05-16 jacksongardner@google.com Use new `unresolvedCodePoints` API from skia. (flutter/engine#41991) 2023-05-16 jason-simmons@users.noreply.github.com Convert public API NativeFieldWrapper classes to abstract interfaces (flutter/engine#41945) 2023-05-16 737941+loic-sharma@users.noreply.github.com [Windows] Add force redraw to the C++ client wrapper (flutter/engine#42061) 2023-05-16 godofredoc@google.com Fix drone_dimension host_engine_builder. (flutter/engine#42068) 2023-05-16 godofredoc@google.com Add linux_clang_tidy builder. (flutter/engine#41990) 2023-05-16 ychris@google.com [ios_platform_view] only recycle maskView when the view is applying mutators (flutter/engine#41573) 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 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

…sions) (flutter#126961) Manual roll requested by zra@google.com flutter/engine@fe24767...1c775e3 2023-05-16 zanderso@users.noreply.github.com Revert "[ios_platform_view] only recycle maskView when the view is applying mutators" (flutter/engine#42080) 2023-05-16 gspencergoog@users.noreply.github.com [macOS] Wait for binding to be ready before requesting exits from framework (flutter/engine#41753) 2023-05-16 gspencergoog@users.noreply.github.com [linux] Wait for binding to be ready before requesting exits from framework (flutter/engine#41782) 2023-05-16 jacksongardner@google.com Initial support for images in Skwasm (flutter/engine#42019) 2023-05-16 jacksongardner@google.com Use new `unresolvedCodePoints` API from skia. (flutter/engine#41991) 2023-05-16 jason-simmons@users.noreply.github.com Convert public API NativeFieldWrapper classes to abstract interfaces (flutter/engine#41945) 2023-05-16 737941+loic-sharma@users.noreply.github.com [Windows] Add force redraw to the C++ client wrapper (flutter/engine#42061) 2023-05-16 godofredoc@google.com Fix drone_dimension host_engine_builder. (flutter/engine#42068) 2023-05-16 godofredoc@google.com Add linux_clang_tidy builder. (flutter/engine#41990) 2023-05-16 ychris@google.com [ios_platform_view] only recycle maskView when the view is applying mutators (flutter/engine#41573) 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 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

eyebrowsoffire added 7 commits May 13, 2023 10:41

Initial pass at picture to image.

e212cd1

Added a unit test and a fix.

7f8db30

Add support for image shaders and binding to fragment shaders.

85fe346

Add test for image shader.

7f56e89

Partway through image rasterization.

1122814

Implement toByteData on images.

0544f10

Image from pixel data.

fcec0ae

flutter-dashboard bot added the platform-web Code specifically for the web engine label May 13, 2023

Update licenses golden.

7009a83

flutter-dashboard bot added the will affect goldens label May 13, 2023

yjbanov reviewed May 15, 2023

View reviewed changes

Add support for rgbaFloat32 pixel format.

4a2f1e5

eyebrowsoffire mentioned this pull request May 16, 2023

Support PixelFormat.rgbaFloat32 on Web flutter/flutter#126895

Open

Addressed more of Yegor's comments.

fb6c2a6

Merge branch 'main' into skwasm_image

36f988b

yjbanov approved these changes May 16, 2023

View reviewed changes

eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label May 16, 2023

auto-submit bot merged commit e8c00a3 into flutter:main May 16, 2023

engine-flutter-autoroll mentioned this pull request May 16, 2023

Manual roll Flutter Engine from fe2476743b59 to 1c775e34e2d5 (10 revisions) flutter/flutter#126961

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 16, 2023

e8c00a320 Initial support for images in Skwasm (flutter/engine#42019)

36a2074

Initial support for images in Skwasm #42019

Initial support for images in Skwasm #42019

Uh oh!

Conversation

eyebrowsoffire commented May 13, 2023

Uh oh!

flutter-dashboard bot commented May 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented May 16, 2023

Uh oh!

flutter-dashboard bot commented May 16, 2023

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!