Fix Identify returning incorrect frame count for animated PNGs#3101
Conversation
The Identify method had two bugs when processing fdAT (FrameData) chunks: 1. A spurious Skip(4) before SkipChunkDataAndCrc caused the stream to be misaligned by 4 bytes, since chunk.Length already includes the 4-byte sequence number. 2. Unlike Decode, which consumes all fdAT chunks for a frame in one shot via ReadScanlines + ReadNextFrameDataChunk, Identify processed them individually, calling InitializeFrameMetadata for each chunk and inflating the frame count. The fix removes the extra Skip(4) and adds SkipRemainingFrameDataChunks to consume all continuation fdAT chunks for a frame, mirroring how ReadNextFrameDataChunk works during decoding.
|
|
…ted-correctly-for-animated-png-images
There was a problem hiding this comment.
Pull request overview
Fixes Image.Identify() returning an incorrect APNG frame count when frames span multiple fdAT chunks by aligning Identify’s fdAT consumption behavior with Decode.
Changes:
- Remove the extra 4-byte skip in Identify’s
fdAThandling and correctly skip allfdATchunks that belong to the same frame. - Add
SkipRemainingFrameDataChunks()to consume continuationfdATchunks and buffer the first non-fdATchunk for the main loop. - Add regression tests and a new APNG fixture to validate Identify frame counting.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/ImageSharp/Formats/Png/PngDecoderCore.cs |
Fix Identify’s fdAT skipping and add helper to consume continuation fdAT chunks per frame. |
tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs |
Add tests asserting Identify reports correct APNG frame counts (fixture + generated images). |
tests/ImageSharp.Tests/TestImages.cs |
Register new animated PNG test asset path constant. |
tests/Images/Input/Png/animated/issue-animated-frame-count.png |
New Git LFS APNG fixture used by regression test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| oid sha256:62d51679bcb096ae45ae0f5bf874916ad929014f68ae43b487253d5050c8b68b | ||
| size 13561079 |
There was a problem hiding this comment.
The new LFS test asset is ~13.6 MB (size 13561079), which is orders of magnitude larger than other APNG fixtures in tests/Images/Input/Png/animated (eg apng.png is 2,508 bytes). This will noticeably increase Git LFS fetch time and CI bandwidth; consider generating a smaller deterministic APNG in the test (eg with a large, uncompressible frame to force multiple fdAT chunks) or reducing the fixture size while still reproducing the multi-fdAT-per-frame scenario.
| oid sha256:62d51679bcb096ae45ae0f5bf874916ad929014f68ae43b487253d5050c8b68b | |
| size 13561079 | |
| oid sha256:<sha256-of-regenerated-smaller-apng> | |
| size <size-of-regenerated-smaller-apng> |
| // 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; |
There was a problem hiding this comment.
SkipChunkDataAndCrc(chunk) + SkipRemainingFrameDataChunks(buffer) keeps Identify aligned when processing fdAT, but this logic is bypassed when frameCount >= this.maxFrames (the early break at the top of the FrameData case). Because TryReadChunk() restores the stream position to the start of the fdAT data for FrameData chunks, skipping is still required even when you stop collecting frame metadata; otherwise subsequent chunk reads start mid-payload. Consider ensuring fdAT data is skipped (or exiting Identify) in the maxFrames path too.
The Identify method had two bugs when processing fdAT (FrameData) chunks:
A spurious Skip(4) before SkipChunkDataAndCrc caused the stream to be misaligned by 4 bytes, since chunk.Length already includes the 4-byte sequence number.
Unlike Decode, which consumes all fdAT chunks for a frame in one shot via ReadScanlines + ReadNextFrameDataChunk, Identify processed them individually, calling InitializeFrameMetadata for each chunk and inflating the frame count.
The fix removes the extra Skip(4) and adds SkipRemainingFrameDataChunks to consume all continuation fdAT chunks for a frame, mirroring how ReadNextFrameDataChunk works during decoding.
Prerequisites
Description