Merge pull request #2713 from SpaceCheetah/FixPngBackport

Backport APNG fix to release/3.1.x

src/ImageSharp/Formats/Png/PngDecoderCore.cs

@@ -228,8 +228,7 @@ public Image<TPixel> Decode<TPixel>(BufferedReadStream stream, CancellationToken
                                 PngThrowHelper.ThrowMissingFrameControl();
                             }
 
-                            previousFrameControl ??= new((uint)this.header.Width, (uint)this.header.Height);
-                            this.InitializeFrame(previousFrameControl.Value, currentFrameControl.Value, image, previousFrame, out currentFrame);
+                            this.InitializeFrame(previousFrameControl, currentFrameControl.Value, image, previousFrame, out currentFrame);
 
                             this.currentStream.Position += 4;
                             this.ReadScanlines(
@@ -240,11 +239,16 @@ public Image<TPixel> Decode<TPixel>(BufferedReadStream stream, CancellationToken
                                 currentFrameControl.Value,
                                 cancellationToken);
 
-                            previousFrame = currentFrame;
-                            previousFrameControl = currentFrameControl;
+                            // if current frame dispose is restore to previous, then from future frame's perspective, it never happened
+                            if (currentFrameControl.Value.DisposeOperation != PngDisposalMethod.RestoreToPrevious)
+                            {
+                                previousFrame = currentFrame;
+                                previousFrameControl = currentFrameControl;
+                            }
+
                             break;
                         case PngChunkType.Data:
-
+                            pngMetadata.AnimateRootFrame = currentFrameControl != null;
                             currentFrameControl ??= new((uint)this.header.Width, (uint)this.header.Height);
                             if (image is null)
                             {
@@ -261,9 +265,12 @@ public Image<TPixel> Decode<TPixel>(BufferedReadStream stream, CancellationToken
                                 this.ReadNextDataChunk,
                                 currentFrameControl.Value,
                                 cancellationToken);
+                            if (pngMetadata.AnimateRootFrame)
+                            {
+                                previousFrame = currentFrame;
+                                previousFrameControl = currentFrameControl;
+                            }
 
-                            previousFrame = currentFrame;
-                            previousFrameControl = currentFrameControl;
                             break;
                         case PngChunkType.Palette:
                             this.palette = chunk.Data.GetSpan().ToArray();
@@ -632,7 +639,7 @@ private void InitializeImage<TPixel>(ImageMetadata metadata, FrameControl frameC
     /// <param name="previousFrame">The previous frame.</param>
     /// <param name="frame">The created frame</param>
     private void InitializeFrame<TPixel>(
-        FrameControl previousFrameControl,
+        FrameControl? previousFrameControl,
         FrameControl currentFrameControl,
         Image<TPixel> image,
         ImageFrame<TPixel>? previousFrame,
@@ -645,12 +652,16 @@ private void InitializeFrame<TPixel>(
         frame = image.Frames.AddFrame(previousFrame ?? image.Frames.RootFrame);
 
         // If the first `fcTL` chunk uses a `dispose_op` of APNG_DISPOSE_OP_PREVIOUS it should be treated as APNG_DISPOSE_OP_BACKGROUND.
-        if (previousFrameControl.DisposeOperation == PngDisposalMethod.RestoreToBackground
-            || (previousFrame is null && previousFrameControl.DisposeOperation == PngDisposalMethod.RestoreToPrevious))
+        // So, if restoring to before first frame, clear entire area. Same if first frame (previousFrameControl null).
+        if (previousFrameControl == null || (previousFrame is null && previousFrameControl.Value.DisposeOperation == PngDisposalMethod.RestoreToPrevious))
+        {
+            Buffer2DRegion<TPixel> pixelRegion = frame.PixelBuffer.GetRegion();
+            pixelRegion.Clear();
+        }
+        else if (previousFrameControl.Value.DisposeOperation == PngDisposalMethod.RestoreToBackground)
         {
-            Rectangle restoreArea = previousFrameControl.Bounds;
-            Rectangle interest = Rectangle.Intersect(frame.Bounds(), restoreArea);
-            Buffer2DRegion<TPixel> pixelRegion = frame.PixelBuffer.GetRegion(interest);
+            Rectangle restoreArea = previousFrameControl.Value.Bounds;
+            Buffer2DRegion<TPixel> pixelRegion = frame.PixelBuffer.GetRegion(restoreArea);
             pixelRegion.Clear();
         }
 

src/ImageSharp/Formats/Png/PngEncoderCore.cs

@@ -161,6 +161,7 @@ public void Encode<TPixel>(Image<TPixel> image, Stream stream, CancellationToken
 
         ImageFrame<TPixel>? clonedFrame = null;
         ImageFrame<TPixel> currentFrame = image.Frames.RootFrame;
+        int currentFrameIndex = 0;
 
         bool clearTransparency = this.encoder.TransparentColorMode is PngTransparentColorMode.Clear;
         if (clearTransparency)
@@ -189,29 +190,50 @@ public void Encode<TPixel>(Image<TPixel> image, Stream stream, CancellationToken
 
         if (image.Frames.Count > 1)
         {
-            this.WriteAnimationControlChunk(stream, (uint)image.Frames.Count, pngMetadata.RepeatCount);
+            this.WriteAnimationControlChunk(stream, (uint)(image.Frames.Count - (pngMetadata.AnimateRootFrame ? 0 : 1)), pngMetadata.RepeatCount);
+        }
+
+        // If the first frame isn't animated, write it as usual and skip it when writing animated frames
+        if (!pngMetadata.AnimateRootFrame || image.Frames.Count == 1)
+        {
+            FrameControl frameControl = new((uint)this.width, (uint)this.height);
+            this.WriteDataChunks(frameControl, currentFrame.PixelBuffer.GetRegion(), quantized, stream, false);
+            currentFrameIndex++;
+        }
 
-            // Write the first frame.
+        if (image.Frames.Count > 1)
+        {
+            // Write the first animated frame.
+            currentFrame = image.Frames[currentFrameIndex];
             PngFrameMetadata frameMetadata = GetPngFrameMetadata(currentFrame);
             PngDisposalMethod previousDisposal = frameMetadata.DisposalMethod;
             FrameControl frameControl = this.WriteFrameControlChunk(stream, frameMetadata, currentFrame.Bounds(), 0);
-            this.WriteDataChunks(frameControl, currentFrame.PixelBuffer.GetRegion(), quantized, stream, false);
+            uint sequenceNumber = 1;
+            if (pngMetadata.AnimateRootFrame)
+            {
+                this.WriteDataChunks(frameControl, currentFrame.PixelBuffer.GetRegion(), quantized, stream, false);
+            }
+            else
+            {
+                sequenceNumber += this.WriteDataChunks(frameControl, currentFrame.PixelBuffer.GetRegion(), quantized, stream, true);
+            }
+
+            currentFrameIndex++;
 
             // Capture the global palette for reuse on subsequent frames.
             ReadOnlyMemory<TPixel>? previousPalette = quantized?.Palette.ToArray();
 
             // Write following frames.
-            uint increment = 0;
             ImageFrame<TPixel> previousFrame = image.Frames.RootFrame;
 
             // This frame is reused to store de-duplicated pixel buffers.
             using ImageFrame<TPixel> encodingFrame = new(image.Configuration, previousFrame.Size());
 
-            for (int i = 1; i < image.Frames.Count; i++)
+            for (; currentFrameIndex < image.Frames.Count; currentFrameIndex++)
             {
                 ImageFrame<TPixel>? prev = previousDisposal == PngDisposalMethod.RestoreToBackground ? null : previousFrame;
-                currentFrame = image.Frames[i];
-                ImageFrame<TPixel>? nextFrame = i < image.Frames.Count - 1 ? image.Frames[i + 1] : null;
+                currentFrame = image.Frames[currentFrameIndex];
+                ImageFrame<TPixel>? nextFrame = currentFrameIndex < image.Frames.Count - 1 ? image.Frames[currentFrameIndex + 1] : null;
 
                 frameMetadata = GetPngFrameMetadata(currentFrame);
                 bool blend = frameMetadata.BlendMethod == PngBlendMethod.Over;
@@ -232,22 +254,17 @@ public void Encode<TPixel>(Image<TPixel> image, Stream stream, CancellationToken
                 }
 
                 // Each frame control sequence number must be incremented by the number of frame data chunks that follow.
-                frameControl = this.WriteFrameControlChunk(stream, frameMetadata, bounds, (uint)i + increment);
+                frameControl = this.WriteFrameControlChunk(stream, frameMetadata, bounds, sequenceNumber);
 
                 // Dispose of previous quantized frame and reassign.
                 quantized?.Dispose();
                 quantized = this.CreateQuantizedImageAndUpdateBitDepth(pngMetadata, encodingFrame, bounds, previousPalette);
-                increment += this.WriteDataChunks(frameControl, encodingFrame.PixelBuffer.GetRegion(bounds), quantized, stream, true);
+                sequenceNumber += this.WriteDataChunks(frameControl, encodingFrame.PixelBuffer.GetRegion(bounds), quantized, stream, true) + 1;
 
                 previousFrame = currentFrame;
                 previousDisposal = frameMetadata.DisposalMethod;
             }
         }
-        else
-        {
-            FrameControl frameControl = new((uint)this.width, (uint)this.height);
-            this.WriteDataChunks(frameControl, currentFrame.PixelBuffer.GetRegion(), quantized, stream, false);
-        }
 
         this.WriteEndChunk(stream);
 

src/ImageSharp/Formats/Png/PngMetadata.cs

@@ -29,6 +29,7 @@ private PngMetadata(PngMetadata other)
         this.InterlaceMethod = other.InterlaceMethod;
         this.TransparentColor = other.TransparentColor;
         this.RepeatCount = other.RepeatCount;
+        this.AnimateRootFrame = other.AnimateRootFrame;
 
         if (other.ColorTable?.Length > 0)
         {
@@ -83,6 +84,11 @@ private PngMetadata(PngMetadata other)
     /// </summary>
     public uint RepeatCount { get; set; } = 1;
 
+    /// <summary>
+    ///  Gets or sets a value indicating whether the root frame is shown as part of the animated sequence
+    /// </summary>
+    public bool AnimateRootFrame { get; set; } = true;
+
     /// <inheritdoc/>
     public IDeepCloneable DeepClone() => new PngMetadata(this);
 

src/ImageSharp/Formats/Png/PngScanlineProcessor.cs

@@ -198,8 +198,9 @@ public static void ProcessInterlacedPaletteScanline<TPixel>(
         ref byte scanlineSpanRef = ref MemoryMarshal.GetReference(scanlineSpan);
         ref TPixel rowSpanRef = ref MemoryMarshal.GetReference(rowSpan);
         ref Color paletteBase = ref MemoryMarshal.GetReference(palette.Value.Span);
+        uint offset = pixelOffset + frameControl.XOffset;
 
-        for (nuint x = pixelOffset, o = 0; x < frameControl.XMax; x += increment, o++)
+        for (nuint x = offset, o = 0; x < frameControl.XMax; x += increment, o++)
         {
             uint index = Unsafe.Add(ref scanlineSpanRef, o);
             pixel.FromRgba32(Unsafe.Add(ref paletteBase, index).ToRgba32());

tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs

@@ -87,7 +87,9 @@ public partial class PngDecoderTests
         TestImages.Png.DisposeBackgroundRegion,
         TestImages.Png.DisposePreviousFirst,
         TestImages.Png.DisposeBackgroundBeforeRegion,
-        TestImages.Png.BlendOverMultiple
+        TestImages.Png.BlendOverMultiple,
+        TestImages.Png.FrameOffset,
+        TestImages.Png.DefaultNotAnimated
     };
 
     [Theory]

tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.Chunks.cs

@@ -3,6 +3,7 @@
 
 using System.Buffers.Binary;
 using SixLabors.ImageSharp.Formats.Png;
+using SixLabors.ImageSharp.Formats.Png.Chunks;
 using SixLabors.ImageSharp.PixelFormats;
 
 // ReSharper disable InconsistentNaming
@@ -59,6 +60,38 @@ public void EndChunk_IsLast()
         }
     }
 
+    [Theory]
+    [WithFile(TestImages.Png.DefaultNotAnimated, PixelTypes.Rgba32)]
+    [WithFile(TestImages.Png.APng, PixelTypes.Rgba32)]
+    public void AcTL_CorrectlyWritten<TPixel>(TestImageProvider<TPixel> provider)
+        where TPixel : unmanaged, IPixel<TPixel>
+    {
+        using Image<TPixel> image = provider.GetImage(PngDecoder.Instance);
+        PngMetadata metadata = image.Metadata.GetPngMetadata();
+        int correctFrameCount = image.Frames.Count - (metadata.AnimateRootFrame ? 0 : 1);
+        using MemoryStream memStream = new();
+        image.Save(memStream, PngEncoder);
+        memStream.Position = 0;
+        Span<byte> bytesSpan = memStream.ToArray().AsSpan(8); // Skip header.
+        bool foundAcTl = false;
+        while (bytesSpan.Length > 0 && !foundAcTl)
+        {
+            int length = BinaryPrimitives.ReadInt32BigEndian(bytesSpan[..4]);
+            PngChunkType type = (PngChunkType)BinaryPrimitives.ReadInt32BigEndian(bytesSpan.Slice(4, 4));
+            if (type == PngChunkType.AnimationControl)
+            {
+                AnimationControl control = AnimationControl.Parse(bytesSpan[8..]);
+                foundAcTl = true;
+                Assert.True(control.NumberFrames == correctFrameCount);
+                Assert.True(control.NumberPlays == metadata.RepeatCount);
+            }
+
+            bytesSpan = bytesSpan[(4 + 4 + length + 4)..];
+        }
+
+        Assert.True(foundAcTl);
+    }
+
     [Theory]
     [InlineData(PngChunkType.Gamma)]
     [InlineData(PngChunkType.Chroma)]

tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs

@@ -447,6 +447,8 @@ public void Encode_WithPngTransparentColorBehaviorClear_Works(PngColorType color
 
     [Theory]
     [WithFile(TestImages.Png.APng, PixelTypes.Rgba32)]
+    [WithFile(TestImages.Png.DefaultNotAnimated, PixelTypes.Rgba32)]
+    [WithFile(TestImages.Png.FrameOffset, PixelTypes.Rgba32)]
     public void Encode_APng<TPixel>(TestImageProvider<TPixel> provider)
         where TPixel : unmanaged, IPixel<TPixel>
     {
@@ -458,15 +460,17 @@ public void Encode_APng<TPixel>(TestImageProvider<TPixel> provider)
         image.DebugSave(provider: provider, encoder: PngEncoder, null, false);
 
         using Image<Rgba32> output = Image.Load<Rgba32>(memStream);
-        ImageComparer.Exact.VerifySimilarity(output, image);
 
-        Assert.Equal(5, image.Frames.Count);
+        // some loss from original, due to compositing
+        ImageComparer.TolerantPercentage(0.01f).VerifySimilarity(output, image);
+
         Assert.Equal(image.Frames.Count, output.Frames.Count);
 
         PngMetadata originalMetadata = image.Metadata.GetPngMetadata();
         PngMetadata outputMetadata = output.Metadata.GetPngMetadata();
 
         Assert.Equal(originalMetadata.RepeatCount, outputMetadata.RepeatCount);
+        Assert.Equal(originalMetadata.AnimateRootFrame, outputMetadata.AnimateRootFrame);
 
         for (int i = 0; i < image.Frames.Count; i++)
         {

tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs

@@ -32,7 +32,8 @@ public void CloneIsDeep()
             InterlaceMethod = PngInterlaceMode.Adam7,
             Gamma = 2,
             TextData = new List<PngTextData> { new PngTextData("name", "value", "foo", "bar") },
-            RepeatCount = 123
+            RepeatCount = 123,
+            AnimateRootFrame = false
         };
 
         PngMetadata clone = (PngMetadata)meta.DeepClone();
@@ -44,6 +45,7 @@ public void CloneIsDeep()
         Assert.False(meta.TextData.Equals(clone.TextData));
         Assert.True(meta.TextData.SequenceEqual(clone.TextData));
         Assert.True(meta.RepeatCount == clone.RepeatCount);
+        Assert.True(meta.AnimateRootFrame == clone.AnimateRootFrame);
 
         clone.BitDepth = PngBitDepth.Bit2;
         clone.ColorType = PngColorType.Palette;
@@ -144,6 +146,26 @@ public void Decode_ReadsExifData<TPixel>(TestImageProvider<TPixel> provider)
         VerifyExifDataIsPresent(exif);
     }
 
+    [Theory]
+    [WithFile(TestImages.Png.DefaultNotAnimated, PixelTypes.Rgba32)]
+    public void Decode_IdentifiesDefaultFrameNotAnimated<TPixel>(TestImageProvider<TPixel> provider)
+        where TPixel : unmanaged, IPixel<TPixel>
+    {
+        using Image<TPixel> image = provider.GetImage(PngDecoder.Instance);
+        PngMetadata meta = image.Metadata.GetFormatMetadata(PngFormat.Instance);
+        Assert.False(meta.AnimateRootFrame);
+    }
+
+    [Theory]
+    [WithFile(TestImages.Png.APng, PixelTypes.Rgba32)]
+    public void Decode_IdentifiesDefaultFrameAnimated<TPixel>(TestImageProvider<TPixel> provider)
+        where TPixel : unmanaged, IPixel<TPixel>
+    {
+        using Image<TPixel> image = provider.GetImage(PngDecoder.Instance);
+        PngMetadata meta = image.Metadata.GetFormatMetadata(PngFormat.Instance);
+        Assert.True(meta.AnimateRootFrame);
+    }
+
     [Theory]
     [WithFile(TestImages.Png.PngWithMetadata, PixelTypes.Rgba32)]
     public void Decode_IgnoresExifData_WhenIgnoreMetadataIsTrue<TPixel>(TestImageProvider<TPixel> provider)

tests/ImageSharp.Tests/TestImages.cs

@@ -73,6 +73,8 @@ public static class Png
         public const string DisposeBackgroundRegion = "Png/animated/15-dispose-background-region.png";
         public const string DisposePreviousFirst = "Png/animated/12-dispose-prev-first.png";
         public const string BlendOverMultiple = "Png/animated/21-blend-over-multiple.png";
+        public const string FrameOffset = "Png/animated/frame-offset.png";
+        public const string DefaultNotAnimated = "Png/animated/default-not-animated.png";
         public const string Issue2666 = "Png/issues/Issue_2666.png";
 
         // Filtered test images from http://www.schaik.com/pngsuite/pngsuite_fil_png.html