CCITT group 4 (Fax4) decoding support #229

stephenjudkins · 2024-04-05T18:57:38Z

This adds support for decoding CCIT group 4 tiff images by using the fax crate.

I've found that the photometric interpretation tag is ignored by all the extant decoders I've found, and attempting to correctly interpret it will break some subset of images. Unfortunate, but I suspect it's best to follow the herd here?

s3bk · 2024-04-05T19:51:36Z

Probably check Tag::PhotometricInterpretation to decide what values are black and white.

stephenjudkins · 2024-04-05T21:35:40Z

OK. I've done some research and found that

other image processors (imagemagick, Apple Preview) ignore the photometric interpretation tag and always assume that it's WhiteIsZero
fax4-encoded images I've found in the wild (which are all, unfortunately, confidential images of business checks) have the tag set correctly WhiteIsZero
imagemagick creates fax4-encoded tiffs with photometric interpretation incorrectly as BlackIsZero but that doesn't matter because everything that loads the files assumes WhiteIsZero.

To conform with how everything in the wild I'd argue we just keep this hardcoded the way it is?

s3bk · 2024-04-05T21:38:02Z

hahaha. Yea, I think just keep it as is.

fintelia

Left some comments. I think might also be a missing check that fax compression is only used for bilevel images?

fintelia · 2024-04-06T01:37:56Z

src/decoder/image.rs

