-
-
Notifications
You must be signed in to change notification settings - Fork 852
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preserve Gif color palettes and deduplicate frame pixels. #2455
Changes from 28 commits
12da625
73067b9
43aaad1
d196b22
fc7219d
b126b77
14f9127
c8fe7f2
53510f0
18167b2
2c7123c
0d4a23a
eaf23dd
925bff0
e307da5
4f6a53c
a29b5fc
f33f67d
98ed0f1
c3fc062
ef08c82
4b15595
5416edb
c8f1f2c
949e6ad
4ef363d
f9e10ae
c0ca0cc
1a6b465
b48dbc5
c17eacf
b56633e
9ddebdd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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> | ||||||
|
@@ -159,6 +169,7 @@ public Image<TPixel> Decode<TPixel>(BufferedReadStream stream, CancellationToken | |||||
finally | ||||||
{ | ||||||
this.globalColorTable?.Dispose(); | ||||||
this.currentLocalColorTable?.Dispose(); | ||||||
} | ||||||
|
||||||
if (image is null) | ||||||
|
@@ -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) | ||||||
|
@@ -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; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Perf-wise So either
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
stream.Skip(1); // Skip the terminator. | ||||||
return; | ||||||
} | ||||||
|
@@ -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) | ||||||
{ | ||||||
|
@@ -448,7 +462,6 @@ private void ReadFrame<TPixel>(BufferedReadStream stream, ref Image<TPixel>? ima | |||||
} | ||||||
finally | ||||||
{ | ||||||
localColorTable?.Dispose(); | ||||||
indices?.Dispose(); | ||||||
} | ||||||
} | ||||||
|
@@ -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); | ||||||
|
||||||
|
@@ -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. | ||||||
|
@@ -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)); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just wanted to avoid the secondary indexer on the raw There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
} | ||||||
|
||||||
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; | ||||||
} | ||||||
|
@@ -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; | ||||||
} | ||||||
} | ||||||
} | ||||||
|
There was a problem hiding this comment.
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
andNumerics
. Originally public methods ofSimdUtils
were supposed to work regardless of the instructions andSimdUtils.HwIntrinsics
used to contain private implementation details for theSystem.Runtime.Intrinsics
branch. Would be nice to figure out new general rules later.There was a problem hiding this comment.
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.