Preserve Gif color palettes and deduplicate frame pixels. #2455

JimBobSquarePants · 2023-05-15T12:28:06Z

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 does a few things.

Reverts an issue introduced in Fix various Gif Decoder/Encoder behaviors. #2289 where a naive attempt to reduce encoded output led to a loss of image data when gifs have been previously optimized.
Introduces a new SIMD accelerated (Avx2, Sse2, Arm64) algorithm that deduplifies the frame index buffers following quantization to allow much better encoder compression.
Introduces a necessary breaking change to the gif metadata and encoder (Next release will be v3.1) that allows taking advantage of these optimizations. Without these changes some gifs could encode at nearly 7x the input size using the default settings.

Fixes #635
Fixes #2450
Fixes #2198
Touches #2404

The new deduplifier does a comparison of the quantized pixel indexes using the previous and current frame. If duplicates are detected in the current frame, then the indices are replaced with the indez of the background or transparent color. Given the increased repeats this enables better compression.

This functionality is limited to gifs encoded using global color palettes as we would require extra work to detect duplicates in local color tables and produce a map of index offsets which could be cumbersome (not to say this might come in a following PR in the future) .

antonfirsov · 2023-05-16T13:40:05Z

An API diff would be nice (for both the PR and the release notes) to make it obvious what specific API-s are we breaking.

JimBobSquarePants · 2023-05-16T22:52:33Z

An API diff would be nice (for both the PR and the release notes) to make it obvious what specific API-s are we breaking.

Yep agreed. I'll get on that.

Update. Here's the API diff.

I've removed the XXColorTableLength properties from each metadata type as they're not trackable with new property values..
the IQuantizer property of QuantizingImageEncoder is now nullable. All encoders have been patched to ensure a default quantizer.

/// <summary>
/// Provides Gif specific metadata information for the image.
/// </summary>
public class GifMetadata : IDeepCloneable
{
-   /// <summary>
-   /// Gets or sets the length of the global color table if present.
-   /// </summary>
-   public int GlobalColorTableLength { get; set; }

+  /// <summary>
+  /// Gets or sets the global color table, if any.
+  /// </summary>
+  public ReadOnlyMemory<Color>? GlobalColorTable { get; set; }

+  /// <summary>
+  /// Gets or sets the index at the <see cref="GlobalColorTable"/> for the background color.
+  /// The background color is the color used for those pixels on the screen that are not covered by an image.
+  /// </summary>
+    public byte BackgroundColor { get; set; }
}

/// <summary>
/// Provides Gif specific metadata information for the image frame.
/// </summary>
public class GifFrameMetadata : IDeepCloneable
{
-    /// <summary>
-    /// Gets or sets the length of the color table.
-    /// If not 0, then this field indicates the maximum number of colors to use when quantizing the
-    /// image frame.
-    /// </summary>
-    public int ColorTableLength { get; set; }

+   /// <summary>
+   /// Gets or sets the local color table, if any.
+   /// </summary>
+   public ReadOnlyMemory<Color>? LocalColorTable { get; set; }

+   /// <summary>
+   /// Gets or sets a value indicating whether the frame has transparency
+   /// </summary>
+   public bool HasTransparency { get; set; }

+   /// <summary>
+   /// Gets or sets the transparency index.
+   /// When <see cref="HasTransparency"/> is set to <see langword="true"/> this value indicates the index within
+   /// the color palette at which the transparent color is located.
+   /// </summary>
+   public byte TransparencyIndex { get; set; }
}

/// <summary>
/// Acts as a base class for all image encoders that allow color palette generation via quantization.
/// </summary>
public abstract class QuantizingImageEncoder : ImageEncoder
{
-   /// <summary>
-   /// Gets the quantizer used to generate the color palette.
-   /// </summary>
-   public IQuantizer Quantizer { get; init; } = KnownQuantizers.Octree;

+   /// <summary>
+   /// Gets the quantizer used to generate the color palette.
+   /// </summary>
+   public IQuantizer? Quantizer { get; init; }
}

