-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
JPEG XL output format support #10044
Conversation
src/imageio/format/jxl.c
Outdated
|
||
int flags(dt_imageio_module_data_t *data) | ||
{ | ||
// fixme: For now exiv2 does not support writing to bmff containers. |
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.
libjxl should support adding the Exif data directly as cjxl seems to do it (see also avif.c which is also not using exiv2 for this)
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.
libjxl does not have any API for adding exif data when encoding images, cjxl has custom code to add the exif data.
I will try and do the same.
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.
Would be nice to have indeed, as exiv2 probably won't have write support for a while...
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 have added support for writing exif (as provided by the exif
parameter to write_image
) to the outputted file, however XMP is still not supported as the XMP logic happens outside of the image format module so I feel would be out of scope for this PR. As suggested in another comment, however, perhaps in the future I can make a necessary PR to exiv2 to support this. Let me know if this resolves this particular review @kmilos .
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.
Yes, this is the same reason XMP in AVIF is also not supported yet, even though libavif provides and API to write it...
I'd love to hear the core devs comment on this PR - it looks like a "complete" and great job to me, but personally I feel a bit uneasy including the extra cruft of that manual BMFF container creation and Exif box writing in dt, there's quite a bit of burden to assure compliance there...
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.
Would be nice to have indeed, as exiv2 probably won't have write support for a while...
The container format is isobmff like avif and heif are using. I think it is just a matter of making sure that exiv2 knows the file extension and then goes to the isobmff parser ...
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.
exiv2 cannot write to isobmff containers currently, read only. libheif and libavif have their own methods of writing Exif and XMP boxes. libjxl does not (yet).
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.
Metadata (exif/xmp, compressed or uncompressed) read/write should be added to the libjxl api soon. It's probably better to wait until that arrives instead of implementing it independently here.
40dfc93
to
f9fb31a
Compare
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.
LG2M
f9fb31a
to
6dc3bde
Compare
src/imageio/format/jxl.c
Outdated
void *runner = JxlThreadParallelRunnerCreate(NULL, num_threads); | ||
JXL_ASSERT(JxlEncoderSetParallelRunner(encoder, JxlThreadParallelRunner, runner)); | ||
|
||
cmsHPROFILE out_profile = dt_colorspaces_get_output_profile(imgid, over_type, over_filename)->profile; |
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.
Perhaps we should also consider setting the color encoding the "native" way using JxlEncoderSetColorEncoding()
if possible first and then fall back to ICC - see also avif.c
?
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.
What would be the benefit of doing this? I would just have to manually gather all the data that is already included in the icc profile and provide that to libjxl instead right?
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.
There might be a risk that some 3rd party apps don't handle ICC profiles for example, so the "native" way of encoding the color profile in the stream might be preferable? This seems to be the case w/ AVIF/HEIF images, but then again, that might be the legacy of the video side of those containers where video players of course don't usually understand ICC profiles so there was always a preference for the "nclx" way of encoding the color space (this is btw using the same CICP enums as JPEG XL). OTOH, some 3rd party apps behave the opposite (ICC is renederd fine, while CICP is not)...
See e.g these for potential pitfalls:
https://discuss.pixls.us/t/experimenting-with-hlg-and-hdr10-for-still-images/23361/17
https://discuss.pixls.us/t/experimenting-with-hlg-and-hdr10-for-still-images/23361/19
I guess there should be a way for the JXL output plugin to generate both, just like the current AVIF one does based on the output profile selection.
And you don't have to "manually gather" anything, it's all already contained in over_type
, it's just about how one chooses to encode it, see again avif.c
:
https://discuss.pixls.us/t/experimenting-with-hlg-and-hdr10-for-still-images/23361/15
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.
Thank You, that makes more sense.
Could you also clarify the purpose of the if(imgid > 0)
check just above the code you sent (https://github.com/darktable-org/darktable/blob/master/src/imageio/format/avif.c#L284).
I saw the same check in the code of lots of the other output format modules but I didn't understand the significance and didn't do anything similar in the jxl module.
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.
Could you also clarify the purpose of the
if(imgid > 0)
check just above
Not sure, sorry; we'll have to wait for someone else to chip in on that one...
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.
The icc profile can be big making the image bigger. The nclx container just takes a few bytes. We want to safe bandwidth with those new formats!
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.
jxl does not use the "nclx" box in a container, but encodes all non-ICC, non-Exif etc. rendering related metadata in the codestream directly if you chose not go with the other option, so you actually have to decode it first even inside exiv2 just to read that metadata :( I'd keep the ICC option supported, a few kB is not that big deal for high res HDR images (jxl plans to brotli compress ICC and Exif boxes anyway)...
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 hope ab289b9 resolves this? @kmilos
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.
The icc profile also gets encoded in the codestream - it is considered rendering related metadata so part of the codestream, regardless of whether it is done as an Enum colorspace or an ICC profile.
It is a good idea to use Enum colorspaces whenever possible (which should be usually the case, since any reasonable RGB colorspace does have an Enum encoding, including sRGB, Adobe RGB 1998, Display P3, DCI-P3, Rec.2020 PQ or HLG, ProPhoto, etc).
49e5327
to
311ba25
Compare
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.
First pass.
BTW, is that otherwise ready for integration ?
Thank You @TurboGit , will apply suggestions soon.
No, need to add code to replace the current ICC profile usage. |
Ok, I'll put this with wip tag. Ping me when ready. TIA. |
@@ -0,0 +1,37 @@ | |||
# Find libjxl |
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.
As libjxl is using CMake they should write out cmake config files and then you should find libjxl using cmake config mode.
See https://cmake.org/cmake/help/latest/command/install.html#export
You can take a look how libavif is doing it :-)
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.
Hi, sorry, could you please clarify what you are suggesting? If it is a change to libjxl that is probably out of scope for this PR but if libjxl adopt the change in the future I will happily change the darktable libjxl code.
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.
@cryptomilk FYI, looks like there is a PR in progress to address this. However, we should probably stick with the custom FindJXL.cmake. I might consider changing all the prefixes to "JPEGXL" though to match upstream, Krita, and WebKit, and ease future migration...
|
||
for(int k = 0; k < channels; k++) pixels[(y * width * channels) + (x * channels) + k] = buf[k]; | ||
} | ||
} |
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.
JPEG XL only supports 32bit?
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.
It seems the encoder does all the desired conversion and quantization by its own, so passing just the 32-bit float should be ok.
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.
But maybe we do need an "advanced" option to control that quantization?
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.
Got this one wrong, it looks like float
really is the jxl internal working type, and adaptive quantization is already controlled by the quality parameters. 8b and 16b integer data is converted to float if the input files were already quantized like that (PPM, PNG...). So there is no real need for dt to do any conversion (apart from any HDR scaling gotchas).
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.
The format you use to pass the buffer can be float in all cases (which is what libjxl will use internally anyway). The bitdepth you actually use for lossless encoding can be anything (8-bit, 10-bit, 16-bit, up to 24-bit integer or up to lossless 32-bit float) - of course higher bit depth will make bigger files. For lossy encoding, the nominal bitdepth doesn't really matter (it doesn't change how the image actually gets encoded, it's just a header field that is used as a suggested bitdepth to represent the image in after decoding).
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 @jonsneyers for your insights. It sounds like we might want to add the 8/10/12/16-bit integer options then as well.
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.
Yes, but implementation-wise, you can always just use floats to pass the data, and adjust what is stored (in case of lossless compression) by simply changing the bits_per_sample in the encoder option struct.
Btw, maybe it's an idea to update for any 0.6 API changes and require it as min version? |
I think they should create cmake configs for discovering libjxl and then we should require that as the min version. I did the same with libavif back in the time ;-) |
Sure. You can chime in at https://github.com/libjxl/libjxl/issues, there are a few packaging related issues raised recently already. |
I think it is ready now - is there any more feedback from anyone? |
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.
Getting there ;)
src/imageio/format/jxl.c
Outdated
JxlEncoderOptions *options = JxlEncoderOptionsCreate(encoder, NULL); | ||
JXL_ASSERT(JxlEncoderOptionsSetLossless(options, params->lossless ? JXL_TRUE : JXL_FALSE)); | ||
|
||
int decode_effort = params->decoding_effort; |
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.
Think it might be better to follow the API and cjxl command line options and actually use decode_speed as a direct parameter instead of decode_effort instead of this hidden acrobatics.
src/imageio/format/jxl.c
Outdated
// We store quality but jxl wants distance, so we reverse the scale | ||
quality = (quality - 15) * -1; |
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.
Same as above, let's stick to the API and cjxl options, as that is what people will refer to:
IMHO we should use the same values as cjxl's --quality
, --effort
, and --faster_decoding
(passed straight to jxl::CompressParams
)
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.
Good idea.
By the way, Thank You so much for your helpful reviews! Getting through them slowly! 😅
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.
Np, hope I'm not 'nagging' too much, keep up the great work ;)
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.
Actually, the API is setting butteraugli distance directly, and the conversion from quality is more complicated than this. I wonder if we should update the parameters to "distance" (potentially even remove "lossless" and use distance==0 to set it), "encode effort", and "decode speed", as these are then simply passed through and documented by JXL API (and are also present for cjxl
)...
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.
0 distance and lossless mode is not the same thing.
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.
0 distance and lossless mode is not the same thing.
Indeed not, but the API documentation says:
Sets the distance level for lossy compression: target max butteraugli distance, lower = higher quality. Range: 0 .. 15. 0.0 = mathematically lossless (however, use JxlEncoderOptionsSetLossless to use true lossless). 1.0 = visually lossless. Recommended range: 0.5 .. 3.0. Default value: 1.0. If JxlEncoderOptionsSetLossless is used, this value is unused and implied to be 0.
So I think it should be safe (implied) to never set the actual distance to 0, but call JxlEncoderOptionsSetLossless
when the GUI setting is at 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.
The actual distance cannot be set to too small values, because at some point lossy encoding makes no sense anymore and will become larger than lossless. So making the gui setting distance 0 correspond to lossless makes sense, and is also what cjxl does. The minimum non-lossless distance is 0.1 or so (or maybe we lowered it to 0.01, but still, anything below 0.3 or so probably does not really make sense).
Speaking of GIMP, it looks like only sRGB is basically supported from the codestream ATM, all other color spaces must come from ICC, which is why I prefer to have that option more accessible/default... @novomesk Would love to hear you comments on the color encoding for export here as well, thanks. |
@kmilos |
src/imageio/format/jxl.c
Outdated
basic_info.exponent_bits_per_sample = 8; | ||
basic_info.intensity_target = 255.f; | ||
basic_info.min_nits = 0.f; | ||
basic_info.relative_to_max_display = JXL_FALSE; |
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.
These I have no experience with:
- I thought
1.f
is the reference? - How does
relative_to_max_display
work out for the PQ transfer curve case?
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.
To be honest I am just using the values from here: https://github.com/libjxl/libjxl/blob/fd06f61d0e49a28b8b0894e227994b40d2e446d0/lib/jxl/encode.cc#L158-L160
The way I understand it is that intensity target and min nits are allowed to be just the min/max possible intensity if you do not want to calculate the actual min/max intensity.
I'm afraid I'm not able to provide any detailed comments on this as I do not have any experience either.
Perhaps someone else can give a more knowledgable opinion?
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.
Intensity target is the brightness of white, in nits. For SDR the value 255 is fine, for HDR it would be the maximum nits (something like 1000 nits for Rec.2100 HLG, or 10000 nits for PQ). Surprisingly, ICC profiles don't specify this, so we added it in the jxl header because otherwise an image viewer cannot really know what the intended display brightness is.
Basically the linear float value (in nominal range 0.f-1.f), multiplied by the intensity target gives you the brightness in nits of the pixel (assuming it's a gray one). That's why we picked the value 255 for SDR: it is a confusing but convenient value, since it is both a reasonable max brightness in nits to expect from an SDR display, and if you multiply values in 0.f-1.f with it (after applying the transfer curve), you also happen to get values that can just be truncated to get 8-bit integer values.
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.
This whole HDR business is so messy... Most photography/videography is still based on mid-gray anchoring, which makes 1.0 = "diffuse white" (for SDR this is the same as "max white"), but then permits values much greater than 1.0 for highlights. Unfortunately a lot of SW (and ICC) don't work this way, and I believe they assume 1.0 is max white (like you seem to do here as well). So not 100% sure what to do here, because from #3880 it sounds like darktable hasn't converged on how to handle this internally yet...
Side note: one is wasting a whole lot of dynamic range if limiting values to 0-1.0 only (still leaving plentiful 126 stops in the float case luckily), which is undoubtedly problematic if quantizing to float16 with that restriction (14 stops instead of available 30, so worse than 16-bit integer actually).
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 for the excellent explanation Jon.
I think this is probably something that I cannot do anything about without more clear guides as how darktable as an entire project wants to handle this...
For now I guess this will work ok except on some images which darktable already does not seem to properly support.
Perhaps if someone has an example image which will fail with the current code I can try and fix it with that as a reference?
Otherwise for now I think it is best left.
src/imageio/format/jxl.c
Outdated
// We store quality but jxl wants distance, so we reverse the scale | ||
quality = (quality - 15) * -1; |
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.
Np, hope I'm not 'nagging' too much, keep up the great work ;)
I have one observation: That's not wrong but mode without original profile gives sometimes much better lossy compression. I just wanted to let you know, feel free to make own decision whether to implement other mode or not. |
Interesting... That conversion is done automatically by the JXL encoder, or we'd have to do it manually before filling the buffer for For this initial PR I'd personally stick to keeping everything nicely aligned and not increase complexity so we can demonstrate good interoperability and image quality first and foremost. |
https://github.com/libjxl/libjxl/blob/main/lib/include/jxl/encode.h#L223 You have to convert the pixels manually. |
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.
Works for me, time to move forward and have more field testing. Thanks a lot, nice to see JXL on darktable.
Yay! Thanks @cyruscook for doing all the hard initial work! |
Great, congratulations! Now I can finalize and PRize the native jxl reading support that has been waiting to not interfere with this PR. |
Trying to compile on ubuntu 22.04, after installing libjxl-dev, I get:
After removing the I had previously compiled dt without libjxl-dev installed: compilation ended ok, but obviously without any jxl support in dt. |
also fails on openSUSE Tumbleweed: |
and, after multiple wipes and re-fetches, it now compiles properly ??? |
@paolobenve It doesn't look like you're using libjxl 0.7.0 - AFAIK it is only available for Ubuntu 22.10 at the moment. |
No, it seems 0.7.0:
|
This is God knows what version/snapshot (April 26, while 0.7.0 was released just recently on Sept 21), from an unknown PPA... In any case, beyond the scope of this PR. |
At the end I dedided to compile libjxl 0.7.x, following instructions in https://github.com/libjxl/libjxl/tree/v0.7.x Apparently everything was ok. But when I try to compile dt I get various warnings:
Actually, compilation end with this error:
What am I missing? |
Has anyone been able to use it on Arch Linux? Does that require rebuilding from source at the moment? None of the available darktable packages seem to have a libjxl dependency, neither required nor optional. And the JPEG-XL choice is not there in GUI. |
@magicgoose it seems like it will require building from source, make sure you have libjxl available when building it. |
Reported on https://bugs.archlinux.org/task/76799 |
Now works with the latest darktable version 🎉 |
Add support for writing JPEG XL formatted images.
More information about the JPEG XL format can be found here: https://jpeg.org/jpegxl/index.html .
Supporting JPEG XL as an output format will allow darktable users to be able to create JPEG XL images which will offer large benefits for any purposes requiring fast transmission or efficient storage of images as JPEG XL images offer far better compression than JPEG images while retaining more quality, and also support features such as progressive decoding.
JPEG XL is currently supported by Chromium (behind a flag) and Firefox Nightly, however, if darktable users hosting websites offer user agents the option to download and render JPEG XL images this will provide motivation for better support in web browsers.
JPEG XL already has support for many image viewers such as Gimp and Eye of Gnome.
Considerations:
I hope this PR is at the required standard as submitted, however there was a large lack of documentation surrounding a lot of the areas I was contributing to, so please do let me know if there are any more changes I should make.