Remove Jpeg Huffman Decoder Bottleneck #643

JimBobSquarePants · 2018-07-01T10:25:29Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

Replaces the slow PdfJsScanDecoder implementation with a much faster implementation based on Stb_Image. Now it's no longer a bottleneck. Fixes #601

We're creeping slowly closer to System.Drawing now for jpeg decoding.

New Benchmarks

BenchmarkDotNet=v0.10.14, OS=Windows 10.0.17134
Intel Core i7-6600U CPU 2.60GHz (Skylake), 1 CPU, 4 logical and 2 physical cores
Frequency=2742191 Hz, Resolution=364.6719 ns, Timer=TSC
.NET Core SDK=2.1.300
  [Host]     : .NET Core 2.0.7 (CoreCLR 4.6.26328.01, CoreFX 4.6.26403.03), 64bit RyuJIT
  Job-CBPIPJ : .NET Framework 4.7.1 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.3110.0
  Job-FVNPWQ : .NET Core 2.0.7 (CoreCLR 4.6.26328.01, CoreFX 4.6.26403.03), 64bit RyuJIT

LaunchCount=1  TargetCount=3  WarmupCount=3

                           Method | Runtime |                    TestImage |      Mean |     Error |    StdDev | Scaled | ScaledSD |    Gen 0 | Allocated |
--------------------------------- |-------- |----------------------------- |----------:|----------:|----------:|-------:|---------:|---------:|----------:|
   'Decode Jpeg - System.Drawing' |     Clr |  Jpg/baseline/Calliphora.jpg |  6.786 ms |  2.364 ms | 0.1336 ms |   1.00 |     0.00 | 117.1875 | 254.47 KB |
       'Decode Jpeg - ImageSharp' |     Clr |  Jpg/baseline/Calliphora.jpg | 33.662 ms |  4.877 ms | 0.2755 ms |   4.96 |     0.09 |        - |  52.63 KB |
 'Decode Jpeg - ImageSharp PdfJs' |     Clr |  Jpg/baseline/Calliphora.jpg | 24.767 ms |  3.341 ms | 0.1888 ms |   3.65 |     0.06 |        - |  25.25 KB |
                                  |         |                              |           |           |           |        |          |          |           |
   'Decode Jpeg - System.Drawing' |    Core |  Jpg/baseline/Calliphora.jpg |  7.079 ms | 11.763 ms | 0.6646 ms |   1.00 |     0.00 | 117.1875 | 254.11 KB |
       'Decode Jpeg - ImageSharp' |    Core |  Jpg/baseline/Calliphora.jpg | 34.337 ms | 12.214 ms | 0.6901 ms |   4.88 |     0.36 |        - |  47.79 KB |
 'Decode Jpeg - ImageSharp PdfJs' |    Core |  Jpg/baseline/Calliphora.jpg | 26.188 ms |  1.067 ms | 0.0603 ms |   3.72 |     0.27 |        - |  21.71 KB |
                                  |         |                              |           |           |           |        |          |          |           |
   'Decode Jpeg - System.Drawing' |     Clr | Jpg/baseline/jpeg420exif.jpg | 20.881 ms | 21.458 ms | 1.2124 ms |   1.00 |     0.00 | 343.7500 | 757.89 KB |
       'Decode Jpeg - ImageSharp' |     Clr | Jpg/baseline/jpeg420exif.jpg | 80.619 ms |  6.260 ms | 0.3537 ms |   3.87 |     0.18 | 250.0000 | 564.28 KB |
 'Decode Jpeg - ImageSharp PdfJs' |     Clr | Jpg/baseline/jpeg420exif.jpg | 56.485 ms |  7.069 ms | 0.3994 ms |   2.71 |     0.13 | 250.0000 | 535.07 KB |
                                  |         |                              |           |           |           |        |          |          |           |
   'Decode Jpeg - System.Drawing' |    Core | Jpg/baseline/jpeg420exif.jpg | 17.653 ms |  4.603 ms | 0.2601 ms |   1.00 |     0.00 | 343.7500 | 757.04 KB |
       'Decode Jpeg - ImageSharp' |    Core | Jpg/baseline/jpeg420exif.jpg | 80.280 ms | 10.145 ms | 0.5732 ms |   4.55 |     0.06 | 250.0000 | 548.49 KB |
 'Decode Jpeg - ImageSharp PdfJs' |    Core | Jpg/baseline/jpeg420exif.jpg | 57.506 ms |  6.344 ms | 0.3584 ms |   3.26 |     0.04 | 250.0000 | 522.19 KB |

Before Performance Trace

