JFifMarker now handles parsing logic

src/ImageSharp/Formats/Jpeg/Common/Decoder/JFifMarker.cs

@@ -5,6 +5,8 @@ using System;
 
 namespace SixLabors.ImageSharp.Formats.Jpeg.Common.Decoder
 {
+    using Guard = SixLabors.Guard;
+
     /// <summary>
     /// Provides information about the JFIF marker segment
     /// TODO: Thumbnail?
@@ -12,32 +14,84 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Common.Decoder
     internal struct JFifMarker : IEquatable<JFifMarker>
     {
         /// <summary>
-        /// The major version
+        /// Gets the length of an JFIF marker segment.
         /// </summary>
-        public byte MajorVersion;
+        public const int Length = 13;
 
         /// <summary>
-        /// The minor version
+        /// Initializes a new instance of the <see cref="JFifMarker"/> struct.
         /// </summary>
-        public byte MinorVersion;
+        /// <param name="majorVersion">The major version</param>
+        /// <param name="minorVersion">The minor version</param>
+        /// <param name="densityUnits">The units for the density values</param>
+        /// <param name="xDensity">The horizontal pixel density</param>
+        /// <param name="yDensity">The veritcal pixel density</param>
+        private JFifMarker(byte majorVersion, byte minorVersion, byte densityUnits, short xDensity, short yDensity)
+        {
+            Guard.MustBeGreaterThan(xDensity, 0, nameof(xDensity));
+            Guard.MustBeGreaterThan(yDensity, 0, nameof(yDensity));
+
+            this.MajorVersion = majorVersion;
+            this.MinorVersion = minorVersion;
+            this.DensityUnits = densityUnits;
+            this.XDensity = xDensity;
+            this.YDensity = yDensity;
+        }
 
         /// <summary>
-        /// Units for the following pixel density fields
+        /// Gets the major version
+        /// </summary>
+        public byte MajorVersion { get; }
+
+        /// <summary>
+        /// Gets the minor version
+        /// </summary>
+        public byte MinorVersion { get; }
+
+        /// <summary>
+        /// Gets the units for the following pixel density fields
         ///  00 : No units; width:height pixel aspect ratio = Ydensity:Xdensity
         ///  01 : Pixels per inch (2.54 cm)
         ///  02 : Pixels per centimeter
         /// </summary>
-        public byte DensityUnits;
+        public byte DensityUnits { get; }
+
+        /// <summary>
+        /// Gets the horizontal pixel density. Must not be zero.
+        /// </summary>
+        public short XDensity { get; }
 
         /// <summary>
-        /// Horizontal pixel density. Must not be zero.
+        /// Gets the vertical pixel density. Must not be zero.
         /// </summary>
-        public short XDensity;
+        public short YDensity { get; }
 
         /// <summary>
-        /// Vertical pixel density. Must not be zero.
+        /// Converts the specified byte array representation of an JFIF marker to its <see cref="JFifMarker"/> equivalent and
+        /// returns a value that indicates whether the conversion succeeded.
         /// </summary>
-        public short YDensity;
+        /// <param name="bytes">The byte array containing metadata to parse</param>
+        /// <param name="marker">The marker to return.</param>
+        public static bool TryParse(byte[] bytes, out JFifMarker marker)
+        {
+            if (ProfileResolver.IsProfile(bytes, ProfileResolver.JFifMarker))
+            {
+                byte majorVersion = bytes[5];
+                byte minorVersion = bytes[6];
+                byte densityUnits = bytes[7];
+                short xDensity = (short)((bytes[8] << 8) | bytes[9]);
+                short yDensity = (short)((bytes[10] << 8) | bytes[11]);
+
+                if (xDensity > 0 && yDensity > 0)
+                {
+                    marker = new JFifMarker(majorVersion, minorVersion, densityUnits, xDensity, yDensity);
+                    return true;
+                }
+            }
+
+            marker = default(JFifMarker);
+            return false;
+        }
 
         /// <inheritdoc/>
         public bool Equals(JFifMarker other)
@@ -62,19 +116,14 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Common.Decoder
 
         /// <inheritdoc/>
         public override int GetHashCode()
-        {
-            return GetHashCode(this);
-        }
-
-        private static int GetHashCode(JFifMarker marker)
         {
             return HashHelpers.Combine(
-                marker.MajorVersion.GetHashCode(),
+                this.MajorVersion.GetHashCode(),
                 HashHelpers.Combine(
-                    marker.MinorVersion.GetHashCode(),
+                    this.MinorVersion.GetHashCode(),
                     HashHelpers.Combine(
-                        marker.DensityUnits.GetHashCode(),
-                        HashHelpers.Combine(marker.XDensity, marker.YDensity))));
+                        this.DensityUnits.GetHashCode(),
+                        HashHelpers.Combine(this.XDensity, this.YDensity))));
         }
     }
 }

