kleisauke / wasm-vips

libvips for the browser and Node.js, compiled to WebAssembly with Emscripten.
https://kleisauke.github.io/wasm-vips/
MIT License
463 stars 25 forks source link

Support JPEG XL images #21

Closed atjn closed 1 year ago

atjn commented 1 year ago

As I see it, JPEG XL is going to be the dominant image format on the web, and probably also outside the web. It would be nice to start the process of supporting it now, so that it is completely ready and stable when people start using it.

The libjxl package makes it very clear that it isn't secure yet, but I don't see that as a major issue, because:

The goal of this PR is to build a working implementation that can handle >90% of images without crashing. libjxl is still under major development, so ironing out all bugs and optimizing speed could be a complete waste of time. We can always do that later when the package stabilizes a bit more.

Things to do:

atjn commented 1 year ago

Everything compiles now, and wasm-vips can be launched and will handle all other image formats normally. Anything that requires libjxl to spin up will result in an infinite loop :)

kleisauke commented 1 year ago

Great! I still need to think about whether to distribute JPEG XL support by default. Issue https://github.com/libvips/build-win64-mxe/issues/40 might also be relevant here.

I had a look at the build, and fixed a few things, see: https://github.com/atjn/wasm-vips/compare/libjxl...kleisauke:wasm-vips:libjxl-atjn

The hwy_list_targets target still seems to fail, and building with:

-DCMAKE_CROSSCOMPILING_EMULATOR="$EMSDK_NODE;--experimental-wasm-threads;--experimental-wasm-simd"

