Merge pull request #2177 from br3aker/dp/jpeg-colorspace-deduction

Jpeg color space deduction fix

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

@@ -86,14 +86,14 @@ namespace SixLabors.ImageSharp.Formats.Jpeg
         private byte[] xmpData;
 
         /// <summary>
-        /// Contains information about the JFIF marker.
+        /// Whether the image has a APP14 adobe marker. This is needed to determine image encoded colorspace.
         /// </summary>
-        private JFifMarker jFif;
+        private bool hasAdobeMarker;
 
         /// <summary>
-        /// Whether the image has a JFIF marker. This is needed to determine, if the colorspace is YCbCr.
+        /// Contains information about the JFIF marker.
         /// </summary>
-        private bool hasJFif;
+        private JFifMarker jFif;
 
         /// <summary>
         /// Contains information about the Adobe marker.
@@ -517,11 +517,12 @@ namespace SixLabors.ImageSharp.Formats.Jpeg
         }
 
         /// <summary>
-        /// Returns the correct colorspace based on the image component count and the jpeg frame component id's.
+        /// Returns encoded colorspace based on the adobe APP14 marker.
         /// </summary>
-        /// <param name="componentCount">The number of components.</param>
+        /// <param name="componentCount">Number of components.</param>
+        /// <param name="adobeMarker">Parsed adobe APP14 marker.</param>
         /// <returns>The <see cref="JpegColorSpace"/></returns>