src/ImageSharp/Formats/Jpeg/GolangPort/OrigJpegDecoderCore.cs

@@ -48,18 +48,14 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.GolangPort
 
         /// <summary>
         /// Whether the image has a JFIF header
+        /// It's faster to check this than to use the equality operator on the struct
         /// </summary>
         private bool isJFif;
 
         /// <summary>
-        /// The horizontal resolution gleaned from a JFIF marker if present
-        /// </summary>
-        private short jFifHorizontalResolution;
-
-        /// <summary>
-        /// The vertical resolution gleaned from a JFIF marker if present
+        /// Contains information about the JFIF marker
         /// </summary>
-        private short jFifVerticalResolution;
+        private JFifMarker jFif;
 
         /// <summary>
         /// Whether the image has a EXIF header
@@ -77,11 +73,6 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.GolangPort
         /// </summary>
         private AdobeMarker adobe;
 
-        /// <summary>
-        /// Contains information about the JFIF marker
-        /// </summary>
-        private JFifMarker jFif;
-
         /// <summary>
         /// Initializes a new instance of the <see cref="OrigJpegDecoderCore" /> class.
         /// </summary>
@@ -418,10 +409,10 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.GolangPort
                     this.MetaData.VerticalResolution = verticalValue;
                 }
             }
-            else if (this.jFifHorizontalResolution > 0 && this.jFifVerticalResolution > 0)
+            else if (this.isJFif)
             {
-                this.MetaData.HorizontalResolution = this.jFifHorizontalResolution;
-                this.MetaData.VerticalResolution = this.jFifVerticalResolution;
+                this.MetaData.HorizontalResolution = this.jFif.XDensity;
+                this.MetaData.VerticalResolution = this.jFif.YDensity;
             }
         }
 
@@ -437,15 +428,11 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.GolangPort
                 return;
             }
 
-            this.InputProcessor.ReadFull(this.Temp, 0, 13);
-            remaining -= 13;
+            const int MarkerLength = JFifMarker.Length;
+            this.InputProcessor.ReadFull(this.Temp, 0, MarkerLength);
+            remaining -= MarkerLength;
 
-            if (ProfileResolver.IsProfile(this.Temp, ProfileResolver.JFifMarker))
-            {
-                this.isJFif = true;
-                this.jFifHorizontalResolution = (short)((this.Temp[8] << 8) | this.Temp[9]);
-                this.jFifVerticalResolution = (short)((this.Temp[10] << 8) | this.Temp[11]);
-            }
+            this.isJFif = JFifMarker.TryParse(this.Temp, out this.jFif);
 
             if (remaining > 0)
             {

src/ImageSharp/Formats/Jpeg/PdfJsPort/PdfJsJpegDecoderCore.cs

@@ -410,26 +410,10 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.PdfJsPort
                 return;
             }
 
-            this.InputStream.Read(this.temp, 0, 13);
-            remaining -= 13;
+            this.InputStream.Read(this.temp, 0, JFifMarker.Length);
+            remaining -= JFifMarker.Length;
 
-            bool isJfif = this.temp[0] == PdfJsJpegConstants.Markers.JFif.J &&
-                          this.temp[1] == PdfJsJpegConstants.Markers.JFif.F &&
-                          this.temp[2] == PdfJsJpegConstants.Markers.JFif.I &&
-                          this.temp[3] == PdfJsJpegConstants.Markers.JFif.F &&
-                          this.temp[4] == PdfJsJpegConstants.Markers.JFif.Null;
-
-            if (isJfif)
-            {
-                this.jFif = new JFifMarker
-                {
-                    MajorVersion = this.temp[5],
-                    MinorVersion = this.temp[6],
-                    DensityUnits = this.temp[7],
-                    XDensity = (short)((this.temp[8] << 8) | this.temp[9]),
-                    YDensity = (short)((this.temp[10] << 8) | this.temp[11])
-                };
-            }
+            JFifMarker.TryParse(this.temp, out this.jFif);
 
             // TODO: thumbnail
             if (remaining > 0)