+                // all extant tiff/fax4 decoders I've found always assume that the photometric interpretation
+                // is `WhiteIsZero`, ignoring the tag. ImageMagick appears to generate fax4-encoded tiffs
+                // with the tag incorrectly set to `BlackIsZero`.
+                fax::decoder::decode_g4(buffer.into_iter(), width, Some(height), |transitions| {


Could you pass reader.take(compressed_length).bytes() here?

It'd be reader.take(compressed_length).bytes().map(|b| b.unwrap()) instead. Is that OK with you, in terms of

performance impact: you'd get a branch for every single byte

error handling: you'd get a panic instead of an Err, and I (a Rust neophyte) don't think there's a way around that

The bit reader in the fax decoder pulls single bytes anway. I started changing the api to accept an Iterator of Result<u8, E>. (so reader.take(compressed_bytes).bytes() will work)

You could also see if taking a BufRead impl would improve performance over having to call into the reader for each individual byte. We're currently in the process of converting various parts of the image-rs APIs to use those over normal Read impls.

FWIW, I do not think it is worth worrying too much about micro-optimizations here. It makes sense to handle malicious inputs without blowing up the heap, but in practice all of the images I'm seeing are crappy low-res scans of checks from god-knows-what device a regional bank purchased in 1993. Really doubt there are many images of this format in the wild that demand fully optimized best-case performance

fintelia · 2024-04-06T01:41:13Z

src/decoder/image.rs

+                let width = u16::try_from(self.width)?;
+                let height = u16::try_from(self.height)?;


Should these be the chunk dimensions rather than the image dimensions?

fintelia · 2024-04-07T00:11:10Z

src/decoder/image.rs

+            CompressionMethod::Fax4 => {
+                let width = u16::try_from(self.width)?;
+                let height = u16::try_from(self.height)?;
+                let mut out: Vec<u8> = Vec::with_capacity(usize::from(width) * usize::from(height));


I have two concerns here:

This is likely to cause fuzzer OOMs because just setting a few header fields is enough to cause a ~4GB allocation.

The size calculation assumes that each pixel takes one byte, but I'd expect Gray(1) to mean one bit per pixel

Hmm. Going to push back a bit on (2), but you're the library maintainer so LMK if I'm looking at this wrong. I expect that this crate reports back what the tiff file says in its header, which is Gray(1). Then in the outer crate (image) we'd do this:

diff --git a/src/codecs/tiff.rs b/src/codecs/tiff.rs index 9f4dd735..256215c5 100644 --- a/src/codecs/tiff.rs +++ b/src/codecs/tiff.rs @@ -53,6 +53,7 @@ where }; let color_type = match tiff_color_type { + tiff::ColorType::Gray(1) => ColorType::L8,

Does that make sense? The alternatives would be to alter the following to special case this and override what's coming from the header: https://github.com/image-rs/image-tiff/blob/master/src/decoder/image.rs#L351

Alternatively we could add an extra data type to represent the actual output.

I'm OK with any of these options, just want to lay them out. What would you like as both the author/maintainer of this library and the primary consumer of its API in the image crate?

This crate should already decode Gray(1) encoded images into packed 1-bit per sample outputs.

To support the image crate use case, I'd expect that this create should either exposes an optional flag to expand sub 8-bit channels (like PNG does), or else not perform any expansion and have code in the image crate to do the conversion from packed 1-bit per sample into L8 encoded.

I'm working at getting packed 1-bit samples working here and, TBH, it's quite a bit of fiddly work. I've got the decoder portion outputting 1-bit samples but the rest of the crate seems to assume it's byte-addressable. I am looking at the chunking (de-chunking?) code in fn expand_chunk and there's a lot that assumes samples are individually addressable. Should I go and start altering this to change this assumption? Is this something you could provide more guidance on?

To step back a bit: especially if the image crate, presumably the main consumer of this, just goes and re-expands the samples back to individual bytes, how much does it offer to have this crate offer 1-bit samples? There's a pretty straightforward path to getting this working in the image crate with this decoder without worrying about any of this.

OK, I've looked a bit further and expand_chunk is the difficult part here.

There are three different code paths:

chunk is as wide as image but with no padding

there's right-padding and we're using a floating point predictor

other cases

All of these assume individually addressable pixels. What kind of changes to these abstractions do you propose we make to get this working?

I realized the reason I thought there was sub-byte sample support was because there's a stalled-out PR for it that I lost track of.

I'll hopefully have a bit more time this weekend, but briefly:

My recollection is that TIFF packs multiple samples per byte across pixels but each row is always an integer number of bytes (i.e. it is padded out to a multiple of 8 bits).

To handle the first case of the chunk width matching the image width, the byte layout should match what's in the file so you can just copy byte-by-byte.

The floating point predictor implies floating point samples, which are always 16-bit, 32-bit, or 64-bit. Thus you don't have to handle it.

The final case can only be hit for tiled images. For here, I think it would be reasonable to return an unsupported error if the tile_width * bit_depth isn't a multiple of 8. With that assumption I don't think the code should be too hard

A final note is to watch out for integer overflow when using u32 or usize. It only takes 512 MB to store 2^32 bits, so a 32-bit system could plausibly want to decode a TIFF file containing more pixels than fit in a usize. And even if the decoded image won't fit in RAM, we still want to be able to return a graceful error rather than panicking due to integer overflow

OK. I'll be away all next week but I'd like to pick this up again after that. Thanks for the context here.

fintelia · 2024-04-07T00:11:20Z

src/decoder/image.rs

@@ -368,6 +369,7 @@ impl Image {
    }

    fn create_reader<'r, R: 'r + Read>(
+        &self,


Please pass in the relevant width/height as arguments rather than taking self

fintelia · 2024-04-07T00:15:00Z

Ideally, I'd like to see the create_reader act more lazily. The best way would be to have incremental decoding, but the fax crate doesn't seem to support that. An intermediate step might be to generate the list of color transitions, and then lazily expand that into the provided output buffer

s3bk · 2024-04-07T16:18:40Z

I don't see a reason why incremental decoding could not be implemented.

It would be pretty easy to write something like

impl Decoder<R: Read> {
  fn new(reader: R) -> Self;
  fn advance(&mut self) -> Result<(), io::Error>;
  fn pels(&self) -> Pels;
}

fintelia · 2024-04-07T17:26:49Z

Yeah, I think an interface like that would work. Then it could be wrapped in another object that implemented Read by tracking how far into the row had been read and expanded Pels pixel by pixel

stephenjudkins · 2024-04-08T20:02:55Z

Appreciate the feedback and all the back-and-forth here. I'm happy to help out as much as I can here to push this forward, but I don't want to step on @s3bk's feet if he's working on these changes.

s3bk · 2024-04-08T21:44:17Z

@stephenjudkins I pushed changes to the fax repo. There are strange differences with the last line again. I need to re-encode my samples with libtiff to check if the error is in the sample data or my code.

But you should be able to use the code to adapt this PR to the next version.

… a time

inzanez · 2024-10-30T11:34:24Z

What is the state of this? Anything one can do?

s3bk · 2024-10-30T11:36:22Z

I don't know, but from what a I remember, the fax tests are all passing now.

inzanez · 2024-10-30T12:12:52Z

Seems that something with the test_fax4 is still messed up, ...

stephenjudkins · 2024-10-30T17:19:30Z

Sorry! I dropped this but would like to pick it back up. Can we confirm that the upstream fax crate can decode the Fax4 files that are included?

fintelia · 2024-10-30T21:13:34Z

src/decoder/image.rs

+                let width = u16::try_from(dimensions.0)?;
+                let height = u16::try_from(dimensions.1)?;
+
+                struct Group4Reader<R2> {


I would like to see this moved into decoder/stream.rs alongside the other decompressors

fintelia · 2024-10-30T21:15:39Z

src/decoder/mod.rs

@@ -1066,7 +1066,10 @@ impl<R: Read + Seek> Decoder<R> {
        let max_sample_bits = self.image().bits_per_sample;
        match self.image().sample_format {
            SampleFormat::Uint => match max_sample_bits {
-                n if n <= 8 => DecodingResult::new_u8(buffer_size, &self.limits),
+                n if n < 8 => {
+                    DecodingResult::new_u8(buffer_size / 8 * usize::from(n), &self.limits)


I'm slightly suspicious that this will round incorrectly if the buffer_size isn't a multiple of 8

stephenjudkins · 2024-10-31T06:53:05Z

OK, I've made some changes, and I've gotta say...I'm putting in a lot of work to support 1-bit samples, both here and in the image crate, when we're just transforming these back into 8-bit pixels in the image crate anyways. I could add in a flag (like in the PNG crate) to optionally expand to 8-bit samples for image crate to use. Then we'd have the 1-bit sample path that I suspect no one would actually ever use? I'd have this done and working already, without any changes to the image crate at all, if I just kept the 8-bit samples to begin with.

If you feel strongly on keeping the 1-bit image option I'll add the flag but there are a lot of edge cases I'm pretty worried about testing. Are you sure that you want this? It's frustrating to write a bunch of code that I suspect will never be used by anyone and might introduce bugs into other code paths.

inzanez · 2024-10-31T07:38:39Z

@stephenjudkins I assume that refers to images like this, with 1 bit per sample?:
TIFF image data, little-endian, direntries=18, height=3565, bps=1, compression=bi-level group 4, PhotometricInterpretation=WhiteIsZero

If so, that is exactly what I am looking for...

s3bk · 2024-10-31T10:21:16Z

Files are decoded and encoded correctly, but bit-wise verification fails for some files that have a strange EOF.

stephenjudkins · 2024-10-31T21:37:59Z

OK. I'm still working on this, and, since this code does not currently support < 8 bit samples it's introducing some significant and pervasive changes across several code paths that I don't reasonably expect to be able to test. Would you like to help me by finding some other files that could test this? Do you know of any 1-bit tile-based (instead of strip-based) files I could check?

fintelia · 2024-11-01T03:26:47Z

@stephenjudkins I made a fix to the sub 8-bpp decoding in #252. It should now be working.

If you want to make test images, you can use gdal_translate:

gdal_translate -ot Byte -co TILED=YES -co BLOCKXSIZE=16 -co BLOCKYSIZE=16 -co NBITS=1 -co COMPRESS=CCITTFAX4 input.png output.tif

For instance, this is a fax4 compressed version of the test image from that other PR: tiled-gray-i1-fax4.tif.zip (inside a ZIP archive because GitHub doesn't allow uploading TIFF files)

# Conflicts: # src/decoder/image.rs # src/decoder/mod.rs

stephenjudkins · 2024-11-01T22:15:59Z

Alright, I've added support for bit-sized samples as well as an optional flag (a la the PNG crate) to expand these to 8bpp. I've confirmed in the upstream image crate that, using that flag, fax4-encoded images work as expected.

stephenjudkins · 2024-11-01T22:24:37Z

See draft PR https://github.com/image-rs/image/pull/2377/files

fintelia · 2024-11-03T22:25:12Z

This adds expand_samples_to_bytes but doesn't actually implement it correctly for non-fax4 compressed images. I'd rather land the fax compression part of this PR now, and then deal with sample expansion afterwards (either in this crate, or IMO preferably in image directly)

CCITT group 4 (Fax4) decoding support

efb23b6

stephenjudkins added 2 commits April 5, 2024 14:41

update fax dependency

ec5a2e6

Leave comment explaining the situation

e3900f1

stephenjudkins marked this pull request as ready for review April 5, 2024 21:45

fintelia reviewed Apr 7, 2024

View reviewed changes

stephenjudkins added 2 commits April 8, 2024 13:07

review changes: pass in width/height, unwrap reader bytes

f475d6d

format

fbe26e9

stephenjudkins added 3 commits April 9, 2024 15:25

Reader instance for group 4-decoded images, that buffer one line at…

de076fa

… a time

decode only explicit number of bytes

2cfe50f

bit-addressable output (doesn't work yet)

d6ab46b

fintelia reviewed Oct 30, 2024

View reviewed changes

Support both bit- and byte-addressed output based on config

6db153f

stephenjudkins added 4 commits November 1, 2024 14:27

Merge remote-tracking branch 'origin/master' into sdj/fax4-support

5b551cc

# Conflicts: # src/decoder/image.rs # src/decoder/mod.rs

post-merge cleanup

8e0bdaa

Simplify decoder

ce025bc

don't allocate for every row

5255371

stephenjudkins requested a review from fintelia November 1, 2024 22:14

		let width = u16::try_from(self.width)?;
		let height = u16::try_from(self.height)?;

CCITT group 4 (Fax4) decoding support #229

Are you sure you want to change the base?

CCITT group 4 (Fax4) decoding support #229

Conversation

stephenjudkins commented Apr 5, 2024 • edited Loading

s3bk commented Apr 5, 2024

stephenjudkins commented Apr 5, 2024

s3bk commented Apr 5, 2024

fintelia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fintelia Apr 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenjudkins Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fintelia commented Apr 7, 2024

s3bk commented Apr 7, 2024 • edited Loading

fintelia commented Apr 7, 2024 • edited Loading

stephenjudkins commented Apr 8, 2024

s3bk commented Apr 8, 2024

inzanez commented Oct 30, 2024

s3bk commented Oct 30, 2024

inzanez commented Oct 30, 2024

stephenjudkins commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenjudkins commented Oct 31, 2024

inzanez commented Oct 31, 2024

s3bk commented Oct 31, 2024

stephenjudkins commented Oct 31, 2024

fintelia commented Nov 1, 2024

stephenjudkins commented Nov 1, 2024

stephenjudkins commented Nov 1, 2024

fintelia commented Nov 3, 2024

stephenjudkins commented Apr 5, 2024 •

edited

Loading

fintelia Apr 7, 2024 •

edited

Loading

stephenjudkins Apr 10, 2024 •

edited

Loading

s3bk commented Apr 7, 2024 •

edited

Loading

fintelia commented Apr 7, 2024 •

edited

Loading