-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
base: main
Are you sure you want to change the base?
Conversation
…xcept for JPG and animated image - https://mimesniff.spec.whatwg.org/#matching-an-image-type-pattern
- adding `DOMExceptionInvalidStateError` in global due to handling not supported image format - animation image is not supported currently
a234586
to
1de527f
Compare
055395a
to
bd77fb9
Compare
111f00b
to
69addc8
Compare
Updated: The Note for supporting AVIF: I've implemented AVIF support using the 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 |
ext/canvas/01_image.js
Outdated
hint: When you want to get a "Blob" from "fetch", make sure to go through a file server that returns the appropriate content-type response header, | ||
and specify the URL to the file server like "await(await fetch('http://localhost:8000/sample.png').blob()". | ||
Alternatively, if you are reading a local file using 'Deno.readFile' etc., | ||
set the appropriate MIME type like "new Blob([await Deno.readFile('sample.png')], { type: 'image/png' })".\n`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions may be better written in runtime/fmt_errors.rs.
Ref #26252
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
@crowlKats
Could I move this hint to runtime/fmt_errors.rs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
ext/canvas/01_image.js
Outdated
info: The following MIME types are supported: | ||
https://mimesniff.spec.whatwg.org/#image-type-pattern-matching-algorithm\n`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@Hajime-san is it ready for review or still working on this? |
@crowlKats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR, thanks a lot for this.
Am really sorry it took so long to review this.
Left a few comments and changes, but overall looks extremely good!
ext/canvas/01_image.js
Outdated
// TODO(Hajime-san): this should be real async | ||
const processedImage = op_create_image_bitmap( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it should be fine if its sync; afterall it makes sense for browsers to be async since they are handling graphics stuff outside of the js apis sandbox, but for us we dont have that so its fine.
Overall looking very good, though one nitpick: in the newly added error messages, could you remove the trailing |
Feature
resolves #26032
basic support for decoding image format
jpeg
,png
,gif
,bmp
,ico
,webp
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.
supportcolorSpace
forImageData
There can convert color space fromsrgb
todisplay-p3
withcreateImageBitmap
.This was a misunderstanding on my part, so I reverted it.
The conversion using the
ImageData
colorSpace option was probably the responsibility ofCanvasRenderingContext2D
.I've left the implementation commented out as it may be useful as a reference for implementing
CanvasRenderingContext2D
orOffscreenCanvasRenderingContext2D
.support
colorSpaceConversion
optionThe 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.
support receives
ImageBitmap
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
API benchmark
extra
The benchmarks below are for when the argument to
op_create_image_bitmap
is a struct except for the first argument ofbuf
.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.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
consider to be data-parallelism with usingrayon
.align error message