JXL: Preserve ICC profile for lossless encoding #8074
Conversation
When losslessly encoding an image to JXL, attempt to retrieve any ICC profile set on the source image, and if one exists, apply it to the output JXL image instead of using JxlEncoderSetColorEncoding. Fixes ImageMagick#8022
| basic_info.uses_original_profile=JXL_TRUE; | ||
| { | ||
| icc_profile = GetImageProfile(image, "icc"); | ||
| if (icc_profile != (StringInfo *) NULL) | ||
| basic_info.uses_original_profile=JXL_TRUE; | ||
| } |
There was a problem hiding this comment.
The uses_original_profile flag is a hint for the JXL decoder, and has no real meaning if there's no profile attached to the encoded image. So, we only set it when we're also applying an ICC profile.
There was a problem hiding this comment.
Unfortunately it gets slightly more confusing.
basic_info.uses_original_profile should be true for lossless and false for lossy, as it controls when XYB is used internally instead of RGB. The naming could probably be better.
There was a problem hiding this comment.
Ohhh! I didn't realize that at all, yeah. So it should still be set unconditionally if quality == 100, then, regardless of whether or not there's an ICC profile? My mistake for assuming otherwise, thanks.
There was a problem hiding this comment.
Yeah. I know it's confusing and we've had a few asking why files aren't working, only to see that flag set incorrectly
There was a problem hiding this comment.
Fixed now, though I wonder if I should also add a comment to the delegate code explaining the meaning of the flag. (I don't love the idea of documenting libjxl in ImageMagick's source, tho, that feels like it'll inevitably come back to bite me eventually.)
There was a problem hiding this comment.
I was just thinking, I'll add a comment to libjxl. Though I think documenting it in the wrapper here can't hurt too. Since most will only see this instead of libjxl itself.
There was a problem hiding this comment.
@jonnyawsom3 Well, if you're going to add something to the JXL documentation (even if just API documentation), I'd much rather the comment here just be something like,
/* Necessary for lossless decoding.
* See: [url]
*/There was a problem hiding this comment.
I mean, it's only gonna be short anyway
Controls internal color space, separate to displayed color space.
True = RGB False = XYB VarDCT requires False, Lossless requires True
or something similar
Prerequisites
Description
When losslessly encoding an image to JXL, attempt to retrieve any ICC profile set on the source image, and if one exists, apply it to the output JXL image instead of using JxlEncoderSetColorEncoding.
Fixes #8022