Expose available YCbCr support from image-tiff crate in image crate (jpeg-compressed tiff and (1,1) ycbcr sampling for now)#2817
Conversation
src/codecs/tiff.rs
Outdated
| icc: Option<Vec<u8>>, | ||
| } | ||
|
|
||
| fn ycbcr_to_rgb8(ycbcr: &[u8]) -> [u8; 3] { |
There was a problem hiding this comment.
There is broad support for YUV formats in the AVIF module which I believe could be reused.
There was a problem hiding this comment.
Nice feature!
Unfortunately, I think you implemented this incorrectly. YCbCr isn't one specific color space, it's a family of color spaces. There are different standards with different conversion coefficients for RGB <-> YCbCr, which produce slightly different versions of YCbCr. For example, you implemented 601 in your ycbcr_to_rgb8 function, but that doesn't mean TIFF is using that version of YCbCr.
If I'm reading the TIFF spec correctly, TIFF actually has a tag that specifies those constants (so TIFF files can use different coefficients). On Page 90-91 of the spec, tag 529 (called "YCbCr Coefficients") is defined and describes the conversion from YCbCr to RGB in detail.
Unfortunately, the tiff crate doesn't seem to recognize tag 529, so you're going to have to do that manually. Tag 530 and 531 for YCbCr SubSampling and YCbCr Positioning are supported, though. I'm not sure why they left out YCbCr Coefficients.
Also, since you only support (1,1) subsampling, could you please add a check that errors if the TIFF file specifies any other subsampling? I think it would be better to check this instead of decoding unsupported files incorrectly.
|
@RunDevelopment Thank you so much for your comment! I have added code to read out tag 529, and use its coefficients when transforming between ycbcr and rgb. The default coefficients 601 are used if the tag is not set. I also added a check on tag 530, and now an error is thrown if the tiff image is non-jpeg compressed and subsampling is anything else than (1,1) (the image-tiff crate also throws an error in this case). |
197g
left a comment
There was a problem hiding this comment.
The approach seems good with a few nits on implementation of the conversion itself.
As a note that does not require any more feedback than thought, I had hoped that tiff's new tag interface could phase out ifd::Value over time and that find_tag_bytes in particular may be used here. However, I see that the code wouldn't be really less complex in this case; it would avoid an allocation though. Fixing this is for another PR but if you have good ideas on a better interface you're welcome to chime in in tiff.
|
Code looks good IMO. Just one last nit about your test files: they are quite large. You added 381 KB in test files to the repo. Small images are generally preferred. Do you have smaller test images that you could use? |
197g
left a comment
There was a problem hiding this comment.
The testfile size is still in my comfort zone, nothing individually approaching 1 MB and adding significant functionality. They'll also stay relevant to testing. The issue for me is the provenance of the images.
This PR enables support for available YCbCr Tiff images decoding from the
image-tiffcrate in the image crate. Previously, any Tiff with YCbCr was rejected immediately. This change exposes the YCbCr color type from the image-tiff decoder and implements the necessary color space conversion to Rgb8. Coefficients to transform between YCbCr and Rgb are extracted from tag 529.Following Tiff images as per the
image-tiffcrate will be decoded successfully:Test images are from https://gitlab.com/libtiff/libtiff-pics, and transformed with ImageMagick (specifically oxford.tif and ycbcr-cat.tif)
Since
image-tiffdoes not yet support other types of YCbCr Tiff images, such as other compression methods or subsamplings, the decoding will not work for those specific cases for now. However the plan is to add upsampling support to be more libtiff conform.