Rework sanitize and set encoder options: BitsPerPixel should be the primary source of truth

src/ImageSharp/Formats/Tiff/TiffEncoderCore.cs

@@ -144,14 +144,8 @@ namespace SixLabors.ImageSharp.Formats.Tiff
                 ?? rootFrameTiffMetaData.Compression
                 ?? DefaultCompression;
 
-            // Make sure, the bits per pixel and PhotometricInterpretation have values which makes sense in combination with the other chosen values.
+            // Make sure, the Encoder options makes sense in combination with each other.
-            bitsPerPixel = this.SanitizeBitsPerPixel(bitsPerPixel, image.PixelType.BitsPerPixel, photometricInterpretation, compression);
+            this.SanitizeAndSetEncoderOptions(bitsPerPixel, image.PixelType.BitsPerPixel, photometricInterpretation, compression, predictor);
-            photometricInterpretation = this.SanitizePhotometricInterpretation(photometricInterpretation, bitsPerPixel, compression);
-
-            this.BitsPerPixel = bitsPerPixel;
-            this.PhotometricInterpretation = photometricInterpretation;
-            this.CompressionType = compression;
-            this.HorizontalPredictor = predictor;
 
             using (var writer = new TiffStreamWriter(stream))
             {
@@ -302,62 +296,49 @@ namespace SixLabors.ImageSharp.Formats.Tiff
             return nextIfdMarker;
         }
 
-        private TiffPhotometricInterpretation SanitizePhotometricInterpretation(TiffPhotometricInterpretation? photometricInterpretation, TiffBitsPerPixel? bitsPerPixel, TiffCompression compression)
+        private void SanitizeAndSetEncoderOptions(TiffBitsPerPixel? bitsPerPixel, int inputBitsPerPixel, TiffPhotometricInterpretation? photometricInterpretation, TiffCompression compression, TiffPredictor predictor)
-        {
-            // Make sure, that the fax compressions are only used together with the WhiteIsZero.
-            if (compression == TiffCompression.CcittGroup3Fax || compression == TiffCompression.Ccitt1D)
-            {
-                // The “normal” PhotometricInterpretation for bilevel CCITT compressed data is WhiteIsZero.
-                return TiffPhotometricInterpretation.WhiteIsZero;
-            }
-
-            // Use the bits per pixel to determine the photometric interpretation.
-            switch (bitsPerPixel)
-            {
-                case TiffBitsPerPixel.Bit1:
-                    return TiffPhotometricInterpretation.BlackIsZero;
-                case TiffBitsPerPixel.Bit4:
-                    return TiffPhotometricInterpretation.PaletteColor;
-                case TiffBitsPerPixel.Bit8:
-                    return photometricInterpretation == TiffPhotometricInterpretation.PaletteColor
-                        ? TiffPhotometricInterpretation.PaletteColor
-                        : TiffPhotometricInterpretation.BlackIsZero;
-            }
-
-            if (photometricInterpretation.HasValue)
-            {
-                return photometricInterpretation.Value;
-            }
-
-            return DefaultPhotometricInterpretation;
-        }
-
-        private TiffBitsPerPixel SanitizeBitsPerPixel(TiffBitsPerPixel? bitsPerPixel, int inputBitsPerPixel, TiffPhotometricInterpretation? photometricInterpretation, TiffCompression compression)
         {
-            // Make sure Palette color is only used with 4 and 8 bit per pixel.
+            // BitsPerPixel should be the primary source of truth for the encoder options.
-            if (photometricInterpretation == TiffPhotometricInterpretation.PaletteColor)
+            if (bitsPerPixel.HasValue)
             {
-                if (bitsPerPixel != TiffBitsPerPixel.Bit8 && bitsPerPixel != TiffBitsPerPixel.Bit4)
+                switch (bitsPerPixel)
                 {
-                    return TiffBitsPerPixel.Bit8;
+                    case TiffBitsPerPixel.Bit1:
+                        if (compression == TiffCompression.Ccitt1D || compression == TiffCompression.CcittGroup3Fax || compression == TiffCompression.CcittGroup4Fax)
+                        {
+                            // The “normal” PhotometricInterpretation for bilevel CCITT compressed data is WhiteIsZero.
+                            this.SetEncoderOptions(bitsPerPixel, TiffPhotometricInterpretation.WhiteIsZero, compression, TiffPredictor.None);
+                            break;
+                        }
+
+                        this.SetEncoderOptions(bitsPerPixel, TiffPhotometricInterpretation.BlackIsZero, compression, TiffPredictor.None);
+                        break;
+                    case TiffBitsPerPixel.Bit4:
+                        this.SetEncoderOptions(bitsPerPixel, TiffPhotometricInterpretation.PaletteColor, compression, TiffPredictor.None);
+                        break;
+                    case TiffBitsPerPixel.Bit8:
+                        this.SetEncoderOptions(bitsPerPixel, photometricInterpretation ?? TiffPhotometricInterpretation.BlackIsZero, compression, predictor);
+                        break;
+                    default:
+                        this.SetEncoderOptions(bitsPerPixel, TiffPhotometricInterpretation.Rgb, compression, predictor);
+                        break;
                 }
-            }
 
-            if (compression == TiffCompression.Ccitt1D || compression == TiffCompression.CcittGroup3Fax)
+                return;
-            {
-                return TiffBitsPerPixel.Bit1;
-            }
-
-            if (bitsPerPixel.HasValue)
-            {
-                return bitsPerPixel.Value;
             }
 
             // If no photometric interpretation was chosen, the input image bit per pixel should be preserved.
-            if (photometricInterpretation == null)
+            if (!photometricInterpretation.HasValue)
             {
                 // At the moment only 8 and 32 bits per pixel can be preserved by the tiff encoder.
-                return inputBitsPerPixel == 8 ? TiffBitsPerPixel.Bit8 : DefaultBitsPerPixel;
+                if (inputBitsPerPixel == 8)
+                {
+                    this.SetEncoderOptions(TiffBitsPerPixel.Bit8, TiffPhotometricInterpretation.BlackIsZero, compression, predictor);
+                    return;
+                }
+
+                this.SetEncoderOptions(TiffBitsPerPixel.Bit24, TiffPhotometricInterpretation.Rgb, compression, predictor);
+                return;
             }
 
             switch (photometricInterpretation)
@@ -368,28 +349,33 @@ namespace SixLabors.ImageSharp.Formats.Tiff
                         this.CompressionType == TiffCompression.CcittGroup3Fax ||
                         this.CompressionType == TiffCompression.CcittGroup4Fax)
                     {
-                        return TiffBitsPerPixel.Bit1;
+                        this.SetEncoderOptions(TiffBitsPerPixel.Bit1, photometricInterpretation, compression, TiffPredictor.None);
+                        return;
                     }
                     else
                     {
-                        return TiffBitsPerPixel.Bit8;
+                        this.SetEncoderOptions(TiffBitsPerPixel.Bit8, photometricInterpretation, compression, predictor);
+                        return;
                     }
 
                 case TiffPhotometricInterpretation.PaletteColor:
