Merge pull request #483 from SixLabors/js/fix-481

Can now read padded RSTn markers. Fix #481

src/ImageSharp/Formats/Jpeg/GolangPort/Components/Decoder/InputProcessor.cs

@@ -380,5 +380,13 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.GolangPort.Components.Decoder
             this.LastErrorCode = this.Bits.ReceiveExtendUnsafe(t, ref this, out x);
             return this.LastErrorCode;
         }
+
+        /// <summary>
+        /// Reset the Huffman decoder.
+        /// </summary>
+        public void ResetHuffmanDecoder()
+        {
+            this.Bits = default(Bits);
+        }
     }
 }

src/ImageSharp/Formats/Jpeg/GolangPort/Components/Decoder/OrigJpegScanDecoder.cs

@@ -94,6 +94,21 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.GolangPort.Components.Decoder
         /// </summary>
         private int eobRun;
 
+        /// <summary>
+        /// The block counter
+        /// </summary>
+        private int blockCounter;
+
+        /// <summary>
+        /// The MCU counter
+        /// </summary>
+        private int mcuCounter;
+
+        /// <summary>
+        /// The expected RST marker value
+        /// </summary>
+        private byte expectedRst;
+
         /// <summary>
         /// Initializes a default-constructed <see cref="OrigJpegScanDecoder"/> instance for reading data from <see cref="OrigJpegDecoderCore"/>-s stream.
         /// </summary>
@@ -139,13 +154,30 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.GolangPort.Components.Decoder
         {
             decoder.InputProcessor.ResetErrorState();
 
-            int blockCount = 0;
+            this.blockCounter = 0;
-            int mcu = 0;
+            this.mcuCounter = 0;
-            byte expectedRst = OrigJpegConstants.Markers.RST0;
+            this.expectedRst = OrigJpegConstants.Markers.RST0;
 
             for (int my = 0; my < decoder.MCUCountY; my++)
             {
                 for (int mx = 0; mx < decoder.MCUCountX; mx++)
+                {
+                    this.DecodeBlocksAtMcuIndex(decoder, mx, my);
+
+                    this.mcuCounter++;
+
+                    // Handling restart intervals
+                    // Useful info: https://stackoverflow.com/a/8751802
+                    if (decoder.IsAtRestartInterval(this.mcuCounter))
+                    {
+                        this.ProcessRSTMarker(decoder);
+                        this.Reset(decoder);
+                    }
+                }
+            }
+        }
+
+        private void DecodeBlocksAtMcuIndex(OrigJpegDecoderCore decoder, int mx, int my)
         {
             for (int scanIndex = 0; scanIndex < this.componentScanCount; scanIndex++)
             {
@@ -165,9 +197,9 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.GolangPort.Components.Decoder
                     else
                     {
                         int q = decoder.MCUCountX * this.hi;
-                                this.bx = blockCount % q;
+                        this.bx = this.blockCounter % q;
-                                this.by = blockCount / q;
+                        this.by = this.blockCounter / q;
-                                blockCount++;
+                        this.blockCounter++;
                         if (this.bx * 8 >= decoder.ImageWidth || this.by * 8 >= decoder.ImageHeight)
                         {
                             continue;
@@ -188,51 +220,70 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.GolangPort.Components.Decoder
                     // Store the result block:
                     blockRefOnHeap = this.data.Block;
                 }
-
+            }
-                        // for j
         }
 
-                    // for i
+        private void ProcessRSTMarker(OrigJpegDecoderCore decoder)
-                    mcu++;
-
-                    if (decoder.RestartInterval > 0 && mcu % decoder.RestartInterval == 0 && mcu < decoder.TotalMCUCount)
         {
-                        // A more sophisticated decoder could use RST[0-7] markers to resynchronize from corrupt input,
+            // Attempt to look for RST[0-7] markers to resynchronize from corrupt input.
-                        // but this one assumes well-formed input, and hence the restart marker follows immediately.
             if (!decoder.InputProcessor.ReachedEOF)
             {
                 decoder.InputProcessor.ReadFullUnsafe(decoder.Temp, 0, 2);
                 if (decoder.InputProcessor.CheckEOFEnsureNoError())
                 {
-                                if (decoder.Temp[0] != 0xff || decoder.Temp[1] != expectedRst)
+                    if (decoder.Temp[0] != 0xFF || decoder.Temp[1] != this.expectedRst)
+                    {
+                        bool invalidRst = true;
+
+                        // Most jpeg's containing well-formed input will have a RST[0-7] marker following immediately
+                        // but some, see Issue #481, contain padding bytes "0xFF" before the RST[0-7] marker.
+                        // If we identify that case we attempt to read until we have bypassed the padded bytes.
+                        // We then check again for our RST marker and throw if invalid.
+                        // No other methods are attempted to resynchronize from corrupt input.
+                        if (decoder.Temp[0] == 0xFF && decoder.Temp[1] == 0xFF)
+                        {
+                            while (decoder.Temp[0] == 0xFF && decoder.InputProcessor.CheckEOFEnsureNoError())
+                            {
+                                decoder.InputProcessor.ReadFullUnsafe(decoder.Temp, 0, 1);
+                                if (!decoder.InputProcessor.CheckEOFEnsureNoError())
+                                {
+                                    break;
+                                }
+                            }
+
+                            // Have we found a valid restart marker?
+                            invalidRst = decoder.Temp[0] != this.expectedRst;
+                        }
+
+                        if (invalidRst)
                         {
                             throw new ImageFormatException("Bad RST marker");
                         }
+                    }
 
-                                expectedRst++;
+                    this.expectedRst++;
-                                if (expectedRst == OrigJpegConstants.Markers.RST7 + 1)
+                    if (this.expectedRst == OrigJpegConstants.Markers.RST7 + 1)
                     {
-                                    expectedRst = OrigJpegConstants.Markers.RST0;
+                        this.expectedRst = OrigJpegConstants.Markers.RST0;
+                    }
                 }
             }
         }
 
-                        // Reset the Huffman decoder.
+        private void Reset(OrigJpegDecoderCore decoder)
-                        decoder.InputProcessor.Bits = default(Bits);
+        {
+            decoder.InputProcessor.ResetHuffmanDecoder();
 
-                        // Reset the DC components, as per section F.2.1.3.1.
+            this.ResetDcValues();
-                        this.ResetDc();
 
             // Reset the progressive decoder state, as per section G.1.2.2.
             this.eobRun = 0;
         }
-                }
 
