From 582fa51229cc9a8c0a8f600546ea3d9f5acfaa7d Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Sun, 21 Nov 2021 05:53:36 +0300 Subject: [PATCH 1/4] Fixed jpeg component mcu size calculation bug --- .../Components/Decoder/HuffmanScanDecoder.cs | 1 + .../Components/Decoder/SpectralConverter.cs | 19 +++++++++++++ .../Decoder/SpectralConverter{TPixel}.cs | 28 ++++++++----------- 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs index bc9a53ea04..622657c48f 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs @@ -151,6 +151,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder if (this.componentsCount == this.frame.ComponentCount) { this.ParseBaselineDataInterleaved(); + this.spectralConverter.CommitConvertion(); } else { diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter.cs index e975b11fbb..aca9dc36b3 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter.cs @@ -13,6 +13,12 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder /// internal abstract class SpectralConverter { + /// + /// Gets a value indicating whether this converter has converted spectral + /// data of the current image or not. + /// + protected bool Converted { get; private set; } + /// /// Injects jpeg image decoding metadata. /// @@ -33,6 +39,19 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder /// public abstract void ConvertStrideBaseline(); + /// + /// Marks current converter state as 'converted'. + /// + /// + /// This must be called only for baseline interleaved jpeg's. + /// + public void CommitConvertion() + { + DebugGuard.IsFalse(this.Converted, nameof(this.Converted), $"{nameof(this.CommitConvertion)} must be called only once"); + + this.Converted = true; + } + /// /// Gets the color converter. /// diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs index ec7f3e5c30..2e965e0ac3 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs @@ -3,6 +3,7 @@ using System; using System.Buffers; +using System.Linq; using System.Numerics; using System.Threading; using SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder.ColorConverters; @@ -29,8 +30,6 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder private Buffer2D pixelBuffer; - private int blockRowsPerStep; - private int pixelRowsPerStep; private int pixelRowCounter; @@ -41,8 +40,6 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder this.cancellationToken = cancellationToken; } - private bool Converted => this.pixelRowCounter >= this.pixelBuffer.Height; - public Buffer2D GetPixelBuffer() { if (!this.Converted) @@ -52,7 +49,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder for (int step = 0; step < steps; step++) { this.cancellationToken.ThrowIfCancellationRequested(); - this.ConvertNextStride(step); + this.ConvertStride(step); } } @@ -65,18 +62,19 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder MemoryAllocator allocator = this.configuration.MemoryAllocator; // iteration data - IJpegComponent c0 = frame.Components[0]; + int majorBlockWidth = frame.Components.Max((component) => component.SizeInBlocks.Width); + int majorVerticalSamplingFactor = frame.Components.Max((component) => component.SamplingFactors.Height); const int blockPixelHeight = 8; - this.blockRowsPerStep = c0.SamplingFactors.Height; - this.pixelRowsPerStep = this.blockRowsPerStep * blockPixelHeight; + this.pixelRowsPerStep = majorVerticalSamplingFactor * blockPixelHeight; // pixel buffer for resulting image this.pixelBuffer = allocator.Allocate2D(frame.PixelWidth, frame.PixelHeight); this.paddedProxyPixelRow = allocator.Allocate(frame.PixelWidth + 3); // component processors from spectral to Rgba32 - var postProcessorBufferSize = new Size(c0.SizeInBlocks.Width * 8, this.pixelRowsPerStep); + const int blockPixelWidth = 8; + var postProcessorBufferSize = new Size(majorBlockWidth * blockPixelWidth, this.pixelRowsPerStep); this.componentProcessors = new JpegComponentPostProcessor[frame.Components.Length]; for (int i = 0; i < this.componentProcessors.Length; i++) { @@ -84,7 +82,6 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder } // single 'stride' rgba32 buffer for conversion between spectral and TPixel - // this.rgbaBuffer = allocator.Allocate(frame.PixelWidth); this.rgbBuffer = allocator.Allocate(frame.PixelWidth * 3); // color converter from Rgba32 to TPixel @@ -95,18 +92,17 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder public override void ConvertStrideBaseline() { // Convert next pixel stride using single spectral `stride' - // Note that zero passing eliminates the need of virtual call from JpegComponentPostProcessor - this.ConvertNextStride(spectralStep: 0); + // Note that zero passing eliminates the need of virtual call + // from JpegComponentPostProcessor + this.ConvertStride(spectralStep: 0); - // Clear spectral stride - this is VERY important as jpeg possibly won't fill entire buffer each stride - // Which leads to decoding artifacts - // Note that this code clears all buffers of the post processors, it's their responsibility to allocate only single stride foreach (JpegComponentPostProcessor cpp in this.componentProcessors) { cpp.ClearSpectralBuffers(); } } + /// public void Dispose() { if (this.componentProcessors != null) @@ -121,7 +117,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder this.paddedProxyPixelRow?.Dispose(); } - private void ConvertNextStride(int spectralStep) + private void ConvertStride(int spectralStep) { int maxY = Math.Min(this.pixelBuffer.Height, this.pixelRowCounter + this.pixelRowsPerStep); From e1c0a39c9d2c8ef712af09f7694068efd0d323bd Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Sun, 21 Nov 2021 06:25:31 +0300 Subject: [PATCH 2/4] Added bug proof image to jpeg decoder test suit --- tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs | 2 ++ .../JpegDecoderTests/DecodeBaselineJpeg_jpeg422.png | 3 +++ 2 files changed, 5 insertions(+) create mode 100644 tests/Images/External/ReferenceOutput/JpegDecoderTests/DecodeBaselineJpeg_jpeg422.png diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs index d12240cba3..ef817154d6 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs @@ -20,6 +20,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Jpg TestImages.Jpeg.Baseline.Jpeg420Small, TestImages.Jpeg.Issues.Fuzz.AccessViolationException922, TestImages.Jpeg.Baseline.Jpeg444, + TestImages.Jpeg.Baseline.Jpeg422, TestImages.Jpeg.Baseline.Bad.BadEOF, TestImages.Jpeg.Baseline.MultiScanBaselineCMYK, TestImages.Jpeg.Baseline.YcckSubsample1222, @@ -100,6 +101,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Jpg [TestImages.Jpeg.Baseline.Bad.BadEOF] = 0.38f / 100, [TestImages.Jpeg.Baseline.Bad.BadRST] = 0.0589f / 100, + [TestImages.Jpeg.Baseline.Jpeg422] = 0.0013f / 100, [TestImages.Jpeg.Baseline.Testorig420] = 0.38f / 100, [TestImages.Jpeg.Baseline.Jpeg420Small] = 0.287f / 100, [TestImages.Jpeg.Baseline.Turtle420] = 1.0f / 100, diff --git a/tests/Images/External/ReferenceOutput/JpegDecoderTests/DecodeBaselineJpeg_jpeg422.png b/tests/Images/External/ReferenceOutput/JpegDecoderTests/DecodeBaselineJpeg_jpeg422.png new file mode 100644 index 0000000000..018ecda7a5 --- /dev/null +++ b/tests/Images/External/ReferenceOutput/JpegDecoderTests/DecodeBaselineJpeg_jpeg422.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:733cc46271c4402974db2536a55e6ecae3110856df73031ca48dad03745d852d +size 35375 From 5e40977eb033fbc3296561f5ebf30cef2bff7467 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Sun, 21 Nov 2021 06:36:57 +0300 Subject: [PATCH 3/4] Fixed typo --- .../Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs | 2 +- .../Formats/Jpeg/Components/Decoder/SpectralConverter.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs index 622657c48f..ad09b50655 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs @@ -151,7 +151,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder if (this.componentsCount == this.frame.ComponentCount) { this.ParseBaselineDataInterleaved(); - this.spectralConverter.CommitConvertion(); + this.spectralConverter.CommitConversion(); } else { diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter.cs index aca9dc36b3..4e74f62269 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter.cs @@ -45,9 +45,9 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder /// /// This must be called only for baseline interleaved jpeg's. /// - public void CommitConvertion() + public void CommitConversion() { - DebugGuard.IsFalse(this.Converted, nameof(this.Converted), $"{nameof(this.CommitConvertion)} must be called only once"); + DebugGuard.IsFalse(this.Converted, nameof(this.Converted), $"{nameof(this.CommitConversion)} must be called only once"); this.Converted = true; } From 2a182d75637702ae8c633545dba1b7fe032bd8b2 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Sun, 21 Nov 2021 07:07:03 +0300 Subject: [PATCH 4/4] Updated benchmark results --- tests/ImageSharp.Benchmarks/Codecs/Jpeg/DecodeJpeg.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/ImageSharp.Benchmarks/Codecs/Jpeg/DecodeJpeg.cs b/tests/ImageSharp.Benchmarks/Codecs/Jpeg/DecodeJpeg.cs index 842eea685a..cb89f90829 100644 --- a/tests/ImageSharp.Benchmarks/Codecs/Jpeg/DecodeJpeg.cs +++ b/tests/ImageSharp.Benchmarks/Codecs/Jpeg/DecodeJpeg.cs @@ -74,8 +74,8 @@ Intel Core i7-6700K CPU 4.00GHz (Skylake), 1 CPU, 8 logical and 4 physical cores | Method | Mean | Error | StdDev | |------------------------------------ |----------:|----------:|----------:| -| 'Baseline 4:4:4 Interleaved' | 12.710 ms | 0.1120 ms | 0.0990 ms | -| 'Baseline 4:2:0 Interleaved' | 8.855 ms | 0.1447 ms | 0.1353 ms | -| 'Baseline 4:0:0 (grayscale)' | 1.660 ms | 0.0106 ms | 0.0088 ms | -| 'Progressive 4:2:0 Non-Interleaved' | 14.138 ms | 0.2797 ms | 0.3330 ms | +| 'Baseline 4:4:4 Interleaved' | 11.781 ms | 0.0737 ms | 0.0654 ms | +| 'Baseline 4:2:0 Interleaved' | 8.688 ms | 0.0345 ms | 0.0306 ms | +| 'Baseline 4:0:0 (grayscale)' | 1.643 ms | 0.0092 ms | 0.0086 ms | +| 'Progressive 4:2:0 Non-Interleaved' | 13.770 ms | 0.0928 ms | 0.0823 ms | */