-                    if (bitsPerPixel != TiffBitsPerPixel.Bit8 && bitsPerPixel != TiffBitsPerPixel.Bit4)
+                    this.SetEncoderOptions(TiffBitsPerPixel.Bit8, photometricInterpretation, compression, predictor);
-                    {
+                    return;
-                        return TiffBitsPerPixel.Bit8;
-                    }
-                    else
-                    {
-                        return bitsPerPixel.Value;
-                    }
 
                 case TiffPhotometricInterpretation.Rgb:
-                    return TiffBitsPerPixel.Bit24;
+                    this.SetEncoderOptions(TiffBitsPerPixel.Bit24, photometricInterpretation, compression, predictor);
+                    return;
             }
 
-            return DefaultBitsPerPixel;
+            this.SetEncoderOptions(DefaultBitsPerPixel, DefaultPhotometricInterpretation, compression, predictor);
+        }
+
+        private void SetEncoderOptions(TiffBitsPerPixel? bitsPerPixel, TiffPhotometricInterpretation? photometricInterpretation, TiffCompression compression, TiffPredictor predictor)
+        {
+            this.BitsPerPixel = bitsPerPixel;
+            this.PhotometricInterpretation = photometricInterpretation;
+            this.CompressionType = compression;
+            this.HorizontalPredictor = predictor;
         }
     }
 }

tests/ImageSharp.Tests/Formats/Tiff/TiffEncoderTests.cs

