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

Support JPEG XL images #21

Merged
merged 20 commits into from
Sep 1, 2022
Merged

Support JPEG XL images #21

merged 20 commits into from
Sep 1, 2022

Conversation

atjn
Copy link
Contributor

@atjn atjn commented Aug 11, 2022

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:

  • It will be running completely sandboxed
  • Given that it is very new and not really used yet, it is not a target (and should be secure when it becomes a target)
  • If users need high security, they can tell vips to disable any call to the libjxl package, or write their own guards against JPEG XL files

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:

  • Get it to compile
  • Build dependencies correctly
  • Resolve build time errors
  • Make it not crash at runtime
  • Make tests
  • Polish everything

Things that are not included in this PR:

  • benchmarking suite
  • advanced tests

@atjn
Copy link
Contributor Author

atjn commented Aug 11, 2022

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
Copy link
Owner

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

I had a look at the build, and fixed a few things, see:
atjn/wasm-vips@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 emscripten-core/emsdk#1064).

@kleisauke
Copy link
Owner

kleisauke commented Aug 14, 2022

jxlload and jxlsave is now working with commit 8a6e02d and 3aff857. 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 libvips/libvips#2987 for this. Edit: fixed with commit 3aff857.

@atjn
Copy link
Contributor Author

atjn commented Aug 17, 2022

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
Copy link
Contributor Author

atjn commented Aug 17, 2022

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
Copy link
Contributor Author

atjn commented Aug 17, 2022

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
Copy link
Owner

Feel free to commit all those changes to this branch.

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

@atjn atjn marked this pull request as ready for review August 19, 2022 11:29
@atjn
Copy link
Contributor Author

atjn commented Aug 21, 2022

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
Copy link
Owner

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:

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Kleis Auke Wolthuizen <[email protected]>
Date: Tue, 25 May 2021 11:00:00 +0200
Subject: [PATCH 7/8] Disable GModule implementation on Emscripten
Upstream-Status: Pending
diff --git a/gmodule/meson.build b/gmodule/meson.build
index 1111111..2222222 100644
--- a/gmodule/meson.build
+++ b/gmodule/meson.build
@@ -13,7 +13,8 @@ if host_system == 'windows'
# dlopen() filepath must be of the form /path/libname.a(libname.so)
elif host_system == 'aix'
g_module_impl = 'G_MODULE_IMPL_AR'
-elif have_dlopen_dlsym
+# Static linking should be preferred on Emscripten whenever possible
+elif have_dlopen_dlsym and host_system != 'emscripten'
g_module_impl = 'G_MODULE_IMPL_DL'
endif

@atjn
Copy link
Contributor Author

atjn commented Aug 21, 2022

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 kleisauke added this to the v0.0.4 milestone Sep 1, 2022
Copy link
Owner

@kleisauke kleisauke left a comment

Choose a reason for hiding this comment

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

Thank you! This will be in v0.0.4.

@kleisauke kleisauke merged commit a366e2e into kleisauke:master Sep 1, 2022
kleisauke added a commit that referenced this pull request Sep 1, 2022
@kleisauke
Copy link
Owner

kleisauke commented Sep 1, 2022

Commit 995eba0 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
Copy link
Contributor Author

atjn commented Sep 1, 2022

Awesome!

@atjn atjn deleted the libjxl branch September 1, 2022 21:48
@kleisauke
Copy link
Owner

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Owner

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 google/oss-fuzz@890953f)

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
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor

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

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

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.

3 participants