darktable
darktable copied to clipboard
darktable-org
JPEG XL output format support
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:
- This PR requires linking darktable builds with libjxl.
- libjxl's encoding API is not yet stable so the code submitted here could require some maintenance.
- libjxl provides a statement that security is not a priority in their development. Therefore the security aspects of including JPEG XL support should be considered. Any vulnerability would require user interaction: the user would have to export an unsafe image with the jxl format selected. There is still a risk here. I can easily imagine a scenario where someone posts malicious image files online, asking other people to edit it for them. I would appreciate input on this.
- libjxl is in active development and there are still frequent bugs. In libjxl 0.5 encoding an image with an encoding effort setting over 4 can cause a crash. Since 0.6 is not yet released I have added a check that will limit the range of the encoding effort setting between 3 and 4. If bugs like this frequently occur this could result in extra maintenance required for this code.
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.
Thank You @TurboGit , will apply suggestions soon.
BTW, is that otherwise ready for integration ?
No, need to add code to replace the current ICC profile usage. Also, potentially need a response to this: https://github.com/darktable-org/darktable/pull/10044#discussion_r718613235
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 ;-)
I think they should create cmake configs for discovering libjxl
Sure. You can chime in at https://github.com/libjxl/libjxl/issues, there are a few packaging related issues raised recently already.
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 The code in GIMP attempt to detect known GIMP profiles, in order to avoid the dialog which is asking user to keep original profile or to convert to GIMP profile. That's merely for user's convenience.
I have one observation:
Encoding is always with basic_info.uses_original_profile = JXL_TRUE;
That's not wrong but mode without original profile gives sometimes much better lossy compression.
Mode with uses_original_profile == JXL_FALSE require that pixels are converted to sRGB with linear or non-linear TRC (it depends on integer or float data type) prior saving.
I just wanted to let you know, feel free to make own decision whether to implement other mode or not.
That's not wrong but mode without original profile gives sometimes much better lossy compression. Mode with
uses_original_profile == JXL_FALSErequire that pixels are converted to sRGB with linear or non-linear TRC (it depends on integer or float data type) prior saving.
Interesting... That conversion is done automatically by the JXL encoder, or we'd have to do it manually before filling the buffer for JxlEncoderAddImageFrame()?
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.
or we'd have to do it manually before filling the buffer
https://github.com/libjxl/libjxl/blob/main/lib/include/jxl/encode.h#L223
You have to convert the pixels manually.
I just wanted to let you know, feel free to make own decision whether to implement other mode or not.
@novomesk Thank You, that can be a future improvement for this code but for now I think I will leave it.
I've set this to priority low, not that it is not important but as not yet supported by Web browsers there is no hard pressure :)
It looks like the API to add the "Exif" box is now there, so I suggest updating the PR with it and require the next (yet to be released) version of libjxl:
https://github.com/libjxl/libjxl/blob/98c69ebd43af0182554a643209cd36d5ed289480/lib/include/jxl/encode.h#L398-L515
I'm afraid due to personal circumstances I do not expect I will be able to complete this pull request, especially with the very volatile requirements and dependencies. Perhaps someone else should take over?
Thanks for the great effort so far! It's completely understandable, and I hope it's just regular life priorities. I might continue poking around this (and the reader as well?) for the next release...
libjxl provides a statement that security is not a priority in their development
I looked at that file now and can't find such a statement. Did it change since then?
Early versions of libjxl had a different security policy that didn't provide security and vulnerability disclosure support. Versions up to and including 0.3.7 are not covered and won't receive any security advisory.
Maybe this is it? (Meaning, it's more serious now)
Looking at their SECURITY.md, it seems that they now have a security policy in place, covering versions 0.5 and above. As @magicgoose wrote, they clearly state that:
Early versions of libjxl had a different security policy that didn't provide security and vulnerability disclosure support. Versions up to and including 0.3.7 are not covered and won't receive any security advisory.
Furthermore, they say in the security vulnerabilities playbook that:
Note: Versions before 0.5 are not covered by the security policy. Those versions have multiple security issues and should not be used anyway.
Regarding which versions will receive security patches, they state in the same file:
New releases that fix the vulnerability should be PATCH releases, that is, a previous release (like 1.2.3) plus the patches that fix the vulnerability, becoming a new version (like 1.2.4). See the release process for details. At least the latest MINOR release branch should have a PATCH release with the fix, however it might make sense to also backport the fix to older minor branch releases, depending on long-term support schedule for certain releases. For example, if many users are still using a particular older version of the library and updating to a new version requires significant changes (due to a redesigned API or new unavailable dependencies) it is helpful to provide a PATCH release there too.
As of now, two security advisories have been published. CVEs and CWEs were issued, and the vulnerabilities were fixed in v0.6.1.
TL;DR: versions up to 0.3.7 were unsafe; a security policy is in place since version 0.5 with a 90 day disclosure timeline; at least the latest minor version will receive a patch release to fix the vulnerability; earlier versions might also receive a patch if they are still widely used.
So it seems that @magicgoose’s interpretation was correct, and that they are now taking security seriously (which makes sense, since as far as I understand libjxl (v1.0?) will soon become part of the standard as the reference implementation).
EDIT: here is the commit (dated 11 Nov 2021) that updated the security policy.
FWIW, I built this PR locally, successfully exported some images and then opened them using JXLook, Firefox (nightly) and Google Chrome, all on macOS Monterey. I tried 4 different output color profiles: sRGB and {linear,PQ,HLG} Rec2020 RGB. I also used 2 different luminance levels: SDR (here underexposed to preserve highlights; in practice one would apply a tone curve, graduated density, ...) and HDR (exposed 2.3 EVs higher than the SDR one so that highlights appear brighter than "diffuse white", whatever that means). For reference I also exported the images to JPEG and EXR.
Here are my findings:
JXLook opens all files correctly, and it correctly displays all the SDR files, with correct colors and contrast (matching the JPEG for sRGB). Significant posterization is visible, however, when using the linear transfer curve, even in lossless mode (!). The posterization is much stronger for quality=12 than for lossless, though, and comes with very strong JPEG artifacts in the shadows (see the 200% crop below). JXLook opens the HDR files too, but the highlights are clipped.
Firefox only correctly displays the SDR/sRGB image (with accurate colors and contrast), both lossless or lossy. The linear image is too contrasty, the PQ one looks like log footage, and the HLG one too but to a lesser extent. The highlighs are clipped in HDR images.
Chrome did the best by far. It displays both the SDR (sRGB and PQ) and HDR (PQ) images correctly (on a 1000-nits HDR display), matching the reference JPEG in color and the EXR in contrast (the EXR was not color managed properly). The shadows are a bit too contrasty when using the HLG transfer curve. The "linear" images still show significant posterization (both for SDR and HDR) but this is clearly not Chrome’s fault, while the HDR image is not rendered as HDR (instead, the highlights are clipped, although Firefox showed that they are present in the file). The main issue with Chrome is that it did not manage to open the (huge) lossless JXL files, but the lossy files (with quality=12) opened just fine.
So, overall, it seems that we are exporting the images fine, however there is still room for improvement among the various viewers.
The only possible issue is the strong posterization and JPEG artifacts in the shadows when using the linear transfer curve. It seems that the compression algorithm gives less importance to the shadows in this case, which is fine for SDR but becomes problematic for HDR images since it starts affecting the midtones too (see the crop below; the highlights were clipped by JXLook).
So I finally found some time to pick this up and might have something to push in the next couple of days (will require the more modern but unreleased libjxl API on main though).
How do we proceed? Do I try to push to @cyruscook branch, or start a new PR?
I'm happy to give you access to my repo so you can work on this PR if that helps...
I'm happy to give you access to my repo so you can work on this PR if that helps...
I guess being able to push directly to your branch makes sense.
This seems to be working now (please test as much as possible), but I'm getting "API errors" if I try to switch on use_original_profile in lossy mode, no matter the other settings... Any clues @novomesk @jonsneyers ?
use_original_profile with lossy should work.
Do you use RelWithDebInfo build of libjxl? Because in case of error, it prints useful information in terminal window. They are often useful, you will not see them in Release builds.
Thanks, indeed more convenient than launching a gdb session (though I did have to do a Debug build)...
Turns out darktable also uses some "unsafe" rendering intent values ("-1" meaning the intent is set from the colorout module and not the export one) in that last commit which raises this error, so will have to think about how to handle this...
With a couple of modifications of the darktable, I have managed to export https://xn--dsseldorf-q9a.reflexion.tv/nextcloud/index.php/s/SzpyrATHNZA5X7R JPEG XL images with all metadata properly using latest and greatest code from the Exiv2 main branch. So far on x86-64, as the AArch64 variant does not link properly.
@kmilos it would be great to add button to enable progressive/responsive decoding.
Thank you so much.
Thanks for testing. I have rebased now to include the latest Exif fixes in dt and some others in libjxl itself (now requiring https://github.com/libjxl/libjxl/commit/341a7ebc699a06c557200fb43a14c8b1c09d4197 or later).
As for the new "progressive/responsive" option: this setting (like most JXL ones so far for this highly complicated codec), is as clear as mud to me - in jxl/encode.h I have no clue which of the 5 I should be setting and how, so any specification of what you want exactly is greatly appreciated:
JXL_ENC_FRAME_SETTING_GROUP_ORDER (w/ JXL_ENC_FRAME_SETTING_GROUP_ORDER_CENTER_X and JXL_ENC_FRAME_SETTING_GROUP_ORDER_CENTER_Y) JXL_ENC_FRAME_SETTING_RESPONSIVE JXL_ENC_FRAME_SETTING_PROGRESSIVE_AC JXL_ENC_FRAME_SETTING_QPROGRESSIVE_AC JXL_ENC_FRAME_SETTING_PROGRESSIVE_DC
Hvala @kmilos
Yes, indeed. JPEG XL is huge.
I will dig deeper in the specs and sources and then try to clearly define what is supposed by progressive/responsive option for decoding by mobile supercomputers. Most likely this week.
