From 46f26ba95ef6e66b92fb666737df80168a2ce627 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Mon, 7 Nov 2022 17:39:37 +1000 Subject: [PATCH] Correctly read/write graphics control extension --- src/ImageSharp/Formats/Gif/GifDecoderCore.cs | 32 ++++++++++------- src/ImageSharp/Formats/Gif/GifEncoderCore.cs | 24 +++++++++---- .../Formats/Gif/MetadataExtensions.cs | 22 +++++++++--- .../Sections/GifGraphicControlExtension.cs | 29 ++++++++++++++- src/ImageSharp/Metadata/ImageFrameMetadata.cs | 31 +++++++++++++++- .../Formats/Gif/GifEncoderTests.cs | 35 +++++++++++++++++++ tests/ImageSharp.Tests/TestImages.cs | 1 + tests/Images/Input/Gif/issues/issue_2288.gif | 3 ++ 8 files changed, 152 insertions(+), 25 deletions(-) create mode 100644 tests/Images/Input/Gif/issues/issue_2288.gif diff --git a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs index ef29863d4f..e67c599e3f 100644 --- a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs @@ -125,6 +125,10 @@ internal sealed class GifDecoderCore : IImageDecoderInternals } this.ReadFrame(ref image, ref previousFrame); + + // Reset per-frame state. + this.imageDescriptor = default; + this.graphicsControlExtension = default; } else if (nextFlag == GifConstants.ExtensionIntroducer) { @@ -464,7 +468,7 @@ internal sealed class GifDecoderCore : IImageDecoderInternals image = new Image(this.configuration, imageWidth, imageHeight, this.metadata); } - this.SetFrameMetadata(image.Frames.RootFrame.Metadata); + this.SetFrameMetadata(image.Frames.RootFrame.Metadata, true); imageFrame = image.Frames.RootFrame; } @@ -477,7 +481,7 @@ internal sealed class GifDecoderCore : IImageDecoderInternals currentFrame = image.Frames.AddFrame(previousFrame); // This clones the frame and adds it the collection - this.SetFrameMetadata(currentFrame.Metadata); + this.SetFrameMetadata(currentFrame.Metadata, false); imageFrame = currentFrame; @@ -603,28 +607,32 @@ internal sealed class GifDecoderCore : IImageDecoderInternals /// Sets the frames metadata. /// /// The metadata. + /// Whether the metadata represents the root frame. [MethodImpl(MethodImplOptions.AggressiveInlining)] - private void SetFrameMetadata(ImageFrameMetadata meta) + private void SetFrameMetadata(ImageFrameMetadata meta, bool isRoot) { - GifFrameMetadata gifMeta = meta.GetGifMetadata(); - if (this.graphicsControlExtension.DelayTime > 0) - { - gifMeta.FrameDelay = this.graphicsControlExtension.DelayTime; - } - // Frames can either use the global table or their own local table. - if (this.logicalScreenDescriptor.GlobalColorTableFlag + if (isRoot && this.logicalScreenDescriptor.GlobalColorTableFlag && this.logicalScreenDescriptor.GlobalColorTableSize > 0) { + GifFrameMetadata gifMeta = meta.GetGifMetadata(); gifMeta.ColorTableLength = this.logicalScreenDescriptor.GlobalColorTableSize; } - else if (this.imageDescriptor.LocalColorTableFlag + + if (this.imageDescriptor.LocalColorTableFlag && this.imageDescriptor.LocalColorTableSize > 0) { + GifFrameMetadata gifMeta = meta.GetGifMetadata(); gifMeta.ColorTableLength = this.imageDescriptor.LocalColorTableSize; } - gifMeta.DisposalMethod = this.graphicsControlExtension.DisposalMethod; + // Graphics control extensions is optional. + if (this.graphicsControlExtension != default) + { + GifFrameMetadata gifMeta = meta.GetGifMetadata(); + gifMeta.FrameDelay = this.graphicsControlExtension.DelayTime; + gifMeta.DisposalMethod = this.graphicsControlExtension.DisposalMethod; + } } /// diff --git a/src/ImageSharp/Formats/Gif/GifEncoderCore.cs b/src/ImageSharp/Formats/Gif/GifEncoderCore.cs index 14d20cf909..716cb9da3f 100644 --- a/src/ImageSharp/Formats/Gif/GifEncoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifEncoderCore.cs @@ -160,8 +160,12 @@ internal sealed class GifEncoderCore : IImageEncoderInternals { ImageFrame frame = image.Frames[i]; ImageFrameMetadata metadata = frame.Metadata; - GifFrameMetadata frameMetadata = metadata.GetGifMetadata(); - this.WriteGraphicalControlExtension(frameMetadata, transparencyIndex, stream); + + if (metadata.TryGetGifMetadata(out GifFrameMetadata frameMetadata)) + { + this.WriteGraphicalControlExtension(frameMetadata, transparencyIndex, stream); + } + this.WriteImageDescriptor(frame, false, stream); if (i == 0) @@ -193,12 +197,13 @@ internal sealed class GifEncoderCore : IImageEncoderInternals { ImageFrame frame = image.Frames[i]; ImageFrameMetadata metadata = frame.Metadata; - GifFrameMetadata frameMetadata = metadata.GetGifMetadata(); + bool hasMetadata = metadata.TryGetGifMetadata(out GifFrameMetadata frameMetadata); if (quantized is null) { // Allow each frame to be encoded at whatever color depth the frame designates if set. - if (previousFrame != null && previousMeta.ColorTableLength != frameMetadata.ColorTableLength - && frameMetadata.ColorTableLength > 0) + if (previousFrame != null && frameMetadata != null + && previousMeta.ColorTableLength != frameMetadata.ColorTableLength + && frameMetadata.ColorTableLength > 0) { QuantizerOptions options = new() { @@ -218,7 +223,12 @@ internal sealed class GifEncoderCore : IImageEncoderInternals } this.bitDepth = ColorNumerics.GetBitsNeededForColorDepth(quantized.Palette.Length); - this.WriteGraphicalControlExtension(frameMetadata, GetTransparentIndex(quantized), stream); + + if (hasMetadata) + { + this.WriteGraphicalControlExtension(frameMetadata, GetTransparentIndex(quantized), stream); + } + this.WriteImageDescriptor(frame, true, stream); this.WriteColorTable(quantized, stream); this.WriteImageData(quantized, stream); @@ -407,7 +417,7 @@ internal sealed class GifEncoderCore : IImageEncoderInternals } /// - /// Writes the graphics control extension to the stream. + /// Writes the optional graphics control extension to the stream. /// /// The metadata of the image or frame. /// The index of the color in the color palette to make transparent. diff --git a/src/ImageSharp/Formats/Gif/MetadataExtensions.cs b/src/ImageSharp/Formats/Gif/MetadataExtensions.cs index 2c1a3cf0d7..7280024e21 100644 --- a/src/ImageSharp/Formats/Gif/MetadataExtensions.cs +++ b/src/ImageSharp/Formats/Gif/MetadataExtensions.cs @@ -14,14 +14,28 @@ public static partial class MetadataExtensions /// /// Gets the gif format specific metadata for the image. /// - /// The metadata this method extends. + /// The metadata this method extends. /// The . - public static GifMetadata GetGifMetadata(this ImageMetadata metadata) => metadata.GetFormatMetadata(GifFormat.Instance); + public static GifMetadata GetGifMetadata(this ImageMetadata source) => source.GetFormatMetadata(GifFormat.Instance); /// /// Gets the gif format specific metadata for the image frame. /// - /// The metadata this method extends. + /// The metadata this method extends. /// The . - public static GifFrameMetadata GetGifMetadata(this ImageFrameMetadata metadata) => metadata.GetFormatMetadata(GifFormat.Instance); + public static GifFrameMetadata GetGifMetadata(this ImageFrameMetadata source) => source.GetFormatMetadata(GifFormat.Instance); + + /// + /// Gets the gif format specific metadata for the image frame. + /// + /// The metadata this method extends. + /// + /// When this method returns, contains the metadata associated with the specified frame, + /// if found; otherwise, the default value for the type of the metadata parameter. + /// This parameter is passed uninitialized. + /// + /// + /// if the gif frame metadata exists; otherwise, . + /// + public static bool TryGetGifMetadata(this ImageFrameMetadata source, out GifFrameMetadata metadata) => source.TryGetFormatMetadata(GifFormat.Instance, out metadata); } diff --git a/src/ImageSharp/Formats/Gif/Sections/GifGraphicControlExtension.cs b/src/ImageSharp/Formats/Gif/Sections/GifGraphicControlExtension.cs index db2fbd79c3..ba5398fa7d 100644 --- a/src/ImageSharp/Formats/Gif/Sections/GifGraphicControlExtension.cs +++ b/src/ImageSharp/Formats/Gif/Sections/GifGraphicControlExtension.cs @@ -11,7 +11,7 @@ namespace SixLabors.ImageSharp.Formats.Gif; /// processing a graphic rendering block. /// [StructLayout(LayoutKind.Sequential, Pack = 1)] -internal readonly struct GifGraphicControlExtension : IGifExtension +internal readonly struct GifGraphicControlExtension : IGifExtension, IEquatable { public GifGraphicControlExtension( byte packed, @@ -64,6 +64,10 @@ internal readonly struct GifGraphicControlExtension : IGifExtension int IGifExtension.ContentLength => 5; + public static bool operator ==(GifGraphicControlExtension left, GifGraphicControlExtension right) => left.Equals(right); + + public static bool operator !=(GifGraphicControlExtension left, GifGraphicControlExtension right) => !(left == right); + public int WriteTo(Span buffer) { ref GifGraphicControlExtension dest = ref Unsafe.As(ref MemoryMarshal.GetReference(buffer)); @@ -101,4 +105,27 @@ internal readonly struct GifGraphicControlExtension : IGifExtension return value; } + + public override bool Equals(object obj) => obj is GifGraphicControlExtension extension && this.Equals(extension); + + public bool Equals(GifGraphicControlExtension other) + => this.BlockSize == other.BlockSize + && this.Packed == other.Packed + && this.DelayTime == other.DelayTime + && this.TransparencyIndex == other.TransparencyIndex + && this.DisposalMethod == other.DisposalMethod + && this.TransparencyFlag == other.TransparencyFlag + && ((IGifExtension)this).Label == ((IGifExtension)other).Label + && ((IGifExtension)this).ContentLength == ((IGifExtension)other).ContentLength; + + public override int GetHashCode() + => HashCode.Combine( + this.BlockSize, + this.Packed, + this.DelayTime, + this.TransparencyIndex, + this.DisposalMethod, + this.TransparencyFlag, + ((IGifExtension)this).Label, + ((IGifExtension)this).ContentLength); } diff --git a/src/ImageSharp/Metadata/ImageFrameMetadata.cs b/src/ImageSharp/Metadata/ImageFrameMetadata.cs index 69ff5201f2..bf65fd6dc1 100644 --- a/src/ImageSharp/Metadata/ImageFrameMetadata.cs +++ b/src/ImageSharp/Metadata/ImageFrameMetadata.cs @@ -69,7 +69,8 @@ public sealed class ImageFrameMetadata : IDeepCloneable public ImageFrameMetadata DeepClone() => new(this); /// - /// Gets the metadata value associated with the specified key. + /// Gets the metadata value associated with the specified key. This method will always return a result creating + /// a new instance and binding it to the frame metadata if none is found. /// /// The type of format metadata. /// The type of format frame metadata. @@ -90,4 +91,32 @@ public sealed class ImageFrameMetadata : IDeepCloneable this.formatMetadata[key] = newMeta; return newMeta; } + + /// + /// Gets the metadata value associated with the specified key. + /// + /// The type of format metadata. + /// The type of format frame metadata. + /// The key of the value to get. + /// + /// When this method returns, contains the metadata associated with the specified key, + /// if the key is found; otherwise, the default value for the type of the metadata parameter. + /// This parameter is passed uninitialized. + /// + /// + /// if the frame metadata exists for the specified key; otherwise, . + /// + public bool TryGetFormatMetadata(IImageFormat key, out TFormatFrameMetadata metadata) + where TFormatMetadata : class + where TFormatFrameMetadata : class, IDeepCloneable + { + if (this.formatMetadata.TryGetValue(key, out IDeepCloneable meta)) + { + metadata = (TFormatFrameMetadata)meta; + return true; + } + + metadata = default; + return false; + } } diff --git a/tests/ImageSharp.Tests/Formats/Gif/GifEncoderTests.cs b/tests/ImageSharp.Tests/Formats/Gif/GifEncoderTests.cs index 18eb7708ca..7c71fd5469 100644 --- a/tests/ImageSharp.Tests/Formats/Gif/GifEncoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Gif/GifEncoderTests.cs @@ -201,4 +201,39 @@ public class GifEncoderTests image.Dispose(); clone.Dispose(); } + + [Theory] + [WithFile(TestImages.Gif.Issues.Issue2288OptionalExtension, PixelTypes.Rgba32)] + public void OptionalExtensionsShouldBeHandledProperly(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using Image image = provider.GetImage(); + + int count = 0; + foreach (ImageFrame frame in image.Frames) + { + if (frame.Metadata.TryGetGifMetadata(out GifFrameMetadata _)) + { + count++; + } + } + + provider.Utility.SaveTestOutputFile(image, extension: "gif"); + + using FileStream fs = File.OpenRead(provider.Utility.GetTestOutputFileName("gif")); + using Image image2 = Image.Load(fs); + + Assert.Equal(image.Frames.Count, image2.Frames.Count); + + count = 0; + foreach (ImageFrame frame in image2.Frames) + { + if (frame.Metadata.TryGetGifMetadata(out GifFrameMetadata _)) + { + count++; + } + } + + Assert.Equal(image2.Frames.Count, count); + } } diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index d47f1136d5..ea8fba998f 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -476,6 +476,7 @@ public static class TestImages public const string Issue1962NoColorTable = "Gif/issues/issue1962_tiniest_gif_1st.gif"; public const string Issue2012EmptyXmp = "Gif/issues/issue2012_Stronghold-Crusader-Extreme-Cover.gif"; public const string Issue2012BadMinCode = "Gif/issues/issue2012_drona1.gif"; + public const string Issue2288OptionalExtension = "Gif/issues/issue_2288.gif"; } public static readonly string[] All = { Rings, Giphy, Cheers, Trans, Kumin, Leo, Ratio4x1, Ratio1x4 }; diff --git a/tests/Images/Input/Gif/issues/issue_2288.gif b/tests/Images/Input/Gif/issues/issue_2288.gif new file mode 100644 index 0000000000..f798e7b8e6 --- /dev/null +++ b/tests/Images/Input/Gif/issues/issue_2288.gif @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:d38bc98de425322bc0d435b7ff538c170897bfc728ea77ee26dd172106dcf99a +size 1223216