-
Notifications
You must be signed in to change notification settings - Fork 667
Added compression and f32 support for TIFF encoding/decoding. #2251
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.
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
base: main
Are you sure you want to change the base?
Conversation
CI seems to be failing because this exposes the tiff crate's |
I somehow missed this, just pushed changes that hide tiff crate's |
@fintelia can you please take a look at this? |
I've been taking a hiatus from most PR review and issue triage because I got burned out following the 0.25 release. Another reviewer might have a chance to review this at some point |
@fintelia I hope you feel better soon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bindings themselves look quite alright, I think the lower-level code could be cleaned up quite a lot with help of bytemuck
and the standard library. Quite boilerplatey.
56a25ca
to
d67ebd2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ideally should have been two PRs for f32 support and compression respectively
src/codecs/tiff.rs
Outdated
|
||
impl Default for CompressionType { | ||
fn default() -> Self { | ||
CompressionType::Deflate(Default::default()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, LZW is more common for TIFF files so that should probably be the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaulting LZW would seem to be more a historical accident. If we're going with libtiff as upstream we might want to also look into zstd support: http://libtiff.maptools.org/v4.0.10.html And I think its default option is no compression?
Please note that COMPRESSION_ZSTD is self-assigned the id 50000
by the libtiff project and is not officially registered with Adobe
since Adobe's registration function is defunct.
Yikes.
We should be making our own comparison in terms of size / speed. Based on the compression test this might favor deflate if Balanced
achieves comparable size to LZW. Which would not be all that surprising, LZW is the oldest of the bunch.
out_path.push(path.file_name().unwrap()); | ||
let encoder = TiffEncoder::new_with_compression( | ||
fs::File::create(&out_path).unwrap(), | ||
CompressionType::Deflate(TiffDeflateLevel::Balanced), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How long does this test take? I could imagine that compressing all the input files with balanced compression could take a while, though I don't know how many / how large they are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.35s
and 0.53s
respectively. Without the IO for compressed output it is 0.33
and 0.45
so I'll remove the file write. Seems fine although may we want to restrict it to a particular set of files with enough coverage at some point.
src/codecs/tiff.rs
Outdated
/// It is best to view the options as a _hint_ to the implementation on the smallest or fastest | ||
/// option for encoding a particular image. That is, using options that map directly to a TIFF | ||
/// image parameter will use this parameter where possible. But variants that have no direct | ||
/// mapping may be interpreted differently in minor versions. The exact output is expressly | ||
/// __not__ part of the SemVer stability guarantee. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a more natural note to put on CompressionType::Deflate
to explain that the compression levels don't have well defined meaning. Getting the requested compression algorithm seems like something that should be guaranteed.
Some(SampleFormat::Uint) if num_bits <= 16 => Ok(()), | ||
Some(SampleFormat::IEEEFP) if num_bits == 32 => Ok(()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the implicitness of "num_bits <= 16 means integer" and "num_bits == 32 means float". But really this should be addressed by having the tiff
crate expose a way to directly get the sample format, so I'm OK with doing it this way for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not implicit, the SampleFormat is itself parsed from a tiff tag. They just happen to not overlap, too. We do not have fp16 support in tiff
(due to predictor math) and we do not support loading u32/i32 channels into image
. But the match here would be unique even without the conditions. Nonetheless, the tiff
API should return all that information in one struct instead of strewn over ColorType
and manual tag fetching so this will change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implicit is probably the wrong word. The concern is that the tiff::ColorType::RGB(32) => ColorType::Rgb32F
line above only works because of the checks that are happening here. The result is you have to read the implementation of check_sample_format
to understand what is being checked, and only afterwards can you understand why new
can match on color type without checking the sample format.
If new
had access to both the sample format and sample type, the logic could be more straightforward:
match sample_format {
SampleFormat::Uint => match color_type {
RGB(8) => ColorType::Rgb8,
...
},
SampleFormat::IEEEFP => match color_type {
RGB(32) => ColorType::Rgb32F
...
}
}
src/codecs/tiff.rs
Outdated
match self.comp { | ||
Compression::Uncompressed => {} | ||
Compression::Lzw | Compression::Deflate(_) | Compression::Packbits => { | ||
encoder = encoder.with_compression(self.comp); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
match self.comp { | |
Compression::Uncompressed => {} | |
Compression::Lzw | Compression::Deflate(_) | Compression::Packbits => { | |
encoder = encoder.with_compression(self.comp); | |
} | |
} | |
if self.comp != Compression::Uncompressed { | |
encoder = encoder.with_compression(self.comp); | |
} |
Alright, I'll split it up. It also seems to me like f32 support was simpler than the compression part. |
I think Iv'e found an issue with either this PR or #2534. I started by forking the image-rs main branch and then I used the following commands to merge this PR into the branch
I then setup my application to use the merge-tiff-compress branch on my fork with the following:
With this setup I am able to successfully load all of the tiff images in the test suite added in PR #2534. But with the attached image I get the following error while attempting to decode:
Tiff info:
Image to reproduce issue: https://drive.google.com/file/d/1qcnFq-LCde0SFxxuKNgCSnr4sq2OnpuS/view?usp=sharing I am able to load this image in ImageJ2/Fiji and Apple Preview. |
@maxall41 This PR adds RGB and RGBA 32-bit floating point support, but notably doesn't add support for single-channel floating point images like the one you attached. Though it would probably be good to adjust the error message... |
@fintelia Ah that explains it. I might write a PR to add support... |
Just made a PR: #2536 |
I license past and future contributions under the dual MIT/Apache-2.0 license,
allowing licensees to choose either at their option.
f32 encoding/decoding support is added without additional byte alignment requirements. Removed 2-byte alignment requirement for u16 encoding/decoding. Added TIFF compression support.