deno icon indicating copy to clipboard operation
deno copied to clipboard

feat(ext/canvas): enhance `createImageBitmap` specification compliance

Open Hajime-san opened this issue 1 year ago • 4 comments

Feature

resolves https://github.com/denoland/deno/issues/26032

basic support for decoding image format

  • jpeg, png, gif, bmp, ico, webp
    • animated image
    • 16-bit image
    • Grayscale image

issues for supporting AVIF

The AVIF image format is supported by all browsers today. However, the standardization of sniffing mime type seems to have hard going.

  • https://github.com/whatwg/mimesniff/issues/143

~~support colorSpace for ImageData~~

~~There can convert color space from srgb to display-p3 with createImageBitmap.~~

This was a misunderstanding on my part, so I reverted it. The conversion using the ImageData colorSpace option was probably the responsibility of CanvasRenderingContext2D. I've left the implementation commented out as it may be useful as a reference for implementing CanvasRenderingContext2D or OffscreenCanvasRenderingContext2D.

support colorSpaceConversion option

The specifications are not particularly clear about the implementation details, and I had a hard time understanding its behavior, but I eventually do reverse engineering the results of wpt and implemented it to be consistent with those results.

  • https://github.com/whatwg/html/issues/10578

port to Rust

To simplify code, almost logic moved to Rust side.

added a simple architecture document for README

It is basically based on the image implementation, and I wrote roughly how you can handle data effectively.

performance impact

I was curious to see if the increased amount of Rust code, especially static dispatch, was affecting the bootstrap time of the runtime. I'm not very confident about the suitability of this benchmark.

bootstrap

console.log('Hello, world!');
  • this pr fb6b87d65
% hyperfine --warmup 5 'target/release/deno run bootstrap.ts'
Benchmark 1: target/release/deno run bootstrap.ts
  Time (mean ± σ):      21.5 ms ±   3.6 ms    [User: 14.2 ms, System: 5.5 ms]
  Range (min … max):    18.7 ms …  54.5 ms    102 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
  • main b1b72a8a4
% hyperfine --warmup 5 'target/release/deno run bootstrap.ts'
Benchmark 1: target/release/deno run bootstrap.ts
  Time (mean ± σ):      22.2 ms ±   4.7 ms    [User: 14.8 ms, System: 5.4 ms]
  Range (min … max):    18.8 ms …  66.8 ms    97 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

API benchmark

const imageData = new ImageData(new Uint8ClampedArray([255, 0, 0, 255]), 1, 1);

Deno.bench("createImageBitmap", async () => {
  await createImageBitmap(imageData);
})
  • this pr acc4119d7
% target/release/deno bench bench.ts
    CPU | Apple M1
Runtime | Deno 2.0.0-rc.2 (aarch64-apple-darwin)

file:///path/to/deno/bench.ts

benchmark           time/iter (avg)        iter/s      (min … max)           p75      p99     p995
------------------- ----------------------------- --------------------- --------------------------
createImageBitmap            1.6 µs       615,000 (  1.6 µs …   1.7 µs)   1.6 µs   1.7 µs   1.7 µs
  • main b1b72a8a4
% target/release/deno bench bench.ts
    CPU | Apple M1
Runtime | Deno 1.46.1 (aarch64-apple-darwin)

file:///path/to/deno/bench.ts

benchmark           time/iter (avg)        iter/s      (min … max)           p75      p99     p995
------------------- ----------------------------- --------------------- --------------------------
createImageBitmap            3.5 µs       287,200 (  3.3 µs …   3.6 µs)   3.5 µs   3.6 µs   3.6 µs

extra

The benchmarks below are for when the argument to op_create_image_bitmap is a struct except for the first argument of buf. It seems that serializing the argument to op as a struct has a fairly large impact on performance. This may apply to things other than op_create_image_bitmap as well.

% target/release/deno bench bench.ts
    CPU | Apple M1
Runtime | Deno 2.0.0-rc.2 (aarch64-apple-darwin)

file:///path/to/deno/bench.ts

benchmark           time/iter (avg)        iter/s      (min … max)           p75      p99     p995
------------------- ----------------------------- --------------------- --------------------------
createImageBitmap            4.2 µs       235,900 (  4.1 µs …   4.5 µs)   4.3 µs   4.5 µs   4.5 µs

plans for enhancement in another PRs

  • improve performance There is known to that processing image is a cpu-bound task. So, op_create_image_bitmap will be async fn and consider to be data-parallelism with using rayon.

  • align error message

    • https://github.com/denoland/deno/issues/25269
    • https://github.com/denoland/deno/pull/25310

Hajime-san avatar Sep 08 '24 22:09 Hajime-san

@Hajime-san FYI I'll review this PR after we release 2.0; not enough time to do it beforehand, especially since its such a big PR

crowlKats avatar Oct 02 '24 13:10 crowlKats

Updated: The image supports various bit depth now.

  • https://github.com/image-rs/image/pull/2373

Note for supporting AVIF: I've implemented AVIF support using the AvifDecoder here 111f00b, although it only supports 8-bit currently. As mentioned in the overview of this PR, sniffing MIME about AVIF is an ongoing issue, but I believe this implementation could support it without deviating from the spec. The AvifDecoder depends on dav1d and seems to require additional tools to build locally and in CI.

