Jpeg color space deduction fix

src/ImageSharp/Formats/Jpeg/Components/Decoder/AdobeMarker.cs

@@ -1,4 +1,4 @@
-// Copyright (c) Six Labors.
+// Copyright (c) Six Labors.
 // Licensed under the Apache License, Version 2.0.
 
 using System;

src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs

@@ -95,7 +95,7 @@ public Buffer2D<TPixel> GetPixelBuffer(CancellationToken cancellationToken)
                 }
             }
 
-            var buffer = this.pixelBuffer;
+            Buffer2D<TPixel> buffer = this.pixelBuffer;
             this.pixelBuffer = null;
             return buffer;
         }

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

@@ -86,14 +86,14 @@ internal sealed class JpegDecoderCore : IRawJpegData, IImageDecoderInternals
         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.
@@ -506,90 +506,48 @@ public void Dispose()
         /// Returns the correct colorspace based on the image component count and the jpeg frame component id's.
         /// </summary>
         /// <param name="componentCount">The 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)
-            {
-                return JpegColorSpace.Grayscale;
-            }
-
             if (componentCount == 3)
             {
-                // We prioritize adobe marker over jfif marker, if somebody really encoded this image with redundant adobe marker,
-                // then it's most likely an adobe jfif image.
-                if (!this.adobe.Equals(default))
+                if (adobeMarker.ColorTransform == JpegConstants.Adobe.ColorTransformUnknown)
                 {
-                    if (this.adobe.ColorTransform == JpegConstants.Adobe.ColorTransformYCbCr)
-                    {
-                        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();
+                    return JpegColorSpace.RGB;
                 }
 
-                if (this.hasJFif)
-                {
-                    // JFIF implies YCbCr.
-                    return JpegColorSpace.YCbCr;
-                }
+                return JpegColorSpace.YCbCr;
+            }
 
-                // Fallback to the id color deduction.
-                // 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 (this.Components[2].Id == 66 && this.Components[1].Id == 71 && this.Components[0].Id == 82)
+            if (componentCount == 4)
+            {
+                if (adobeMarker.ColorTransform == JpegConstants.Adobe.ColorTransformYcck)
                 {
-                    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.
-                if (this.Components[2].Id == 3 && this.Components[1].Id == 2 && this.Components[0].Id == 1)
-                {
-                    return JpegColorSpace.YCbCr;
-                }
+                return JpegColorSpace.Cmyk;
+            }
 
-                // 3-channel non-subsampled images are assumed to be RGB.
-                if (this.Components[2].VerticalSamplingFactor == 1 && this.Components[1].VerticalSamplingFactor == 1 && this.Components[0].VerticalSamplingFactor == 1 &&
-                    this.Components[2].HorizontalSamplingFactor == 1 && this.Components[1].HorizontalSamplingFactor == 1 && this.Components[0].HorizontalSamplingFactor == 1)
-                {
-                    return JpegColorSpace.RGB;
-                }
+            JpegThrowHelper.ThrowNotSupportedComponentCount(componentCount);
+            return default;
+        }
+
+        internal static JpegColorSpace DeduceJpegColorSpace(byte componentCount)
+        {
+            if (componentCount == 1)
+            {
+                return JpegColorSpace.Grayscale;
+            }
 
-                // Some images are poorly encoded and contain incorrect colorspace transform metadata.
-                // We ignore that and always fall back to the default colorspace.
+            if (componentCount == 3)
+            {
                 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;
             }
 
@@ -757,8 +715,6 @@ private void ExtendProfile(ref byte[] profile, byte[] extension)
         /// <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.
@@ -1061,7 +1017,10 @@ private void ProcessApp14Marker(BufferedReadStream stream, int remaining)
             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)
             {
@@ -1308,7 +1267,9 @@ private void ProcessStartOfFrameMarker(BufferedReadStream stream, int remaining,
                 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

@@ -73,8 +73,9 @@ protected override void Decompress(BufferedReadStream stream, int byteCount, int
                     case TiffPhotometricInterpretation.YCbCr:
                     case TiffPhotometricInterpretation.Rgb:
                     {
-                        using SpectralConverter<Rgb24> spectralConverter = this.photometricInterpretation == TiffPhotometricInterpretation.YCbCr ?
-                            new RgbJpegSpectralConverter<Rgb24>(this.configuration) : new SpectralConverter<Rgb24>(this.configuration);
+                        // The jpeg data should treated as RGB color space. If the PhotometricInterpretation is YCbCr,
+                        // the conversion to RGB will be handled in the next step by the YCbCr color decoder.
+                        using SpectralConverter<Rgb24> spectralConverter = new RgbJpegSpectralConverter<Rgb24>(this.configuration);
                         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

@@ -8,8 +8,9 @@
 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.
+    /// Spectral converter for TIFF's which use the JPEG compression.
+    /// The compressed jpeg data should be always treated as RGB color space.
+    /// If PhotometricInterpretation indicates the data is YCbCr, the color decoder will handle the conversion.
     /// </summary>
     /// <typeparam name="TPixel">The type of the pixel.</typeparam>
     internal sealed class RgbJpegSpectralConverter<TPixel> : SpectralConverter<TPixel>

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

@@ -0,0 +1,69 @@
+// 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(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));
+    }
+}