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

jxl - check option combinations & bit depth #17487

Open
TurboGit opened this issue Sep 17, 2024 · 14 comments · May be fixed by #17624
Open

jxl - check option combinations & bit depth #17487

TurboGit opened this issue Sep 17, 2024 · 14 comments · May be fixed by #17624
Assignees
Labels
bug: pending someone needs to start working on that depends: external lib priority: high core features are broken and not usable at all, software crashes reproduce: confirmed a way to make the bug re-appear 99% of times has been found
Milestone

Comments

@TurboGit
Copy link
Member

I just found out that whatever option is used for bit depth (8, 10, 12, 16, 32) the file size is the same and according to GIMP it is 32bit flow version. Is that expected ?

@kmilos
Copy link
Contributor

kmilos commented Sep 17, 2024

Is that expected ?

For lossy encoding, yes. JPEG XL is a very different beast to other formats.

file size is the same

This is because starting point is the same - we go straight from our float output buffer, and bpp is basically just a metadata hint to the decoder, but they don't have to respect it (especially if they support float input buffers). The alternative would be to forcefully quantize our float output first, then encode it (but encoding in lossy JPEG XL is done internally in float anyway, so that's why we didn't do it that way; we could if we think explicitly introducing quantization errors is somehow a desired feature...?)

For lossess it is a different story obviously.

@TurboGit
Copy link
Member Author

I have the Quality set to 95% so lossy indeed. So if I put to 100% the bit depth should be effective?

@kmilos
Copy link
Contributor

kmilos commented Sep 17, 2024

So if I put to 100% the bit depth should be effective?

Mos def.

@TurboGit
Copy link
Member Author

Mos def.

Sorry I don't parse this :)

@TurboGit
Copy link
Member Author

Also some settings seems to be incompatible. For example 16bit / Float makes JXL to fail. Maybe there is way to improve the interface to avoid selecting incompatible options?

@kmilos
Copy link
Contributor

kmilos commented Sep 17, 2024

Sorry I don't parse this :)

https://www.urbandictionary.com/define.php?term=mos+def 😉

For example 16bit / Float makes JXL to fail.

That should be working. At least at some point all combos were valid and working. Maybe change the title and let's track any eventual issue here? Likely depends on libjxl version and highway (libhwy) version...

@TurboGit TurboGit changed the title jxl - question about bpp jxl - check option combinations & bit depth Sep 17, 2024
@TurboGit
Copy link
Member Author

Maybe change the title and let's track any eventual issue here?

Done.

@kmilos
Copy link
Contributor

kmilos commented Sep 17, 2024

11/12 combos work here (4.9.0+516~g09e219c19d nightly on Windows 11 and i7-1355U; libjxl 0.10.3, highway 1.2.0)

Indeed, only lossless 16b float results in "could not export ..."

Looks like this combo was introduced recently in #17053 @kiwixz

@kiwixz
Copy link
Contributor

kiwixz commented Sep 17, 2024

On my system (debian 12, libjxl 0.7), lossy 16b float fails:

./lib/jxl/enc_modular.cc:288: JXL_FAILURE: 0.207843 is losing precision (mant: 54d4cd)
./lib/jxl/enc_modular.cc:288: JXL_FAILURE: 0.207843 is losing precision (mant: 54d4cd)
./lib/jxl/enc_modular.cc:288: JXL_FAILURE: 0.207843 is losing precision (mant: 54d4cd)
./lib/jxl/enc_modular.cc:288: JXL_FAILURE: 0.207843 is losing precision (mant: 54d4cd)
./lib/jxl/enc_modular.cc:590: JXL_FAILURE: Error in float to integer conversion
./lib/jxl/enc_frame.cc:1318: JXL_RETURN_IF_ERROR code=1: modular_frame_encoder->ComputeEncodingData( *frame_header, *ib.metadata(), &opsin, *extra_channels, lossy_frame_encoder.State(), cms, pool, aux_out, frame_header->encoding == FrameEncoding::kModular)
./lib/jxl/encode.cc:496: Failed to encode frame
    32.1444 [imageio_storage_disk] could not export to file: `/tmp/clown.jxl'!

And lossless 16b float crashes:

./lib/jxl/enc_modular.cc:288: JXL_FAILURE: 0.682353 is losing precision (mant: 2eaeae)
./lib/jxl/enc_modular.cc:288: JXL_FAILURE: 0.101961 is losing precision (mant: 50d0b9)
./lib/jxl/enc_modular.cc:288: JXL_FAILURE: 0.407843 is losing precision (mant: 50d0ce)
./lib/jxl/enc_modular.cc:288: JXL_FAILURE: 0.101961 is losing precision (mant: 50d0b9)
./lib/jxl/enc_modular.cc:590: JXL_FAILURE: Error in float to integer conversion
./lib/jxl/enc_frame.cc:1318: JXL_RETURN_IF_ERROR code=1: modular_frame_encoder->ComputeEncodingData( *frame_header, *ib.metadata(), &opsin, *extra_channels, lossy_frame_encoder.State(), cms, pool, aux_out, frame_header->encoding == FrameEncoding::kModular)
./lib/jxl/enc_patch_dictionary.cc:774: JXL_CHECK: EncodeFrame(cparams, patch_frame_info, state->shared.metadata, ib, &roundtrip_state, cms, pool, special_frame.get(), aux_out ? &patch_aux_out : nullptr)
zsh: illegal hardware instruction (core dumped)  ./build/bin/darktable

Do you get a better error on your end ? I see nothing in libjxl doc or code that suggests lossless 16b float is not supported; but if we can't get it working I guess we need to disable it.

@kmilos
Copy link
Contributor

kmilos commented Sep 17, 2024

I'm only seeing the log (no time to build and run debug, sorry):

[jxl] libjxl call failed with err 1 (src/imageio/format/jxl.c#L370)

Edit: same w/ libjxl 0.11.0...

@TurboGit TurboGit added this to the 5.0 milestone Sep 18, 2024
@TurboGit TurboGit added priority: high core features are broken and not usable at all, software crashes reproduce: confirmed a way to make the bug re-appear 99% of times has been found bug: pending someone needs to start working on that depends: external lib labels Sep 18, 2024
@kiwixz
Copy link
Contributor

kiwixz commented Sep 22, 2024

Opened libjxl/libjxl#3844 to see what they plan to do with this.

@jonnyawsom3
Copy link

Came here from the libjxl repo and read through the related issues and PRs about lossless float JXLs. Thought I would bring up this issue too, since while I tested it with 32 bit floats, it could affect 16 bit floats too libjxl/libjxl#3511.

As far as I'm aware we've only tested floats using PFM, so while lossless 16 bit is supported, we've never had a 32 bit input requested to be stored as lower. Should be a relatively simple fix, but we'll see what they decide on.

@kmilos
Copy link
Contributor

kmilos commented Oct 8, 2024

@kiwixz Looks like there are two options currently:

  1. Disable fp16 lossless for the time being
  2. Do an intermediate conversion using Imath (like we do for e.g. TIFF and EXR for this case only. However, looks like there is then Encoding float16 lossless appears to produce artifacts for specific values libjxl/libjxl#3881

@kiwixz
Copy link
Contributor

kiwixz commented Oct 8, 2024

Nice find, it seems clear that lossless float16 is largely untested. I'll write a patch to disable it.

@kiwixz kiwixz linked a pull request Oct 8, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: pending someone needs to start working on that depends: external lib priority: high core features are broken and not usable at all, software crashes reproduce: confirmed a way to make the bug re-appear 99% of times has been found
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants