From 8f83f0fa82c315d31e2706f9d0cc97c95d3d3491 Mon Sep 17 00:00:00 2001 From: aarshivv <89091351+aarshivv@users.noreply.github.com> Date: Thu, 23 Jul 2026 00:39:19 +0530 Subject: [PATCH] chore: address review feedback on CIELab/TransparencyMask PR --- src/decoder/image.rs | 77 +++++++++++++++++++++--------------------- src/decoder/mod.rs | 30 +++++++++++++--- tests/decode_images.rs | 34 +++++++++++-------- 3 files changed, 83 insertions(+), 58 deletions(-) diff --git a/src/decoder/image.rs b/src/decoder/image.rs index ccc2b0f..5aa98c7 100644 --- a/src/decoder/image.rs +++ b/src/decoder/image.rs @@ -592,16 +592,15 @@ impl Image { )), }, // CIE L*a*b* (TIFF6 Section 23, pp 110-115) encodes a/b as signed int8 - // (-128..127) while ICC L*a*b* uses unsigned with +128 bias. For 8-bit we - // re-bias a/b during expand_chunk so callers see the same encoding as ICCLab. + // (-128..=127) while ICC L*a*b* (photometric=9) uses unsigned samples with + // a +128 bias. For 8-bit we re-bias a/b in post-processing (see + // `cielab_to_icclab`) so callers see the same encoding as ICCLab. // - // libtiff takes the same approach — accepts only 8-bit CIELab and converts - // without checking SampleFormat: - // https://libtiff.gitlab.io/libtiff/functions/TIFFcolor.html - // https://gitlab.com/libtiff/libtiff/-/blob/master/libtiff/tif_getimage.c - // - // 16-bit CIELab would need L=uint16 + a/b=int16, which SampleFormat can't - // express (it applies uniformly). libtiff doesn't support it either. + // Only 8-bit is accepted here. 16-bit CIELab is a valid format that libtiff + // does decode (`putcontig8bitCIELab16`, selected by `initCIELabConversion` + // in tif_getimage.c line 2216 @ 04884181e8903915038be553bfab404302d98ea0), + // but it uses L=uint16 with a/b=int16 — a mixed layout that `SampleFormat`, + // which applies uniformly to every sample, cannot express. Left as follow-up. PhotometricInterpretation::CIELab => match self.photometric_samples { 3 if matches!(self.bits_per_sample, 8) => Ok(ColorType::Lab(self.bits_per_sample)), _ => Err(TiffError::UnsupportedError( @@ -1202,12 +1201,12 @@ impl Image { scratch, ); - if photometric_interpretation == PhotometricInterpretation::WhiteIsZero { - super::invert_colors(out_row, color_type, self.sample_format)?; - } - if photometric_interpretation == PhotometricInterpretation::CIELab { - super::cielab_to_icclab(out_row); - } + super::post_process_row( + out_row, + photometric_interpretation, + color_type, + self.sample_format, + )?; } } else if is_output_chunk_rows && is_all_bits { // Here we can read directly into the output buffer itself. @@ -1225,12 +1224,12 @@ impl Image { ); } - if photometric_interpretation == PhotometricInterpretation::WhiteIsZero { - super::invert_colors(tile, color_type, self.sample_format)?; - } - if photometric_interpretation == PhotometricInterpretation::CIELab { - super::cielab_to_icclab(tile); - } + super::post_process_row( + tile, + photometric_interpretation, + color_type, + self.sample_format, + )?; } else if chunk_row_bytes > data_row_bytes && self.predictor == Predictor::FloatingPoint { // The floating point predictor shuffles the padding bytes into the encoded output, so // this case is handled specially when needed. @@ -1254,12 +1253,12 @@ impl Image { } _ => unreachable!(), } - if photometric_interpretation == PhotometricInterpretation::WhiteIsZero { - super::invert_colors(row, color_type, self.sample_format)?; - } - if photometric_interpretation == PhotometricInterpretation::CIELab { - super::cielab_to_icclab(row); - } + super::post_process_row( + row, + photometric_interpretation, + color_type, + self.sample_format, + )?; } } else if is_all_bits { // We read row-by-row but each row fits in its output buffer. @@ -1286,12 +1285,12 @@ impl Image { scratch, ); - if photometric_interpretation == PhotometricInterpretation::WhiteIsZero { - super::invert_colors(row, color_type, self.sample_format)?; - } - if photometric_interpretation == PhotometricInterpretation::CIELab { - super::cielab_to_icclab(row); - } + super::post_process_row( + row, + photometric_interpretation, + color_type, + self.sample_format, + )?; } } else { // The encoded data potentially takes up more space than the output data so we must be @@ -1331,12 +1330,12 @@ impl Image { scratch, ); - if photometric_interpretation == PhotometricInterpretation::WhiteIsZero { - super::invert_colors(row, color_type, self.sample_format)?; - } - if photometric_interpretation == PhotometricInterpretation::CIELab { - super::cielab_to_icclab(row); - } + super::post_process_row( + row, + photometric_interpretation, + color_type, + self.sample_format, + )?; } } diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs index bd318ba..9ab8d3f 100644 --- a/src/decoder/mod.rs +++ b/src/decoder/mod.rs @@ -3,8 +3,8 @@ use std::io::{self, Read, Seek}; use std::num::NonZeroUsize; use crate::tags::{ - ByteOrder, IfdPointer, PlanarConfiguration, Predictor, SampleFormat, Tag, TiffVariant, Type, - ValueBuffer, + ByteOrder, IfdPointer, PhotometricInterpretation, PlanarConfiguration, Predictor, SampleFormat, + Tag, TiffVariant, Type, ValueBuffer, }; use crate::{ ColorType, Directory, TiffError, TiffFormatError, TiffResult, TiffUnsupportedError, UsageError, @@ -667,12 +667,32 @@ fn invert_colors( /// See libtiff's TIFFCIELabToXYZ for reference: /// https://gitlab.com/libtiff/libtiff/-/blob/master/libtiff/tif_color.c fn cielab_to_icclab(buf: &mut [u8]) { - for pixel in buf.chunks_exact_mut(3) { - pixel[1] = pixel[1].wrapping_add(128); - pixel[2] = pixel[2].wrapping_add(128); + for [_, a, b] in buf.as_chunks_mut::<3>().0 { + *a = a.wrapping_add(128); + *b = b.wrapping_add(128); } } +/// Apply photometric-interpretation-specific post-processing to a decoded row. +/// +/// Some interpretations need a fixup once the raw samples are in the output +/// buffer: `WhiteIsZero` inverts intensities, and `CIELab` re-biases the signed +/// a/b channels into the unsigned ICCLab encoding. These are mutually exclusive, +/// so at most one applies to any given row. +fn post_process_row( + buf: &mut [u8], + photometric_interpretation: PhotometricInterpretation, + color_type: ColorType, + sample_format: SampleFormat, +) -> TiffResult<()> { + match photometric_interpretation { + PhotometricInterpretation::WhiteIsZero => invert_colors(buf, color_type, sample_format)?, + PhotometricInterpretation::CIELab => cielab_to_icclab(buf), + _ => {} + } + Ok(()) +} + /// Fix endianness. If `byte_order` matches the host, then conversion is a no-op. fn fix_endianness(buf: &mut [u8], byte_order: ByteOrder, bit_depth: u8) { let host = ByteOrder::native(); diff --git a/tests/decode_images.rs b/tests/decode_images.rs index 1e2fe84..7810359 100644 --- a/tests/decode_images.rs +++ b/tests/decode_images.rs @@ -1,7 +1,7 @@ extern crate tiff; use tiff::decoder::{ifd, Decoder, DecodingSampleBuffer}; -use tiff::tags::{ByteOrder, Type}; +use tiff::tags::{ByteOrder, PhotometricInterpretation, Type}; use tiff::ColorType; use std::fs::File; @@ -1063,7 +1063,7 @@ fn test_gray_u12_hpredict() { /// Build a minimal uncompressed TIFF in memory with the given photometric interpretation. /// Returns a 2x2 image with the provided pixel data as a single strip. fn build_raw_tiff( - photometric: u16, + photometric: PhotometricInterpretation, bits_per_sample: u16, samples_per_pixel: u16, pixel_data: &[u8], @@ -1081,19 +1081,20 @@ fn build_raw_tiff( buf.extend_from_slice(&ifd_offset.to_le_bytes()); buf.extend_from_slice(&num_tags.to_le_bytes()); - // IFD entries (must be ascending tag order) + // IFD entries (must be ascending tag order). Tag IDs stay raw for the + // ascending-order property; the field type uses the `Type` enum. for &(tag, typ, value) in &[ - (256u16, 3u16, width as u32), // ImageWidth - (257, 3, height as u32), // ImageLength - (258, 3, bits_per_sample as u32), // BitsPerSample - (259, 3, 1), // Compression = None - (262, 3, photometric as u32), // PhotometricInterpretation - (273, 4, strip_offset), // StripOffsets - (277, 3, samples_per_pixel as u32), // SamplesPerPixel - (279, 4, pixel_data.len() as u32), // StripByteCounts + (256u16, Type::SHORT, width as u32), // ImageWidth + (257, Type::SHORT, height as u32), // ImageLength + (258, Type::SHORT, bits_per_sample as u32), // BitsPerSample + (259, Type::SHORT, 1), // Compression = None + (262, Type::SHORT, photometric.to_u16() as u32), // PhotometricInterpretation + (273, Type::LONG, strip_offset), // StripOffsets + (277, Type::SHORT, samples_per_pixel as u32), // SamplesPerPixel + (279, Type::LONG, pixel_data.len() as u32), // StripByteCounts ] { buf.extend_from_slice(&tag.to_le_bytes()); - buf.extend_from_slice(&typ.to_le_bytes()); + buf.extend_from_slice(&typ.to_u16().to_le_bytes()); buf.extend_from_slice(&1u32.to_le_bytes()); buf.extend_from_slice(&value.to_le_bytes()); } @@ -1106,7 +1107,12 @@ fn build_raw_tiff( #[test] fn test_transparency_mask() { // 2x2 mask: values 0, 255, 128, 64 - let data = build_raw_tiff(4, 8, 1, &[0, 255, 128, 64]); + let data = build_raw_tiff( + PhotometricInterpretation::TransparencyMask, + 8, + 1, + &[0, 255, 128, 64], + ); let mut decoder = Decoder::open(Cursor::new(&data)).unwrap(); decoder.next_image().unwrap(); @@ -1140,7 +1146,7 @@ fn test_cielab_decode() { 0, 128, 128, // pixel 3: L=0, a=-128 (as i8), b=-128 (as i8) ]; - let data = build_raw_tiff(8, 8, 3, &pixels); + let data = build_raw_tiff(PhotometricInterpretation::CIELab, 8, 3, &pixels); let mut decoder = Decoder::open(Cursor::new(&data)).unwrap(); decoder.next_image().unwrap();