Multipage + Tile support

[Jondeen]

Hi,

I'm looking into encoding tiled and multipaged TIFF images using this library, but found that the usage of IFD directories was defaulting to striped images.

I looked into the code and made a rough implementation for my own internal tools.

This is far from ready for a MR, but before I spend too much time on this:

• Is there work being done on this already?
• How should I approach the current code interfaces with regards to backwards compatibility? For now I've kept all methods and constructors intact and defaulting to striped images, and added more specific names for explicitly typed versions (e.g. fn new_image_with_type instead of fn new image which defaults to striped data).
• How would you ideally implement these types. Generics? Currently I allowed for a struct to be shared for both Tiles and Stripes since it doesn't add a lot of overhead on either, but of course this needs to be condensed into something more rustic. At least the majority of the code base for implementing tiled multipages is there.

My rust is still, well, a bit rusty, but I'd be happy to contribute if any of this is helpful.

[fintelia]

This isn't currently being worked on, but would be a good addition.

I'd say the first step would be to sketch out the API here. It is worth trying to add it in a backwards compatible way, but also look for whether a better design would be possible with an API break. We've done this on the decoding side where new methods were added referring to "chunks" that could either be strips or tiles depending on the image, and then the old methods deprecated and eventually removed.

[Jondeen]

Hi,

I apologise for the delay, I'm currently travelling.

I'll try to sketch an overview of the current API with a proposal for changes. I think there are some strip specific methods that can be #[deprecated] on the long term, for the better of 'chunk' oriented methods. Most higher level interfaces (e.g. TiffEncoder) may need a bit of more harsh rewrite, changing either generics layout or arguments on the long term. Currently there is a new_image and new_image_with_compression etc.. If we are to introduce tiled options in the same manner we get an awful lot very specific methods, although in a transitional period this could be ok.

I'll write up a proposal and get back to you!

[Jondeen]

Sorry it took a while to get back to you.

As I was looking for the path of least resistance for tearing down the default setup with using stripes, I came across a few related issues as well.

1. Implementing Chunks instead of Stripes

First off, I think the best way to implement tiles, and with the least amount of breakage, is to introduce tile as a generic (ChunkType) at the DirectoryEncoder level. This we we can retain all methods as they stand, and introduce StripedChunk as the default type in a ChunkType generic inherited by ImageEncoder:

pub struct ImageEncoder<
    'a,
    W: 'a + Write + Seek,
    C: ColorType,
    K: TiffKind,
    D: Compression = Uncompressed,
    CT: ChunkType = StripedChunk,
>

This can be implemented in a soft transition where we depreciate the stripe-named methods and properties in favor of chunks, most of which are low level.

Since stripes and chunks are so closely related, I vote for implementing them both in the same object (pretty much as is), and rather use the generic ChunkType as a phantom to indicate if additional logic should be applied to deal with the added dimension of TiledChunks. Alternatively, static helper functions can be embedded in the StripedChunk and TiledChunk objects could be pulled in, with shared code base in the main trait ChunkType. I'm down for both. The latter could probably blow up low level code bases to a greater extent.

2. ImageEncoder/DirectoryEncoder relationship

There are a lot of fields that ties ImageEncoder and DirectoryEncoder closely together, and that are shared between them. This comes from the virtue of IFDs (DirectoryEncoder) dictating a lot of homogeneiety in their encapsulated objects (Images/ImageEncoder) from the TIFF specification.

I would suggest to bring much of the generic settings from ImageEncoder (e.g. compression, colortype, chunktype), into DirectoryEncoder via bounds:

pub struct DirectoryEncoder<'a, W: 'a + Write + Seek, K: TiffKind, C: ColorType, D: Compression, CT: ChunkType> {
    writer: &'a mut TiffWriter<W>,
    dropped: bool,
    // We use BTreeMap to make sure tags are written in correct order
    ifd_pointer_pos: u64,
    ifd: BTreeMap<u16, DirectoryEntry<K::OffsetType>>,
    phantomcolor: PhantomData<C>,
    phantomcompression: PhantomData<D>,
    phantomchunk: PhantomData<CT>,
}

...

impl<W: Write + Seek, K: TiffKind> TiffEncoder<W, K> {
    pub fn new_image<C: ColorType>(
        &mut self,
        width: u32,
        height: u32,
    ) -> TiffResult<ImageEncoder<W, C, K, Uncompressed>> {
        let encoder = DirectoryEncoder::new(&mut self.writer)?;
        ImageEncoder::new(encoder, width, height)
    }
}

...

impl<'a, W: 'a + Write + Seek, T: ColorType, K: TiffKind, D: Compression, CT: ChunkType>
    ImageEncoder<'a, W, T, K, D, CT>
{
    fn new(encoder: DirectoryEncoder<'a, W, K, T, D, CT>, width: u32, height: u32) -> TiffResult<Self>
    where
        D: Default,
    {
        Self::with_compression(encoder, width, height, D::default())
    }
}

This will enforce tighter checking to make sure the resulting object is specification compliant. Aside from code that breaks the TIFF specification, this would not break any existing code itself.

Hierarchically, I feel IFDs are put aside in the code (e.g. helper functions in TiffEncoder rely heavily on front-end logic from ImageEncoder, and utilising DirectoryEncoder as a helper struct to implement this), while in terms of the specification, it could make more sense to let DirectoryEncoder be a more visible middle man. I understand that most use cases will not need to interfere much with DirectoryEncoder, unless you need more complex usage of the TIFF specification. I think it is worth keeping a lot of the existing shortcuts for creating simple images, but with growing number of options, a better way to specify the types and variations of the TIFF format could be needed:

3. Builders

Thus I propose to introduce builders, possibly combining DirectoryEncoder and ImageEncoder specification to obtain the correct types and variations through serial function chaining. Something like so:

let mut img_encoder: TiffEncoder<_, > = TiffEncoder::new(&mut file).unwrap();
let mut dir_enc: DirectoryEncoder<...> = tiff::encoder::DirectoryBuilder::new()
    .color::<colortype::Gray8>()
    .chunk::<tiff::encoder::TiledChunk>()
    .compression::<tiff::encoder::compression::Deflate>()
    .build(&mut img_encoder);

I can aid in most of these issues if any of them could be interesting to implement.

[jayvdb]

Hi @Jondeen , I have looked through master...Jondeen:image-tiff:tiled_multipage and it looks like there are no breaking changes required. It may not be beautiful, but it works. IMO you should run clippy to fix the obvious, and then start a PR to start the process of getting it in as-is.

The proposals you have to improve the API can then be done while the basic functionality is in and can be iterated on.

[fintelia]

Sorry, I somewhat lost track of this proposal. If you're still interested in working on this, please do go ahead and open a PR with what you have.

The main piece of feedback on the API design I have is that I've found it generally easier to work with enums rather than generic arguments for setting options. For instance, the png crate has an Encoder::set_compression method to pick which compression strategy to use. That ends up resulting in a very similar interface to your builder proposal, but the implementation can branch on the enum value when necessary, and otherwise all the code can easily be shared.