You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Backlog feature request: CanvasNet supports GIF loading (decode-only, first frame) but \ImageInfo\ didn't report how many frames a GIF actually contains, making it hard for callers to know upfront that only the first frame of a multi-frame GIF will be loaded.
Changes
Added \ImageInfo.FrameCount\ (init-only property, default \1, following the same IL-signature-preserving pattern as the existing \CanDecode\ property) to the shared \ImageInfo\ record struct used by all 5 raster codecs.
\GifCodec.GetInfo\ now performs true frame counting by walking the GIF block structure (image descriptors) and skipping each frame's LZW-compressed sub-blocks, reusing existing block-walking helpers rather than fully decoding each frame.
BMP/PNG/TIFF/JPEG's \GetInfo\ implicitly report \FrameCount == 1\ (the default).
Preserved both documented \ImageInfo\ invariants: \GetInfo\ never enforces \Surface.MaxDimension, and never throws for input \Load\ would accept; malformed GIFs still throw \InvalidDataException\ from \GetInfo\ consistent with \Load.
Updated design/verification/user-guide docs and ReqStream requirements traceability (new \CanvasNet-Lib-GifFrameCount/\CanvasNet-Codecs-GifFrameCount\ requirements, modeled on the \CanDecode\ precedent) and README.
Testing
Added unit tests: single-frame GIF reports \FrameCount == 1; multi-frame GIF reports the correct count; malformed GIF still throws \InvalidDataException\ from \GetInfo.
Full suite: 4104 -> 4122 tests, all passing (net8.0/net9.0/net10.0).
Adds ImageInfo.FrameCount (init-only, defaults to 1, same pattern as CanDecode) and
implements genuine frame counting in GifCodec.GetInfo by walking the GIF block
structure, skipping (never decoding) each frame's LZW-compressed data via the
existing sub-block-skipping helpers. Refactors Load's ImageSeparator handling into
shared ReadImageDescriptorHeader/ValidateFrameRegionAndResolveColorTable helpers so
Load and the new CountFrames walk apply identical per-frame structural validation.
- GetInfo still never enforces Surface.MaxDimension and never throws for input Load
would accept; it now walks the full block structure (previously only the Logical
Screen Descriptor) but still never invokes the LZW decoder for any frame - not even
the first - so a first-frame LZW corruption that makes Load throw does not make
GetInfo throw.
- Frame counting reuses the existing MaxTotalSubBlockBytes cumulative budget shared
with Load, introducing no new unbounded-loop/resource-exhaustion risk (documented in
code and design docs).
- BMP/PNG/TIFF/JPEG report FrameCount == 1 automatically via the ImageInfo default (no
source changes required; verified by grep and by new assertions on their existing
GetInfo happy-path tests).
Tests: added GifCodec_GetInfo_SingleFrame_ReportsFrameCountOne,
GifCodec_GetInfo_MultiFrame_ReportsCorrectFrameCount,
GifCodec_GetInfo_SecondFrameMissingColorTable_ThrowsInvalidDataException, and
GifCodec_GetInfo_NeverDecodesLzwPixelData_AcceptsCorruptFirstFrameCompressedData
(replacing the now-invalid GifCodec_GetInfo_NeverReadsPixelData). Updated
GifCodec_GetInfo_ReturnsExpectedDimensionsChannelsAndCanDecode and
GifCodec_GetInfo_OversizedDimensions_ReturnsRawValue_ButLoadThrows to include a full
valid frame + Trailer, since GetInfo now walks the whole block structure. Added a
FrameCount column to GifFixtureTests' fixture-corpus GetInfo test (1 for solid-color
fixtures, 3 for animated fixtures, confirmed via Pillow n_frames). Added
ImageInfoTests coverage for FrameCount default/override/default(ImageInfo) caveat.
Docs: updated codecs.md and gif-codec.md design docs, gif-codec.md verification doc,
gif-codec.yaml requirements (corrected the outdated GetInfo requirement title, added
FrameCount and GetInfoFrameValidation requirements), user_guide/introduction.md, and
README.md.
Corrects the requirements-traceability quality-gate failure from the prior
review (.agent-logs/quality-gif-framecount-7e2a19f4.md):
- docs/reqstream/canvas-net/codecs.yaml and docs/reqstream/canvas-net.yaml:
CanvasNet-Codecs-GifGetInfo / CanvasNet-Lib-GifGetInfo still referenced the
stale, renamed test GifCodec_GetInfo_NeverReadsPixelData, causing both
requirements to be reported unsatisfied by `dotnet reqstream --enforce`
against real trx evidence. Updated to the renamed
GifCodec_GetInfo_NeverDecodesLzwPixelData_AcceptsCorruptFirstFrameCompressedData
and reworded title/justification to mention FrameCount.
- Added new sibling subsystem-level requirement CanvasNet-Codecs-GifFrameCount
and system-level requirement CanvasNet-Lib-GifFrameCount, following the
CanDecode precedent (commit 5feced7), with children: decomposition reaching
the two previously-orphaned unit-level requirements
CanvasNet-Codecs-GifCodec-FrameCount and
CanvasNet-Codecs-GifCodec-GetInfoFrameValidation.
- docs/reqstream/canvas-net/codecs/gif-codec.yaml: reworded
CanvasNet-Codecs-GifCodec-GetInfoFrameValidation's justification to remove
the false blanket "GetInfo never accepts what Load would reject" claim
(GetInfo does tolerate first-frame LZW corruption Load rejects, and never
enforces Surface.MaxDimension), scoping it to the structural per-frame
validation (region bounds, color-table resolution) this requirement
actually covers.
Verified: `dotnet test --report-trx` (4122/4122 passing) followed by
`dotnet reqstream --requirements requirements.yaml --tests "TestResults/**/*.trx"
--matrix ... --enforce` now reports 0 unsatisfied and 0 orphaned requirements
for GIF/FrameCount (previously 2 unsatisfied + 2 orphaned). Remaining
unsatisfied requirements are pre-existing CI-pipeline-evidence gaps
(Quality-*, Platform-*, OTS-*) unrelated to this change and reproduce
identically on the base commit. pwsh ./build.ps1, ./fix.ps1, and ./lint.ps1
all pass cleanly with zero regressions.
Skipping every frame's LZW data allows GetInfo to return CanDecode == true for a GIF whose compressed pixel stream is invalid; the new GifCodec_GetInfo_NeverDecodesLzwPixelData_AcceptsCorruptFirstFrameCompressedData test demonstrates that Load throws on the same bytes. This contradicts ImageInfo.CanDecode's existing contract (it is true when a subsequent Load is expected to succeed, src/DemaConsulting.CanvasNet/Codecs/ImageInfo.cs:149-160) and gives callers a false capability result. Either validate the first frame's compressed data before returning, or revise the CanDecode contract/result and its documentation/tests rather than retaining true here.
Clarify malformed GIF handling in PR summary and requirements
This new test intentionally proves that malformed LZW pixel data is accepted by GetInfo while Load throws. That contradicts the PR summary's unqualified claim that malformed GIFs still throw from GetInfo; please clarify the summary and requirements as applying only to structural malformation, or change the implementation if all malformed GIFs must be rejected.
…contract, correct docs
- GifCodec.GetInfo now attempts the same first-frame LZW decode Load performs
(discarding the decoded output instead of resolving it into a Surface), so
CanDecode is false when the first frame's compressed data is corrupt in a
way that would make Load throw - restoring ImageInfo.CanDecode's documented
contract for this case, mirroring PngCodec's Adam7-interlacing precedent.
- Frames after the first are still only skipped (never buffered or decoded),
matching Load's own decode-only-the-first-frame scope.
- Rewrote the misleading GifCodec_GetInfo_NeverDecodesLzwPixelData_... test
as GifCodec_GetInfo_CorruptFirstFrameLzwData_ReportsCanDecodeFalse, and
added GifCodec_GetInfo_CorruptLaterFrameLzwData_StillReportsCanDecodeTrue
to confirm later-frame corruption does not affect CanDecode or FrameCount.
- Updated GifCodec.cs/ImageInfo.cs XML docs, the GIF codec design doc, the
cross-codec codecs.md design doc, the reqstream requirements/evidence at
all three tiers (canvas-net.yaml, codecs.yaml, gif-codec.yaml), the
verification doc, and the user guide to accurately describe the new
CanDecode behavior and the full, corrected InvalidDataException case list
for GetInfo (structural malformation only; corrupt LZW payload data is
reported via CanDecode = false instead).
- CountFrames now checks every first-frame decoded LZW index against the
resolved color table length (the same bounds check BlitIndexedFrame
performs during Load), reporting CanDecode = false for a structurally
valid LZW stream that decodes to an out-of-range palette index, instead
of incorrectly reporting CanDecode = true.
- Updated GifCodec.cs XML doc remarks (type-level, MaxTotalSubBlockBytes,
GetInfo, CountFrames) to describe this additional validation.
- Corrected README.md GIF example wording: GetInfo decodes the first
frame's LZW pixel data to validate CanDecode but never resolves it into
a rendered Surface.
- Updated design doc, verification doc, and reqstream yaml for GifCodec to
describe the new index-bounds check.
- Added GifCodec_GetInfo_FirstFrameIndexOutOfRangeForColorTable_ReportsCanDecodeFalse
test asserting CanDecode is false for this case.
This new GIF bullet leaves the generic Header-Only Probing bullet immediately below claiming that GetInfo reads headers without decoding pixels. GIF GetInfo now scans the whole file and decodes the first frame's indices, so please qualify the generic README claim (or explicitly call out this GIF exception) to avoid misleading callers about the cost and safety of probing.
Document GIF GetInfo's full scan and first-frame decode
docs/design/canvas-net/codecs.md:115
The new GIF behavior makes the generic probing claim above this section inaccurate: GetInfo now reads the entire GIF and decodes/allocates the first frame's index buffer, rather than inspecting only dimensions and channel layout without paying the decode cost. Please qualify that cross-codec contract with the GIF exception so callers do not rely on this section as a decompression-bomb safeguard.
Because GetInfo deliberately bypasses Surface.MaxDimension, a frame descriptor can still declare dimensions up to 65535 here. The int multiplication can overflow for inputs such as 50,000×50,000 before DecodeGifLzw runs, and even non-overflowing large products make that decoder allocate an output array proportional to the untrusted frame size (DecodeGifLzw allocates new byte[expectedIndexCount]). That can produce an uncaught overflow exception or an out-of-memory failure instead of returning raw info/CanDecode or the documented InvalidDataException; keep the validation streaming/bounded or guard the product and allocation before calling the decoder.
- CountFrames widens header.Width * header.Height to long before use and
skips the first-frame LZW-decode-validation attempt (leaving CanDecode
true) when that product would exceed Surface.MaxDimension squared,
preventing an int overflow or an unbounded allocation from an
attacker-controlled declared frame size. Load is unaffected since it
already enforces Surface.MaxDimension before reaching this code path.
- Add GifCodec_GetInfo_PathologicallyLargeFirstFrame_SkipsValidationAndReportsCanDecodeTrue
covering the new bound.
- Document the GIF exception to header-only probing in README.md and
docs/design/canvas-net/codecs.md (and its GifCodec unit-design
companion) now that GetInfo scans the whole file and decodes/validates
the first frame's compressed data.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Unresolved GIF processing and ImageInfo contract issues remain.
Review effort: Lite Findings: None
Previously missed (2)
In code that hasn't changed since last review
Qualify GetInfo's non-decoding description for GIFs
docs/user_guide/introduction.md:290
The shared description above still calls GetInfo a header-only probe that never decodes pixel data, but this newly documented GIF behavior calls DecodeGifLzw for the first frame. Please qualify that shared statement for GIF as well; otherwise callers are promised a cheap, non-decoding probe that this implementation no longer provides.
The public type summary still describes ImageInfo as being reported without decoding any pixel data, while the new GIF contract below now decodes the first frame to determine CanDecode. The added GIF explanation does not correct that summary, so generated API documentation remains contradictory; qualify the summary for GIF as well.
This makes every GIF GetInfo call buffer and fully LZW-decode the first frame, allocating an index buffer up to Surface.MaxDimension² (about 64 MiB) even when the caller only needs FrameCount. That is a substantial latency/memory regression from the previous header-only probe and is not required to count frames; keep the frame walk structural-only (or expose decode validation separately) unless this cost is an explicit API contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Backlog feature request: CanvasNet supports GIF loading (decode-only, first frame) but \ImageInfo\ didn't report how many frames a GIF actually contains, making it hard for callers to know upfront that only the first frame of a multi-frame GIF will be loaded.
Changes
Testing