OSGeo
TIFF+JXL: lossy compression of 16bit data is not supported correctly when the data does not span the full 16bit spectrum
From libjxl/libjxl#1763, lossy 16bit compression is not correctly supported for data with less than 16 bits of range. Investigate whether we can use GTiff's NBIT configuration option to pad the low bits of the pixels.
Not related to libjxl specific expectations, but one tricky aspect of libtiff API to have in mind is that is you use nbits != 8, 16, 32, libtiff expects samples to form a fully dense stream of bits, without padding bits. That is, for nbits in [9,15], in 2 bytes you'll have a part of one sample and a part of another one. See GTiffOddBitsBand::IReadBlock() / IWriteBlock() for how to convert between one sample per uint16 (GDAL expectation) and a fully dense stream of bits (libtiff expectation), without padding bits. In a GDAL perspective, this is a bit sad, since you have to (on the writing side) uint16 (GDAL) -> dense stream of bits (libtiff) -> uint16 with value left shifted (libjxl)
See GTiffOddBitsBand::IReadBlock() / IWriteBlock()
Ah great, thanks for the pointer @rouault , I was as we speak browsing through the libtiff code to see where I could find such a function :)
I was as we speak browsing through the libtiff code to see where I could find such a function :)
it wouldn't probably hurt to move that part of GDAL code in libtiff itself as it might be useful for external users (GDAL) and internal ones (libtiff codecs)
hum, actually I was thinking to an alternative that would avoid all that useless bit shuffling. Just sharing it for the sake of brainstorming, not saying this is necessarily the best solution. If the JXL codec declared a TIFFTAG_JXL_ACTUAL_BIT_DEPTH pseudo-tag, that could be used a bit similarly to GDAL NBITS=x metadata item, that is you'd still use one-sample-per-uint16 for nbits in [9,16] range. But that would require some complications in GDAL GTiff driver to always use the generic purpose GTiffRasterBand instead of the particular GTiffOddBitsBand when compression = JXL, and get/set TIFFTAG_JXL_ACTUAL_BIT_DEPTH appropriately. The downside of it is that (once/if the JXL codec gets upstream to libtiff), standard TIFF utilities wouldn't work out of the box for those "odd" bits value.
hum, actually I was thinking to an alternative that would avoid all that useless bit shuffling. Just sharing it for the sake of brainstorming, not saying this is necessarily the best solution. If the JXL codec declared a TIFFTAG_JXL_ACTUAL_BIT_DEPTH pseudo-tag, that could be used a bit similarly to GDAL NBITS=x metadata item, that is you'd still use one-sample-per-uint16 for nbits in [9,16] range. But that would require some complications in GDAL GTiff driver to always use the generic purpose GTiffRasterBand instead of the particular GTiffOddBitsBand when compression = JXL, and get/set TIFFTAG_JXL_ACTUAL_BIT_DEPTH appropriately. The downside of it is that (once/if the JXL codec gets upstream to libtiff), standard TIFF utilities wouldn't work out of the box for those "odd" bits value.
I have quickly implemented that (if I understood you correctly) as the bit padding was much simpler in this case (i.e. I've added a JXL_NBITS creation option, so td_bitspersamples remains at 8|16 ). The JXL encoded image quality is now correct, however when decoding from such a file we don't have the info at hand in order to apply the inverse bitshift (as TIFFTAG_JXL_ACTUAL_BIT_DEPTH is not saved). Was this what you had in mind ?
Was this what you had in mind ?
perhaps not completely. What I had in mind is that the left/right shifting to please libjxl expectation should be done by the JXL codec itself. On reading, TIFFTAG_JXL_ACTUAL_BIT_DEPTH value should be set from the codestream info, but unfortunately it is not available until we have data. So it looks that my idea doesn't work in fact. And I wouldn't really want JXL_NBITS to surface in the GTiff driver options as we have already too many of them. This would better be done automatically by the GTiff driver.
On reading, TIFFTAG_JXL_ACTUAL_BIT_DEPTH value should be set from the codestream info, but unfortunately it is not available until we have data. So it looks that my idea doesn't work in fact.
We do have access to basic_info.bits_per_sample in the predecode phase, so I believe it can be set from here. Using that I have now managed to make the jxl_codec correctly shift the decoded buffer before sending it back to gdal.
Agreed on your other points.
We do have access to basic_info.bits_per_sample in the predecode phase, so I believe it can be set from here.
yes but the predecode phase is only triggered by a GDAL IReadBlock() call, not just at dataset opening where we need to be able to set the NBITS metadata item.
This issue needs revisiting now that upstream libjxl seems to have added some support for this in libjxl/libjxl#1812
This issue needs revisiting now that upstream libjxl seems to have added some support for this in libjxl/libjxl#1812
Implemented in https://github.com/OSGeo/gdal/pull/7198. I've verified that lossless mode is lossless, and the file is (at least sometimes) smaller with NBITS=12 than with NBITS=16 For lossy mode, I'm less convinced by how libjxl operates. The file is sometimes bigger than in lossless mode, and even with a low value of JXL_DISTANCE like 0.1, the result is quite lossy.
Here's a hardcoded diff where lossy mode on a 16bit image which only has 12bits of range works correctly (i.e. results are visually identical at distance=1):
diff --git a/frmts/gtiff/tif_jxl.c b/frmts/gtiff/tif_jxl.c
index 4cbca2aef5..b7f9218119 100644
--- a/frmts/gtiff/tif_jxl.c
+++ b/frmts/gtiff/tif_jxl.c
@@ -302,12 +302,12 @@ static int JXLPreDecode(TIFF *tif, uint16_t s)
if (td->td_bitspersample != info.bits_per_sample)
{
- TIFFErrorExtR(
- tif, module,
- "JXL basic info bits_per_sample = %d, whereas %d was expected",
- info.bits_per_sample, td->td_bitspersample);
- JxlDecoderReleaseInput(sp->decoder);
- return 0;
+ //TIFFErrorExtR(
+ // tif, module,
+ // "JXL basic info bits_per_sample = %d, whereas %d was expected",
+ // info.bits_per_sample, td->td_bitspersample);
+ //JxlDecoderReleaseInput(sp->decoder);
+ //return 0;
}
if (td->td_planarconfig == PLANARCONFIG_CONTIG)
@@ -499,6 +499,9 @@ static int JXLPreDecode(TIFF *tif, uint16_t s)
_TIFFfreeExt(tif, extra_channel_buffer);
return 0;
}
+ JxlBitDepth jbd ={};
+ jbd.type=JXL_BIT_DEPTH_FROM_CODESTREAM;
+ JxlDecoderSetImageOutBitDepth(sp->decoder, &jbd);
status = JxlDecoderProcessInput(sp->decoder);
if (status != JXL_DEC_FULL_IMAGE)
@@ -695,7 +698,7 @@ static int JXLPostEncode(TIFF *tif)
JxlEncoderInitBasicInfo(&basic_info);
basic_info.xsize = sp->segment_width;
basic_info.ysize = sp->segment_height;
- basic_info.bits_per_sample = td->td_bitspersample;
+ basic_info.bits_per_sample = 12;//td->td_bitspersample;
basic_info.orientation = JXL_ORIENT_IDENTITY;
if (td->td_sampleformat == SAMPLEFORMAT_IEEEFP)
{
@@ -892,6 +895,16 @@ static int JXLPostEncode(TIFF *tif)
}
#endif
+ JxlBitDepth jbd = {};
+ //jbd.bits_per_sample = 16;
+ jbd.type = JXL_BIT_DEPTH_FROM_CODESTREAM;
+ if (JxlEncoderSetFrameBitDepth(opts, &jbd) != JXL_ENC_SUCCESS)
+ {
+ TIFFErrorExtR(tif, module, "JxlEncoderSetFrameBitDepth() failed");
+ JxlEncoderDestroy(enc);
+ return 0;
+ }
+
int retCode =
JxlEncoderAddImageFrame(opts, &format, main_buffer, main_size);
// cleanup now
Here's a hardcoded diff where lossy mode on a 16bit image which only has 12bits of range works correctly (i.e. results are visually identical at distance=1):
I guess this was just an artifact of me just looking at the statistics and not checking visually (sometimes I forget GIS users actually look at images) and/or the image having padding, which is stressing for lossy compression. Trying with an image without padding with JXL_DISTANCE=0.1, the result is actually visually identical. So #7198 is probably OK