After Performance Trace

I'm genuinely amazed I managed to pull this off!

codecov · 2018-07-01T11:22:06Z

Codecov Report

Merging #643 into master will increase coverage by 0.14%.
The diff coverage is 91.63%.

@@            Coverage Diff             @@
##           master     #643      +/-   ##
==========================================
+ Coverage   89.16%   89.31%   +0.14%     
==========================================
  Files         890      893       +3     
  Lines       37946    38020      +74     
  Branches     2661     2667       +6     
==========================================
+ Hits        33835    33956     +121     
+ Misses       3302     3267      -35     
+ Partials      809      797      -12

Impacted Files	Coverage Δ
tests/ImageSharp.Tests/TestImages.cs	`100% <ø> (ø)`	⬆️
...ts/Jpeg/PdfJsPort/Components/PdfJsHuffmanTables.cs	`100% <ø> (ø)`	⬆️
src/ImageSharp/Formats/Jpeg/JpegThrowHelper.cs	`0% <0%> (ø)`
...ts/Jpeg/PdfJsPort/Components/FixedInt32Buffer18.cs	`100% <100%> (ø)`
...arp/Formats/Jpeg/PdfJsPort/PdfJsJpegDecoderCore.cs	`83.01% <100%> (-1.25%)`	⬇️
...s/Jpeg/PdfJsPort/Components/FixedUInt32Buffer18.cs	`100% <100%> (ø)`
...s/Jpeg/PdfJsPort/Components/FixedInt16Buffer257.cs	`100% <100%> (ø)`
...ts/Jpeg/PdfJsPort/Components/FixedByteBuffer512.cs	`100% <100%> (ø)`
...ats/Jpeg/PdfJsPort/Components/PdfJsHuffmanTable.cs	`100% <100%> (ø)`	⬆️
...s/Jpeg/PdfJsPort/Components/PdfJsFrameComponent.cs	`95.74% <100%> (-2.04%)`	⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7bd82b...2277896. Read the comment docs.

antonfirsov

Good job, but I think the code is worth an other refactor/cleanup round before we move on.

It might be also worth to investigate if we can reduce the code duplications.

antonfirsov · 2018-07-01T20:56:40Z

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

@@ -9,14 +9,14 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.PdfJsPort.Components
    [StructLayout(LayoutKind.Sequential)]
    internal unsafe struct FixedInt64Buffer18


The type name should be renamed accordingly.

Good catch!

antonfirsov · 2018-07-01T21:00:58Z

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

+    /// <summary>
+    /// The collection of lookup tables used for fast AC entropy scan decoding.
+    /// </summary>
+    internal sealed class FastACTables : IDisposable


I wonder if we really need this class in it's current form. Either we should hide the Table property and encapsulate actual logic (initialization, retrieval of rows) inside the class, or we should drop it and define a helper method that creates Buffer2d<short>.

I like having a class but I think you're right about the tables property. I'll hide that away and introduce a GetTableSpan() method.

antonfirsov · 2018-07-01T21:06:18Z

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

+        private byte marker;
+        private bool badMarker;
+        private long markerPosition;
+        private int todo;


Would be nice to have better names or some comments about the semantics of less trivial variables.

antonfirsov · 2018-07-01T21:07:54Z

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

+                            PdfJsFrameComponent component = this.components[k];
+                            ref short blockDataRef = ref MemoryMarshal.GetReference(MemoryMarshal.Cast<Block8x8, short>(component.SpectralBlocks.Span));
+                            ref PdfJsHuffmanTable dcHuffmanTable = ref dcHuffmanTables[component.DCHuffmanTableId];
+                            ref PdfJsHuffmanTable acHuffmanTable = ref acHuffmanTables[component.ACHuffmanTableId];


acHuffmanTable and fastAC are unused

Sometimes I miss Resharper.

Got the unused, different line though so Github won't see it.

antonfirsov · 2018-07-01T21:10:03Z

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

