Add Tiff Format Support

[JimBobSquarePants]

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open
• I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
• I have provided test coverage for my change (where applicable)

Description

Note: This Replaces #1456 which cannot be reopened following a force push.

This PR adds support for TIFF decoding and encoding.

The specification can be found here: TIFF spec
Or as a PDF in the tiff folder (ImageSharp\Formats\Tiff).

Currently the Encoder and Decoder support the following:

Baseline TIFF

• Bilevel Images aka Black and White images.
• Grayscale Images
• Palette-color Images
• RGB Full Color Images
• PackBits Compression
• Modified Huffman Compression (Ccitt1D)

TIFF Extensions

• CcittGroup3Fax
• LZW compression
• Deflate compression
• Differencing Predictor

The following extension features will probably not be part of the first release of the TIFF format support.

• CcittGroup4Fax
• JPEG Compression Technote 2
• Tiled Images
• TransparencyMask
• Separated (TIFF Extension color spaces)
• YCbCr (TIFF Extension color spaces)
• CieLab (TIFF Extension color spaces)
• IccLab (TechNote 1)

TODO's

• Issue with LZW decoding with some images, for example: rgb_lzw_no_predictor.tiff Fixed with #12 Tiff specific fixes for LZW #1457
• Deflate compression uses deflate implementation of PNG, we should move that now to a common place
• There is still an issue with LZW. Compression a byte array with LZW and then decompress it again does not yield in the original byte array. See failing tests. fixed with da51a4f

[JimBobSquarePants]

@brianpopow @IldarKhayrutdinov There's something very funky about how we determine encoding properties.

I've done some work locally to make TiffFrameMetadata public and ensuring that we actually carry the properties through from decode to encode (currently we're creating instances and abandoning them, falling back to the profile tags) but I'm seeing that BitsPerPixel is being overwritten by a TiffEncodingMode calculation. We're not even using the BitsPerSample property!

I'm gonna have to push a broken build as I don't know the spec enough to fix it. There's 3 failing tests.

We should be using the BitsPerPixel from either the TiffEncoder or the TiffFrameMetadata types (in that order) to determine the TiffEncodingMode and TiffPhotometricInterpretation properties. If an incorrect combination of BBP and Mode are chosen during encoding then we should automatically fix that like we do in the Png encoder.

I don't like the name TiffEncodingMode. Wouldn't TiffColorType be more appropriate and inline with Png? Also the Default property shouldn't exist. If not set then our default should be should be assigned during encoding based upon the current pixel format (see PngEncoderOptionsHelpers for reference).

[brianpopow]

We're not even using the BitsPerSample property!

I have removed this now from the TiffFrameMetadata: dcb3e6f

I'm gonna have to push a broken build as I don't know the spec enough to fix it. There's 3 failing tests.

Fixed the failing tests with 508844a

I don't like the name TiffEncodingMode. Wouldn't TiffColorType be more appropriate and inline with Png? Also the Default property shouldn't exist. If not set then our default should be should be assigned during encoding based upon the current pixel format (see PngEncoderOptionsHelpers for reference).

TiffEncodingMode is basically an enum which represents which PhotometricInterpretation the encoder supports. I thought it would be easier to understand for the user then PhotometricInterpretation, but it seems that is not the case. Note that there are several values which we do not support yet (and maybe never will), like YCbCr, CieLab, IccLab, etc.
Should i change it to PhotometricInterpretation?

[JimBobSquarePants]

@brianpopow thanks for the assist. I thought it was best to stop before I broke anything further 😅

Yes, I think we should use the enum directly and comment any unsupported members (same for anything else we’re proxying). Is this value something we should be capturing during decode in metadata to use if not set?

[IldarKhayrutdinov]

@JimBobSquarePants @brianpopow sorry for delay