@@ -35,6 +35,14 @@ namespace SixLabors.ImageSharp.Tests.Formats.Tiff
         [InlineData(TiffPhotometricInterpretation.Rgb, TiffBitsPerPixel.Bit24)]
         [InlineData(TiffPhotometricInterpretation.PaletteColor, TiffBitsPerPixel.Bit8)]
         [InlineData(TiffPhotometricInterpretation.BlackIsZero, TiffBitsPerPixel.Bit8)]
+        [InlineData(TiffPhotometricInterpretation.WhiteIsZero, TiffBitsPerPixel.Bit8)]
+        //// Unsupported TiffPhotometricInterpretation should default to 24 bits
+        [InlineData(TiffPhotometricInterpretation.CieLab, TiffBitsPerPixel.Bit24)]
+        [InlineData(TiffPhotometricInterpretation.ColorFilterArray, TiffBitsPerPixel.Bit24)]
+        [InlineData(TiffPhotometricInterpretation.ItuLab, TiffBitsPerPixel.Bit24)]
+        [InlineData(TiffPhotometricInterpretation.LinearRaw, TiffBitsPerPixel.Bit24)]
+        [InlineData(TiffPhotometricInterpretation.Separated, TiffBitsPerPixel.Bit24)]
+        [InlineData(TiffPhotometricInterpretation.TransparencyMask, TiffBitsPerPixel.Bit24)]
         public void EncoderOptions_SetPhotometricInterpretation_Works(TiffPhotometricInterpretation? photometricInterpretation, TiffBitsPerPixel expectedBitsPerPixel)
         {
             // arrange
@@ -155,7 +163,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Tiff
             // assert
             memStream.Position = 0;
             using var output = Image.Load<Rgba32>(Configuration, memStream);
-            var frameMetaData = output.Frames.RootFrame.Metadata.GetTiffMetadata();
+            TiffFrameMetadata frameMetaData = output.Frames.RootFrame.Metadata.GetTiffMetadata();
             Assert.Equal(expectedBitsPerPixel, frameMetaData.BitsPerPixel);
         }
 