JimBobSquarePants · 2023-06-09T12:57:23Z

@SixLabors/core Would it be possible to get some eyes on this please?

src/ImageSharp/Formats/Gif/GifEncoderCore.cs

gfoidl · 2023-07-17T08:07:41Z

@JimBobSquarePants FYI regarding

I've put that on my todo list, and hope that next week I can give this a stab.

Got a business work-item which I put above this one in my todo 😉
Hoping that I can look into this at the end of the week.

JimBobSquarePants · 2023-07-18T06:27:53Z

@JimBobSquarePants FYI regarding

I've put that on my todo list, and hope that next week I can give this a stab.

Go a business work-item which I put above this one in my todo 😉 Hoping that I can look into this at the end of the week.

Thanks.. I think I've simplified it enough to maybe make it possible.

JimBobSquarePants · 2023-07-21T10:21:51Z

253 of the changed files here are reference images. If I could get someone to have a look at the actual code, I'd really appreciate it.

gfoidl · 2023-07-27T19:30:08Z

My take on vectorizing the TrimTransparentPixels is in #2500 (to be merged into this PR / branch).

Hm, I remembered that movemask isn't the fastest, and ptest (TestZ in .NET-terms) is faster but current benchmarks didn't prove this, also Intel's instruction table didn't show any benefit in terms of latency or throughput. Thus simplified that check.

Vectorize TrimTransparentPixels in GifEncoderCore

JimBobSquarePants · 2023-08-17T09:41:06Z

@SixLabors/core Can I please get some eyes on this? I'd like to keep moving as I have a huge backlog of work to do.

antonfirsov

A couple of recommendations and a few general comments.

antonfirsov · 2023-08-19T14:41:15Z

src/ImageSharp/Formats/Png/Filters/PaethFilter.cs

@@ -50,7 +50,7 @@ public static void Decode(Span<byte> scanline, Span<byte> previousScanline, int
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]


Not related to the PR, but I wonder if our heavy usage of AggressiveInlining has any benefits, especially now that we target modern runtimes only.

Yeah, we need to start reviewing this. Need good tooling to inspect the codegen though.

antonfirsov · 2023-08-19T14:45:35Z

src/ImageSharp/Common/Helpers/SimdUtils.HwIntrinsics.cs

+            Vector128<short> signedMask = AdvSimd.ShiftRightArithmetic(mask.AsInt16(), 7);
+            return AdvSimd.BitwiseSelect(signedMask, right.AsInt16(), left.AsInt16()).AsByte();


Again, nothing to do with the PR but it's a bit messy now that it's unclear for a caller which instruction set is supported in various methods on SimdUtils and Numerics. Originally public methods of SimdUtils were supposed to work regardless of the instructions and SimdUtils.HwIntrinsics used to contain private implementation details for the System.Runtime.Intrinsics branch. Would be nice to figure out new general rules later.

This has become a bit of a dumping ground over the years. My plan for v4 is a mass simplification of intrinsics utilizing things like Vector128<T> to allow us to implement better cross chipset approaches. We can do a tidy up at the same time.

antonfirsov · 2023-08-19T14:51:18Z

src/ImageSharp/Formats/Gif/GifMetadata.cs

    /// </summary>
-    public int GlobalColorTableLength { get; set; }
+    public byte BackgroundColor { get; set; }


Would be BackgroundColorIndex a better name?

Yes, much better, will change.

antonfirsov · 2023-08-19T15:06:02Z

src/ImageSharp/Formats/Gif/GifEncoderCore.cs