Really there is dilemma TiffEncodingMode vs PhotometricInterpretation,BitsPerSample. and palette (Quantizer). By analogy with Compression we can add in the documentation supported modes info and throw an exception for not supported. I think we can replace encodingMode with photometricInterpretation, bitsPerSample already have.
I also think that encodingMode be easier to understand for the user , but probably the above method will be more correct, although it will complicate.

Anyway, the encoder parses the parameters and adjust them safely. Missing/default parameters as before can be taken from an exist frame metadata. Those the dilemma concerns only user API.

TiffEncodingMode is useful and easy, maybe use something like?

public class TiffEncoder : IImageEncoder, ITiffEncoderOptions
{
    public TiffEncoder() { }

    public TiffEncoder(TiffEncodingColorMode colorMode)
    {
        // set BitsPerPixel, PhotometricInterpretation, Quantizer
    }

    public TiffBitsPerPixel? BitsPerPixel { get; set; }
    public TiffCompression Compression { get; set; } = TiffCompression.None;
    public DeflateCompressionLevel CompressionLevel { get; set; } = DeflateCompressionLevel.DefaultCompression;
    public TiffPhotometricInterpretation PhotometricInterpretation { get; set; }
    public TiffPredictor HorizontalPredictor { get; set; }
    public IQuantizer Quantizer { get; set; }
...
}

Also, in the future, when will be support for writing multi-frames, for each frame the options may be different, how will make options API?

[JimBobSquarePants]

Yep, we really do not want to abstract elements present in the specification, anyone changing properties would be familiar with Tiff.

I would change the options to the following. All properties should be nullable with the core encoder determining the correct value to set using first any metadata parsed from decode as default when not set, then a sensible default.

Note the lack of constructors, this is by design.

public class TiffEncoder : IImageEncoder, ITiffEncoderOptions
{
    public TiffBitsPerPixel? BitsPerPixel { get; set; }
    public TiffCompression? Compression { get; set; }
    public DeflateCompressionLevel? CompressionLevel { get; set; }
    public TiffPhotometricInterpretation? PhotometricInterpretation { get; set; }
    public TiffPredictor? HorizontalPredictor { get; set; }
    public IQuantizer Quantizer { get; set; }
}

The hierarchy should be as follows.
EncoderOptions > Metadata > Default.

How many of the above properties can we parse on decode?

Note, upon re-reading PngEncoder as a reference I can see we have violated that pattern and have mixed non-nullable properties with nullable. We should fix this at some point.

[brianpopow]

I have changed the EncodingMode now to be the TiffPhotometricInterpretation: d22692e

I also think that encodingMode be easier to understand for the user , but probably the above method will be more correct, although it will complicate.

Yeah it was indeed complicated. There are alot of possible combinations, I hope i have got now all valid combinations covered with tests. I have the feeling i have forgotten some.

I have changed the TiffEncoder Options as James suggested above.

How many of the above properties can we parse on decode?

We got BitsPerPixel, Compression , HorizontalPredictor and PhotometricInterpretation

[JimBobSquarePants]

We got BitsPerPixel, Compression , HorizontalPredictor and PhotometricInterpretation

OK.... So I know we've gone round in circles a lot here but do you both think we should populate the TiffFrameMetadata class with all those properties to provide a parallel API to the encoder options?

[brianpopow]

We got BitsPerPixel, Compression , HorizontalPredictor and PhotometricInterpretation

OK.... So I know we've gone round in circles a lot here but do you both think we should populate the TiffFrameMetadata class with all those properties to provide a parallel API to the encoder options?

While I think it makes sense accessing those values a bit more convenient for the user, it will introduce the problem for us, that we have the same information in two places TiffFrameMetadata and the ExifProfile of the frame. What should happen if the user changes the values in TiffFrameMetadata? Should the data in TiffFrameMetadata overrule the data in the ExifProfile or should we keep both in sync?

So im not totally convinced about the idea. @IldarKhayrutdinov what is your opinion on that?

[IldarKhayrutdinov]

The hierarchy should be as follows.
EncoderOptions > Metadata > Default.

