chore: address review feedback on CIELab/TransparencyMask PR

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
+            // (-128..=127) while ICC L*a*b* (photometric=9) uses unsigned samples with
-            // re-bias a/b during expand_chunk so callers see the same encoding as ICCLab.
+            // 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
+            // Only 8-bit is accepted here. 16-bit CIELab is a valid format that libtiff
-            // without checking SampleFormat:
+            // does decode (`putcontig8bitCIELab16`, selected by `initCIELabConversion`
-            // https://libtiff.gitlab.io/libtiff/functions/TIFFcolor.html
+            // in tif_getimage.c line 2216 @ 04884181e8903915038be553bfab404302d98ea0),
-            // https://gitlab.com/libtiff/libtiff/-/blob/master/libtiff/tif_getimage.c
+            // 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.
-            // 16-bit CIELab would need L=uint16 + a/b=int16, which SampleFormat can't
-            // express (it applies uniformly). libtiff doesn't support it either.
             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::post_process_row(
-                    super::invert_colors(out_row, color_type, self.sample_format)?;
+                    out_row,
-                }
+                    photometric_interpretation,
-                if photometric_interpretation == PhotometricInterpretation::CIELab {
+                    color_type,
-                    super::cielab_to_icclab(out_row);
+                    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::post_process_row(
-                super::invert_colors(tile, color_type, self.sample_format)?;
+                tile,
-            }
+                photometric_interpretation,
-            if photometric_interpretation == PhotometricInterpretation::CIELab {
+                color_type,
-                super::cielab_to_icclab(tile);
+                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::post_process_row(
-                    super::invert_colors(row, color_type, self.sample_format)?;
+                    row,
-                }
+                    photometric_interpretation,
-                if photometric_interpretation == PhotometricInterpretation::CIELab {
+                    color_type,
-                    super::cielab_to_icclab(row);
+                    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::post_process_row(
-                    super::invert_colors(row, color_type, self.sample_format)?;
+                    row,
-                }
+                    photometric_interpretation,
-                if photometric_interpretation == PhotometricInterpretation::CIELab {
+                    color_type,
-                    super::cielab_to_icclab(row);
+                    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::post_process_row(
-                    super::invert_colors(row, color_type, self.sample_format)?;
+                    row,
-                }
+                    photometric_interpretation,
-                if photometric_interpretation == PhotometricInterpretation::CIELab {
+                    color_type,
-                    super::cielab_to_icclab(row);
+                    self.sample_format,
-                }
+                )?;
             }
         }
 

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,
+    ByteOrder, IfdPointer, PhotometricInterpretation, PlanarConfiguration, Predictor, SampleFormat,
-    ValueBuffer,
+    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) {
+    for [_, a, b] in buf.as_chunks_mut::<3>().0 {
-        pixel[1] = pixel[1].wrapping_add(128);
+        *a = a.wrapping_add(128);
-        pixel[2] = pixel[2].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();

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
+        (256u16, Type::SHORT, width as u32),             // ImageWidth
-        (257, 3, height as u32),            // ImageLength
+        (257, Type::SHORT, height as u32),               // ImageLength
-        (258, 3, bits_per_sample as u32),   // BitsPerSample
+        (258, Type::SHORT, bits_per_sample as u32),      // BitsPerSample
-        (259, 3, 1),                        // Compression = None
+        (259, Type::SHORT, 1),                           // Compression = None
-        (262, 3, photometric as u32),       // PhotometricInterpretation
+        (262, Type::SHORT, photometric.to_u16() as u32), // PhotometricInterpretation
-        (273, 4, strip_offset),             // StripOffsets
+        (273, Type::LONG, strip_offset),                 // StripOffsets
-        (277, 3, samples_per_pixel as u32), // SamplesPerPixel
+        (277, Type::SHORT, samples_per_pixel as u32),    // SamplesPerPixel
-        (279, 4, pixel_data.len() as u32),  // StripByteCounts
+        (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();