+    {
+        this.WriteGraphicalControlExtension(metadata, transparencyIndex, stream);
+
+        Buffer2DRegion<byte> region = ((IPixelSource)quantized).PixelBuffer.GetRegion();


Nit: do we need the region here? I don't see us slicing down the full buffer.

WriteImageData requires a region as we slice down the buffer in subsequent frames. I'll rewrite it though, so it accepts a buffer and a rectangle.

antonfirsov · 2023-08-19T15:14:26Z

src/ImageSharp/Formats/Gif/GifDecoderCore.cs

@@ -332,7 +344,7 @@ private void ReadApplicationExtension(BufferedReadStream stream)
            if (subBlockSize == GifConstants.NetscapeLoopingSubBlockSize)
            {
                stream.Read(this.buffer.Span, 0, GifConstants.NetscapeLoopingSubBlockSize);
-                this.gifMetadata!.RepeatCount = GifNetscapeLoopingApplicationExtension.Parse(this.buffer.Span.Slice(1)).RepeatCount;
+                this.gifMetadata!.RepeatCount = GifNetscapeLoopingApplicationExtension.Parse(this.buffer.Span[1..]).RepeatCount;


I don't think this is a critical path to care about if Slice is faster. Simpler looking code is better.

antonfirsov · 2023-08-19T15:17:16Z

src/ImageSharp/Formats/Gif/GifDecoderCore.cs

+            ref Rgb24 localBase = ref MemoryMarshal.GetReference(MemoryMarshal.Cast<byte, Rgb24>(this.currentLocalColorTable!.GetSpan()[..this.currentLocalColorTableSize]));
+            for (int i = 0; i < colorTable.Length; i++)
+            {
+                colorTable[i] = new Color(Unsafe.Add(ref localBase, (uint)i));


Since this is not a critical path, I wouldn't go unsafe here and just use span indexers.

I just wanted to avoid the secondary indexer on the raw Rgb24 span.

But is there a measurable perf gain from avoiding the indexer here? This method is only called once per frame on a relatively small (<256) set of data. On the other hand, each usage of Unsafe reduces readability and may introduce a serious bug in case we accidentally create a buffer overflow as we refactor the code in the future. IMO each usage of Unsafe should be always justified (=hottest hot path) instead of being the default way of indexing things.

antonfirsov · 2023-08-19T15:21:00Z

src/ImageSharp/Formats/Gif/GifMetadata.cs

-    /// Gets or sets the length of the global color table if present.
+    /// Gets or sets the global color table, if any.
+    /// </summary>
+    public ReadOnlyMemory<Color>? GlobalColorTable { get; set; }


Is color resolution different from 24 bits ever expected according to the gif standard? If not, wouldn't it be better to go with ReadOnlyMemory<Rgb24> and avoiding the conversions?

It doesn't and I did consider using Rgb24 originally but thought that Color gave us greater portability on the API surface. For example, someone could now easily create a pallet quantizer from this color table.

If we think such use-cases will be common it might be ok, but then I would extend the xml doc describing that GlobalColorTable is 24bit.

For me it still feels better to expose the raw gif data as-is (communicating the limitations on the API surface), and provide helper API-s to make work with PaletteQuantizer easier.

I think a comment is probably best. When I started to convert, I realized while it meant the decoder was simpler, encoding then required an interim allocation of a Color[] to supply to the palette quantizer which got messy.

antonfirsov · 2023-08-19T15:33:39Z

src/ImageSharp/Processing/Processors/Quantization/EuclideanPixelMap{TPixel}.cs

+        if (this.transparentIndex >= 0 && rgba == default)
+        {
+            // We have explicit instructions. No need to search.
+            index = this.transparentIndex;
+            this.cache.Add(rgba, (byte)index);
+
+            if (index >= 0 && index < this.Palette.Length)
+            {
+                match = Unsafe.Add(ref paletteRef, (uint)index);
+            }
+            else
+            {
+                Unsafe.SkipInit(out TPixel pixel);
+                pixel.FromScaledVector4(Vector4.Zero);
+                match = pixel;
+            }
+
+            return index;
+        }


This is now penalizing all quantizers and decoders while only gif is using transparentIndex. Can't we keep the transparency detection logic in GifDecoderCore?

I couldn't figure out a way to do it. Given that the slow code is actually the loop through the palette I thought this addition check outside the loop would be ok.

I actually think Bmp will end up using it eventually when we finally expose a decoded palette there.

"Slow" actually means we run it every time a new value is added to ColorDistanceCache which should be still quite common for an image with a wide original palette. I also don't see an easy way to optimize this, we should probably introduce multiple variants of EuclideanPixelMap<TPixel> and PalettQuantizer<TPixel> in the future.

I've been thinking about this and compared to the actual iteration the >=0 check should be barely detectable. The comparison operator won't even kick in.

The comparison is indeed cheap, the problem is not with that. We have a lot of code added to the method which may lead to more frequent L1 instruction cache misses. Branches can also hurt CPU pipelining.

Since GetQuantizedColor is being called per-pixel, the method should be as slim as possible, ideally containing no branching except the pixelmap lookup. We should be able to achieve this (and greatly reduce the memory footprint for pixel types without an alpha!) by separating different OctreeQuantizer<T> setups to separate classes.

If we could look into this together with optimization opportunities in ErrorDitherer, we could achieve significant perf improvements in the quantization space, and consequentially in lossless encoders. Not a small amount of work though.

Yeah true. I'll not touch this until you have a chance to make changes. We can look at greater optimization opportunities for v4

antonfirsov · 2023-08-22T20:45:40Z

src/ImageSharp/Processing/Processors/Quantization/EuclideanPixelMap{TPixel}.cs

+            index = this.transparentIndex;
+            this.cache.Add(rgba, (byte)index);
+
+            if (index >= 0 && index < this.Palette.Length)


Given that in this branch index == this.transparentIndex >= 0, the index >= 0 condition seems to be redundant.

Regarding the index < this.Palette.Length condition: shouldn't we make this an invariant rule of PaletteQuantizer & EuclideanPixelMap instead, and Guard it in the constructor and SetTransparentIndex?

This would simplify this code a lot, making it also faster.

I'm not sure I follow. If you have time, could you push to the branch?

Will do it later this week.

Ah yeah, that's much better! Not sure what I was thinking when I wrote mine! 🤣

JimBobSquarePants · 2023-09-11T10:52:09Z

@antonfirsov I think I've covered everything now.

antonfirsov

LGTM, thanks for the patience!

Preserve color palettes and deduplicate frame pixels.

12da625

JimBobSquarePants added this to the v3.1.0 milestone May 15, 2023

JimBobSquarePants mentioned this pull request May 15, 2023

Loading a GIF results in a bigger file #2198

Closed

4 tasks

Fix NRE

73067b9

JimBobSquarePants added bug formats:gif breaking Signifies a binary breaking change. arch:x64 labels May 15, 2023

JimBobSquarePants added 2 commits May 17, 2023 14:48

Make color tables mutable

43aaad1

Merge branch 'main' into js/gif-fixes

d196b22

JimBobSquarePants added arch:arm64 and removed arch:x64 labels May 18, 2023

Add quantizer property tests

fc7219d

JimBobSquarePants marked this pull request as ready for review May 19, 2023 14:05

JimBobSquarePants changed the title ~~WIP. Preserve color palettes and deduplicate frame pixels.~~ Preserve color palettes and deduplicate frame pixels. May 19, 2023

JimBobSquarePants requested review from brianpopow, antonfirsov, tocsoft and dlemstra May 19, 2023 14:16

JimBobSquarePants added 3 commits May 20, 2023 00:38

Use background index as fallback during dedup.

b126b77

Merge branch 'main' into js/gif-fixes

14f9127

Merge branch 'main' into js/gif-fixes

c8fe7f2

This was referenced Jun 9, 2023

V12: Fix ImageSharp3 performance issue umbraco/Umbraco-CMS#14364

Merged

Resizing animated GIFs causes quality loss after 3.0.0 #2450

Closed

JimBobSquarePants added 3 commits June 10, 2023 15:42

Add explicit tests for #2450

53510f0

Merge branch 'main' into js/gif-fixes

18167b2

Merge branch 'main' into js/gif-fixes

2c7123c

Update src/ImageSharp/Formats/Gif/GifEncoderCore.cs

c3fc062

JimBobSquarePants commented Jul 9, 2023

View reviewed changes

src/ImageSharp/Formats/Gif/GifEncoderCore.cs Outdated Show resolved Hide resolved

JimBobSquarePants added 2 commits July 9, 2023 21:51

Update src/ImageSharp/Formats/Gif/GifEncoderCore.cs

ef08c82

Merge branch 'main' into js/gif-fixes

4b15595

Vectorize TrimTransparentPixels in GifEncoderCore

5416edb

gfoidl mentioned this pull request Jul 27, 2023

Vectorize TrimTransparentPixels in GifEncoderCore #2500

Merged

4 tasks

gfoidl and others added 4 commits July 27, 2023 22:26

Merge pull request #2500 from gfoidl/git-transparency-simd

949e6ad

Vectorize TrimTransparentPixels in GifEncoderCore

Only compare a subset of frames.

4ef363d

Merge branch 'main' into js/gif-fixes

f9e10ae

Merge branch 'main' into js/gif-fixes

c0ca0cc

antonfirsov reviewed Aug 19, 2023

View reviewed changes

Cleanup based on feedback

1a6b465

JimBobSquarePants mentioned this pull request Aug 22, 2023

APNG support #2511

Merged

4 tasks

antonfirsov reviewed Aug 22, 2023

View reviewed changes

antonfirsov and others added 3 commits September 2, 2023 00:00

if transparencyIndex is outside the palette, ignore it

b48dbc5

Merge branch 'main' into js/gif-fixes

c17eacf

Review fixes

b56633e

JimBobSquarePants requested a review from antonfirsov September 11, 2023 10:52

Merge branch 'main' into js/gif-fixes

9ddebdd

antonfirsov approved these changes Sep 15, 2023

View reviewed changes

JimBobSquarePants merged commit 945a83f into main Sep 18, 2023
9 checks passed

JimBobSquarePants deleted the js/gif-fixes branch September 18, 2023 04:59

antonfirsov mentioned this pull request Oct 4, 2023

Expose non-nullable configuration to remove AOT limiting null check #2514

Merged

4 tasks

		@@ -50,7 +50,7 @@ public static void Decode(Span<byte> scanline, Span<byte> previousScanline, int
		}

		[MethodImpl(MethodImplOptions.AggressiveInlining)]

		Vector128<short> signedMask = AdvSimd.ShiftRightArithmetic(mask.AsInt16(), 7);
		return AdvSimd.BitwiseSelect(signedMask, right.AsInt16(), left.AsInt16()).AsByte();

Preserve Gif color palettes and deduplicate frame pixels. #2455

Preserve Gif color palettes and deduplicate frame pixels. #2455

Conversation

JimBobSquarePants commented May 15, 2023 • edited

Prerequisites

Description

antonfirsov commented May 16, 2023

JimBobSquarePants commented May 16, 2023 • edited

JimBobSquarePants commented Jun 9, 2023

gfoidl commented Jul 17, 2023 • edited

JimBobSquarePants commented Jul 18, 2023

JimBobSquarePants commented Jul 21, 2023

gfoidl commented Jul 27, 2023

JimBobSquarePants commented Aug 17, 2023

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

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 Aug 21, 2023 • edited

Choose a reason for hiding this comment

antonfirsov Aug 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Aug 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Aug 22, 2023 • edited

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

antonfirsov Aug 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Aug 22, 2023 • edited

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

JimBobSquarePants commented Sep 11, 2023

antonfirsov left a comment

Choose a reason for hiding this comment

JimBobSquarePants commented May 15, 2023 •

edited

JimBobSquarePants commented May 16, 2023 •

edited

gfoidl commented Jul 17, 2023 •

edited

JimBobSquarePants Aug 21, 2023 •

edited

antonfirsov Aug 19, 2023 •

edited

antonfirsov Aug 22, 2023 •

edited

antonfirsov Aug 22, 2023 •

edited

antonfirsov Aug 29, 2023 •

edited

antonfirsov Aug 22, 2023 •

edited