Doesn't seem to work. I think it's safe to ignore that error for now. emsdk probably needs to update its Node version to 16.4.0 or higher to match the final SIMD opcodes (see https://github.com/emscripten-core/emsdk/issues/1064).

kleisauke commented 1 year ago

jxlload and jxlsave is now working with commit https://github.com/kleisauke/wasm-vips/commit/8a6e02d3309b5836277bdc6fa8b40b11d74fb5e6 and https://github.com/kleisauke/wasm-vips/commit/3aff8575365702b63bfb377bf9acb47759d34cd1. Here are some playground links to test with: JPEG XL to PNG JPEG to JPEG XL PNG to JPEG XL (lossless)

~jxlsave produces broken images, but I was able to reproduce that on Linux as well, I just opened issue https://github.com/libvips/libvips/issues/2987 for this.~ Edit: fixed with commit https://github.com/kleisauke/wasm-vips/commit/3aff8575365702b63bfb377bf9acb47759d34cd1.

atjn commented 1 year ago

Wow, that is some nice and speedy work you made here! Feel free to commit all those changes to this branch. I can also do it, but I think it might attribute the changes to me.

atjn commented 1 year ago

I still need to think about whether to distribute JPEG XL support by default.

I would strongly argue for distributing JXL support by default. However, I do think it would be completely reasonable to disable JXL parsing by default, but make it possible to turn it on at runtime through the API somehow. Or you can count on the user to disable JXL themselves if they feel it is a safety issue for them.

My reasoning:

All browsers support methods of delivering JXL images today, so that when browsers eventually support it by default, it will instantly be used (see picture, image-set). I already use JXL on some sites today to gather experience before they are supported by default.

You could argue that we can just wait and enable support the moment that the first browser supports JXL, but it is very likely that some systems will be using wasm-vips as a dependency of a dependency of a dependency which might not be able to update to a newer version immediately. However if older versions already supported JXL with a runtime flag, tools could enable that flag and immediately enable support.

By using a flag, you are also able to keep that flag well after JXL has been supported in browsers, in case you still don't think that it is mature in wasm-vips.

There is also the case of non-browser workflows. Some software already supports JXL, and I have heard of places where JXL is the new standard. If somebody wants to build a quick webapp that handles these images, wasm-vips would be a great solution, but only if it support JXL.

atjn commented 1 year ago

The hwy_list_targets target still seems to fail

Yeah, highway recommends using the V8 engine directly to build it, exactly because SIMD isn't supported in node 14. Another solution is to force a newer node version. I thought the same as you; if it compiles and works, then just let it be and wait for the inevitable upstream patch that solves this problem.

kleisauke commented 1 year ago

Feel free to commit all those changes to this branch.

I just pushed it to the branch associated with this PR.

atjn commented 1 year ago

IMO the best path forward would be to include libjxl by default and then support vips_block_untrusted_set and vips_operation_block_set. These functions allow people to easily block all untrusted operations (including anything that uses libjxl) with:

vips.blockUntrustedSet(true);

Or specifically disable JPEG XL features with:

vips.operationBlockSet("VipsForeignLoadJxl", true);
vips.operationBlockSet("VipsForeignSaveJxl", true);

You could even apply the JXL-specific block by default, and tell people to disable the block in their code if they want to have JXL support.

I will see if I can figure out how to make a PR for supporting those two functions.

kleisauke commented 1 year ago

I think exposing vips_block_untrusted_set() and vips_operation_block_set() is good idea anyway.

Another concern with enabling JPEG XL support by default is that the binary size (vips.wasm) would increase by ~44% (~2.1 MB). This is fine for non-web/CLI usage (the pre-built binaries provided by sharp are ~16 MB in comparison), but for web usage it's probably a bit too big, especially if a user doesn't use jxlload* and jxlsave*.

To fix this, we could enable the GModule implementation and build a dynamically loadable module for libjxl (vips-jxl.wasm). JPEG XL support is then enabled on the web once the dynamic module is served, and on non-web environments it's enabled by default when you install wasm-vips via the package manager.

If you want, I can look into this next week, it would probably require dropping this patch: https://github.com/kleisauke/wasm-vips/blob/5e3b052fb6cdfc1a04d4e791f7bd56935fd00565/build/patches/glib-emscripten.patch#L169-L189

atjn commented 1 year ago

we could enable the GModule implementation and build a dynamically loadable module for libjxl

If you are willing to spend the time on it, I think that would be awesome! It would also allow us to support much less used formats like AVIF, SVG and JPEG 2000 without worrying about file size ballooning.

With that said, I don't think it is strictly necessary for the libjxl implementation. It will only increase load times ~400ms -> ~530ms over the global median mobile speed. Over a state-of-the-art connection, it is more like ~35ms -> ~46ms. And it's not just for a gimmick, everything is pointing towards JPEG XL becoming the universal standard for web encoded images.

kleisauke commented 1 year ago

Commit 995eba0242b674332107dc91087371aaec9916f6 builds this as a dynamically loadable module (vips-jxl.wasm). JPEG XL support is still enabled by default on all environments, but you can choose to opt-out by doing:

const vips = await Vips({
  // Disable dynamic modules
  dynamicLibraries: []
});
atjn commented 1 year ago

Awesome!

kleisauke commented 1 year ago

v0.0.4 is now available. Dynamic modules can be disabled on the playground by passing the ?disableModules query string. https://wasm-vips.kleisauke.nl/playground/?disableModules

Also, /playground-jxl is now being redirected to /playground, to ensure that the above URLs still work.

RReverser commented 1 year ago

@kleisauke Do I understand right that JPEG-XL is built only when building with dynamic modules support? If so, can "Compiling jxl" be skipped when using --disable-modules? Or is it switched to static linking in that case?

RReverser commented 1 year ago

As I see it, JPEG XL is going to be the dominant image format on the web, and probably also outside the web.

By the way, ICYMI: Chrome removes the experimental JPEG-XL in upcoming versions, and other browsers never shipped it. That makes a lot of people unhappy, but the chances for it to become a common format seem pretty low if this goes through as currently expected. https://bugs.chromium.org/p/chromium/issues/detail?id=1178058

kleisauke commented 1 year ago

Do I understand right that JPEG-XL is built only when building with dynamic modules support? If so, can "Compiling jxl" be skipped when using --disable-modules? Or is it switched to static linking in that case?

The --disable-modules build parameter will disable the build of dynamic modules and load-time dynamic linking. When this parameter is passed, modules are instead being statically linked in vips.wasm. Note that JPEG XL load/save can also be disabled at runtime using:

vips.blockUntrusted(true);
// Or:
// vips.operationBlock('VipsForeignLoadJxl', true)
// vips.operationBlock('VipsForeignSaveJxl', true)

(see commit https://github.com/google/oss-fuzz/commit/890953f0a0029ef889c8c866c674bb8609f90bd0)

This libvips binding is the first to ship with JPEG XL support by default, and I intend to maintain/release this as a dynamic module in further releases. We could argue for having this opt-out by default on web (see modules-web-pre.js), but I imagine this library is also suitable for use as a polyfill.

I'm happy to accept a PR that adds a --disable-jxl build parameter or something similar, most people will not need to run the build script, but it might be an convenient way to reduce the binary size (https://packagephobia.com/result?p=wasm-vips).

Chrome removes the experimental JPEG-XL in upcoming versions

It looks like Google already changed its tone from "we will remove it" to "no support at this time". https://www.thehindubusinessline.com/info-tech/google-chrome-removes-jpeg-xl-image-support/article66094598.ece

RReverser commented 1 year ago

It looks like Google already changed its tone from "we will remove it" to "no support at this time". thehindubusinessline.com/info-tech/google-chrome-removes-jpeg-xl-image-support/article66094598.ece

I'm not sure 3rd-party publisher's interpretation should be taken for a source of truth. I didn't see change in tone on the public communication on the bug.

kleisauke commented 1 year ago

I'm not sure 3rd-party publisher's interpretation should be taken for a source of truth.

You're right, I should probably link CNET's article here, which includes the same statement from Google. https://www.cnet.com/tech/computing/chrome-banishes-jpeg-xl-photo-format-that-could-save-phone-space/

I didn't see change in tone on the public communication on the bug.

Indeed, it's a bit curious that it was communicated this way.

RReverser commented 1 year ago

Indeed, it's a bit curious that it was communicated this way.

I think it's just "business-speak" language.