feat(ext/canvas): enhance `createImageBitmap` specification compliance
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_bitmapwill be async fn and consider to be data-parallelism with usingrayon. -
align error message
- https://github.com/denoland/deno/issues/25269
- https://github.com/denoland/deno/pull/25310
@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
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 is it ready for review or still working on this?
@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.👍
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
@Hajime-san Happy new year!
I updated copyright notice by #27509. Please merge main into this branch and update the same 🙏
@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!
@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 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
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 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:
- to remove some specific format from
dependencies.image.featuresinext/canvas/Cargo.toml - to comment out
use image::codecs::foo::FooDecoderinext/canvas/op_create_image_bitmap.rsthat followsext/canvas/Cargo.tomland insertunreachable!()macro to theMimeTypeenum match arm -
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 lets disable gif and webp for now then
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?
commenting out sounds better so we can re-enable it when we want to
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 @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 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_rowofDynamicImage
overall just more image crate usage work.
would you be interested in pushing that branch forward?
@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 sure! I opened #28105. Feel free to create a PR to my branch
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 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