tests/ImageSharp.Tests/Formats/Jpg/AdobeMarkerTests.cs

@@ -13,7 +13,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Jpg
         // Taken from actual test image
         private readonly byte[] bytes = { 0x41, 0x64, 0x6F, 0x62, 0x65, 0x0, 0x64, 0x0, 0x0, 0x0, 0x0, 0x2 };
 
-        /// Altered components
+        // Altered components
         private readonly byte[] bytes2 = { 0x41, 0x64, 0x6F, 0x62, 0x65, 0x0, 0x64, 0x0, 0x0, 0x1, 0x1, 0x1 };
 
         [Fact]

tests/ImageSharp.Tests/Formats/Jpg/JFifMarkerTests.cs

@@ -0,0 +1,95 @@
+// Copyright (c) Six Labors and contributors.
+// Licensed under the Apache License, Version 2.0.
+
+namespace SixLabors.ImageSharp.Tests.Formats.Jpg
+{
+    using SixLabors.ImageSharp.Formats.Jpeg.Common.Decoder;
+    using SixLabors.ImageSharp.Formats.Jpeg.GolangPort;
+
+    using Xunit;
+
+    public class JFifMarkerTests
+    {
+        // Taken from actual test image
+        private readonly byte[] bytes = { 0x4A, 0x46, 0x49, 0x46, 0x0, 0x1, 0x1, 0x1, 0x0, 0x60, 0x0, 0x60, 0x0 };
+
+        // Altered components
+        private readonly byte[] bytes2 = { 0x4A, 0x46, 0x49, 0x46, 0x0, 0x1, 0x1, 0x1, 0x0, 0x48, 0x0, 0x48, 0x0 };
+
+        // Incorrect density values. Zero is invalid.
+        private readonly byte[] bytes3 = { 0x4A, 0x46, 0x49, 0x46, 0x0, 0x1, 0x1, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0 };
+
+        [Fact]
+        public void MarkerLengthIsCorrect()
+        {
+            Assert.Equal(13, JFifMarker.Length);
+        }
+
+        [Fact]
+        public void MarkerReturnsCorrectParsedValue()
+        {
+            bool isJFif = JFifMarker.TryParse(this.bytes, out var marker);
+
+            Assert.True(isJFif);
+            Assert.Equal(1, marker.MajorVersion);
+            Assert.Equal(1, marker.MinorVersion);
+            Assert.Equal(1, marker.DensityUnits);
+            Assert.Equal(96, marker.XDensity);
+            Assert.Equal(96, marker.YDensity);
+        }
+
+        [Fact]
+        public void MarkerIgnoresIncorrectValue()
+        {
+            bool isJFif = JFifMarker.TryParse(new byte[] { 0, 0, 0, 0 }, out var marker);
+
+            Assert.False(isJFif);
+            Assert.Equal(default(JFifMarker), marker);
+        }
+
+        [Fact]
+        public void MarkerIgnoresCorrectHeaderButInvalidDensities()
+        {
+            bool isJFif = JFifMarker.TryParse(this.bytes3, out var marker);
+
+            Assert.False(isJFif);
+            Assert.Equal(default(JFifMarker), marker);
+        }
+
+        [Fact]
+        public void MarkerEqualityIsCorrect()
+        {
+            JFifMarker.TryParse(this.bytes, out var marker);
+            JFifMarker.TryParse(this.bytes, out var marker2);
+
+            Assert.True(marker.Equals(marker2));
+        }
+
+        [Fact]
+        public void MarkerInEqualityIsCorrect()
+        {
+            JFifMarker.TryParse(this.bytes, out var marker);
+            JFifMarker.TryParse(this.bytes2, out var marker2);
+
+            Assert.False(marker.Equals(marker2));
+        }
+
+        [Fact]
+        public void MarkerHashCodeIsReplicable()
+        {
+            JFifMarker.TryParse(this.bytes, out var marker);
+            JFifMarker.TryParse(this.bytes, out var marker2);
+
+            Assert.True(marker.GetHashCode().Equals(marker2.GetHashCode()));
+        }
+
+        [Fact]
+        public void MarkerHashCodeIsUnique()
+        {
+            JFifMarker.TryParse(this.bytes, out var marker);
+            JFifMarker.TryParse(this.bytes2, out var marker2);
+
+            Assert.False(marker.GetHashCode().Equals(marker2.GetHashCode()));
+        }
+    }
+}