Improvements to Apple CgBI PNG handling by Erik-White · Pull Request #3137 · SixLabors/ImageSharp · GitHub
Skip to content

Improvements to Apple CgBI PNG handling - #3137

Merged
JimBobSquarePants merged 15 commits into
SixLabors:mainfrom
Erik-White:png-cgbi-improvements
Jul 23, 2026
Merged

Improvements to Apple CgBI PNG handling#3137
JimBobSquarePants merged 15 commits into
SixLabors:mainfrom
Erik-White:png-cgbi-improvements

Conversation

@Erik-White

@Erik-White Erik-White commented May 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Following on from #3136 , this attempts to streamline and improve the related code as well as improve test coverage.

  • CgBI images that are not supported are now explicitly rejected (i.e. bit depth != 8 or truecolor)
  • Broke out ChunkedReadStream from ZlibInflateStream to separate the concerns instead of mixing modes in ZlibInflateStream
  • Broke out CgBI transform from PngDecoderCore into new PngCgbiProcessor static class
  • Added tests for PngCgbiProcessor
  • Small improvements to the CgBI shuffle methods (e.g. construct the shuffle patterns only once, remove redundant floor(), replace magic numbers where possible)

@Erik-White Erik-White mentioned this pull request May 27, 2026
Merged
4 tasks done
@Erik-White

Erik-White commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

@JimBobSquarePants

JimBobSquarePants commented May 27, 2026

Copy link
Copy Markdown
Member

This looks like a lot of changes. Will take time to review

@Erik-White

Erik-White commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

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?

JimBobSquarePants
JimBobSquarePants reviewed Jun 11, 2026
View reviewed changes
Comment thread tests/ImageSharp.Tests/Formats/Png/PngCgbiProcessorTests.cs Outdated
JimBobSquarePants
JimBobSquarePants reviewed Jun 11, 2026
View reviewed changes
Comment thread tests/ImageSharp.Tests/Formats/Png/PngCgbiProcessorTests.cs Outdated
JimBobSquarePants
JimBobSquarePants reviewed Jun 11, 2026
View reviewed changes
Comment thread src/ImageSharp/Compression/Zlib/ZlibInflateStream.cs
JimBobSquarePants
JimBobSquarePants reviewed Jun 11, 2026
View reviewed changes
Comment thread src/ImageSharp/Compression/Zlib/ZlibInflateStream.cs Outdated
/// Adler-32 trailer is not validated.
/// </summary>
internal sealed class ZlibInflateStream : Stream
internal sealed class ZlibInflateStream : IDisposable

@JimBobSquarePants JimBobSquarePants Jun 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Erik-White Erik-White Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@JimBobSquarePants JimBobSquarePants Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ZlibInflateReader is best IMO.

@JimBobSquarePants

JimBobSquarePants commented Jun 11, 2026

Copy link
Copy Markdown
Member

replace magic numbers where possible
What magic numbers?

@Erik-White

Erik-White commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

replace magic numbers where possible
What magic numbers?

See this commit: e6f09d6
Just tried to use existing values where possible

@JimBobSquarePants

JimBobSquarePants commented Jul 23, 2026

Copy link
Copy Markdown
Member

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

@JimBobSquarePants
JimBobSquarePants marked this pull request as ready for review July 23, 2026 08:25
Copilot AI review requested due to automatic review settings July 23, 2026 08:25
Copilot AI reviewed Jul 23, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 PngCgbiProcessor with dedicated scalar/SIMD coverage tests.
  • Extract chunk-boundary reading into ChunkedReadStream, simplifying ZlibInflateStream and 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.

Comment thread src/ImageSharp/Formats/Png/PngDecoderCore.cs
Copilot AI mentioned this pull request Jul 23, 2026
Closed
This was referenced Oct 15, 2026
Open
Open
Open
Open
Open
Open
Open
Merged
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
@Erik-White
Erik-White deleted the png-cgbi-improvements branch October 19, 2026 08:56
This was referenced Oct 19, 2026
Open
Merged
Closed
Closed
Open
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JimBobSquarePants JimBobSquarePants JimBobSquarePants left review comments
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

v4.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Erik-White @JimBobSquarePants