Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CCITT group 4 (Fax4) decoding support #229

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
+80 −2
Open

CCITT group 4 (Fax4) decoding support #229

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
efb23b6
CCITT group 4 (Fax4) decoding support
stephenjudkins Apr 5, 2024
ec5a2e6
update fax dependency
stephenjudkins Apr 5, 2024
e3900f1
Leave comment explaining the situation
stephenjudkins Apr 5, 2024
f475d6d
review changes: pass in width/height, unwrap reader bytes
stephenjudkins Apr 8, 2024
fbe26e9
format
stephenjudkins Apr 8, 2024
de076fa
`Reader` instance for group 4-decoded images, that buffer one line at…
stephenjudkins Apr 9, 2024
2cfe50f
decode only explicit number of bytes
stephenjudkins Apr 9, 2024
d6ab46b
bit-addressable output (doesn't work yet)
stephenjudkins Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ exclude = ["tests/images/*", "tests/fuzz_images/*"]
weezl = "0.1.0"
jpeg = { package = "jpeg-decoder", version = "0.3.0", default-features = false }
flate2 = "1.0.20"
fax = "0.2.2"

[dev-dependencies]
criterion = "0.3.1"
Expand Down
23 changes: 22 additions & 1 deletion src/decoder/image.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use crate::tags::{
CompressionMethod, PhotometricInterpretation, PlanarConfiguration, Predictor, SampleFormat, Tag,
};
use crate::{ColorType, TiffError, TiffFormatError, TiffResult, TiffUnsupportedError, UsageError};
use fax;
use std::io::{self, Cursor, Read, Seek};
use std::sync::Arc;

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

reader: R,
photometric_interpretation: PhotometricInterpretation,
compression_method: CompressionMethod,
Expand Down Expand Up @@ -447,6 +449,25 @@ impl Image {

Box::new(Cursor::new(data))
}
CompressionMethod::Fax4 => {
let width = u16::try_from(self.width)?;
let height = u16::try_from(self.height)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let mut out: Vec<u8> = Vec::with_capacity(usize::from(width) * usize::from(height));
Copy link
Contributor

@fintelia fintelia Apr 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two concerns here:

  1. This is likely to cause fuzzer OOMs because just setting a few header fields is enough to cause a ~4GB allocation.
  2. The size calculation assumes that each pixel takes one byte, but I'd expect Gray(1) to mean one bit per pixel

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@stephenjudkins stephenjudkins Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


let mut buffer = Vec::with_capacity(usize::try_from(compressed_length)?);
reader.take(compressed_length).read_to_end(&mut buffer)?;

// 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| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

out.extend(fax::decoder::pels(transitions, width).map(|c| match c {
fax::Color::Black => 255,
fax::Color::White => 0,
}))
});
Box::new(Cursor::new(out))
}
method => {
return Err(TiffError::UnsupportedError(
TiffUnsupportedError::UnsupportedCompressionMethod(method),
Expand Down Expand Up @@ -633,7 +654,7 @@ impl Image {

let padding_right = chunk_dims.0 - data_dims.0;

let mut reader = Self::create_reader(
let mut reader = self.create_reader(
reader,
photometric_interpretation,
compression_method,
Expand Down
5 changes: 5 additions & 0 deletions tests/decode_images.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,3 +510,8 @@ fn test_predictor_3_rgb_f32() {
fn test_predictor_3_gray_f32() {
test_image_sum_f32("predictor-3-gray-f32.tif", ColorType::Gray(32), 20008.275);
}

#[test]
fn test_fax4() {
test_image_sum_u8("fax4.tiff", ColorType::Gray(1), 62384985);
}
Binary file added tests/images/fax4.tiff
View file
Open in desktop
Binary file not shown.
Loading