Tiff encoding

[birktj]

This is a very much WIP implementation of tiff encoding. I believe that it is correct and can handle most cases, however the api is currently not very ergonomic or safe. It also lacks built-inn compression, however that kind of requires nwin/lzw#1.

Currently the api looks something like this, but I would very much like to improve it.

let file = std::fs::File::create("test.tiff").unwrap();
let mut tiff = TiffEncoder::new_le(file);

tiff.write_header().unwrap();

tiff.new_ifd().unwrap();
tiff.new_ifd_entry(Tag::ImageWidth, &[100u32][..]);
tiff.new_ifd_entry(Tag::ImageLength, &[100u32][..]);
tiff.new_ifd_entry(Tag::BitsPerSample, &[8u16,8,8][..]);
tiff.new_ifd_entry(Tag::Compression, &[1u16][..]);
tiff.new_ifd_entry(Tag::PhotometricInterpretation, &[2u16][..]);
tiff.new_ifd_entry(Tag::SamplesPerPixel, &[3u16][..]);
tiff.new_ifd_entry(Tag::RowsPerStrip, &[100u32][..]);
tiff.new_ifd_entry(Tag::XResolution, &[Rational {n: 1, d: 1}][..]);
tiff.new_ifd_entry(Tag::YResolution, &[Rational {n: 1, d: 1}][..]);
tiff.new_ifd_entry(Tag::ResolutionUnit, &[1u16][..]);
// The two following tags are overwritten by `write_strip`
tiff.new_ifd_entry(Tag::StripOffsets, &[0u32][..]);
tiff.new_ifd_entry(Tag::StripByteCounts, &[0u32][..]);
tiff.finish_ifd().unwrap();

tiff.write_strip(&image_data).unwrap();

[birktj]

I have now made some improvements to the encoder api and the above could now be written as:

let file = std::fs::File::create("test.tiff").unwrap();
let mut tiff = TiffEncoder::new(file).unwrap();

let mut image = tiff.new_image::<RGB8>(100, 100).unwrap();
    
let mut idx = 0;
while image.next_strip_sample_count() > 0 {
        let sample_count = image.next_strip_sample_count() as usize;
        image.write_strip(&image_data[idx..idx+sample_count]).unwrap();
        idx += sample_count;
}
image.finish().unwrap();

Currently I have created two apis, one low level and one semi-high level.

I think that after a simple clean-up and some documentation this patch should be ready.

[Robzz]

Thank you for your work on this! I'll look a quick look at it now and one more when you think it's finished.

[Robzz]

Aside from the TODOs and lack of docs, looks good! More tests would be nice but that can always be added later.

Also, I noticed a few new compiler and clippy warnings, please try to fix these.

[birktj]

@Robzz I think I have fixed most of your concerns, however I have two concerns myself that I would like you to comment on before I am happy with the result.

The first is that I have used quite a bit of associated constants and the image crate has rust 1.24.1 as minimum supported version. I am not entirely sure if rust 1.24 supports them, should I simply use methods instead?

On a related note I have used a lot of traits to control decoding. I think the TiffValue trait is a good idea, but I am not entirely sure if a ColorType trait is such a good idea?

If you do not think that these are big issues that I would say that this PR is finished.

[Robzz]

Regarding associated constants, they were stabilized in rust 1.20 so compatibility with the image crate is ok. It might be a good idea to specify the version requirement in the README though.

I'll take a deeper look in the next few days with the TIFF spec my by side and play with it a bit.
I see you also added more tests, great!

[birktj]

A few things i have noted when using this:

• Directly using a writer such as File is really slow, using 'BufWriter' helps a lot, but it would probably be better to optimize the writing. Optimizing writing strips of u8 should be simple: remove impl TiffValue for u8 and add a specialized impl TiffValue for &[u8]. However this won't help with strips of u16 and other bitdepths. I was thinking that since the writer is writing using NativeEdian it should be safe to cast slices of different number types to slices of u8 and write those using write_all. @Robzz what do you think of this?
• Currently the decoder doesn't support compression. I don't think this is a big issue and it should be simple to add a patch for this later, however maybe we should modify new_image to take another argument with an enum for compressions so this could be added in a non-breaking way later.
• Another reason that the lack of support for compression is interesting is that without compression it is possible to write images in a streaming manner without the Seek bound by simply precalculating where the ifd will end up. For some usecases this may be useful and we should maybe think of some sort of api for writing non-compressed images without the 'Seek' bound.

[Robzz]

Hey, I still haven't looked at it in depth but I haven't forgotten, it's planned for this week end ;)

Adressing your points in order:

• Yeah, if writing values one by one then I'd expect writing without a BufWriter to be slow. Regarding encoding in NativeEndian, I don't see any reason why casting wouldn't work. Also the tiff spec says it's fine for an encoder to use whatever endianness it wants, so we're okay on this front.
• Sounds good, then we just need to return TiffUnsupportedError::UnsupportedCompressionMethod when a compression method is specified.
• I'll actually need to sit down and look at the code for that one.

[Robzz]

I'm mostly done reading the code, looks good to me. I'll still have to play with it a bit, I'll let you know if I run into any issues.

[birktj]

On more thing that I have noticed is that there maybe should be some more convenience methods on ImageEncoder like set_resolution. And we could probably use those for compression with a set_compression as well, in which case we don't need to add an extra compression argument to any of the functions in this PR.

[birktj]

@Robzz I have done some small fixes and I think all of your points are covered. Please let me know if there is anything more you would like me to do.

[Robzz]

I reviewed your latest commit, everything looks good to me. I'm for merging this, problems we encounter we can fix later, an we'll certainly get more feedback from people trying to use this than you'll get from me in this PR. However, I don't have merge powers 'round here. @bvssvni (or anyone else with magic merge powers) could you please give it a look and merge if OK ?

[HeroicKatora]

There's nothing major, but the part about encoding of strings is potentially be problematic. Would it be too much of a problem to cleanup some code style along the way?

[HeroicKatora]

And I can merge this as well, feel free to ping me on the weekend about it.

[birktj]

@HeroicKatora I have fixed the string issue, and changed the impls to be on [T] instead of &[T]. Anything more you would like me too do?

[HeroicKatora]

Thank you for your contribution!