Add support for decoding jpeg's with arithmetic coding #2073

brianpopow · 2022-03-24T16:30:43Z

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 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

This PR adds support for decoding jpeg's with arithmetic coding.

gfoidl

I always enjoy reading such fine code.
At the same time I'm sorry for so much comments.

src/ImageSharp/Formats/Jpeg/Components/Decoder/ArithmeticScanDecoder.cs

gfoidl · 2022-03-25T09:17:37Z

src/ImageSharp/Formats/Jpeg/Components/Decoder/ArithmeticScanDecoder.cs

+                                    return;
+                                }
+
+                                int blockCol = (mcuCol * h) + x;


You could hoist mcuCol * h before the loop for (int x ..., as it's invariant for this loop and I don't expect JIT to hoist it for us (especially on older runtimes).

gfoidl · 2022-03-25T09:19:58Z

src/ImageSharp/Formats/Jpeg/Components/Decoder/ArithmeticScanDecoder.cs

+
+                                this.DecodeBlockBaseline(
+                                    component,
+                                    ref Unsafe.Add(ref blockRef, blockCol),


Suggested change

ref Unsafe.Add(ref blockRef, blockCol),

ref Unsafe.Add(ref blockRef, (uint)blockCol),

to avoid the sign-extending move.

@gfoidl I dont understand the suggestion: How can I use uint here, when Unsafe.Add expects int?

Here it's for the overload Unsafe.Add<T>(ref T source, nuint elementOffset) and uses the implicit conversion from uint -> nuint.
So the long form of the suggestion is

Suggested change

ref Unsafe.Add(ref blockRef, blockCol),

ref Unsafe.Add(ref blockRef, (nuint)(uint)blockCol),

but as the compiler does the second conversion for us, I've shortened it to

Suggested change

ref Unsafe.Add(ref blockRef, blockCol),

ref Unsafe.Add(ref blockRef, (uint)blockCol),

In essence, and to avoid the movsxd (the sign extending move), we need to convert the int to any of IntPtr / nint or UIntPtr / nuint. As first step here is to prove to the JIT that there's only positive numbers by the uint-cast, which elides the movsxd emitting.
The actual target type (nint or nuint respectively there IntPtr base variants) doesn't really matter then.

Thank you for the explanation. Now I see why it does not work: The uint overload is only available on .net6.0, so cant use that here.

Ah, sorry, didn't check that. The use (nint)(uint) for the cast.

src/ImageSharp/Formats/Jpeg/Components/Decoder/ArithmeticScanDecoder.cs

gfoidl · 2022-03-25T09:56:19Z

src/ImageSharp/Formats/Jpeg/Components/Decoder/ArithmeticScanDecoder.cs

+                    v = -v;
+                }
+
+                Unsafe.Add(ref destinationRef, ZigZag.TransposingOrder[k]) = (short)v;


Suggested change

Unsafe.Add(ref destinationRef, ZigZag.TransposingOrder[k]) = (short)v;

Unsafe.Add(ref destinationRef, (uint)ZigZag.TransposingOrder[k]) = (short)v;

gfoidl · 2022-03-25T10:12:51Z

src/ImageSharp/Formats/Jpeg/Components/Decoder/ArithmeticStatistics.cs

+
+namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
+{
+    internal class ArithmeticStatistics


Can this be written as

using System.Diagnostics; using System.Runtime.CompilerServices; internal unsafe struct ArithmeticStatistics { private fixed byte statistics[256]; public ArithmeticStatistics(bool dc, int identifier) { this.IsDcStatistics = dc; this.Identifier = identifier; } public bool IsDcStatistics { get; private set; } public int Identifier { get; private set; } public ref byte GetReference() => ref this.statistics[0]; public ref byte GetReference(int offset) { Debug.Assert(offset < 256); return ref this.statistics[(uint)offset]; } public void Reset() { Unsafe.InitBlockUnaligned(ref this.GetReference(), 0x00, (uint)(this.IsDcStatistics ? 64 : 256)); } }

?

avoids the additional allocation for each stats-object

downside: size is fixed to 256, so potentially the list will be larger

codegen is better than with the class especially for GetReference and Reset (which are the hottest methods here?)

gfoidl · 2022-03-25T10:15:26Z

src/ImageSharp/Formats/Jpeg/Components/Decoder/IJpegComponent.cs

+        /// <summary>
+        /// Gets the component id.
+        /// </summary>
+        public byte Id { get; }


The existing interface members don't have the access modifier specified.
It's not strictly needed, but C# allows this (since C# 8 I think).

At least it should be consistent withing this file.

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

gfoidl · 2022-03-25T10:24:45Z

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

@@ -1055,18 +1127,18 @@ private void ProcessStartOfFrameMarker(BufferedReadStream stream, int remaining,
            // 2 byte: Width
            int frameWidth = (this.temp[3] << 8) | this.temp[4];

-            // Validate: width/height > 0 (they are upper-bounded by 2 byte max value so no need to check that)
+            // Validate: width/height > 0 (they are upper-bounded by 2 byte max value so no need to check that).
            if (frameHeight == 0 || frameWidth == 0)


Suggested change

if (frameHeight == 0 || frameWidth == 0)

if ((frameHeight | frameWidth) == 0)

produces less code and avoids a branch.

Note: current JIT will do this optimizaiton for use, but older runtimes miss that optimization.

br3aker · 2022-03-25T11:50:03Z

I know that I'm asking for too much but can we postpone this PR from merging for a bit?
There's so much virtual types existing in current jpeg decoder and new ones from this PR which already led to explicit downcasting. I'm working on the downscaling decoding for jpeg which would also need some sort of a virtuality for both the huffman decoder and component post processing which combined can cause a mess IMO.

I'll push some fixes on your branch if you don't mind.

EDIT:
There's actually a lot of other fixes I've planned to do during refactoring PR so I'll fix everything later, sorry for interfering.

brianpopow · 2022-03-25T18:05:08Z

@gfoidl thanks for your review, its always appreciated!

@br3aker:

I know that I'm asking for too much but can we postpone this PR from merging for a bit?

Its not a problem, there is no need to rush this PR.

I'll push some fixes on your branch if you don't mind.

I would not mind, if you push directly to this branch, your help is very welcome, but I dont think you have permission to do so.
I am pretty sure its somehow possible to give you those write access to this branch, but I did not see how at the moment.
You will have to create a regular PR with this branch as the target branch instead of main.

br3aker · 2022-03-25T18:22:45Z

Code itself is very good, it's just those tiny little spots I'd want to fix but they do not block merging so it's okay. I will create a PR with general refactoring of the decoder part.

Co-authored-by: Günther Foidl <gue@korporal.at>

…ated in LoadTables

# Conflicts: # src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

br3aker

Looks good overall but please revert component type to JpegComponent in JpegDecoderCore.

br3aker · 2022-04-21T21:41:34Z

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

+        /// <summary>
+        /// The arithmetic decoding tables.
+        /// </summary>
+        private List<ArithmeticDecodingTable> arithmeticDecodingTables;
+
+        /// <summary>
+        /// The restart interval.
+        /// </summary>
+        private int? resetInterval;


I don't like that this arithmetic coding specific property is leaked in the 'master' class, same goes for reset interval which should only be used inside scanDecoder but it's a broad question for some further refactoring - I'll work on it right after scaled decoding PR.

br3aker · 2022-04-21T21:48:43Z

src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegFrame.cs

-        public JpegComponent[] Components { get; set; }
+        public IJpegComponent[] Components { get; set; }


As far as I remember, IJpegComponent interface is used in tests for libjpeg output comparison but I think we shouldn't use this interface everywhere - this leads to virtual calls for every component field getter .

br3aker · 2022-05-01T08:08:52Z

@brianpopow hi! Anything blocking this PR from being merged?

brianpopow · 2022-05-01T10:44:00Z

@brianpopow hi! Anything blocking this PR from being merged?

@br3aker: I think this can be merged now.
@JimBobSquarePants do you have any objection on merging this?

JimBobSquarePants · 2022-05-01T10:54:07Z

Nope. Rock on

brianpopow added 2 commits March 24, 2022 14:57

Add support for decoding jpeg's with arithmetic coding

2c4b4cf

Add tests for arithmetic coding

c988208

brianpopow added the formats:jpeg label Mar 24, 2022

Merge branch 'main' into bp/arithmeticcoding

7e0540f

gfoidl reviewed Mar 25, 2022

View reviewed changes

JimBobSquarePants added this to the 3.0.0 milestone Mar 27, 2022

brianpopow and others added 6 commits March 30, 2022 13:18

Apply suggestions from code review

6a281b6

Co-authored-by: Günther Foidl <gue@korporal.at>

Fix build issue

0154e4e

Fix issue with jpeg's embedded in tiff's: scan decoder is already cre…

52c1f20

…ated in LoadTables

Additional review changes

0bc0cd8

Revert fixedBin back to byte[]

7371ed0

Merge remote-tracking branch 'origin/main' into bp/arithmeticcoding

af70b1d

# Conflicts: # src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

brianpopow requested a review from a team April 21, 2022 10:17

br3aker suggested changes Apr 21, 2022

View reviewed changes

IJpegComponent -> JpegComponent

0a08940

br3aker approved these changes Apr 22, 2022

View reviewed changes

Merge branch 'main' into bp/arithmeticcoding

1d37679

br3aker mentioned this pull request May 1, 2022

Jpeg downscaling decoding #2076

Merged

9 tasks

Merge branch 'main' into bp/arithmeticcoding

5ed690d

brianpopow merged commit 3e7b2c9 into main May 1, 2022

brianpopow deleted the bp/arithmeticcoding branch May 1, 2022 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for decoding jpeg's with arithmetic coding #2073

Add support for decoding jpeg's with arithmetic coding #2073

brianpopow commented Mar 24, 2022

gfoidl left a comment

gfoidl Mar 25, 2022

gfoidl Mar 25, 2022

brianpopow Mar 30, 2022

gfoidl Mar 30, 2022

brianpopow Mar 30, 2022

gfoidl Mar 30, 2022

gfoidl Mar 25, 2022

gfoidl Mar 25, 2022 •

edited

gfoidl Mar 25, 2022

gfoidl Mar 25, 2022

br3aker commented Mar 25, 2022 •

edited

brianpopow commented Mar 25, 2022

br3aker commented Mar 25, 2022

br3aker left a comment

br3aker Apr 21, 2022 •

edited

br3aker Apr 21, 2022

br3aker commented May 1, 2022

brianpopow commented May 1, 2022

JimBobSquarePants commented May 1, 2022

	ref Unsafe.Add(ref blockRef, blockCol),
	ref Unsafe.Add(ref blockRef, (uint)blockCol),

	ref Unsafe.Add(ref blockRef, blockCol),
	ref Unsafe.Add(ref blockRef, (nuint)(uint)blockCol),

	Unsafe.Add(ref destinationRef, ZigZag.TransposingOrder[k]) = (short)v;
	Unsafe.Add(ref destinationRef, (uint)ZigZag.TransposingOrder[k]) = (short)v;

	if (frameHeight == 0 \|\| frameWidth == 0)
	if ((frameHeight \| frameWidth) == 0)

		public JpegComponent[] Components { get; set; }
		public IJpegComponent[] Components { get; set; }

Add support for decoding jpeg's with arithmetic coding #2073

Add support for decoding jpeg's with arithmetic coding #2073

Conversation

brianpopow commented Mar 24, 2022

Prerequisites

Description

gfoidl 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

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

gfoidl Mar 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

br3aker commented Mar 25, 2022 • edited

brianpopow commented Mar 25, 2022

br3aker commented Mar 25, 2022

br3aker left a comment

Choose a reason for hiding this comment

br3aker Apr 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

br3aker commented May 1, 2022

brianpopow commented May 1, 2022

JimBobSquarePants commented May 1, 2022

gfoidl Mar 25, 2022 •

edited

br3aker commented Mar 25, 2022 •

edited

br3aker Apr 21, 2022 •

edited