@JimBobSquarePants totally agree, briefly and clear!

Note the lack of constructors, this is by design.

Yeah, the example with TiffEncoder constructor was unsuccessful. I meant that for the convenience of user, so that he would not manually fill in options (for an inexperienced user may seem complicated and he may fill them in incorrectly and get an exception). Somehow for most typical scenarios we could provide methods for auto filling options.

While I think it makes accessing those values a bit more convenient for the user, it will introduce the problem for us, that we have the same information in two places TiffFrameMetadata and the ExifProfile of the frame. What should happen if the user changes the values in TiffFrameMetadata? Should the data in TiffFrameMetadata overrule the data in the ExifProfile or should we keep both in sync?

So im not totally convinced about the idea. @IldarKhayrutdinov what is your opinion on that?

@brianpopow Yeah, it’s a problem that the same information is in several places.

I think there are 2 ways, needs to choose one of them and take it as agreement.

1. ExifProfile use for all tags, pure metadata and fileInfo (Compression, PhotometricInterpretation...) tags. TiffFrameMetadata don't use for tags. Hierarchy for this case:

EncoderOptions > ImageFrameMetadata.ExifProfile > Default

2. ExifProfile use for only pure metadata tags (in fact other formats works like this). When decoding we can all tags are filled in there, including fileInfo (or probably more correct not fill fileInfo tags), but when saving, we ignore them. Add fileInfo properties in TiffFrameMetadata. Then hierarchy:

EncoderOptions > TiffFrameMetadata > Default

[brianpopow]

I think there are 2 ways, needs to choose one of them and take it as agreement.

1. ExifProfile use for all tags, pure metadata and fileInfo (Compression, PhotometricInterpretation...) tags. TiffFrameMetadata don't use for tags. Hierarchy for this case:

EncoderOptions > ImageFrameMetadata.ExifProfile > Default

1. ExifProfile use for only pure metadata tags (in fact other formats works like this). When decoding we can all tags are filled in there, including fileInfo (or probably more correct not fill fileInfo tags), but when saving, we ignore them. Add fileInfo properties in TiffFrameMetadata. Then hierarchy:

EncoderOptions > TiffFrameMetadata > Default

So option 1 is basically what we have right now.

For option 2. we would add Compression , HorizontalPredictor and PhotometricInterpretation to the TiffFrameMetadata and do not care about syncing those values with the ExifProfile of the ImageFrameMetadata. If the user wants to change those values, he should first and foremost use the TiffEncoder Options. If he does not, the values from the TiffFrameMetadata (or sensible defaults will be taken).
That's sounds feasible to me. @JimBobSquarePants what do you think, should we go for option 2?

[JimBobSquarePants]

Yep so Options 2 seems sound.
We copy the values to the metadata then remove them from the profile via RemoveValue(ExifTag tag)? We can re-encode them to the exported profile on save.

[brianpopow]

Yep so Options 2 seems sound.
We copy the values to the metadata then remove them from the profile via RemoveValue(ExifTag tag)? We can re-encode them to the exported profile on save.

Ok option 2 it is then, see 11a4e20. I have also removed the values from the ExifProfile.

[JimBobSquarePants]

I can't actually hit approve on this (since I created this version of the PR) but I think it's ready to hit the main branch!

Congratulations @Andy-Wilkinson @brianpopow @IldarKhayrutdinov and anyone else I've missed. This is such a MASSIVE achievement!

[brianpopow]

Congratulations @Andy-Wilkinson @brianpopow @IldarKhayrutdinov and anyone else I've missed. This is such a MASSIVE achievement!

I took quite some while to finish this, but I am now very happy what we achieved!

Thanks a lot to @Andy-Wilkinson for starting the work on the tiff branch and also of course @IldarKhayrutdinov who contributed to this branch and also helped with his opinions on all things tiff related!

[IldarKhayrutdinov]

I'm happy to contribute! 🎉
At the end I got a little lost, but I hope to continue developing the format!