diff --git a/Cargo.toml b/Cargo.toml index c61633b..f6c7bec 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ half = { version = "2.4.1" } quick-error = "2.0.1" # Rename to avoid confusion with the feature name. -fax34 = { package = "fax", version = "0.2.4", optional = true } +fax34 = { package = "fax", version = "0.2.6", optional = true } flate2 = { version = "1.0.20", optional = true } weezl = { version = "0.1.10", optional = true } zstd = { version = "0.13", optional = true } diff --git a/src/decoder/image.rs b/src/decoder/image.rs index 3ce9b3e..a2673ce 100644 --- a/src/decoder/image.rs +++ b/src/decoder/image.rs @@ -344,6 +344,9 @@ impl Image { ), )), }, + // TODO: treatment of WhiteIsZero is not quite consistent with `invert_colors` that is + // later called when that interpretation is read. That function does not support + // Multiband as a color type and will error. It's unclear how to resolve that exactly. PhotometricInterpretation::BlackIsZero | PhotometricInterpretation::WhiteIsZero => { match self.samples { 1 => Ok(ColorType::Gray(self.bits_per_sample)), @@ -684,7 +687,7 @@ impl Image { ); } if photometric_interpretation == PhotometricInterpretation::WhiteIsZero { - super::invert_colors(tile, color_type, self.sample_format); + super::invert_colors(tile, 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 @@ -701,7 +704,7 @@ impl Image { _ => unreachable!(), } if photometric_interpretation == PhotometricInterpretation::WhiteIsZero { - super::invert_colors(row, color_type, self.sample_format); + super::invert_colors(row, color_type, self.sample_format)?; } } } else { @@ -723,7 +726,7 @@ impl Image { predictor, ); if photometric_interpretation == PhotometricInterpretation::WhiteIsZero { - super::invert_colors(row, color_type, self.sample_format); + super::invert_colors(row, color_type, self.sample_format)?; } } } diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs index 3e03a9d..d4db1c2 100644 --- a/src/decoder/mod.rs +++ b/src/decoder/mod.rs @@ -475,11 +475,22 @@ fn fix_endianness_and_predict( } } -fn invert_colors(buf: &mut [u8], color_type: ColorType, sample_format: SampleFormat) { +fn invert_colors( + buf: &mut [u8], + color_type: ColorType, + sample_format: SampleFormat, +) -> TiffResult<()> { match (color_type, sample_format) { - (ColorType::Gray(8), SampleFormat::Uint) => { + // Where pixels do not cross a byte boundary + (ColorType::Gray(1 | 2 | 4 | 8), SampleFormat::Uint) => { for x in buf { - *x = 0xff - *x; + // Equivalent to both of the following: + // + // *x = 0xff - *x + // *x = !*x + // + // since -x = !x+1 + *x = !*x; } } (ColorType::Gray(16), SampleFormat::Uint) => { @@ -512,8 +523,14 @@ fn invert_colors(buf: &mut [u8], color_type: ColorType, sample_format: SampleFor x.copy_from_slice(&(1.0 - v).to_ne_bytes()); } } - _ => {} + _ => { + return Err(TiffError::UnsupportedError( + TiffUnsupportedError::UnknownInterpretation, + )) + } } + + Ok(()) } /// Fix endianness. If `byte_order` matches the host, then conversion is a no-op. diff --git a/src/decoder/stream.rs b/src/decoder/stream.rs index d0b05a5..6330969 100644 --- a/src/decoder/stream.rs +++ b/src/decoder/stream.rs @@ -322,6 +322,10 @@ impl Read for Group4Reader { self.y += 1; // We known `transitions` yields exactly `self.width` items (per doc). + // FIXME: performance. We do not need an individual pixel iterator, filling + // memory especially for long runs can be much quicker. The `transitions` are + // the positions at which each run-length ends, i.e. the prefix sum of run + // lengths. Runs in fax4 start with white. let transitions = fax34::decoder::pels(self.decoder.transition(), self.width); let buffer = self.line_buf.get_mut(); @@ -329,6 +333,23 @@ impl Read for Group4Reader { let target = &mut buffer[..]; + // Note: it may seem strange to treat black as 0b1 and white as 0b0 despite all + // our streams by default decoding as-if PhotometricInterpretation::BlackIsMin. + // This is however consistent with libtiff. It seems that fax4's "White" + // differs from what libtiff thinks of as "White". For content, a line of data + // is in runlength encoding of white-black-white-black always starting with + // white. In libtiff, the loop always does both colors in one go and the + // structure is: + // + // ``` + // for (; runs < erun; runs += 2) + // // white run + // do { *lp++ = 0L; } while (…) + // // black run + // do { *lp++ = -1L; } while (…) + // ``` + // + // So indeed the Fax4::White run is implemented by filling with zeros. let mut bits = transitions.map(|c| match c { fax34::Color::Black => true, fax34::Color::White => false, diff --git a/tests/decode_images.rs b/tests/decode_images.rs index 2ebe4fc..f6d7aa6 100644 --- a/tests/decode_images.rs +++ b/tests/decode_images.rs @@ -678,3 +678,9 @@ fn bytes_gray_f32() { fn test_fax4() { test_image_sum_u8("fax4.tiff", ColorType::Gray(1), 7802706); } + +#[test] +#[cfg(feature = "fax")] +fn test_fax4_white_is_min() { + test_image_sum_u8("imagemagick_group4.tiff", ColorType::Gray(1), 3742820); +} diff --git a/tests/images/imagemagick_group4.tiff b/tests/images/imagemagick_group4.tiff new file mode 100644 index 0000000..a474528 Binary files /dev/null and b/tests/images/imagemagick_group4.tiff differ