Improvements to Apple CgBI PNG handling - #3137
Conversation
|
Curious to hear your thoughts. The changes should all be relatively self-contained per commit in case there are sections that you don't think should be included. |
|
This looks like a lot of changes. Will take time to review |
|
Understood. As I mentioned in the description, each commit is fairly self contained but I could also break up the PR into a couple of smaller units if that would be helpful? |
| /// Adler-32 trailer is not validated. | ||
| /// </summary> | ||
| internal sealed class ZlibInflateStream : Stream | ||
| internal sealed class ZlibInflateStream : IDisposable |
There was a problem hiding this comment.
I'm not sure I'm comfortable with a type suffixed Stream that is no longer inherits Stream. This feels like we're violating runtime design guidelines.
There was a problem hiding this comment.
My thinking was to use composition over inheritance here, especially since most the of the stream methods on ZlibInflateStream just throw NotSupportedException anyway.
But I'm sure ZlibInflateStream could just inherit from ChunkedReadStream (which inherits from Stream) if that would be preferrable? Or would it make sense to just rename this to something like ZlibInflateReader?
There was a problem hiding this comment.
ZlibInflateReader is best IMO.
|
See this commit: e6f09d6 |
…/ImageSharp into png-cgbi-improvements
|
Sorry for the radio silence here @Erik-White I've had my head down on some complex drawing stuff. I'll give this one last pass now |
There was a problem hiding this comment.
Pull request overview
This PR refactors and strengthens Apple CgBI PNG decoding in ImageSharp by separating concerns (chunked reads vs zlib header parsing, and CgBI pixel transforms vs decoding) and by adding explicit rejection + tests for unsupported CgBI variants.
Changes:
- Reject unsupported CgBI images (non–8-bit or non-truecolor) with a clear
InvalidImageContentException. - Extract CgBI scanline transform logic into a new
PngCgbiProcessorwith dedicated scalar/SIMD coverage tests. - Extract chunk-boundary reading into
ChunkedReadStream, simplifyingZlibInflateStreamand enabling raw-DEFLATE handling for CgBI IDAT payloads.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/ImageSharp.Tests/TestImages.cs | Adds new synthetic CgBI fixtures for header-validation tests. |
| tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs | Adds Identify/Decode tests asserting incompatible CgBI headers are rejected. |
| tests/ImageSharp.Tests/Formats/Png/PngCgbiProcessorTests.cs | New tests validating SIMD/scalar parity for CgBI RGBA transform. |
| tests/Images/Input/Png/cgbi/colors-cgbi-palette.png | New LFS fixture for incompatible palette CgBI case. |
| tests/Images/Input/Png/cgbi/colors-cgbi-bitdepth16.png | New LFS fixture for incompatible 16-bit CgBI case. |
| src/ImageSharp/Formats/Png/PngDecoderCore.cs | Routes CgBI IDATs through raw DEFLATE + extracted processor; adds header validation hook. |
| src/ImageSharp/Formats/Png/PngCgbiProcessor.cs | New extracted CgBI scanline transform implementation (scalar + SIMD). |
| src/ImageSharp/Compression/Zlib/ZlibInflateStream.cs | Refactor: wrap zlib header parsing + deflate exposure over a chunked segment stream. |
| src/ImageSharp/Compression/Zlib/ChunkedReadStream.cs | New stream to read a sequence of length-delimited segments via a delegate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Handle CgBI chunks consistently when they appear after IHDR by routing both identify/decode paths through a new `ReadCgbiChunk()` method that immediately validates already-read headers. This prevents invalid CgBI images from bypassing compatibility checks when chunk order is unusual. Added PNG tests that reorder the first two chunks to confirm incompatible CgBI headers now throw in both identify and decode flows. Also renames `ZlibInflateStream` to `ZlibInflateReader` and updates EXR/PNG/TIFF call sites for clearer intent.
Prerequisites
Description
Following on from #3136 , this attempts to streamline and improve the related code as well as improve test coverage.