-        private JpegColorSpace DeduceJpegColorSpace(byte componentCount)
+        internal static JpegColorSpace DeduceJpegColorSpace(byte componentCount, ref AdobeMarker adobeMarker)
         {
             if (componentCount == 1)
             {
@@ -530,80 +531,47 @@ namespace SixLabors.ImageSharp.Formats.Jpeg
 
             if (componentCount == 3)
             {
-                // We prioritize adobe marker over jfif marker, if somebody really encoded this image with redundant adobe marker,
+                if (adobeMarker.ColorTransform == JpegConstants.Adobe.ColorTransformUnknown)
-                // then it's most likely an adobe jfif image.
-                if (!this.adobe.Equals(default))
                 {
-                    if (this.adobe.ColorTransform == JpegConstants.Adobe.ColorTransformYCbCr)
+                    return JpegColorSpace.RGB;
-                    {
-                        return JpegColorSpace.YCbCr;
-                    }
-
-                    if (this.adobe.ColorTransform == JpegConstants.Adobe.ColorTransformUnknown)
-                    {
-                        return JpegColorSpace.RGB;
-                    }
-
-                    // Fallback to the id color deduction: If these values are 1-3 for a 3-channel image, then the image is assumed to be YCbCr.
-                    if (this.Components[2].Id == 3 && this.Components[1].Id == 2 && this.Components[0].Id == 1)
-                    {
-                        return JpegColorSpace.YCbCr;
-                    }
-
-                    JpegThrowHelper.ThrowNotSupportedColorSpace();
                 }
 
-                if (this.hasJFif)
+                return JpegColorSpace.YCbCr;
-                {
+            }
-                    // JFIF implies YCbCr.
-                    return JpegColorSpace.YCbCr;
-                }
 
-                // Fallback to the id color deduction.
+            if (componentCount == 4)
-                // If the component Id's are R, G, B in ASCII the colorspace is RGB and not YCbCr.
+            {
-                // See: https://docs.oracle.com/javase/7/docs/api/javax/imageio/metadata/doc-files/jpeg_metadata.html#color
+                if (adobeMarker.ColorTransform == JpegConstants.Adobe.ColorTransformYcck)
-                if (this.Components[2].Id == 66 && this.Components[1].Id == 71 && this.Components[0].Id == 82)
                 {
-                    return JpegColorSpace.RGB;
+                    return JpegColorSpace.Ycck;
                 }
 
-                // If these values are 1-3 for a 3-channel image, then the image is assumed to be YCbCr.
+                return JpegColorSpace.Cmyk;
-                if (this.Components[2].Id == 3 && this.Components[1].Id == 2 && this.Components[0].Id == 1)
+            }
-                {
-                    return JpegColorSpace.YCbCr;
-                }
 
-                // 3-channel non-subsampled images are assumed to be RGB.
+            JpegThrowHelper.ThrowNotSupportedComponentCount(componentCount);
-                if (this.Components[2].VerticalSamplingFactor == 1 && this.Components[1].VerticalSamplingFactor == 1 && this.Components[0].VerticalSamplingFactor == 1 &&
+            return default;
-                    this.Components[2].HorizontalSamplingFactor == 1 && this.Components[1].HorizontalSamplingFactor == 1 && this.Components[0].HorizontalSamplingFactor == 1)
+        }
-                {
+
-                    return JpegColorSpace.RGB;
+        /// <summary>
-                }
+        /// Returns encoded colorspace based on the component count.
+        /// </summary>
+        /// <param name="componentCount">Number of components.</param>
+        /// <returns>The <see cref="JpegColorSpace"/></returns>
+        internal static JpegColorSpace DeduceJpegColorSpace(byte componentCount)
+        {
+            if (componentCount == 1)
+            {
+                return JpegColorSpace.Grayscale;
+            }
 
-                // Some images are poorly encoded and contain incorrect colorspace transform metadata.
+            if (componentCount == 3)
-                // We ignore that and always fall back to the default colorspace.
+            {
                 return JpegColorSpace.YCbCr;
             }
 
             if (componentCount == 4)
             {
-                // jfif images doesn't not support 4 component images, so we only check adobe.
-                if (!this.adobe.Equals(default))
-                {
-                    if (this.adobe.ColorTransform == JpegConstants.Adobe.ColorTransformYcck)
-                    {
-                        return JpegColorSpace.Ycck;
-                    }
-
-                    if (this.adobe.ColorTransform == JpegConstants.Adobe.ColorTransformUnknown)
-                    {
-                        return JpegColorSpace.Cmyk;
-                    }
-
-                    JpegThrowHelper.ThrowNotSupportedColorSpace();
-                }
-
-                // Fallback to cmyk as neither of cmyk nor ycck have 'special' component ids.
                 return JpegColorSpace.Cmyk;
             }
 
@@ -771,8 +739,6 @@ namespace SixLabors.ImageSharp.Formats.Jpeg
         /// <param name="remaining">The remaining bytes in the segment block.</param>
         private void ProcessApplicationHeaderMarker(BufferedReadStream stream, int remaining)
         {
-            this.hasJFif = true;
-
             // We can only decode JFif identifiers.
             // Some images contain multiple JFIF markers (Issue 1932) so we check to see
             // if it's already been read.
@@ -1075,7 +1041,10 @@ namespace SixLabors.ImageSharp.Formats.Jpeg
             stream.Read(this.temp, 0, MarkerLength);
             remaining -= MarkerLength;
 
-            AdobeMarker.TryParse(this.temp, out this.adobe);
+            if (AdobeMarker.TryParse(this.temp, out this.adobe))
+            {
+                this.hasAdobeMarker = true;
+            }
 
             if (remaining > 0)
             {
@@ -1268,7 +1237,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg
             int maxH = 0;
             int maxV = 0;
             int index = 0;
-            for (int i = 0; i < componentCount; i++)
+            for (int i = 0; i < this.Frame.Components.Length; i++)
             {
                 // 1 byte: component identifier
                 byte componentId = this.temp[index];
@@ -1319,7 +1288,9 @@ namespace SixLabors.ImageSharp.Formats.Jpeg
                 index += componentBytes;
             }
 
-            this.ColorSpace = this.DeduceJpegColorSpace(componentCount);
+            this.ColorSpace = this.hasAdobeMarker
+                ? DeduceJpegColorSpace(componentCount, ref this.adobe)
+                : DeduceJpegColorSpace(componentCount);
             this.Metadata.GetJpegMetadata().ColorType = this.DeduceJpegColorType();
 
             if (!metadataOnly)

src/ImageSharp/Formats/Tiff/Compression/Decompressors/JpegTiffCompression.cs

@@ -59,7 +59,8 @@ namespace SixLabors.ImageSharp.Formats.Tiff.Compression.Decompressors
                     case TiffPhotometricInterpretation.BlackIsZero:
                     case TiffPhotometricInterpretation.WhiteIsZero:
                     {
-                        using SpectralConverter<L8> spectralConverterGray = new GrayJpegSpectralConverter<L8>(this.configuration);
+                        using SpectralConverter<L8> spectralConverterGray =
+                            new GrayJpegSpectralConverter<L8>(this.configuration);
                         var scanDecoderGray = new HuffmanScanDecoder(stream, spectralConverterGray, CancellationToken.None);
                         jpegDecoder.LoadTables(this.jpegTables, scanDecoderGray);
                         jpegDecoder.ParseStream(stream, spectralConverterGray, CancellationToken.None);
@@ -73,8 +74,8 @@ namespace SixLabors.ImageSharp.Formats.Tiff.Compression.Decompressors
                     case TiffPhotometricInterpretation.YCbCr:
                     case TiffPhotometricInterpretation.Rgb:
                     {
-                        using SpectralConverter<Rgb24> spectralConverter = this.photometricInterpretation == TiffPhotometricInterpretation.YCbCr ?
+                        using SpectralConverter<Rgb24> spectralConverter =
-                            new RgbJpegSpectralConverter<Rgb24>(this.configuration) : new SpectralConverter<Rgb24>(this.configuration);
+                            new TiffJpegSpectralConverter<Rgb24>(this.configuration, this.photometricInterpretation);
                         var scanDecoder = new HuffmanScanDecoder(stream, spectralConverter, CancellationToken.None);
                         jpegDecoder.LoadTables(this.jpegTables, scanDecoder);
                         jpegDecoder.ParseStream(stream, spectralConverter, CancellationToken.None);

src/ImageSharp/Formats/Tiff/Compression/Decompressors/RgbJpegSpectralConverter.cs

@@ -1,31 +0,0 @@
-// Copyright (c) Six Labors.
-// Licensed under the Six Labors Split License.
-
-using SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder;
-using SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder.ColorConverters;
-using SixLabors.ImageSharp.PixelFormats;
-
-namespace SixLabors.ImageSharp.Formats.Tiff.Compression.Decompressors
-{
-    /// <summary>
-    /// Spectral converter for YCbCr TIFF's which use the JPEG compression.
-    /// The jpeg data should be always treated as RGB color space.
-    /// </summary>
-    /// <typeparam name="TPixel">The type of the pixel.</typeparam>
-    internal sealed class RgbJpegSpectralConverter<TPixel> : SpectralConverter<TPixel>
-        where TPixel : unmanaged, IPixel<TPixel>
-    {
-        /// <summary>
-        /// Initializes a new instance of the <see cref="RgbJpegSpectralConverter{TPixel}"/> class.
-        /// This Spectral converter will always convert the pixel data to RGB color.
-        /// </summary>
-        /// <param name="configuration">The configuration.</param>
-        public RgbJpegSpectralConverter(Configuration configuration)
-            : base(configuration)
-        {
-        }
-
-        /// <inheritdoc/>
-        protected override JpegColorConverterBase GetColorConverter(JpegFrame frame, IRawJpegData jpegData) => JpegColorConverterBase.GetConverter(JpegColorSpace.RGB, frame.Precision);
-    }
-}

src/ImageSharp/Formats/Tiff/Compression/Decompressors/TiffJpegSpectralConverter{TPixel}.cs

@@ -0,0 +1,50 @@
+// Copyright (c) Six Labors.
+// Licensed under the Six Labors Split License.
+
+using SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder;
+using SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder.ColorConverters;
+using SixLabors.ImageSharp.Formats.Tiff.Constants;
+using SixLabors.ImageSharp.PixelFormats;
+
+namespace SixLabors.ImageSharp.Formats.Tiff.Compression.Decompressors
+{
+    /// <summary>
+    /// Spectral converter for YCbCr TIFF's which use the JPEG compression.
+    /// The jpeg data should be always treated as RGB color space.
+    /// </summary>
+    /// <typeparam name="TPixel">The type of the pixel.</typeparam>
+    internal sealed class TiffJpegSpectralConverter<TPixel> : SpectralConverter<TPixel>
+        where TPixel : unmanaged, IPixel<TPixel>
+    {
+        private readonly TiffPhotometricInterpretation photometricInterpretation;
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="TiffJpegSpectralConverter{TPixel}"/> class.
+        /// This Spectral converter will always convert the pixel data to RGB color.
+        /// </summary>
+        /// <param name="configuration">The configuration.</param>
+        /// <param name="photometricInterpretation">Tiff photometric interpretation.</param>
+        public TiffJpegSpectralConverter(Configuration configuration, TiffPhotometricInterpretation photometricInterpretation)
+            : base(configuration)
+            => this.photometricInterpretation = photometricInterpretation;
+
+        /// <inheritdoc/>
+        protected override JpegColorConverterBase GetColorConverter(JpegFrame frame, IRawJpegData jpegData)
+        {
+            JpegColorSpace colorSpace = GetJpegColorSpaceFromPhotometricInterpretation(this.photometricInterpretation);
+            return JpegColorConverterBase.GetConverter(colorSpace, frame.Precision);
+        }
+
+        /// <remarks>
+        /// This converter must be used only for RGB and YCbCr color spaces for performance reasons.
+        /// For grayscale images <see cref="GrayJpegSpectralConverter{TPixel}"/> must be used.
+        /// </remarks>
+        private static JpegColorSpace GetJpegColorSpaceFromPhotometricInterpretation(TiffPhotometricInterpretation interpretation)
+            => interpretation switch
+            {
+                TiffPhotometricInterpretation.Rgb => JpegColorSpace.RGB,
+                TiffPhotometricInterpretation.YCbCr => JpegColorSpace.RGB,
+                _ => throw new InvalidImageContentException($"Invalid tiff photometric interpretation for jpeg encoding: {interpretation}"),
+            };
+    }
+}

tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Internal.cs

@@ -0,0 +1,71 @@
+// Copyright (c) Six Labors.
+// Licensed under the Apache License, Version 2.0.
+
+using System;
+using System.IO;
+using System.Linq;
+using System.Threading;
+using System.Threading.Tasks;
+using SixLabors.ImageSharp.Formats.Jpeg;
+using SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder;
+using SixLabors.ImageSharp.IO;
+using SixLabors.ImageSharp.Memory;
+using SixLabors.ImageSharp.PixelFormats;
+using SixLabors.ImageSharp.Tests.Formats.Jpg.Utils;
+using SixLabors.ImageSharp.Tests.TestUtilities;
+using SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison;
+using SixLabors.ImageSharp.Tests.TestUtilities.ReferenceCodecs;
+using Xunit;
+using Xunit.Abstractions;
+
+// ReSharper disable InconsistentNaming
+namespace SixLabors.ImageSharp.Tests.Formats.Jpg
+{
+    [Trait("Format", "Jpg")]
+    public partial class JpegDecoderTests
+    {
+        [Theory]
+        [InlineData(1, 0, JpegColorSpace.Grayscale)]
+        [InlineData(3, JpegConstants.Adobe.ColorTransformUnknown, JpegColorSpace.RGB)]
+        [InlineData(3, JpegConstants.Adobe.ColorTransformYCbCr, JpegColorSpace.YCbCr)]
+        [InlineData(4, JpegConstants.Adobe.ColorTransformUnknown, JpegColorSpace.Cmyk)]
+        [InlineData(4, JpegConstants.Adobe.ColorTransformYcck, JpegColorSpace.Ycck)]
+        internal void DeduceJpegColorSpaceAdobeMarker_ShouldReturnValidColorSpace(byte componentCount, byte adobeFlag, JpegColorSpace expectedColorSpace)
+        {
+            byte[] adobeMarkerPayload = new byte[] { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, adobeFlag };
+            ProfileResolver.AdobeMarker.CopyTo(adobeMarkerPayload);
+            _ = AdobeMarker.TryParse(adobeMarkerPayload, out AdobeMarker adobeMarker);
+
+            JpegColorSpace actualColorSpace = JpegDecoderCore.DeduceJpegColorSpace(componentCount, ref adobeMarker);
+
+            Assert.Equal(expectedColorSpace, actualColorSpace);
+        }
+
+        [Theory]
+        [InlineData(2)]
+        [InlineData(5)]
+        public void DeduceJpegColorSpaceAdobeMarker_ShouldThrowOnUnsupportedComponentCount(byte componentCount)
+        {
+            AdobeMarker adobeMarker = default;
+
+            Assert.Throws<NotSupportedException>(() => JpegDecoderCore.DeduceJpegColorSpace(componentCount, ref adobeMarker));
+        }
+
+        [Theory]
+        [InlineData(1, JpegColorSpace.Grayscale)]
+        [InlineData(3, JpegColorSpace.YCbCr)]
+        [InlineData(4, JpegColorSpace.Cmyk)]
+        internal void DeduceJpegColorSpace_ShouldReturnValidColorSpace(byte componentCount, JpegColorSpace expectedColorSpace)
+        {
+            JpegColorSpace actualColorSpace = JpegDecoderCore.DeduceJpegColorSpace(componentCount);
+
+            Assert.Equal(expectedColorSpace, actualColorSpace);
+        }
+
+        [Theory]
+        [InlineData(2)]
+        [InlineData(5)]
+        public void DeduceJpegColorSpace_ShouldThrowOnUnsupportedComponentCount(byte componentCount)
+            => Assert.Throws<NotSupportedException>(() => JpegDecoderCore.DeduceJpegColorSpace(componentCount));
+    }
+}