Correctly read/write graphics control extension

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<TPixel>(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.
     /// </summary>
     /// <param name="meta">The metadata.</param>
+    /// <param name="isRoot">Whether the metadata represents the root frame.</param>
     [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;
+        }
     }
 
     /// <summary>

src/ImageSharp/Formats/Gif/GifEncoderCore.cs

@@ -160,8 +160,12 @@ internal sealed class GifEncoderCore : IImageEncoderInternals
         {
             ImageFrame<TPixel> 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<TPixel> 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
     }
 
     /// <summary>
-    /// Writes the graphics control extension to the stream.
+    /// Writes the optional graphics control extension to the stream.
     /// </summary>
     /// <param name="metadata">The metadata of the image or frame.</param>
     /// <param name="transparencyIndex">The index of the color in the color palette to make transparent.</param>

src/ImageSharp/Formats/Gif/MetadataExtensions.cs

@@ -14,14 +14,28 @@ public static partial class MetadataExtensions
     /// <summary>
     /// Gets the gif format specific metadata for the image.
     /// </summary>
-    /// <param name="metadata">The metadata this method extends.</param>
+    /// <param name="source">The metadata this method extends.</param>
     /// <returns>The <see cref="GifMetadata"/>.</returns>
-    public static GifMetadata GetGifMetadata(this ImageMetadata metadata) => metadata.GetFormatMetadata(GifFormat.Instance);
+    public static GifMetadata GetGifMetadata(this ImageMetadata source) => source.GetFormatMetadata(GifFormat.Instance);
 
     /// <summary>
     /// Gets the gif format specific metadata for the image frame.
     /// </summary>
-    /// <param name="metadata">The metadata this method extends.</param>
+    /// <param name="source">The metadata this method extends.</param>
     /// <returns>The <see cref="GifFrameMetadata"/>.</returns>
-    public static GifFrameMetadata GetGifMetadata(this ImageFrameMetadata metadata) => metadata.GetFormatMetadata(GifFormat.Instance);
+    public static GifFrameMetadata GetGifMetadata(this ImageFrameMetadata source) => source.GetFormatMetadata(GifFormat.Instance);
+
+    /// <summary>
+    /// Gets the gif format specific metadata for the image frame.
+    /// </summary>
+    /// <param name="source">The metadata this method extends.</param>
+    /// <param name="metadata">
+    /// 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.
+    /// </param>
+    /// <returns>
+    /// <see langword="true"/> if the gif frame metadata exists; otherwise, <see langword="false"/>.
+    /// </returns>
+    public static bool TryGetGifMetadata(this ImageFrameMetadata source, out GifFrameMetadata metadata) => source.TryGetFormatMetadata(GifFormat.Instance, out metadata);
 }

src/ImageSharp/Formats/Gif/Sections/GifGraphicControlExtension.cs

@@ -11,7 +11,7 @@ namespace SixLabors.ImageSharp.Formats.Gif;
 /// processing a graphic rendering block.
 /// </summary>
 [StructLayout(LayoutKind.Sequential, Pack = 1)]
-internal readonly struct GifGraphicControlExtension : IGifExtension
+internal readonly struct GifGraphicControlExtension : IGifExtension, IEquatable<GifGraphicControlExtension>
 {
     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<byte> buffer)
     {
         ref GifGraphicControlExtension dest = ref Unsafe.As<byte, GifGraphicControlExtension>(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);
 }

src/ImageSharp/Metadata/ImageFrameMetadata.cs

@@ -69,7 +69,8 @@ public sealed class ImageFrameMetadata : IDeepCloneable<ImageFrameMetadata>
     public ImageFrameMetadata DeepClone() => new(this);
 
     /// <summary>
-    /// 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.
     /// </summary>
     /// <typeparam name="TFormatMetadata">The type of format metadata.</typeparam>
     /// <typeparam name="TFormatFrameMetadata">The type of format frame metadata.</typeparam>
@@ -90,4 +91,32 @@ public sealed class ImageFrameMetadata : IDeepCloneable<ImageFrameMetadata>
         this.formatMetadata[key] = newMeta;
         return newMeta;
     }
+
+    /// <summary>
+    /// Gets the metadata value associated with the specified key.
+    /// </summary>
+    /// <typeparam name="TFormatMetadata">The type of format metadata.</typeparam>
+    /// <typeparam name="TFormatFrameMetadata">The type of format frame metadata.</typeparam>
+    /// <param name="key">The key of the value to get.</param>
+    /// <param name="metadata">
+    /// 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.
+    /// </param>
+    /// <returns>
+    /// <see langword="true"/> if the frame metadata exists for the specified key; otherwise, <see langword="false"/>.
+    /// </returns>
+    public bool TryGetFormatMetadata<TFormatMetadata, TFormatFrameMetadata>(IImageFormat<TFormatMetadata, TFormatFrameMetadata> 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;
+    }
 }

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<TPixel>(TestImageProvider<TPixel> provider)
+        where TPixel : unmanaged, IPixel<TPixel>
+    {
+        using Image<TPixel> image = provider.GetImage();
+
+        int count = 0;
+        foreach (ImageFrame<TPixel> 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<TPixel> image2 = Image.Load<TPixel>(fs);
+
+        Assert.Equal(image.Frames.Count, image2.Frames.Count);
+
+        count = 0;
+        foreach (ImageFrame<TPixel> frame in image2.Frames)
+        {
+            if (frame.Metadata.TryGetGifMetadata(out GifFrameMetadata _))
+            {
+                count++;
+            }
+        }
+
+        Assert.Equal(image2.Frames.Count, count);
+    }
 }

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 };

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