-
-
Notifications
You must be signed in to change notification settings - Fork 699
Add jxl chunked save #4174
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
base: master
Are you sure you want to change the base?
Add jxl chunked save #4174
Conversation
wrote some NOTES on what we need to do next
instead, try to drive libvips from data_at it should be A LOT simpler
Add preeval, eval, posteval, minimise signals. We have to move vips_image_preeval(), vips_image_eval(), vips_image_posteval() to the public API.
It seems (from #libjxl on discord) that we'll need to make a threadpool and generate libjxl tiles in parallel. We can't rely on |
…to add-jxl-chunked-save2
experiment with various caching strategies ... but it looks like none will work, since we will always throw away the cache on the original jpegload on minimise we're going to have to use the super-complicated sink_disc plan
We are doing parallel pixel generation by running a libvips pipeline for each 2k x 2k JXL tile. But this means that we emit a This PR works, but only by making We should probably bite the bullet and implement the incredibly complicated |
Ah, a better fix! Don't emit |
Sample JXL HDR image for testing: https://github.com/immich-app/immich/files/14755028/JPEG.XL.HDR.Test.zip libvips loads this as a ushort, it looks fine, I guess the HDR element is in the transfer function. There's a synthetic profile attached, but it only supports
You can import to make a LAB image:
It looks nice: But I can't see any out of range brightnesses:
(the second column in the second row is peak L*, 94.8 here, I'd expect to see values over 100 for specular highlights) You can get the LAB->sRGB conversion automatically like this:
Making this image: That will use the libvips LAB->sRGB conversion. You could also write:
That will convert to sRGB using LCMS and the srgb profile bundled with libvips. |
We'll should probably make libvips ICC import fall back to perceptual if relative is not supported. |
Useful JXL HDR test images: |
With libjxl 0.11.1 in release mode and the current version of this PR I see:
So it's still better, though less than it used to be thanks to libjxl improvements. I bump the min libjxl version requirement to 0.11, which rules out a lot of current linuxes. I'll give it another once-over and see if I can can improve it a bit more. |
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.
g_mutex_{init,clear}()
still needs to be done for mutexes that are part of dynamically allocated structures.
Co-authored-by: Kleis Auke Wolthuizen <github@kleisauke.nl>
if (jxl->delay && | ||
n < jxl->delay_length) | ||
header.duration = jxl->delay[n]; | ||
else | ||
header.duration = jxl->gif_delay * 10; |
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.
Incorporates the changes from 94502d8 and 2672bb6.
if (jxl->delay && | |
n < jxl->delay_length) | |
header.duration = jxl->delay[n]; | |
else | |
header.duration = jxl->gif_delay * 10; | |
if (!jxl->is_animated) | |
header.duration = 0xffffffff; | |
else | |
header.duration = | |
vips_foreign_save_jxl_get_delay(jxl, n); |
*/ | ||
memcpy(exif_data + 4, data, length); | ||
if (jxl->info.have_animation) { | ||
JxlFrameHeader header = { 0 }; |
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.
Nice! It looks like the memset()
in vips_foreign_save_jxl_add_frame()
can also be replaced with zero initialization now (= { 0 }
).
macOS CI failures looks real, but I couldn't reproduce that locally (on Fedora 42 with libjxl 0.11.1). |
We could also consider keeping it behind a |
This PR adds support for the new libjxl chunked save API.
We were preparing a whole frame of pixels then writing in one shot, but that's obviously not great for large images. This adds a new save path using
JxlEncoderAddChunkedFrame()
. With a 30k x 30k sample image I see:It's very slightly faster, since it's able to overlap JPEG decode with JXL encode, and needs 7x less memory.
This is version 3 of this PR I think :( and is quite simple. The
NOTES
file outlines earlier (and crazily complicated) schemes I tried.TODO:
JxlEncoderAddChunkedFrame()
to drive libvips evaluation (and notvips_sink_*()
), so it's missing things likepreeval
,eval
etc. -- we should add these