Jump to conversation
Unresolved conversations (23)
@TurboGit TurboGit Oct 21, 2022
I suppose here and below we want to use `< >` as those are external OS includes.
Outdated
src/imageio/format/jxl.c
kmilos
@TurboGit TurboGit Oct 21, 2022
I know nothing about JXL... But won't 16bits or above be a nice default to have better quality by default and move away from standard jpeg 8bits?
data/darktableconfig.xml.in
TurboGit kmilos
@TurboGit TurboGit Oct 21, 2022
const
Outdated
src/imageio/format/jxl.c
kmilos
@TurboGit TurboGit Oct 21, 2022
const
Outdated
src/imageio/format/jxl.c
kmilos
@TurboGit TurboGit Oct 21, 2022
const
Outdated
src/imageio/format/jxl.c
kmilos
@TurboGit TurboGit Oct 21, 2022
I would add a tooltip for this, to me it is not obvious what this is about.
Outdated
src/imageio/format/jxl.c
kmilos
@TurboGit TurboGit Oct 21, 2022
encode time -> encoding time ?
Outdated
src/imageio/format/jxl.c
kmilos
@TurboGit TurboGit Oct 5, 2022
I would add here: `color_encoding.white_point = JXL_WHITE_POINT_D65;` as this is common to all types and removed the setting in the `switch()` below.
src/imageio/format/jxl.c
kmilos
@dterrahe dterrahe Sep 23, 2022
`dt_bauhaus_widget_set_label` should be fed untranslated names (like `N_("quality")`) so that shortcut definitions don't break when the language is changed. Same below.
Outdated
src/imageio/format/jxl.c
kmilos
@kmilos kmilos Oct 15, 2021
```suggestion // If we haven't managed to find the color encoding we need to fallback to ICC if(!write_icc) { LIBJXL_ASSERT(JxlEncoderSetColorEncoding(encoder, &color_encoding)); } else ```
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 15, 2021
```suggestion ```
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 15, 2021
```suggestion write_icc = true; ```
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 15, 2021
Could even remove this check because [all dt profiles are RGB currently](https://github.com/darktable-org/darktable/blob/80839f97d898c213edeecb543d46deb6a6f48452/src/common/colorspaces.c#L987)...
Outdated
src/imageio/format/jxl.c
cyruscook
@kmilos kmilos Oct 15, 2021
```suggestion ```
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 15, 2021
```suggestion bool write_icc = false; JxlColorEncoding color_encoding; ```
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 15, 2021
rename to `LIBJXL_FAIL` to be consistent w/ macro above?
src/imageio/format/jxl.c
cyruscook
@kmilos kmilos Oct 14, 2021
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](https://github.com/darktable-org/darktable/issues/3880) case?
Outdated
src/imageio/format/jxl.c
cyruscook kmilos
jonsneyers
@kmilos kmilos Oct 13, 2021
Same as above, let's stick to the API and cjxl options, as that is what people will refer to: https://github.com/libjxl/libjxl/blob/0f387788921c76e9e3490c5aba3fd80cf2fb3cfe/tools/cjxl.cc#L328-L353 IMHO we should use the same values as cjxl's `--quality`, `--effort`, and `--faster_decoding` (passed straight to `jxl::CompressParams`)
Outdated
src/imageio/format/jxl.c
jonsneyers kmilos
novomesk
@kmilos kmilos Oct 13, 2021
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.
Outdated
src/imageio/format/jxl.c
@cryptomilk cryptomilk Oct 12, 2021
JPEG XL only supports 32bit?
src/imageio/format/jxl.c
jonsneyers kmilos
@cryptomilk cryptomilk Oct 12, 2021
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 :-)
cmake/modules/FindJXL.cmake
kmilos cyruscook
@kmilos kmilos Sep 22, 2021
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`?
Outdated
src/imageio/format/jxl.c
jonsneyers cyruscook
kmilos
@kmilos kmilos Sep 21, 2021
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)
Outdated
src/imageio/format/jxl.c
jonsneyers kmilos
cryptomilk
Resolved conversations (37)
@kmilos kmilos Oct 14, 2021
Please remove this last part of the sentence, as it could be misleading - I don't believe it's true, ICC has be around for ages, JXL native color encoding is new and less likely to be widely supported.
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 14, 2021
I'd revert back to setting and testing `bool write_icc` for exclusive `JxlEncoderSetColorEncoding` or `JxlEncoderSetICCProfile`, `goto`s are not that easy to follow (ok only for the drop out case on some failure...)
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 14, 2021
remove the CUSTOM related stuff
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 14, 2021
not in CICP/H.273, use ICC
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 14, 2021
ditto
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 14, 2021
set `color_encoding.primaries` instead (for primaries, 709 = sRGB)
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 14, 2021
This one doesn't exist in CICP/H.273, so just fall back to ICC
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 14, 2021
No need to set these at all - we never want to use CUSTOM values: either standard ones, or go the ICC way
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 13, 2021
malloc check?
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 13, 2021
Not really an error, ok to print as a warning on stdout
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 13, 2021
For the same reasons as above, the standard output color spaces should use existing CICP/H.273 enums like JXL_PRIMARIES_SRGB etc. Please leave the custom values just for ones that cannot be covered. Also, please move this section to be exclusive w/ the `if(write_icc)` block below as it expresses the intent better. [The exclusivity is mandated by the spec.](https://github.com/libjxl/libjxl/blob/0f387788921c76e9e3490c5aba3fd80cf2fb3cfe/lib/include/jxl/encode.h#L253-L282)
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 13, 2021
These two are for proofing on display only, not used for export AFAIK
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 13, 2021
Ditto, I'd stick to the really usable ones (see avif.c again)
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 13, 2021
Don't think we want to cover this one as channels/primaries are switched?
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 13, 2021
Section might be more readable with switch/case? (see avif.c)
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 13, 2021
I don't think this is ideal: you want to store JXL_WHITEPOINT_D65, JXL_WHITEPOINT_DCI, etc. when known, as I envisage any JXL readers will leverage those known values to figure out the reverse process... The custom white point setting should only happen if you don't hit any of the "standard" `over_type`s below, and the cases below should include the white point as well.
Outdated
src/imageio/format/jxl.c
novomesk kmilos
@kmilos kmilos Oct 13, 2021
error check?
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Oct 13, 2021
```suggestion option(USE_JXL "Enable JPEG XL export support" ON) ```
Outdated
DefineOptions.cmake
@TurboGit TurboGit Oct 4, 2021
can probably be a const (likewise just below).
Outdated
src/imageio/format/jxl.c
@TurboGit TurboGit Oct 4, 2021
const
Outdated
src/imageio/format/jxl.c
@TurboGit TurboGit Oct 4, 2021
const
Outdated
src/imageio/format/jxl.c
@TurboGit TurboGit Oct 4, 2021
const
Outdated
src/imageio/format/jxl.c
@TurboGit TurboGit Oct 4, 2021
please use calloc() here to ensure all is initialized to 0.
Outdated
src/imageio/format/jxl.c
@TurboGit TurboGit Oct 4, 2021
const
Outdated
src/imageio/format/jxl.c
@TurboGit TurboGit Oct 4, 2021
const
Outdated
src/imageio/format/jxl.c
@TurboGit TurboGit Oct 4, 2021
const
Outdated
src/imageio/format/jxl.c
@TurboGit TurboGit Oct 4, 2021
const
Outdated
src/imageio/format/jxl.c
@TurboGit TurboGit Oct 4, 2021
const
Outdated
src/imageio/format/jxl.c
@TurboGit TurboGit Oct 4, 2021
const
Outdated
src/imageio/format/jxl.c
@TurboGit TurboGit Oct 4, 2021
Is that some remaining debug code ?
Outdated
src/CMakeLists.txt
cyruscook
@kmilos kmilos Sep 27, 2021
Remove this option from default config, we always use BMFF container anyway?
data/darktableconfig.xml.in
@kmilos kmilos Sep 27, 2021
Should be JXL_TRUE?
Outdated
src/imageio/format/jxl.c
cyruscook kmilos
x4e
@kmilos kmilos Sep 21, 2021
This deleted macro is still found a in a few places
Outdated
src/imageio/format/jxl.c
@kmilos kmilos Sep 21, 2021
remove dead code related to this macro if unused
Outdated
src/imageio/format/jxl.c
cyruscook kmilos
@kmilos kmilos Sep 21, 2021
File not changed, please remove from commit
Outdated
cmake/modules/FindOpenJPEG.cmake
@kmilos kmilos Sep 21, 2021
Consider requiring min 0.5 version and removing support for anything below, as it seems those were not mature/stable anyway?
Outdated
src/CMakeLists.txt
@TurboGit TurboGit Sep 21, 2021
2021 only here :)
Outdated
src/imageio/format/jxl.c