-                // for mx
+        /// <summary>
-            }
+        /// Reset the DC components, as per section F.2.1.3.1.
-        }
+        /// </summary>
-
+        private void ResetDcValues()
-        private void ResetDc()
         {
             Unsafe.InitBlock(this.pointers.Dc, default(byte), sizeof(int) * OrigJpegDecoderCore.MaxComponents);
         }

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

@@ -411,6 +411,15 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.GolangPort
             this.InitDerivedMetaDataProperties();
         }
 
+        /// <summary>
+        /// Returns true if 'mcuCounter' is at restart interval
+        /// </summary>
+        public bool IsAtRestartInterval(int mcuCounter)
+        {
+            return this.RestartInterval > 0 && mcuCounter % this.RestartInterval == 0
+                                            && mcuCounter < this.TotalMCUCount;
+        }
+
         /// <summary>
         /// Assigns derived metadata properties to <see cref="MetaData"/>, eg. horizontal and vertical resolution if it has a JFIF header.
         /// </summary>

src/ImageSharp/Formats/Jpeg/PdfJsPort/Components/PdfJsScanDecoder.cs

@@ -154,7 +154,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.PdfJsPort.Components
 
                 ushort marker = fileMarker.Marker;
 
-                // RSTn - We've alread read the bytes and altered the position so no need to skip
+                // RSTn - We've already read the bytes and altered the position so no need to skip
                 if (marker >= PdfJsJpegConstants.Markers.RST0 && marker <= PdfJsJpegConstants.Markers.RST7)
                 {
                     continue;

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

@@ -137,7 +137,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.PdfJsPort
                         return new PdfJsFileMarker(PdfJsJpegConstants.Markers.EOI, (int)stream.Length - 2);
                     }
 
-                    marker[1] = (byte)value;
+                    marker[1] = (byte)suffix;
                 }
 
                 return new PdfJsFileMarker((ushort)((marker[0] << 8) | marker[1]), (int)(stream.Position - 2));

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

@@ -42,7 +42,8 @@ namespace SixLabors.ImageSharp.Tests.Formats.Jpg
                 TestImages.Jpeg.Baseline.Jpeg444,
                 TestImages.Jpeg.Baseline.Bad.BadEOF,
                 TestImages.Jpeg.Issues.MultiHuffmanBaseline394,
-                TestImages.Jpeg.Baseline.MultiScanBaselineCMYK
+                TestImages.Jpeg.Baseline.MultiScanBaselineCMYK,
+                TestImages.Jpeg.Baseline.Bad.BadRST
             };
 
         public static string[] ProgressiveTestJpegs =
@@ -61,6 +62,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Jpg
             [TestImages.Jpeg.Baseline.Calliphora] = 0.00002f / 100,
             [TestImages.Jpeg.Baseline.Bad.BadEOF] = 0.38f / 100,
             [TestImages.Jpeg.Baseline.Testorig420] = 0.38f / 100,
+            [TestImages.Jpeg.Baseline.Bad.BadRST] = 0.0589f / 100,
 
             // Progressive:
             [TestImages.Jpeg.Issues.MissingFF00ProgressiveGirl159] = 0.34f / 100,

tests/ImageSharp.Tests/TestImages.cs

@@ -96,6 +96,7 @@ namespace SixLabors.ImageSharp.Tests
                 public static class Bad
                 {
                     public const string BadEOF = "Jpg/baseline/badeof.jpg";
+                    public const string BadRST = "Jpg/baseline/badrst.jpg";
                 }
 
                 public const string Cmyk = "Jpg/baseline/cmyk.jpg";

tests/Images/External

@@ -1 +1 @@
-Subproject commit 8714b94dc4bab6788fcbb6254174db2b9c8f69c9
+Subproject commit 653f0c7e3c84657f68dd46e5a380186b3696b956

tests/Images/Input/Jpg/baseline/badrst.jpg

@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:af18f0bf30231d2c4c0e6b80d4636237c2851f67763788de12931fd1960c4ff3
+size 74497