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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Remove Unsafe usage from GifDecoderCore and optimize loops #2851

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Remove Unsafe usage from GifDecoderCore and optimize loops #2851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dee312d
GifDecode benchmark: use large GIF
antonfirsov Dec 16, 2024
b38146d
GifDecoder: get rid of Unsafe
antonfirsov Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 19 additions & 20 deletions src/ImageSharp/Formats/Gif/GifDecoderCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,27 +423,22 @@ private void ReadFrame<TPixel>(BufferedReadStream stream, ref Image<TPixel>? ima

// Determine the color table for this frame. If there is a local one, use it otherwise use the global color table.
bool hasLocalColorTable = this.imageDescriptor.LocalColorTableFlag;

Span<byte> rawColorTable = default;
if (hasLocalColorTable)
{
// 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]);
}

Span<byte> rawColorTable = default;
if (hasLocalColorTable)
{
rawColorTable = this.currentLocalColorTable!.GetSpan()[..this.currentLocalColorTableSize];
rawColorTable = this.currentLocalColorTable!.GetSpan()[..length];
}
else if (this.globalColorTable != null)
{
rawColorTable = this.globalColorTable.GetSpan();
}

ReadOnlySpan<Rgb24> colorTable = MemoryMarshal.Cast<byte, Rgb24>(rawColorTable);
this.ReadFrameColors(stream, ref image, ref previousFrame, colorTable, this.imageDescriptor);
this.ReadFrameColors(stream, ref image, ref previousFrame, colorTable);

// Skip any remaining blocks
SkipBlock(stream);
Expand All @@ -457,15 +452,14 @@ private void ReadFrame<TPixel>(BufferedReadStream stream, ref Image<TPixel>? ima
/// <param name="image">The image to decode the information to.</param>
/// <param name="previousFrame">The previous frame.</param>
/// <param name="colorTable">The color table containing the available colors.</param>
/// <param name="descriptor">The <see cref="GifImageDescriptor"/></param>
private void ReadFrameColors<TPixel>(
BufferedReadStream stream,
ref Image<TPixel>? image,
ref ImageFrame<TPixel>? previousFrame,
ReadOnlySpan<Rgb24> colorTable,
in GifImageDescriptor descriptor)
ReadOnlySpan<Rgb24> colorTable)
where TPixel : unmanaged, IPixel<TPixel>
{
GifImageDescriptor descriptor = this.imageDescriptor;
int imageWidth = this.logicalScreenDescriptor.Width;
int imageHeight = this.logicalScreenDescriptor.Height;
bool transFlag = this.graphicsControlExtension.TransparencyFlag;
Expand Down Expand Up @@ -528,7 +522,6 @@ private void ReadFrameColors<TPixel>(
// However we have images that exceed this that can be decoded by other libraries. #1530
using IMemoryOwner<byte> indicesRowOwner = this.memoryAllocator.Allocate<byte>(descriptor.Width);
Span<byte> indicesRow = indicesRowOwner.Memory.Span;
ref byte indicesRowRef = ref MemoryMarshal.GetReference(indicesRow);

int minCodeSize = stream.ReadByte();
if (LzwDecoder.IsValidMinCodeSize(minCodeSize))
Expand Down Expand Up @@ -572,30 +565,36 @@ private void ReadFrameColors<TPixel>(
}

lzwDecoder.DecodePixelRow(indicesRow);
ref TPixel rowRef = ref MemoryMarshal.GetReference(imageFrame.PixelBuffer.DangerousGetRowSpan(writeY));

// #403 The left + width value can be larger than the image width
int maxX = Math.Min(descriptorRight, imageWidth);
Span<TPixel> row = imageFrame.PixelBuffer.DangerousGetRowSpan(writeY);

// Take the descriptorLeft..maxX slice of the row, so the loop can be simplified.
row = row[descriptorLeft..maxX];
Copy link
Member

Choose a reason for hiding this comment

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

This is so much neater. I'm sure there's other places we do bounds based looping in decoders like this.


if (!transFlag)
{
// #403 The left + width value can be larger than the image width
for (int x = descriptorLeft; x < descriptorRight && x < imageWidth; x++)
for (int x = 0; x < row.Length; x++)
{
int index = Numerics.Clamp(Unsafe.Add(ref indicesRowRef, (uint)(x - descriptorLeft)), 0, colorTableMaxIdx);
Unsafe.Add(ref rowRef, (uint)x) = TPixel.FromRgb24(colorTable[index]);
int index = indicesRow[x];
index = Numerics.Clamp(index, 0, colorTableMaxIdx);
row[x] = TPixel.FromRgb24(colorTable[index]);
}
}
else
{
for (int x = descriptorLeft; x < descriptorRight && x < imageWidth; x++)
for (int x = 0; x < row.Length; x++)
{
int index = Unsafe.Add(ref indicesRowRef, (uint)(x - descriptorLeft));
int index = indicesRow[x];

// Treat any out of bounds values as transparent.
if (index > colorTableMaxIdx || index == transIndex)
{
continue;
}

Unsafe.Add(ref rowRef, (uint)x) = TPixel.FromRgb24(colorTable[index]);
row[x] = TPixel.FromRgb24(colorTable[index]);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/ImageSharp.Benchmarks/Codecs/Gif/DecodeGif.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public void ReadImages()
}
}

[Params(TestImages.Gif.Rings)]
[Params(TestImages.Gif.Cheers)]
public string TestImage { get; set; }

[Benchmark(Baseline = true, Description = "System.Drawing Gif")]
Expand Down
Loading