From ba064b02521bcd2b1b04352365c91de7a6381507 Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Thu, 19 Jan 2017 20:21:51 +0000 Subject: [PATCH 1/6] stop processing jpeg file once image loaded. --- .../Components/Decoder/JpegScanDecoder.cs | 5 ++- .../JpegDecoderCore.cs | 15 +++++++- .../Formats/Jpg/BadEofJpegTests.cs | 38 +++++++++++++++++++ tests/ImageSharp.Tests/TestImages.cs | 1 + .../TestImages/Formats/Jpg/badeof.jpg | 3 ++ 5 files changed, 59 insertions(+), 3 deletions(-) create mode 100644 tests/ImageSharp.Tests/Formats/Jpg/BadEofJpegTests.cs create mode 100644 tests/ImageSharp.Tests/TestImages/Formats/Jpg/badeof.jpg diff --git a/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs b/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs index 39ee6687b0..329c58951b 100644 --- a/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs +++ b/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs @@ -118,7 +118,8 @@ namespace ImageSharp.Formats.Jpg /// Reads the blocks from the -s stream, and processes them into the corresponding instances. /// /// The instance - public void ProcessBlocks(JpegDecoderCore decoder) + /// MCUs processed + public int ProcessBlocks(JpegDecoderCore decoder) { int blockCount = 0; int mcu = 0; @@ -229,6 +230,8 @@ namespace ImageSharp.Formats.Jpg // for mx } + + return mcu; } private void ResetDc() diff --git a/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs b/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs index 3edd6f70e2..5ab5764e26 100644 --- a/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs +++ b/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs @@ -79,6 +79,16 @@ namespace ImageSharp.Formats /// private YCbCrImage ycbcrImage; + /// + /// The MCU target + /// + private int mcuTarget; + + /// + /// The MCUs processed + /// + private int mcusProcessed; + /// /// Initializes a new instance of the class. /// @@ -187,7 +197,7 @@ namespace ImageSharp.Formats } // Process the remaining segments until the End Of Image marker. - while (true) + while (this.mcuTarget < 1 || this.mcuTarget != this.mcusProcessed) { this.ReadFull(this.Temp, 0, 2); while (this.Temp[0] != 0xff) @@ -864,6 +874,7 @@ namespace ImageSharp.Formats /// The vertical MCU count private void MakeImage(int mxx, int myy) { + this.mcuTarget = mxx * myy; if (this.grayImage.IsInitialized || this.ycbcrImage != null) { return; @@ -1395,7 +1406,7 @@ namespace ImageSharp.Formats JpegScanDecoder.Init(&scan, this, remaining); this.Bits = default(Bits); this.MakeImage(scan.XNumberOfMCUs, scan.YNumberOfMCUs); - scan.ProcessBlocks(this); + this.mcusProcessed += scan.ProcessBlocks(this); } /// diff --git a/tests/ImageSharp.Tests/Formats/Jpg/BadEofJpegTests.cs b/tests/ImageSharp.Tests/Formats/Jpg/BadEofJpegTests.cs new file mode 100644 index 0000000000..0697dbb641 --- /dev/null +++ b/tests/ImageSharp.Tests/Formats/Jpg/BadEofJpegTests.cs @@ -0,0 +1,38 @@ +// +// Copyright (c) James Jackson-South and contributors. +// Licensed under the Apache License, Version 2.0. +// + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using ImageSharp.Formats; +using Xunit; +using Xunit.Abstractions; +// ReSharper disable InconsistentNaming + +namespace ImageSharp.Tests +{ + using System.Numerics; + + using ImageSharp.Formats.Jpg; + using ImageSharp.Processing; + + public class BadEOFJpegTests : MeasureFixture + { + public BadEOFJpegTests(ITestOutputHelper output) + : base(output) + { + } + + [Theory] + [WithFile(TestImages.Jpeg.BadEOF, PixelTypes.Color)] + public void LoadImage(TestImageProvider provider) + where TColor : struct, IPackedPixel, IEquatable + { + var image = provider.GetImage(); + Assert.NotNull(image); + } + } +} \ No newline at end of file diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 89b3c0f0d1..9e2d21a9bd 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -51,6 +51,7 @@ namespace ImageSharp.Tests public const string Festzug = "Jpg/Festzug.jpg"; public const string Hiyamugi = "Jpg/Hiyamugi.jpg"; + public const string BadEOF = "Jpg/badeof.jpg"; public const string Snake = "Jpg/Snake.jpg"; public const string Lake = "Jpg/Lake.jpg"; diff --git a/tests/ImageSharp.Tests/TestImages/Formats/Jpg/badeof.jpg b/tests/ImageSharp.Tests/TestImages/Formats/Jpg/badeof.jpg new file mode 100644 index 0000000000..e7f3b2fd4d --- /dev/null +++ b/tests/ImageSharp.Tests/TestImages/Formats/Jpg/badeof.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:781a46b044fd8a8637919481f5dd4426874d5acf22eae2376f853b35a6250628 +size 5770 From c329941f9744a6d1e6aa026058c3cacb5a70e177 Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Thu, 19 Jan 2017 22:00:18 +0000 Subject: [PATCH 2/6] prevent progressive images breaking. top progressive images from shorting the loop as they break if you do. --- .../JpegDecoderCore.cs | 22 ++++++++++--------- .../Formats/Jpg/BadEofJpegTests.cs | 2 ++ 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs b/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs index 5ab5764e26..c463ad18e5 100644 --- a/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs +++ b/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs @@ -80,14 +80,9 @@ namespace ImageSharp.Formats private YCbCrImage ycbcrImage; /// - /// The MCU target + /// The MCU counter /// - private int mcuTarget; - - /// - /// The MCUs processed - /// - private int mcusProcessed; + private int mcuCounter; /// /// Initializes a new instance of the class. @@ -197,7 +192,10 @@ namespace ImageSharp.Formats } // Process the remaining segments until the End Of Image marker. - while (this.mcuTarget < 1 || this.mcuTarget != this.mcusProcessed) + bool mcuSet = false; + + // we can't currently short circute progressive images so don't try. + while (this.IsProgressive || !mcuSet || this.mcuCounter > 0) { this.ReadFull(this.Temp, 0, 2); while (this.Temp[0] != 0xff) @@ -304,6 +302,9 @@ namespace ImageSharp.Formats return; } + // when this is a progressive image this gets called a number of times + // need to know how many times this should be called in total. + mcuSet = true; this.ProcessStartOfScan(remaining); break; case JpegConstants.Markers.DRI: @@ -874,7 +875,6 @@ namespace ImageSharp.Formats /// The vertical MCU count private void MakeImage(int mxx, int myy) { - this.mcuTarget = mxx * myy; if (this.grayImage.IsInitialized || this.ycbcrImage != null) { return; @@ -1406,7 +1406,9 @@ namespace ImageSharp.Formats JpegScanDecoder.Init(&scan, this, remaining); this.Bits = default(Bits); this.MakeImage(scan.XNumberOfMCUs, scan.YNumberOfMCUs); - this.mcusProcessed += scan.ProcessBlocks(this); + + this.mcuCounter = scan.XNumberOfMCUs * scan.YNumberOfMCUs; + this.mcuCounter -= scan.ProcessBlocks(this); } /// diff --git a/tests/ImageSharp.Tests/Formats/Jpg/BadEofJpegTests.cs b/tests/ImageSharp.Tests/Formats/Jpg/BadEofJpegTests.cs index 0697dbb641..5dd0e51aca 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/BadEofJpegTests.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/BadEofJpegTests.cs @@ -28,11 +28,13 @@ namespace ImageSharp.Tests [Theory] [WithFile(TestImages.Jpeg.BadEOF, PixelTypes.Color)] + [WithFile(TestImages.Jpeg.Progress, PixelTypes.Color)] public void LoadImage(TestImageProvider provider) where TColor : struct, IPackedPixel, IEquatable { var image = provider.GetImage(); Assert.NotNull(image); + provider.Utility.SaveTestOutputFile(image, "bmp"); } } } \ No newline at end of file From beff783ce9c25087a22f1bad2e08aee2effa291a Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Fri, 20 Jan 2017 08:12:20 +0000 Subject: [PATCH 3/6] Just drop out when image data is loaded only for non-progressive images --- .../JpegDecoderCore.cs | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs b/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs index c463ad18e5..2dd547c916 100644 --- a/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs +++ b/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs @@ -79,11 +79,6 @@ namespace ImageSharp.Formats /// private YCbCrImage ycbcrImage; - /// - /// The MCU counter - /// - private int mcuCounter; - /// /// Initializes a new instance of the class. /// @@ -192,10 +187,10 @@ namespace ImageSharp.Formats } // Process the remaining segments until the End Of Image marker. - bool mcuSet = false; + bool processBytes = true; // we can't currently short circute progressive images so don't try. - while (this.IsProgressive || !mcuSet || this.mcuCounter > 0) + while (processBytes) { this.ReadFull(this.Temp, 0, 2); while (this.Temp[0] != 0xff) @@ -304,8 +299,13 @@ namespace ImageSharp.Formats // when this is a progressive image this gets called a number of times // need to know how many times this should be called in total. - mcuSet = true; this.ProcessStartOfScan(remaining); + if (!this.IsProgressive) + { + // if this is not a progressive image we can stop processing bytes as we now have the image data. + processBytes = false; + } + break; case JpegConstants.Markers.DRI: if (configOnly) @@ -1406,9 +1406,7 @@ namespace ImageSharp.Formats JpegScanDecoder.Init(&scan, this, remaining); this.Bits = default(Bits); this.MakeImage(scan.XNumberOfMCUs, scan.YNumberOfMCUs); - - this.mcuCounter = scan.XNumberOfMCUs * scan.YNumberOfMCUs; - this.mcuCounter -= scan.ProcessBlocks(this); + scan.ProcessBlocks(this); } /// From 56702de2345f22a047c5995d2cd5b99593234e23 Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Fri, 20 Jan 2017 08:15:39 +0000 Subject: [PATCH 4/6] drop return --- .../Components/Decoder/JpegScanDecoder.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs b/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs index 329c58951b..39ee6687b0 100644 --- a/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs +++ b/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs @@ -118,8 +118,7 @@ namespace ImageSharp.Formats.Jpg /// Reads the blocks from the -s stream, and processes them into the corresponding instances. /// /// The instance - /// MCUs processed - public int ProcessBlocks(JpegDecoderCore decoder) + public void ProcessBlocks(JpegDecoderCore decoder) { int blockCount = 0; int mcu = 0; @@ -230,8 +229,6 @@ namespace ImageSharp.Formats.Jpg // for mx } - - return mcu; } private void ResetDc() From 55214175b2eaa8c82135fa363400333ff5925329 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Sat, 21 Jan 2017 11:10:02 +1100 Subject: [PATCH 5/6] At least render something when progressive. This allows parsing of the broken eof images. The non-progressive renders perfectly, the progressive appears to not render the final scan.... or something. --- .../Components/Decoder/JpegScanDecoder.cs | 33 +- .../JpegDecoderCore.cs | 284 +++++++++--------- tests/ImageSharp.Tests/FileTestBase.cs | 6 +- tests/ImageSharp.Tests/TestImages.cs | 1 + .../TestImages/Formats/Jpg/badeofprog.jpg | 3 + 5 files changed, 177 insertions(+), 150 deletions(-) create mode 100644 tests/ImageSharp.Tests/TestImages/Formats/Jpg/badeofprog.jpg diff --git a/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs b/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs index 39ee6687b0..09baf904b6 100644 --- a/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs +++ b/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs @@ -356,11 +356,21 @@ namespace ImageSharp.Formats.Jpg throw new ImageFormatException("Excessive DC component"); } - int deltaDC = decoder.Bits.ReceiveExtend(value, decoder); - this.pointers.Dc[compIndex] += deltaDC; + // TODO: Handle the EOFException and piece together the final progressive scan. + try + { + int deltaDC = decoder.Bits.ReceiveExtend(value, decoder); + this.pointers.Dc[compIndex] += deltaDC; - // b[0] = dc[compIndex] << al; - Block8x8F.SetScalarAt(b, 0, this.pointers.Dc[compIndex] << this.al); + // b[0] = dc[compIndex] << al; + Block8x8F.SetScalarAt(b, 0, this.pointers.Dc[compIndex] << this.al); + } + catch (JpegDecoderCore.EOFException) + { + // Do something clever here. I'm just undoing ReceiveExtend + decoder.Bits.UnreadBits += value; + decoder.Bits.Mask >>= value; + } } if (zig <= this.zigEnd && this.eobRun > 0) @@ -383,7 +393,20 @@ namespace ImageSharp.Formats.Jpg break; } - int ac = decoder.Bits.ReceiveExtend(val1, decoder); + int ac; // = decoder.Bits.ReceiveExtend(val1, decoder); + + // TODO: Handle the EOFException and piece together the final progressive scan. + try + { + ac = decoder.Bits.ReceiveExtend(val1, decoder); + } + catch (JpegDecoderCore.EOFException) + { + // Do something clever here. I'm just undoing ReceiveExtend + decoder.Bits.UnreadBits += val1; + decoder.Bits.Mask >>= val1; + break; + } // b[Unzig[zig]] = ac << al; Block8x8F.SetScalarAt(b, this.pointers.Unzig[zig], ac << this.al); diff --git a/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs b/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs index 2dd547c916..6ba578a3ae 100644 --- a/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs +++ b/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs @@ -186,164 +186,162 @@ namespace ImageSharp.Formats throw new ImageFormatException("Missing SOI marker."); } - // Process the remaining segments until the End Of Image marker. - bool processBytes = true; - - // we can't currently short circute progressive images so don't try. - while (processBytes) + while (true) { - this.ReadFull(this.Temp, 0, 2); - while (this.Temp[0] != 0xff) + try { - // Strictly speaking, this is a format error. However, libjpeg is - // liberal in what it accepts. As of version 9, next_marker in - // jdmarker.c treats this as a warning (JWRN_EXTRANEOUS_DATA) and - // continues to decode the stream. Even before next_marker sees - // extraneous data, jpeg_fill_bit_buffer in jdhuff.c reads as many - // bytes as it can, possibly past the end of a scan's data. It - // effectively puts back any markers that it overscanned (e.g. an - // "\xff\xd9" EOI marker), but it does not put back non-marker data, - // and thus it can silently ignore a small number of extraneous - // non-marker bytes before next_marker has a chance to see them (and - // print a warning). - // We are therefore also liberal in what we accept. Extraneous data - // is silently ignore - // This is similar to, but not exactly the same as, the restart - // mechanism within a scan (the RST[0-7] markers). - // Note that extraneous 0xff bytes in e.g. SOS data are escaped as - // "\xff\x00", and so are detected a little further down below. - this.Temp[0] = this.Temp[1]; - this.Temp[1] = this.ReadByte(); - } + this.ReadFull(this.Temp, 0, 2); + while (this.Temp[0] != 0xff) + { + // Strictly speaking, this is a format error. However, libjpeg is + // liberal in what it accepts. As of version 9, next_marker in + // jdmarker.c treats this as a warning (JWRN_EXTRANEOUS_DATA) and + // continues to decode the stream. Even before next_marker sees + // extraneous data, jpeg_fill_bit_buffer in jdhuff.c reads as many + // bytes as it can, possibly past the end of a scan's data. It + // effectively puts back any markers that it overscanned (e.g. an + // "\xff\xd9" EOI marker), but it does not put back non-marker data, + // and thus it can silently ignore a small number of extraneous + // non-marker bytes before next_marker has a chance to see them (and + // print a warning). + // We are therefore also liberal in what we accept. Extraneous data + // is silently ignore + // This is similar to, but not exactly the same as, the restart + // mechanism within a scan (the RST[0-7] markers). + // Note that extraneous 0xff bytes in e.g. SOS data are escaped as + // "\xff\x00", and so are detected a little further down below. + this.Temp[0] = this.Temp[1]; + this.Temp[1] = this.ReadByte(); + } - byte marker = this.Temp[1]; - if (marker == 0) - { - // Treat "\xff\x00" as extraneous data. - continue; - } + byte marker = this.Temp[1]; + if (marker == 0) + { + // Treat "\xff\x00" as extraneous data. + continue; + } - while (marker == 0xff) - { - // Section B.1.1.2 says, "Any marker may optionally be preceded by any - // number of fill bytes, which are bytes assigned code X'FF'". - marker = this.ReadByte(); - } + while (marker == 0xff) + { + // Section B.1.1.2 says, "Any marker may optionally be preceded by any + // number of fill bytes, which are bytes assigned code X'FF'". + marker = this.ReadByte(); + } - // End Of Image. - if (marker == JpegConstants.Markers.EOI) - { - break; - } + // End Of Image. + if (marker == JpegConstants.Markers.EOI) + { + break; + } - if (marker >= JpegConstants.Markers.RST0 && marker <= JpegConstants.Markers.RST7) - { - // Figures B.2 and B.16 of the specification suggest that restart markers should - // only occur between Entropy Coded Segments and not after the final ECS. - // However, some encoders may generate incorrect JPEGs with a final restart - // marker. That restart marker will be seen here instead of inside the ProcessSOS - // method, and is ignored as a harmless error. Restart markers have no extra data, - // so we check for this before we read the 16-bit length of the segment. - continue; - } + if (marker >= JpegConstants.Markers.RST0 && marker <= JpegConstants.Markers.RST7) + { + // Figures B.2 and B.16 of the specification suggest that restart markers should + // only occur between Entropy Coded Segments and not after the final ECS. + // However, some encoders may generate incorrect JPEGs with a final restart + // marker. That restart marker will be seen here instead of inside the ProcessSOS + // method, and is ignored as a harmless error. Restart markers have no extra data, + // so we check for this before we read the 16-bit length of the segment. + continue; + } - // Read the 16-bit length of the segment. The value includes the 2 bytes for the - // length itself, so we subtract 2 to get the number of remaining bytes. - this.ReadFull(this.Temp, 0, 2); - int remaining = (this.Temp[0] << 8) + this.Temp[1] - 2; - if (remaining < 0) - { - throw new ImageFormatException("Short segment length."); - } + // Read the 16-bit length of the segment. The value includes the 2 bytes for the + // length itself, so we subtract 2 to get the number of remaining bytes. + this.ReadFull(this.Temp, 0, 2); + int remaining = (this.Temp[0] << 8) + this.Temp[1] - 2; + if (remaining < 0) + { + throw new ImageFormatException("Short segment length."); + } - switch (marker) - { - case JpegConstants.Markers.SOF0: - case JpegConstants.Markers.SOF1: - case JpegConstants.Markers.SOF2: - this.IsProgressive = marker == JpegConstants.Markers.SOF2; - this.ProcessStartOfFrameMarker(remaining); - if (configOnly && this.isJfif) - { - return; - } + switch (marker) + { + case JpegConstants.Markers.SOF0: + case JpegConstants.Markers.SOF1: + case JpegConstants.Markers.SOF2: + this.IsProgressive = marker == JpegConstants.Markers.SOF2; + this.ProcessStartOfFrameMarker(remaining); + if (configOnly && this.isJfif) + { + return; + } - break; - case JpegConstants.Markers.DHT: - if (configOnly) - { - this.Skip(remaining); - } - else - { - this.ProcessDefineHuffmanTablesMarker(remaining); - } + break; + case JpegConstants.Markers.DHT: + if (configOnly) + { + this.Skip(remaining); + } + else + { + this.ProcessDefineHuffmanTablesMarker(remaining); + } - break; - case JpegConstants.Markers.DQT: - if (configOnly) - { - this.Skip(remaining); - } - else - { - this.ProcessDqt(remaining); - } + break; + case JpegConstants.Markers.DQT: + if (configOnly) + { + this.Skip(remaining); + } + else + { + this.ProcessDqt(remaining); + } - break; - case JpegConstants.Markers.SOS: - if (configOnly) - { - return; - } + break; + case JpegConstants.Markers.SOS: + if (configOnly) + { + return; + } - // when this is a progressive image this gets called a number of times - // need to know how many times this should be called in total. - this.ProcessStartOfScan(remaining); - if (!this.IsProgressive) - { - // if this is not a progressive image we can stop processing bytes as we now have the image data. - processBytes = false; - } + this.ProcessStartOfScan(remaining); - break; - case JpegConstants.Markers.DRI: - if (configOnly) - { - this.Skip(remaining); - } - else - { - this.ProcessDefineRestartIntervalMarker(remaining); - } + break; + case JpegConstants.Markers.DRI: + if (configOnly) + { + this.Skip(remaining); + } + else + { + this.ProcessDefineRestartIntervalMarker(remaining); + } - break; - case JpegConstants.Markers.APP0: - this.ProcessApplicationHeader(remaining); - break; - case JpegConstants.Markers.APP1: - this.ProcessApp1Marker(remaining, image); - break; - case JpegConstants.Markers.APP14: - this.ProcessApp14Marker(remaining); - break; - default: - if ((marker >= JpegConstants.Markers.APP0 && marker <= JpegConstants.Markers.APP15) - || marker == JpegConstants.Markers.COM) - { - this.Skip(remaining); - } - else if (marker < JpegConstants.Markers.SOF0) - { - // See Table B.1 "Marker code assignments". - throw new ImageFormatException("Unknown marker"); - } - else - { - throw new ImageFormatException("Unknown marker"); - } + break; + case JpegConstants.Markers.APP0: + this.ProcessApplicationHeader(remaining); + break; + case JpegConstants.Markers.APP1: + this.ProcessApp1Marker(remaining, image); + break; + case JpegConstants.Markers.APP14: + this.ProcessApp14Marker(remaining); + break; + default: + if ((marker >= JpegConstants.Markers.APP0 && marker <= JpegConstants.Markers.APP15) + || marker == JpegConstants.Markers.COM) + { + this.Skip(remaining); + } + else if (marker < JpegConstants.Markers.SOF0) + { + // See Table B.1 "Marker code assignments". + throw new ImageFormatException("Unknown marker"); + } + else + { + throw new ImageFormatException("Unknown marker"); + } - break; + break; + } + } + catch (EOFException) + { + // For non-progressive images this is a simple way to handle a missing EOI + // TODO: For progressive we still have to handle the exception within JpegScanDecoder to include last scan + break; } } diff --git a/tests/ImageSharp.Tests/FileTestBase.cs b/tests/ImageSharp.Tests/FileTestBase.cs index e9343b20dd..a2d7397dab 100644 --- a/tests/ImageSharp.Tests/FileTestBase.cs +++ b/tests/ImageSharp.Tests/FileTestBase.cs @@ -22,9 +22,11 @@ namespace ImageSharp.Tests // TestFile.Create(TestImages.Png.Pd), // Perf: Enable for local testing only // TestFile.Create(TestImages.Jpeg.Floorplan), // Perf: Enable for local testing only TestFile.Create(TestImages.Jpeg.Calliphora), + // TestFile.Create(TestImages.Jpeg.BadEOF), // Perf: Enable for local testing only + // TestFile.Create(TestImages.Jpeg.BadEOFProgressive), // TestFile.Create(TestImages.Jpeg.Ycck), // Perf: Enable for local testing only // TestFile.Create(TestImages.Jpeg.Cmyk), // Perf: Enable for local testing only - TestFile.Create(TestImages.Jpeg.Turtle), + // TestFile.Create(TestImages.Jpeg.Turtle), // Perf: Enable for local testing only // TestFile.Create(TestImages.Jpeg.Fb), // Perf: Enable for local testing only // TestFile.Create(TestImages.Jpeg.Progress), // Perf: Enable for local testing only // TestFile.Create(TestImages.Jpeg.GammaDalaiLamaGray), // Perf: Enable for local testing only @@ -33,7 +35,7 @@ namespace ImageSharp.Tests // TestFile.Create(TestImages.Png.Blur), // Perf: Enable for local testing only // TestFile.Create(TestImages.Png.Indexed), // Perf: Enable for local testing only TestFile.Create(TestImages.Png.Splash), - TestFile.Create(TestImages.Png.Powerpoint), + // TestFile.Create(TestImages.Png.Powerpoint), // Perf: Enable for local testing only // TestFile.Create(TestImages.Png.SplashInterlaced), // Perf: Enable for local testing only // TestFile.Create(TestImages.Png.Interlaced), // Perf: Enable for local testing only // TestFile.Create(TestImages.Png.Filter0), // Perf: Enable for local testing only diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 9e2d21a9bd..7948079596 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -52,6 +52,7 @@ namespace ImageSharp.Tests public const string Hiyamugi = "Jpg/Hiyamugi.jpg"; public const string BadEOF = "Jpg/badeof.jpg"; + public const string BadEOFProgressive = "Jpg/badeofprog.jpg"; public const string Snake = "Jpg/Snake.jpg"; public const string Lake = "Jpg/Lake.jpg"; diff --git a/tests/ImageSharp.Tests/TestImages/Formats/Jpg/badeofprog.jpg b/tests/ImageSharp.Tests/TestImages/Formats/Jpg/badeofprog.jpg new file mode 100644 index 0000000000..82a5707e05 --- /dev/null +++ b/tests/ImageSharp.Tests/TestImages/Formats/Jpg/badeofprog.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:22697fcab865bda38c760c6402382bffd47dccdef34d93ce5f8ae63f9b7918aa +size 67503 From adf70f8760fe08df159299a36e4b1dbe7910614a Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Sat, 21 Jan 2017 17:19:26 +0000 Subject: [PATCH 6/6] Revert "At least render something when progressive." This reverts commit d05b06bbbd7f746cb8fdf352d151a7e5fd4cb389. --- .../Components/Decoder/JpegScanDecoder.cs | 33 +- .../JpegDecoderCore.cs | 284 +++++++++--------- tests/ImageSharp.Tests/FileTestBase.cs | 6 +- tests/ImageSharp.Tests/TestImages.cs | 1 - .../TestImages/Formats/Jpg/badeofprog.jpg | 3 - 5 files changed, 150 insertions(+), 177 deletions(-) delete mode 100644 tests/ImageSharp.Tests/TestImages/Formats/Jpg/badeofprog.jpg diff --git a/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs b/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs index 09baf904b6..39ee6687b0 100644 --- a/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs +++ b/src/ImageSharp.Formats.Jpeg/Components/Decoder/JpegScanDecoder.cs @@ -356,21 +356,11 @@ namespace ImageSharp.Formats.Jpg throw new ImageFormatException("Excessive DC component"); } - // TODO: Handle the EOFException and piece together the final progressive scan. - try - { - int deltaDC = decoder.Bits.ReceiveExtend(value, decoder); - this.pointers.Dc[compIndex] += deltaDC; + int deltaDC = decoder.Bits.ReceiveExtend(value, decoder); + this.pointers.Dc[compIndex] += deltaDC; - // b[0] = dc[compIndex] << al; - Block8x8F.SetScalarAt(b, 0, this.pointers.Dc[compIndex] << this.al); - } - catch (JpegDecoderCore.EOFException) - { - // Do something clever here. I'm just undoing ReceiveExtend - decoder.Bits.UnreadBits += value; - decoder.Bits.Mask >>= value; - } + // b[0] = dc[compIndex] << al; + Block8x8F.SetScalarAt(b, 0, this.pointers.Dc[compIndex] << this.al); } if (zig <= this.zigEnd && this.eobRun > 0) @@ -393,20 +383,7 @@ namespace ImageSharp.Formats.Jpg break; } - int ac; // = decoder.Bits.ReceiveExtend(val1, decoder); - - // TODO: Handle the EOFException and piece together the final progressive scan. - try - { - ac = decoder.Bits.ReceiveExtend(val1, decoder); - } - catch (JpegDecoderCore.EOFException) - { - // Do something clever here. I'm just undoing ReceiveExtend - decoder.Bits.UnreadBits += val1; - decoder.Bits.Mask >>= val1; - break; - } + int ac = decoder.Bits.ReceiveExtend(val1, decoder); // b[Unzig[zig]] = ac << al; Block8x8F.SetScalarAt(b, this.pointers.Unzig[zig], ac << this.al); diff --git a/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs b/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs index 6ba578a3ae..2dd547c916 100644 --- a/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs +++ b/src/ImageSharp.Formats.Jpeg/JpegDecoderCore.cs @@ -186,162 +186,164 @@ namespace ImageSharp.Formats throw new ImageFormatException("Missing SOI marker."); } - while (true) + // Process the remaining segments until the End Of Image marker. + bool processBytes = true; + + // we can't currently short circute progressive images so don't try. + while (processBytes) { - try + this.ReadFull(this.Temp, 0, 2); + while (this.Temp[0] != 0xff) { - this.ReadFull(this.Temp, 0, 2); - while (this.Temp[0] != 0xff) - { - // Strictly speaking, this is a format error. However, libjpeg is - // liberal in what it accepts. As of version 9, next_marker in - // jdmarker.c treats this as a warning (JWRN_EXTRANEOUS_DATA) and - // continues to decode the stream. Even before next_marker sees - // extraneous data, jpeg_fill_bit_buffer in jdhuff.c reads as many - // bytes as it can, possibly past the end of a scan's data. It - // effectively puts back any markers that it overscanned (e.g. an - // "\xff\xd9" EOI marker), but it does not put back non-marker data, - // and thus it can silently ignore a small number of extraneous - // non-marker bytes before next_marker has a chance to see them (and - // print a warning). - // We are therefore also liberal in what we accept. Extraneous data - // is silently ignore - // This is similar to, but not exactly the same as, the restart - // mechanism within a scan (the RST[0-7] markers). - // Note that extraneous 0xff bytes in e.g. SOS data are escaped as - // "\xff\x00", and so are detected a little further down below. - this.Temp[0] = this.Temp[1]; - this.Temp[1] = this.ReadByte(); - } + // Strictly speaking, this is a format error. However, libjpeg is + // liberal in what it accepts. As of version 9, next_marker in + // jdmarker.c treats this as a warning (JWRN_EXTRANEOUS_DATA) and + // continues to decode the stream. Even before next_marker sees + // extraneous data, jpeg_fill_bit_buffer in jdhuff.c reads as many + // bytes as it can, possibly past the end of a scan's data. It + // effectively puts back any markers that it overscanned (e.g. an + // "\xff\xd9" EOI marker), but it does not put back non-marker data, + // and thus it can silently ignore a small number of extraneous + // non-marker bytes before next_marker has a chance to see them (and + // print a warning). + // We are therefore also liberal in what we accept. Extraneous data + // is silently ignore + // This is similar to, but not exactly the same as, the restart + // mechanism within a scan (the RST[0-7] markers). + // Note that extraneous 0xff bytes in e.g. SOS data are escaped as + // "\xff\x00", and so are detected a little further down below. + this.Temp[0] = this.Temp[1]; + this.Temp[1] = this.ReadByte(); + } - byte marker = this.Temp[1]; - if (marker == 0) - { - // Treat "\xff\x00" as extraneous data. - continue; - } + byte marker = this.Temp[1]; + if (marker == 0) + { + // Treat "\xff\x00" as extraneous data. + continue; + } - while (marker == 0xff) - { - // Section B.1.1.2 says, "Any marker may optionally be preceded by any - // number of fill bytes, which are bytes assigned code X'FF'". - marker = this.ReadByte(); - } + while (marker == 0xff) + { + // Section B.1.1.2 says, "Any marker may optionally be preceded by any + // number of fill bytes, which are bytes assigned code X'FF'". + marker = this.ReadByte(); + } - // End Of Image. - if (marker == JpegConstants.Markers.EOI) - { - break; - } + // End Of Image. + if (marker == JpegConstants.Markers.EOI) + { + break; + } - if (marker >= JpegConstants.Markers.RST0 && marker <= JpegConstants.Markers.RST7) - { - // Figures B.2 and B.16 of the specification suggest that restart markers should - // only occur between Entropy Coded Segments and not after the final ECS. - // However, some encoders may generate incorrect JPEGs with a final restart - // marker. That restart marker will be seen here instead of inside the ProcessSOS - // method, and is ignored as a harmless error. Restart markers have no extra data, - // so we check for this before we read the 16-bit length of the segment. - continue; - } + if (marker >= JpegConstants.Markers.RST0 && marker <= JpegConstants.Markers.RST7) + { + // Figures B.2 and B.16 of the specification suggest that restart markers should + // only occur between Entropy Coded Segments and not after the final ECS. + // However, some encoders may generate incorrect JPEGs with a final restart + // marker. That restart marker will be seen here instead of inside the ProcessSOS + // method, and is ignored as a harmless error. Restart markers have no extra data, + // so we check for this before we read the 16-bit length of the segment. + continue; + } - // Read the 16-bit length of the segment. The value includes the 2 bytes for the - // length itself, so we subtract 2 to get the number of remaining bytes. - this.ReadFull(this.Temp, 0, 2); - int remaining = (this.Temp[0] << 8) + this.Temp[1] - 2; - if (remaining < 0) - { - throw new ImageFormatException("Short segment length."); - } + // Read the 16-bit length of the segment. The value includes the 2 bytes for the + // length itself, so we subtract 2 to get the number of remaining bytes. + this.ReadFull(this.Temp, 0, 2); + int remaining = (this.Temp[0] << 8) + this.Temp[1] - 2; + if (remaining < 0) + { + throw new ImageFormatException("Short segment length."); + } - switch (marker) - { - case JpegConstants.Markers.SOF0: - case JpegConstants.Markers.SOF1: - case JpegConstants.Markers.SOF2: - this.IsProgressive = marker == JpegConstants.Markers.SOF2; - this.ProcessStartOfFrameMarker(remaining); - if (configOnly && this.isJfif) - { - return; - } + switch (marker) + { + case JpegConstants.Markers.SOF0: + case JpegConstants.Markers.SOF1: + case JpegConstants.Markers.SOF2: + this.IsProgressive = marker == JpegConstants.Markers.SOF2; + this.ProcessStartOfFrameMarker(remaining); + if (configOnly && this.isJfif) + { + return; + } - break; - case JpegConstants.Markers.DHT: - if (configOnly) - { - this.Skip(remaining); - } - else - { - this.ProcessDefineHuffmanTablesMarker(remaining); - } + break; + case JpegConstants.Markers.DHT: + if (configOnly) + { + this.Skip(remaining); + } + else + { + this.ProcessDefineHuffmanTablesMarker(remaining); + } - break; - case JpegConstants.Markers.DQT: - if (configOnly) - { - this.Skip(remaining); - } - else - { - this.ProcessDqt(remaining); - } + break; + case JpegConstants.Markers.DQT: + if (configOnly) + { + this.Skip(remaining); + } + else + { + this.ProcessDqt(remaining); + } - break; - case JpegConstants.Markers.SOS: - if (configOnly) - { - return; - } + break; + case JpegConstants.Markers.SOS: + if (configOnly) + { + return; + } - this.ProcessStartOfScan(remaining); + // when this is a progressive image this gets called a number of times + // need to know how many times this should be called in total. + this.ProcessStartOfScan(remaining); + if (!this.IsProgressive) + { + // if this is not a progressive image we can stop processing bytes as we now have the image data. + processBytes = false; + } - break; - case JpegConstants.Markers.DRI: - if (configOnly) - { - this.Skip(remaining); - } - else - { - this.ProcessDefineRestartIntervalMarker(remaining); - } + break; + case JpegConstants.Markers.DRI: + if (configOnly) + { + this.Skip(remaining); + } + else + { + this.ProcessDefineRestartIntervalMarker(remaining); + } - break; - case JpegConstants.Markers.APP0: - this.ProcessApplicationHeader(remaining); - break; - case JpegConstants.Markers.APP1: - this.ProcessApp1Marker(remaining, image); - break; - case JpegConstants.Markers.APP14: - this.ProcessApp14Marker(remaining); - break; - default: - if ((marker >= JpegConstants.Markers.APP0 && marker <= JpegConstants.Markers.APP15) - || marker == JpegConstants.Markers.COM) - { - this.Skip(remaining); - } - else if (marker < JpegConstants.Markers.SOF0) - { - // See Table B.1 "Marker code assignments". - throw new ImageFormatException("Unknown marker"); - } - else - { - throw new ImageFormatException("Unknown marker"); - } + break; + case JpegConstants.Markers.APP0: + this.ProcessApplicationHeader(remaining); + break; + case JpegConstants.Markers.APP1: + this.ProcessApp1Marker(remaining, image); + break; + case JpegConstants.Markers.APP14: + this.ProcessApp14Marker(remaining); + break; + default: + if ((marker >= JpegConstants.Markers.APP0 && marker <= JpegConstants.Markers.APP15) + || marker == JpegConstants.Markers.COM) + { + this.Skip(remaining); + } + else if (marker < JpegConstants.Markers.SOF0) + { + // See Table B.1 "Marker code assignments". + throw new ImageFormatException("Unknown marker"); + } + else + { + throw new ImageFormatException("Unknown marker"); + } - break; - } - } - catch (EOFException) - { - // For non-progressive images this is a simple way to handle a missing EOI - // TODO: For progressive we still have to handle the exception within JpegScanDecoder to include last scan - break; + break; } } diff --git a/tests/ImageSharp.Tests/FileTestBase.cs b/tests/ImageSharp.Tests/FileTestBase.cs index a2d7397dab..e9343b20dd 100644 --- a/tests/ImageSharp.Tests/FileTestBase.cs +++ b/tests/ImageSharp.Tests/FileTestBase.cs @@ -22,11 +22,9 @@ namespace ImageSharp.Tests // TestFile.Create(TestImages.Png.Pd), // Perf: Enable for local testing only // TestFile.Create(TestImages.Jpeg.Floorplan), // Perf: Enable for local testing only TestFile.Create(TestImages.Jpeg.Calliphora), - // TestFile.Create(TestImages.Jpeg.BadEOF), // Perf: Enable for local testing only - // TestFile.Create(TestImages.Jpeg.BadEOFProgressive), // TestFile.Create(TestImages.Jpeg.Ycck), // Perf: Enable for local testing only // TestFile.Create(TestImages.Jpeg.Cmyk), // Perf: Enable for local testing only - // TestFile.Create(TestImages.Jpeg.Turtle), // Perf: Enable for local testing only + TestFile.Create(TestImages.Jpeg.Turtle), // TestFile.Create(TestImages.Jpeg.Fb), // Perf: Enable for local testing only // TestFile.Create(TestImages.Jpeg.Progress), // Perf: Enable for local testing only // TestFile.Create(TestImages.Jpeg.GammaDalaiLamaGray), // Perf: Enable for local testing only @@ -35,7 +33,7 @@ namespace ImageSharp.Tests // TestFile.Create(TestImages.Png.Blur), // Perf: Enable for local testing only // TestFile.Create(TestImages.Png.Indexed), // Perf: Enable for local testing only TestFile.Create(TestImages.Png.Splash), - // TestFile.Create(TestImages.Png.Powerpoint), // Perf: Enable for local testing only + TestFile.Create(TestImages.Png.Powerpoint), // TestFile.Create(TestImages.Png.SplashInterlaced), // Perf: Enable for local testing only // TestFile.Create(TestImages.Png.Interlaced), // Perf: Enable for local testing only // TestFile.Create(TestImages.Png.Filter0), // Perf: Enable for local testing only diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 7948079596..9e2d21a9bd 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -52,7 +52,6 @@ namespace ImageSharp.Tests public const string Hiyamugi = "Jpg/Hiyamugi.jpg"; public const string BadEOF = "Jpg/badeof.jpg"; - public const string BadEOFProgressive = "Jpg/badeofprog.jpg"; public const string Snake = "Jpg/Snake.jpg"; public const string Lake = "Jpg/Lake.jpg"; diff --git a/tests/ImageSharp.Tests/TestImages/Formats/Jpg/badeofprog.jpg b/tests/ImageSharp.Tests/TestImages/Formats/Jpg/badeofprog.jpg deleted file mode 100644 index 82a5707e05..0000000000 --- a/tests/ImageSharp.Tests/TestImages/Formats/Jpg/badeofprog.jpg +++ /dev/null @@ -1,3 +0,0 @@ -version https://git-lfs.github.com/spec/v1 -oid sha256:22697fcab865bda38c760c6402382bffd47dccdef34d93ce5f8ae63f9b7918aa -size 67503