@@ -213,7 +221,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Tiff
             where TPixel : unmanaged, IPixel<TPixel>
         {
             // arrange
-            var encoder = new TiffEncoder() { Compression = compression };
+            var encoder = new TiffEncoder() { Compression = compression, BitsPerPixel = TiffBitsPerPixel.Bit1 };
             using Image<TPixel> input = provider.GetImage();
             using var memStream = new MemoryStream();
 
@@ -333,27 +341,52 @@ namespace SixLabors.ImageSharp.Tests.Formats.Tiff
 
         [Theory]
         [WithFile(Calliphora_BiColorUncompressed, PixelTypes.Rgba32)]
-        public void TiffEncoder_EncodeBiColor_Works<TPixel>(TestImageProvider<TPixel> provider)
+        public void TiffEncoder_EncodeBiColor_BlackIsZero_Works<TPixel>(TestImageProvider<TPixel> provider)
             where TPixel : unmanaged, IPixel<TPixel> => TestTiffEncoderCore(provider, TiffBitsPerPixel.Bit1, TiffPhotometricInterpretation.BlackIsZero);
 
         [Theory]
         [WithFile(Calliphora_BiColorUncompressed, PixelTypes.Rgba32)]
-        public void TiffEncoder_EncodeBiColor_WithDeflateCompression_Works<TPixel>(TestImageProvider<TPixel> provider)
+        public void TiffEncoder_EncodeBiColor_WhiteIsZero_Works<TPixel>(TestImageProvider<TPixel> provider)
+            where TPixel : unmanaged, IPixel<TPixel> => TestTiffEncoderCore(provider, TiffBitsPerPixel.Bit1, TiffPhotometricInterpretation.WhiteIsZero);
+
+        [Theory]
+        [WithFile(Calliphora_BiColorUncompressed, PixelTypes.Rgba32)]
+        public void TiffEncoder_EncodeBiColor_WithDeflateCompression_BlackIsZero_Works<TPixel>(TestImageProvider<TPixel> provider)
             where TPixel : unmanaged, IPixel<TPixel> => TestTiffEncoderCore(provider, TiffBitsPerPixel.Bit1, TiffPhotometricInterpretation.BlackIsZero, TiffCompression.Deflate);
 
         [Theory]
         [WithFile(Calliphora_BiColorUncompressed, PixelTypes.Rgba32)]
-        public void TiffEncoder_EncodeBiColor_WithPackBitsCompression_Works<TPixel>(TestImageProvider<TPixel> provider)
+        public void TiffEncoder_EncodeBiColor_WithDeflateCompression_WhiteIsZero_Works<TPixel>(TestImageProvider<TPixel> provider)
+            where TPixel : unmanaged, IPixel<TPixel> => TestTiffEncoderCore(provider, TiffBitsPerPixel.Bit1, TiffPhotometricInterpretation.WhiteIsZero, TiffCompression.Deflate);
+
+        [Theory]
+        [WithFile(Calliphora_BiColorUncompressed, PixelTypes.Rgba32)]
+        public void TiffEncoder_EncodeBiColor_WithPackBitsCompression_BlackIsZero_Works<TPixel>(TestImageProvider<TPixel> provider)
             where TPixel : unmanaged, IPixel<TPixel> => TestTiffEncoderCore(provider, TiffBitsPerPixel.Bit1, TiffPhotometricInterpretation.BlackIsZero, TiffCompression.PackBits);
 
         [Theory]
         [WithFile(Calliphora_BiColorUncompressed, PixelTypes.Rgba32)]
-        public void TiffEncoder_EncodeBiColor_WithCcittGroup3FaxCompression_Works<TPixel>(TestImageProvider<TPixel> provider)
+        public void TiffEncoder_EncodeBiColor_WithPackBitsCompression_WhiteIsZero_Works<TPixel>(TestImageProvider<TPixel> provider)
+            where TPixel : unmanaged, IPixel<TPixel> => TestTiffEncoderCore(provider, TiffBitsPerPixel.Bit1, TiffPhotometricInterpretation.WhiteIsZero, TiffCompression.PackBits);
+
+        [Theory]
+        [WithFile(Calliphora_BiColorUncompressed, PixelTypes.Rgba32)]
+        public void TiffEncoder_EncodeBiColor_WithCcittGroup3FaxCompression_WhiteIsZero_Works<TPixel>(TestImageProvider<TPixel> provider)
+            where TPixel : unmanaged, IPixel<TPixel> => TestTiffEncoderCore(provider, TiffBitsPerPixel.Bit1, TiffPhotometricInterpretation.WhiteIsZero, TiffCompression.CcittGroup3Fax);
+
+        [Theory]
+        [WithFile(Calliphora_BiColorUncompressed, PixelTypes.Rgba32)]
+        public void TiffEncoder_EncodeBiColor_WithCcittGroup3FaxCompression_BlackIsZero_Works<TPixel>(TestImageProvider<TPixel> provider)
             where TPixel : unmanaged, IPixel<TPixel> => TestTiffEncoderCore(provider, TiffBitsPerPixel.Bit1, TiffPhotometricInterpretation.BlackIsZero, TiffCompression.CcittGroup3Fax);
 
         [Theory]
         [WithFile(Calliphora_BiColorUncompressed, PixelTypes.Rgba32)]
-        public void TiffEncoder_EncodeBiColor_WithModifiedHuffmanCompression_Works<TPixel>(TestImageProvider<TPixel> provider)
+        public void TiffEncoder_EncodeBiColor_WithModifiedHuffmanCompression_WhiteIsZero_Works<TPixel>(TestImageProvider<TPixel> provider)
+            where TPixel : unmanaged, IPixel<TPixel> => TestTiffEncoderCore(provider, TiffBitsPerPixel.Bit1, TiffPhotometricInterpretation.WhiteIsZero, TiffCompression.Ccitt1D);
+
+        [Theory]
+        [WithFile(Calliphora_BiColorUncompressed, PixelTypes.Rgba32)]
+        public void TiffEncoder_EncodeBiColor_WithModifiedHuffmanCompression_BlackIsZero_Works<TPixel>(TestImageProvider<TPixel> provider)
             where TPixel : unmanaged, IPixel<TPixel> => TestTiffEncoderCore(provider, TiffBitsPerPixel.Bit1, TiffPhotometricInterpretation.BlackIsZero, TiffCompression.Ccitt1D);
 
         [Theory]