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

feat(ext/canvas): enhance createImageBitmap specification compliance #25517

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

Hajime-san
Copy link
Contributor

@Hajime-san Hajime-san commented Sep 8, 2024

Feature

resolves #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.

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.

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!');
% 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.
% 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);
})
% 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
% 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

Comment on lines +652 to +653
#[cfg(test)]
mod tests {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to add a test for EXIF orientation

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 added some unit tests that based on wpt.
It seems that image crate will soon release an API for image orientation, so we will be able to pass tests correctly with using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@crowlKats crowlKats self-requested a review September 29, 2024 20:56
@crowlKats
Copy link
Member

@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

* NOTE: Some browsers have implementation-defined image formats.
* For example, The AVIF image format is supported by all browsers today.
* However, the standardization seems to have hard going.
* See: https://github.com/whatwg/mimesniff/issues/143
* @param {Uint8Array} input
* @returns {string | undefined}
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This question is unrelated to quoted the line.
I think that the sniffImage function should receives another argument which is a byte sequence as Uint8Array and passes to imageTypePatternMatchingAlgorithm.
It seems that the imageTypeMatched variable will always become undefined with the current implementation.

/**
* Ref: https://mimesniff.spec.whatwg.org/#rules-for-sniffing-images-specifically
* @param {string} mimeTypeString
* @returns {string}
*/
function sniffImage(mimeTypeString) {
const mimeType = parseMimeType(mimeTypeString);
if (mimeType === null) {
return mimeTypeString;
}
if (isXML(mimeType)) {
return mimeTypeString;
}
const imageTypeMatched = imageTypePatternMatchingAlgorithm(
new TextEncoder().encode(mimeTypeString),
);
if (imageTypeMatched !== undefined) {
return imageTypeMatched;
}
return mimeTypeString;
}

Comment on lines +256 to +257
// TODO(Hajime-san): this should be real async
const processedImage = op_create_image_bitmap(
Copy link
Contributor Author

@Hajime-san Hajime-san Oct 4, 2024

Choose a reason for hiding this comment

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

Initially, I thought it would be necessary to make it an async op, but when I actually implemented it that seemed to reduce performance compared to fb6b87d #25517 (comment).
In this case, is it not necessary to make it an async op?🤔

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

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

benchmark           time/iter (avg)        iter/s      (min … max)           p75      p99     p995
------------------- ----------------------------- --------------------- --------------------------
createImageBitmap            4.8 µs       208,800 (  4.7 µs …   5.1 µs)   4.8 µs   5.1 µs   5.1 µs

@Hajime-san
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

createImageBitmap from blob [v.2.0.0-rc10]
3 participants