Build Failure log

% cargo b   
   ...
   Compiling dav1d-sys v0.8.2
error: failed to run custom build command for `dav1d-sys v0.8.2`

Caused by:
  process didn't exit successfully: `/path/to/deno/target/debug/build/dav1d-sys-7acb1adeb047a692/build-script-build` (exit status: 101)
  --- stdout
  cargo:rerun-if-env-changed=DAV1D_NO_PKG_CONFIG
  cargo:rerun-if-env-changed=PKG_CONFIG_aarch64-apple-darwin
  cargo:rerun-if-env-changed=PKG_CONFIG_aarch64_apple_darwin
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG
  cargo:rerun-if-env-changed=PKG_CONFIG
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH_aarch64-apple-darwin
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH_aarch64_apple_darwin
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_PATH
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_aarch64-apple-darwin
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_aarch64_apple_darwin
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_LIBDIR
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_aarch64-apple-darwin
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_aarch64_apple_darwin
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_SYSROOT_DIR
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR

  --- stderr
  thread 'main' panicked at /path/to/.cargo/registry/src/index.crates.io-6f17d22bba15001f/dav1d-sys-0.8.2/build.rs:82:10:
  called `Result::unwrap()` on an `Err` value: PkgConfig(
  pkg-config exited with status code 1
  > PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=1 pkg-config --libs --cflags dav1d dav1d >= 1.3.0

  The system library `dav1d` required by crate `dav1d-sys` was not found.
  The file `dav1d.pc` needs to be installed and the PKG_CONFIG_PATH environment variable must contain its parent directory.
  The PKG_CONFIG_PATH environment variable is not set.

  HINT: if you have installed the library, try setting PKG_CONFIG_PATH to the directory containing `dav1d.pc`.
  )
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

% uname -a
Darwin user.local 23.5.0 Darwin Kernel Version 23.5.0: Wed May  1 20:16:51 PDT 2024; root:xnu-10063.121.3~5/RELEASE_ARM64_T8103 arm64

% brew install dav1d
% cargo b

Hajime-san avatar Oct 06 '24 08:10 Hajime-san

@Hajime-san is it ready for review or still working on this?

crowlKats avatar Oct 24 '24 08:10 crowlKats

@crowlKats Thanks, there were a lot of changes in this PR though it's ready for review. From now on, I will not be making any new changes until I receive your review.👍

Hajime-san avatar Oct 24 '24 09:10 Hajime-san

Overall looking very good, though one nitpick: in the newly added error messages, could you remove the trailing .? This is just a design decision that we have made to have a unified style for error messages

crowlKats avatar Nov 28 '24 01:11 crowlKats

@Hajime-san Happy new year!

I updated copyright notice by #27509. Please merge main into this branch and update the same 🙏

petamoriken avatar Jan 02 '25 09:01 petamoriken

@Hajime-san I know this took a while, but it had its reasons. The merge window for 2.2 is open now, so i want to get this landed this week; could you rebase it? and i will review it once more to be sure and then we can land it!

crowlKats avatar Jan 28 '25 13:01 crowlKats

@Hajime-san so i just compiled the PR locally, and noticed that the binary increased 604kb; do you have any insight into the cause behind this? probably one of the dependencies being really big? regardless, it is a bit too much of an increase. that aside this PR looks good to land, so thats the only blocker.

crowlKats avatar Jan 31 '25 00:01 crowlKats

@crowlKats I didn't care about the increased build size. Yes, it's huge. I will take a look soon.

MEMO:

% uname -a
Darwin user.local 23.5.0 Darwin Kernel Version 23.5.0: Wed May  1 20:16:51 PDT 2024; root:xnu-10063.121.3~5/RELEASE_ARM64_T8103 arm64

main on d9db0b35e

% cargo b -r --target-dir target/d9db0b35e && ls -l target/d9db0b35e/release/deno
-rwxr-xr-x  1 user  46682944  110354728 Jan 31 11:39 target/d9db0b35e/release/deno

this pr ca52fe82f

% cargo b -r --target-dir target/ca52fe82f && ls -l target/ca52fe82f/release/deno         
-rwxr-xr-x  1 user  46682944  110646296 Jan 31 12:46 target/ca52fe82f/release/deno

approximately 291kb larger

Hajime-san avatar Jan 31 '25 02:01 Hajime-san

if its one of the image formats that is causing massive increase, i think its fine to drop support for it and we can think about adding it in the future

crowlKats avatar Jan 31 '25 02:01 crowlKats

@crowlKats Thanks!

From my research, of the newly added image formats, jpeg and webp are the largest, followed by gif, while bmp and ico do not seem to be affected as they have no external dependencies. Also, although the color space conversion library lcms2 requires static linking on aarch64-unknown-linux-gnu, which may account as much as jpeg and webp.

