Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve Gif color palettes and deduplicate frame pixels. #2455

Merged
merged 33 commits into from
Sep 18, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
12da625
Preserve color palettes and deduplicate frame pixels.
JimBobSquarePants May 15, 2023
73067b9
Fix NRE
JimBobSquarePants May 15, 2023
43aaad1
Make color tables mutable
JimBobSquarePants May 17, 2023
d196b22
Merge branch 'main' into js/gif-fixes
JimBobSquarePants May 18, 2023
fc7219d
Add quantizer property tests
JimBobSquarePants May 19, 2023
b126b77
Use background index as fallback during dedup.
JimBobSquarePants May 19, 2023
14f9127
Merge branch 'main' into js/gif-fixes
JimBobSquarePants May 22, 2023
c8fe7f2
Merge branch 'main' into js/gif-fixes
JimBobSquarePants Jun 8, 2023
53510f0
Add explicit tests for #2450
JimBobSquarePants Jun 10, 2023
18167b2
Merge branch 'main' into js/gif-fixes
JimBobSquarePants Jun 10, 2023
2c7123c
Merge branch 'main' into js/gif-fixes
JimBobSquarePants Jun 12, 2023
0d4a23a
Merge branch 'main' into js/gif-fixes
JimBobSquarePants Jun 12, 2023
eaf23dd
Merge branch 'main' into js/gif-fixes
JimBobSquarePants Jun 13, 2023
925bff0
Merge branch 'main' into js/gif-fixes
JimBobSquarePants Jun 25, 2023
e307da5
Update SimdUtils.HwIntrinsics.cs
JimBobSquarePants Jun 29, 2023
4f6a53c
Compare to zero
JimBobSquarePants Jun 29, 2023
a29b5fc
Trim buffer to minimum region.
JimBobSquarePants Jul 5, 2023
f33f67d
Merge branch 'main' into js/gif-fixes
JimBobSquarePants Jul 5, 2023
98ed0f1
Refactor and fix gif encoder
JimBobSquarePants Jul 9, 2023
c3fc062
Update src/ImageSharp/Formats/Gif/GifEncoderCore.cs
JimBobSquarePants Jul 9, 2023
ef08c82
Update src/ImageSharp/Formats/Gif/GifEncoderCore.cs
JimBobSquarePants Jul 9, 2023
4b15595
Merge branch 'main' into js/gif-fixes
JimBobSquarePants Jul 9, 2023
5416edb
Vectorize TrimTransparentPixels in GifEncoderCore
gfoidl Jul 27, 2023
c8f1f2c
Simplified check if there are any non-equal bytes
gfoidl Jul 27, 2023
949e6ad
Merge pull request #2500 from gfoidl/git-transparency-simd
JimBobSquarePants Aug 9, 2023
4ef363d
Only compare a subset of frames.
JimBobSquarePants Aug 10, 2023
f9e10ae
Merge branch 'main' into js/gif-fixes
JimBobSquarePants Aug 10, 2023
c0ca0cc
Merge branch 'main' into js/gif-fixes
JimBobSquarePants Aug 17, 2023
1a6b465
Cleanup based on feedback
JimBobSquarePants Aug 21, 2023
b48dbc5
if transparencyIndex is outside the palette, ignore it
antonfirsov Sep 1, 2023
c17eacf
Merge branch 'main' into js/gif-fixes
JimBobSquarePants Sep 2, 2023
b56633e
Review fixes
JimBobSquarePants Sep 11, 2023
9ddebdd
Merge branch 'main' into js/gif-fixes
JimBobSquarePants Sep 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/ImageSharp/Common/Helpers/SimdUtils.HwIntrinsics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,33 @@ private static Vector256<byte> ShuffleMaskShiftAlpha() => Vector256.Create((byte
return Avx.Subtract(c, Avx.Multiply(a, b));
}

/// <summary>
/// Blend packed 8-bit integers from <paramref name="left"/> and <paramref name="right"/> using <paramref name="mask"/>.
/// The high bit of each corresponding <paramref name="mask"/> byte determines the selection.
/// If the high bit is set the element of <paramref name="left"/> is selected.
/// The element of <paramref name="right"/> is selected otherwise.
/// </summary>
/// <param name="left">The left vector.</param>
/// <param name="right">The right vector.</param>
/// <param name="mask">The mask vector.</param>
/// <returns>The <see cref="Vector256{T}"/>.</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static Vector128<byte> BlendVariable(in Vector128<byte> left, in Vector128<byte> right, in Vector128<byte> mask)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static Vector128<byte> BlendVariable(in Vector128<byte> left, in Vector128<byte> right, in Vector128<byte> mask)
public static Vector128<byte> BlendVariable(Vector128<byte> left, Vector128<byte> right, Vector128<byte> mask)

in modifier is equivalent to readonly ref, so the vectors can't be passed in registers, rather they need to be passed via memory which is slower.
Due the AggressiveInlining this method is likely to be inlined, so the JIT can figure out that no passing via memory is needed and can it can pass these vectors via registers -- but: that's not guaranteed, and the JIT has more (unnecessary) work to do.

I see this similar to a method like int Add(int a, int b); where one can assume that a, and b are passed by value and via registers at cpu-level. No need to put some emphasis on the read-only nature of a, and b. Some goes for vectors.


Side note: starting with .NET 7 this could just be Vector128.ConditionalSelect (I know we have a .NET 6 target, but I mention it for completness)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AH yes, I keep meaning to make this change!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

{
if (Sse41.IsSupported)
{
return Sse41.BlendVariable(left, right, mask);
}
else if (Sse2.IsSupported)
{
return Sse2.Or(Sse2.And(right, mask), Sse2.AndNot(mask, left));
}

// Use a signed shift right to create a mask with the sign bit.
Vector128<short> signedMask = AdvSimd.ShiftRightArithmetic(mask.AsInt16(), 7);
return AdvSimd.BitwiseSelect(signedMask, right.AsInt16(), left.AsInt16()).AsByte();
Comment on lines +655 to +656
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/// <summary>
/// <see cref="ByteToNormalizedFloat"/> as many elements as possible, slicing them down (keeping the remainder).
/// </summary>
Expand Down
6 changes: 6 additions & 0 deletions src/ImageSharp/Formats/Bmp/BmpEncoder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Six Labors Split License.

using SixLabors.ImageSharp.Advanced;
using SixLabors.ImageSharp.Processing;

namespace SixLabors.ImageSharp.Formats.Bmp;

Expand All @@ -10,6 +11,11 @@ namespace SixLabors.ImageSharp.Formats.Bmp;
/// </summary>
public sealed class BmpEncoder : QuantizingImageEncoder
{
/// <summary>
/// Initializes a new instance of the <see cref="BmpEncoder"/> class.
/// </summary>
public BmpEncoder() => this.Quantizer = KnownQuantizers.Octree;

/// <summary>
/// Gets the number of bits per pixel.
/// </summary>
Expand Down
3 changes: 2 additions & 1 deletion src/ImageSharp/Formats/Bmp/BmpEncoderCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using SixLabors.ImageSharp.Memory;
using SixLabors.ImageSharp.Metadata;
using SixLabors.ImageSharp.PixelFormats;
using SixLabors.ImageSharp.Processing;
using SixLabors.ImageSharp.Processing.Processors.Quantization;

namespace SixLabors.ImageSharp.Formats.Bmp;
Expand Down Expand Up @@ -100,7 +101,7 @@ public BmpEncoderCore(BmpEncoder encoder, MemoryAllocator memoryAllocator)
{
this.memoryAllocator = memoryAllocator;
this.bitsPerPixel = encoder.BitsPerPixel;
this.quantizer = encoder.Quantizer;
this.quantizer = encoder.Quantizer ?? KnownQuantizers.Octree;
this.pixelSamplingStrategy = encoder.PixelSamplingStrategy;
this.infoHeaderType = encoder.SupportTransparency ? BmpInfoHeaderType.WinVersion4 : BmpInfoHeaderType.WinVersion3;
}
Expand Down
67 changes: 51 additions & 16 deletions src/ImageSharp/Formats/Gif/GifDecoderCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ internal sealed class GifDecoderCore : IImageDecoderInternals
/// </summary>
private IMemoryOwner<byte>? globalColorTable;

/// <summary>
/// The current local color table.
/// </summary>
private IMemoryOwner<byte>? currentLocalColorTable;

/// <summary>
/// Gets the size in bytes of the current local color table.
/// </summary>
private int currentLocalColorTableSize;

/// <summary>
/// The area to restore.
/// </summary>
Expand Down Expand Up @@ -159,6 +169,7 @@ public Image<TPixel> Decode<TPixel>(BufferedReadStream stream, CancellationToken
finally
{
this.globalColorTable?.Dispose();
this.currentLocalColorTable?.Dispose();
}

if (image is null)
Expand Down Expand Up @@ -229,6 +240,7 @@ public ImageInfo Identify(BufferedReadStream stream, CancellationToken cancellat
finally
{
this.globalColorTable?.Dispose();
this.currentLocalColorTable?.Dispose();
}

if (this.logicalScreenDescriptor.Width == 0 && this.logicalScreenDescriptor.Height == 0)
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.gifMetadata!.RepeatCount = GifNetscapeLoopingApplicationExtension.Parse(this.buffer.Span[1..]).RepeatCount;
this.gifMetadata!.RepeatCount = GifNetscapeLoopingApplicationExtension.Parse(this.buffer.Span.Slice(1)).RepeatCount;

Perf-wise Slice is better than the range-operator. Unfortunately Roslyn compiles "bad" IL here, which the JIT doesn't optimize away.

So either

  • keep Slice
  • wait for Roslyn to be fixed (which takes some years already for that issue)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just taking the hit here as we have elsewhere. It's annoying but worth the readability improvements.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

stream.Skip(1); // Skip the terminator.
return;
}
Expand Down Expand Up @@ -415,25 +427,27 @@ private void ReadFrame<TPixel>(BufferedReadStream stream, ref Image<TPixel>? ima
{
this.ReadImageDescriptor(stream);

IMemoryOwner<byte>? localColorTable = null;
Buffer2D<byte>? indices = null;
try
{
// Determine the color table for this frame. If there is a local one, use it otherwise use the global color table.
if (this.imageDescriptor.LocalColorTableFlag)
bool hasLocalColorTable = this.imageDescriptor.LocalColorTableFlag;

if (hasLocalColorTable)
{
int length = this.imageDescriptor.LocalColorTableSize * 3;
localColorTable = this.configuration.MemoryAllocator.Allocate<byte>(length, AllocationOptions.Clean);
stream.Read(localColorTable.GetSpan());
// Read and store the local color table. We allocate the maximum possible size and slice to match.
int length = this.currentLocalColorTableSize = this.imageDescriptor.LocalColorTableSize * 3;
this.currentLocalColorTable ??= this.configuration.MemoryAllocator.Allocate<byte>(768, AllocationOptions.Clean);
stream.Read(this.currentLocalColorTable.GetSpan()[..length]);
}

indices = this.configuration.MemoryAllocator.Allocate2D<byte>(this.imageDescriptor.Width, this.imageDescriptor.Height, AllocationOptions.Clean);
this.ReadFrameIndices(stream, indices);

Span<byte> rawColorTable = default;
if (localColorTable != null)
if (hasLocalColorTable)
{
rawColorTable = localColorTable.GetSpan();
rawColorTable = this.currentLocalColorTable!.GetSpan()[..this.currentLocalColorTableSize];
}
else if (this.globalColorTable != null)
{
Expand All @@ -448,7 +462,6 @@ private void ReadFrame<TPixel>(BufferedReadStream stream, ref Image<TPixel>? ima
}
finally
{
localColorTable?.Dispose();
indices?.Dispose();
}
}
Expand Down Expand Up @@ -509,7 +522,10 @@ private void ReadFrameColors<TPixel>(ref Image<TPixel>? image, ref ImageFrame<TP
prevFrame = previousFrame;
}

currentFrame = image!.Frames.CreateFrame();
// We create a clone of the frame and add it.
// We will overpaint the difference of pixels on the current frame to create a complete image.
// This ensures that we have enough pixel data to process without distortion. #2450
currentFrame = image!.Frames.AddFrame(previousFrame);

this.SetFrameMetadata(currentFrame.Metadata);

Expand Down Expand Up @@ -631,7 +647,10 @@ private void ReadFrameMetadata(BufferedReadStream stream, List<ImageFrameMetadat
// Skip the color table for this frame if local.
if (this.imageDescriptor.LocalColorTableFlag)
{
stream.Skip(this.imageDescriptor.LocalColorTableSize * 3);
// Read and store the local color table. We allocate the maximum possible size and slice to match.
int length = this.currentLocalColorTableSize = this.imageDescriptor.LocalColorTableSize * 3;
this.currentLocalColorTable ??= this.configuration.MemoryAllocator.Allocate<byte>(768, AllocationOptions.Clean);
stream.Read(this.currentLocalColorTable.GetSpan()[..length]);
}

// Skip the frame indices. Pixels length + mincode size.
Expand Down Expand Up @@ -682,21 +701,30 @@ private void SetFrameMetadata(ImageFrameMetadata metadata)
{
GifFrameMetadata gifMeta = metadata.GetGifMetadata();
gifMeta.ColorTableMode = GifColorTableMode.Global;
gifMeta.ColorTableLength = this.logicalScreenDescriptor.GlobalColorTableSize;
}

if (this.imageDescriptor.LocalColorTableFlag
&& this.imageDescriptor.LocalColorTableSize > 0)
{
GifFrameMetadata gifMeta = metadata.GetGifMetadata();
gifMeta.ColorTableMode = GifColorTableMode.Local;
gifMeta.ColorTableLength = this.imageDescriptor.LocalColorTableSize;

Color[] colorTable = new Color[this.imageDescriptor.LocalColorTableSize];
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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

gifMeta.LocalColorTable = colorTable;
}

// Graphics control extensions is optional.
if (this.graphicsControlExtension != default)
{
GifFrameMetadata gifMeta = metadata.GetGifMetadata();
gifMeta.HasTransparency = this.graphicsControlExtension.TransparencyFlag;
gifMeta.TransparencyIndex = this.graphicsControlExtension.TransparencyIndex;
gifMeta.FrameDelay = this.graphicsControlExtension.DelayTime;
gifMeta.DisposalMethod = this.graphicsControlExtension.DisposalMethod;
}
Expand Down Expand Up @@ -751,14 +779,21 @@ private void ReadLogicalScreenDescriptorAndGlobalColorTable(BufferedReadStream s
if (this.logicalScreenDescriptor.GlobalColorTableFlag)
{
int globalColorTableLength = this.logicalScreenDescriptor.GlobalColorTableSize * 3;
this.gifMetadata.GlobalColorTableLength = globalColorTableLength;

if (globalColorTableLength > 0)
{
this.globalColorTable = this.memoryAllocator.Allocate<byte>(globalColorTableLength, AllocationOptions.Clean);

// Read the global color table data from the stream
// Read the global color table data from the stream and preserve it in the gif metadata
stream.Read(this.globalColorTable.GetSpan());

Color[] colorTable = new Color[this.logicalScreenDescriptor.GlobalColorTableSize];
ref Rgb24 globalBase = ref MemoryMarshal.GetReference(MemoryMarshal.Cast<byte, Rgb24>(this.globalColorTable.GetSpan()));
for (int i = 0; i < colorTable.Length; i++)
{
colorTable[i] = new Color(Unsafe.Add(ref globalBase, (uint)i));
}

this.gifMetadata.GlobalColorTable = colorTable;
}
}
}
Expand Down