+            }
+        }
+
+        private void DecodeBlockProgressiveAC(


The method is quite long, might be worth to refactor internal branches into separate methods.

I'll split out the Refinement part into a separate method.

antonfirsov · 2018-07-01T21:32:43Z

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

+        private int PeekBits() => (int)((this.codeBuffer >> (32 - FastBits)) & ((1 << FastBits) - 1));
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private uint LRot(uint x, int y) => (x << y) | (x >> (32 - y));


The method can be static

Good catch!

antonfirsov · 2018-07-02T09:43:10Z

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

+        /// Builds a lookup table for fast AC entropy scan decoding.
+        /// </summary>
+        /// <param name="index">The table index.</param>
+        private void BuildFastACTable(int index)


This should be probably a member of the FastACTables class.

dlemstra · 2018-07-02T09:49:32Z

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

@@ -9,14 +9,14 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.PdfJsPort.Components
    [StructLayout(LayoutKind.Sequential)]
    internal unsafe struct FixedInt16Buffer18


Should we rename the struct because it no longer is a short/ Int16?

antonfirsov

Everything looks good for a merge now!

antonfirsov · 2018-07-02T23:32:38Z

@JimBobSquarePants pushed some code simplification, and a benchmark to quickly test ParseStream(). There is no change in performance:

Baseline, Before:

                           Method | Runtime |                    TestImage |     Mean |     Error |    StdDev | Scaled | ScaledSD |    Gen 0 | Allocated |
--------------------------------- |-------- |----------------------------- |---------:|----------:|----------:|-------:|---------:|---------:|----------:|
            'System.Drawing FULL' |     Clr | Jpg/baseline/jpeg420exif.jpg | 18.35 ms |  8.224 ms | 0.4647 ms |   1.00 |     0.00 | 218.7500 | 757.88 KB |
 PdfJsJpegDecoderCore.ParseStream |     Clr | Jpg/baseline/jpeg420exif.jpg | 19.64 ms | 13.984 ms | 0.7901 ms |   1.07 |     0.04 |        - |     15 KB |
                                  |         |                              |          |           |           |        |          |          |           |
            'System.Drawing FULL' |    Core | Jpg/baseline/jpeg420exif.jpg | 18.36 ms |  4.017 ms | 0.2270 ms |   1.00 |     0.00 | 218.7500 | 757.04 KB |
 PdfJsJpegDecoderCore.ParseStream |    Core | Jpg/baseline/jpeg420exif.jpg | 21.21 ms | 10.237 ms | 0.5784 ms |   1.16 |     0.03 |        - |  14.84 KB |

Baseline, After:

                           Method | Runtime |                    TestImage |     Mean |    Error |    StdDev | Scaled | ScaledSD |    Gen 0 | Allocated |
--------------------------------- |-------- |----------------------------- |---------:|---------:|----------:|-------:|---------:|---------:|----------:|
            'System.Drawing FULL' |     Clr | Jpg/baseline/jpeg420exif.jpg | 18.38 ms | 7.007 ms | 0.3959 ms |   1.00 |     0.00 | 218.7500 | 757.88 KB |
 PdfJsJpegDecoderCore.ParseStream |     Clr | Jpg/baseline/jpeg420exif.jpg | 19.71 ms | 1.502 ms | 0.0849 ms |   1.07 |     0.02 |        - |     15 KB |
                                  |         |                              |          |          |           |        |          |          |           |
            'System.Drawing FULL' |    Core | Jpg/baseline/jpeg420exif.jpg | 17.86 ms | 1.077 ms | 0.0609 ms |   1.00 |     0.00 | 218.7500 | 757.04 KB |
 PdfJsJpegDecoderCore.ParseStream |    Core | Jpg/baseline/jpeg420exif.jpg | 20.84 ms | 7.756 ms | 0.4382 ms |   1.17 |     0.02 |        - |  14.88 KB |

JimBobSquarePants · 2018-07-03T00:13:59Z

Nice changes! That class looks very clean now and I like the inline and error helpers!

How the hell does S.D decode the full stream so damn quickly, it's infuriating!

antonfirsov · 2018-07-03T00:20:53Z

Don't ask how, but it's possible to optimize huffman decoding with SIMD.

JimBobSquarePants · 2018-07-03T01:39:06Z

I'm gonna merge this now to work on namespacing.

Remove Jpeg Huffman Decoder Bottleneck

JimBobSquarePants added 19 commits May 9, 2018 01:30

Begin porting stb_image

285f8dd

Minor cleanup

71903c5

Merge branch 'master' into js/new-jpeg-scan-decoder

5516bfb

Wire up huffman tables. (doesn't work)

c8a9b30

Meh

6d928f6

Baseline decoding seems to work

f064d95

Merge branch 'master' into js/new-jpeg-scan-decoder

31d5fa4

Update reference types

bdd3291

Merge branch 'master' into js/new-jpeg-scan-decoder

f597321

6/10 baseline images now pass.

9e59ac1

Merge branch 'master' into js/new-jpeg-scan-decoder

5716dbe

9/10 baseline now pass!

588e423

Can now decode baseline + progressive

681f99f

Split decode method.

c69c3ac

void methods

1a79cfb

Minor perf changes.

f2e05bf

Remove unused huffman table code.

d8f4b25

Delete unused code.

b5e2401

Update reference images from master

cfe5b5c

JimBobSquarePants added area:performance formats:jpeg labels Jul 1, 2018

JimBobSquarePants added this to the 1.0.0-beta5 milestone Jul 1, 2018

JimBobSquarePants requested review from tocsoft, antonfirsov and dlemstra July 1, 2018 10:25

antonfirsov reviewed Jul 1, 2018

View reviewed changes

Add descriptive comments, remove unused, and make method static.

43c818f

JimBobSquarePants added 5 commits July 2, 2018 11:44

Split progressive AC method and rename GrowBufferUnsafe

1e493ab

Fix updated struct name

8370d4f

Update name reference

6360a8a

Refactor FastACTables and reduce trivial duplication.

fcabcdd

private static ordering.

69f228b

antonfirsov reviewed Jul 2, 2018

View reviewed changes

dlemstra reviewed Jul 2, 2018

View reviewed changes

JimBobSquarePants added 3 commits July 2, 2018 22:22

Rename struct

9cb8c15

Update Huffman table property

bd79c84

Move method where it belongs.

d4fc8a0

antonfirsov approved these changes Jul 2, 2018

View reviewed changes

antonfirsov added 7 commits July 3, 2018 00:25

add regression test for #624

65a0504

ParseStream -only benchmark

c21e971

Introduce InliningOptions

d04611e

use JpegThrowHelper

468797f

separate Interleaved / Non-Interleaved code path

b9a6804

simplify + uniformize blockDataRef retrieval

ff1a24c

ScanDecoder: refactor parameters to members

df557c9

temporal vortex attacked again

2277896

JimBobSquarePants merged commit 68ca7ff into master Jul 3, 2018

JimBobSquarePants deleted the js/new-jpeg-scan-decoder branch July 3, 2018 01:40

antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019

Merge pull request SixLabors#643 from SixLabors/js/new-jpeg-scan-decoder

97d3895

Remove Jpeg Huffman Decoder Bottleneck

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Jpeg Huffman Decoder Bottleneck #643

Remove Jpeg Huffman Decoder Bottleneck #643

JimBobSquarePants commented Jul 1, 2018 •

edited

Loading

codecov bot commented Jul 1, 2018 •

edited

Loading

antonfirsov left a comment

antonfirsov Jul 1, 2018

JimBobSquarePants Jul 2, 2018

antonfirsov Jul 1, 2018

JimBobSquarePants Jul 2, 2018 •

edited

Loading

antonfirsov Jul 1, 2018

JimBobSquarePants Jul 2, 2018

antonfirsov Jul 1, 2018

JimBobSquarePants Jul 2, 2018

JimBobSquarePants Jul 2, 2018

antonfirsov Jul 1, 2018

JimBobSquarePants Jul 2, 2018

antonfirsov Jul 1, 2018

JimBobSquarePants Jul 2, 2018

antonfirsov Jul 2, 2018

dlemstra Jul 2, 2018 •

edited

Loading

antonfirsov left a comment

antonfirsov commented Jul 2, 2018

JimBobSquarePants commented Jul 3, 2018

antonfirsov commented Jul 3, 2018

JimBobSquarePants commented Jul 3, 2018

		@@ -9,14 +9,14 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.PdfJsPort.Components
		[StructLayout(LayoutKind.Sequential)]
		internal unsafe struct FixedInt64Buffer18

		@@ -9,14 +9,14 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.PdfJsPort.Components
		[StructLayout(LayoutKind.Sequential)]
		internal unsafe struct FixedInt16Buffer18

Remove Jpeg Huffman Decoder Bottleneck #643

Remove Jpeg Huffman Decoder Bottleneck #643

Conversation

JimBobSquarePants commented Jul 1, 2018 • edited Loading

Prerequisites

Description

codecov bot commented Jul 1, 2018 • edited Loading

Codecov Report

antonfirsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlemstra Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

antonfirsov left a comment

Choose a reason for hiding this comment

antonfirsov commented Jul 2, 2018

Baseline, Before:

Baseline, After:

JimBobSquarePants commented Jul 3, 2018

antonfirsov commented Jul 3, 2018

JimBobSquarePants commented Jul 3, 2018

JimBobSquarePants commented Jul 1, 2018 •

edited

Loading

codecov bot commented Jul 1, 2018 •

edited

Loading

JimBobSquarePants Jul 2, 2018 •

edited

Loading

dlemstra Jul 2, 2018 •

edited

Loading