In addition, I think that in terms of actual demand, png and jpeg(https://github.com/denoland/deno/issues/26032) are the most popular, followed by webp and gif.

I can follow your instructions and make additional commits to not include certain image formats in this PR. However, if the 2.2 release is imminent, feel free to make commits at your decision.


Here are simplified steps to reproduce for comparison of build sizes:

  1. to remove some specific format from dependencies.image.features in ext/canvas/Cargo.toml
  2. to comment out use image::codecs::foo::FooDecoder in ext/canvas/op_create_image_bitmap.rs that follows ext/canvas/Cargo.toml and insert unreachable!() macro to the MimeType enum match arm
  3. cargo build --release

build result

my pc

% uname -a
Darwin user.local 23.5.0 Darwin Kernel Version 23.5.0: Wed May  1 20:16:51 PDT 2024; root:xnu-10063.121.3~5/RELEASE_ARM64_T8103 arm64

main on https://github.com/denoland/deno/commit/d9db0b35e0336bd6d48d9bee2312485049c8313e

% ls -l target/d9db0b35e/release/deno
-rwxr-xr-x  1 user  46682944  110354728 Jan 31 11:39 target/d9db0b35e/release/deno

based on this pr https://github.com/denoland/deno/commit/ca52fe82f87c9d5df14406564e9e9ef0a557bb4b

original

% ls -l target/ca52fe82f/release/deno         
-rwxr-xr-x  1 user  46682944  110646296 Jan 31 12:46 target/ca52fe82f/release/deno

291kb larger

dynamic link lcms2

% ls -l target/ca52fe82f_lcms2_dynamic_link/release/deno
-rwxr-xr-x  1 user  46682944  110447720 Jan 31 22:35 target/ca52fe82f_lcms2_dynamic_link/release/deno

92kb larger

remove gif

% ls -l target/ca52fe82f_png_jpeg_webp_bmp_ico/release/deno      
-rwxr-xr-x  1 user  46682944  110629416 Jan 31 15:33 target/ca52fe82f_png_jpeg_webp_bmp_ico/release/deno

274kb larger

remove gif,webp

% ls -l target/ca52fe82f_png_jpeg_bmp_ico/release/deno
-rwxr-xr-x  1 user  46682944  110499304 Jan 31 15:59 target/ca52fe82f_png_jpeg_bmp_ico/release/deno

144kb larger

remove gif,webp,jpeg

% ls -l target/ca52fe82f_png_bmp_ico/release/deno     
-rwxr-xr-x  1 user  46682944  110408040 Jan 31 16:23 target/ca52fe82f_png_bmp_ico/release/deno

53kb larger

Hajime-san avatar Jan 31 '25 07:01 Hajime-san

@Hajime-san lets disable gif and webp for now then

crowlKats avatar Jan 31 '25 15:01 crowlKats

I commented out the relevant lines with an explanation of the implementation, changed them to throw an error for each MIME type gif and webp, and modified the unit test to verify that an error is thrown. Cloud you tell me if it is more appropriate to delete the source code itself rather than commenting it out?

Hajime-san avatar Jan 31 '25 16:01 Hajime-san

commenting out sounds better so we can re-enable it when we want to

crowlKats avatar Jan 31 '25 16:01 crowlKats

As a follow-up and to enable #23783 given the changes in #27665, it would be nice if the ImageBitmap class would be changed to be defined in rust instead of JS, and be returned by the op. not sure if this is something you'd like to do @Hajime-san

crowlKats avatar Feb 05 '25 12:02 crowlKats

@crowlKats @petamoriken Thanks a lot! I was thinking of adding avif support next, but since an increase in build size would be unavoidable, so I will propose a way to reduce the size on the image crate side. Indeed, since we ported a lot of the core logic to Rust with this PR, object wrap shouldn't be so hard.

BTW, it would be nice if you update the doc of the colorSpaceConversion option in MDN when you commit to the repo. https://github.com/mdn/browser-compat-data/blob/c04c5b51405486a780d78fa1c43b473f5214c6da/api/_globals/createImageBitmap.json#L55-L57

Hajime-san avatar Feb 06 '25 00:02 Hajime-san

@Hajime-san I started https://github.com/crowlKats/deno/tree/copy_external_image_to_texture (superseeds #23783); only things left are:

  • handle source.origin
  • handle destination.color_space
  • don't hardcode the bytes_per_row of DynamicImage

overall just more image crate usage work.

would you be interested in pushing that branch forward?

crowlKats avatar Feb 12 '25 14:02 crowlKats

@crowlKats That sounds interesting. Yeah, I will tackle the feature.

Would be better that I add commits after you open a draft pr to make discussion easy?

Hajime-san avatar Feb 13 '25 02:02 Hajime-san

@Hajime-san sure! I opened #28105. Feel free to create a PR to my branch

crowlKats avatar Feb 13 '25 15:02 crowlKats

Hi folks, is there an issue to track when webp support will be re-enabled in createImageBitmap? I can't seem to find anything.

shannon avatar Nov 24 '25 14:11 shannon

@shannon Hi, there seems to be a demand for this, so I created a PR It's up to the team to decide if it would be appropriate.

  • https://github.com/denoland/deno/pull/31402

Hajime-san avatar Nov 25 '25 09:11 Hajime-san