Merge pull request #3101 from andreas-eriksson/ImageInfo.FrameMetadataCollection-not-populated-correctly-for-animated-png-images

Fix Identify returning incorrect frame count for animated PNGs

Author: JimBobSquarePants

src/ImageSharp/Formats/Png/PngDecoderCore.cs

@@ -214,7 +214,7 @@ protected override Image<TPixel> Decode<TPixel>(BufferedReadStream stream, Cance
                             break;
                         case PngChunkType.FrameData:
                         {
-                            if (frameCount >= this.maxFrames)
+                            if (frameCount > this.maxFrames)
                             {
                                 goto EOF;
                             }
@@ -275,7 +275,7 @@ protected override Image<TPixel> Decode<TPixel>(BufferedReadStream stream, Cance
                                 previousFrameControl = currentFrameControl;
                             }
 
-                            if (frameCount >= this.maxFrames)
+                            if (frameCount > this.maxFrames)
                             {
                                 goto EOF;
                             }
@@ -402,7 +402,7 @@ protected override ImageInfo Identify(BufferedReadStream stream, CancellationTok
                             break;
                         case PngChunkType.FrameControl:
                             ++frameCount;
-                            if (frameCount >= this.maxFrames)
+                            if (frameCount > this.maxFrames)
                             {
                                 break;
                             }
@@ -411,8 +411,12 @@ protected override ImageInfo Identify(BufferedReadStream stream, CancellationTok
 
                             break;
                         case PngChunkType.FrameData:
-                            if (frameCount >= this.maxFrames)
+                            if (frameCount > this.maxFrames)
                             {
+                                // Must skip the chunk data even when we've hit maxFrames, because TryReadChunk
+                                // restores the stream position to the start of the fdAT data after CRC validation.
+                                this.SkipChunkDataAndCrc(chunk);
+                                this.SkipRemainingFrameDataChunks(buffer);
                                 break;
                             }
 
@@ -428,9 +432,10 @@ protected override ImageInfo Identify(BufferedReadStream stream, CancellationTok
 
                             InitializeFrameMetadata(framesMetadata, currentFrameControl.Value);
 
-                            // Skip sequence number
-                            this.currentStream.Skip(4);
+                            // Skip data for this and all remaining FrameData chunks belonging to the same frame
+                            // (comparable to how Decode consumes them via ReadScanlines + ReadNextFrameDataChunk).
                             this.SkipChunkDataAndCrc(chunk);
+                            this.SkipRemainingFrameDataChunks(buffer);
                             break;
                         case PngChunkType.Data:
 
@@ -2093,6 +2098,31 @@ private int ReadNextFrameDataChunk()
         return 0;
     }
 
+    /// <summary>
+    /// Skips any remaining <see cref="PngChunkType.FrameData"/> chunks belonging to the current frame.
+    /// This mirrors how <see cref="ReadNextFrameDataChunk"/> is used during decoding:
+    /// consecutive fdAT chunks are consumed until a non-fdAT chunk is encountered,
+    /// which is stored in <see cref="nextChunk"/> for the next iteration.
+    /// </summary>
+    /// <param name="buffer">Temporary buffer.</param>
+    private void SkipRemainingFrameDataChunks(Span<byte> buffer)
+    {
+        while (this.TryReadChunk(buffer, out PngChunk chunk))
+        {
+            if (chunk.Type is PngChunkType.FrameData)
+            {
+                chunk.Data?.Dispose();
+                this.SkipChunkDataAndCrc(chunk);
+            }
+            else
+            {
+                // Not a FrameData chunk; store it so the next TryReadChunk call returns it.
+                this.nextChunk = chunk;
+                return;
+            }
+        }
+    }
+
     /// <summary>
     /// Reads a chunk from the stream.
     /// </summary>

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

@@ -411,6 +411,91 @@ public void Identify_IgnoreCrcErrors(string imagePath, int expectedPixelSize)
         Assert.Equal(expectedPixelSize, imageInfo.PixelType.BitsPerPixel);
     }
 
+    [Fact]
+    public void Identify_AnimatedPng_ReadsFrameCountCorrectly()
+    {
+        TestFile testFile = TestFile.Create(TestImages.Png.AnimatedFrameCount);
+
+        using MemoryStream stream = new(testFile.Bytes, false);
+        ImageInfo imageInfo = Image.Identify(stream);
+
+        Assert.NotNull(imageInfo);
+        Assert.Equal(48, imageInfo.FrameMetadataCollection.Count);
+    }
+
+    [Fact]
+    public void Identify_AnimatedPngWithMaxFrames_ReadsFrameCountCorrectly()
+    {
+        TestFile testFile = TestFile.Create(TestImages.Png.AnimatedFrameCount);
+
+        using MemoryStream stream = new(testFile.Bytes, false);
+        ImageInfo imageInfo = Image.Identify(new DecoderOptions { MaxFrames = 40 }, stream);
+
+        Assert.NotNull(imageInfo);
+        Assert.Equal(40, imageInfo.FrameMetadataCollection.Count);
+    }
+
+    [Fact]
+    public void Load_AnimatedPngWithMaxFrames_ReadsFrameCountCorrectly()
+    {
+        TestFile testFile = TestFile.Create(TestImages.Png.AnimatedFrameCount);
+
+        using MemoryStream stream = new(testFile.Bytes, false);
+        using Image image = Image.Load(new DecoderOptions { MaxFrames = 40 }, stream);
+
+        Assert.NotNull(image);
+        Assert.Equal(40, image.Frames.Count);
+    }
+
+    [Theory]
+    [InlineData(1)]
+    [InlineData(2)]
+    [InlineData(5)]
+    [InlineData(10)]
+    [InlineData(100)]
+    public void Identify_AnimatedPng_FrameCount_MatchesDecode(int frameCount)
+    {
+        using Image<Rgba32> image = new(10, 10, Color.Red.ToPixel<Rgba32>());
+        for (int i = 1; i < frameCount; i++)
+        {
+            using ImageFrame<Rgba32> frame = new(Configuration.Default, 10, 10);
+            image.Frames.AddFrame(frame);
+        }
+
+        using MemoryStream stream = new();
+        image.Save(stream, new PngEncoder());
+        stream.Position = 0;
+
+        ImageInfo imageInfo = Image.Identify(stream);
+
+        Assert.NotNull(imageInfo);
+        Assert.Equal(frameCount, imageInfo.FrameMetadataCollection.Count);
+    }
+
+    [Theory]
+    [InlineData(1)]
+    [InlineData(2)]
+    [InlineData(5)]
+    [InlineData(10)]
+    [InlineData(100)]
+    public void Decode_AnimatedPng_FrameCount(int frameCount)
+    {
+        using Image<Rgba32> image = new(10, 10, Color.Red.ToPixel<Rgba32>());
+        for (int i = 1; i < frameCount; i++)
+        {
+            using ImageFrame<Rgba32> frame = new(Configuration.Default, 10, 10);
+            image.Frames.AddFrame(frame);
+        }
+
+        using MemoryStream stream = new();
+        image.Save(stream, new PngEncoder());
+        stream.Position = 0;
+
+        using Image<Rgba32> decoded = Image.Load<Rgba32>(stream);
+
+        Assert.Equal(frameCount, decoded.Frames.Count);
+    }
+
     [Theory]
     [WithFile(TestImages.Png.Bad.MissingDataChunk, PixelTypes.Rgba32)]
     public void Decode_MissingDataChunk_ThrowsException<TPixel>(TestImageProvider<TPixel> provider)

tests/ImageSharp.Tests/TestImages.cs

@@ -76,6 +76,7 @@ public static class 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 AnimatedFrameCount = "Png/animated/issue-animated-frame-count.png";
         public const string Issue2666 = "Png/issues/Issue_2666.png";
         public const string Issue2882 = "Png/issues/Issue_2882.png";