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? |
| [MemberNotNullWhen(true, nameof(CompressedStream))] | ||
| private bool InitializeInflateStream(bool isCriticalChunk) | ||
| { | ||
| // Apple CgBI IDATs omit the zlib CMF/FLG header and the Adler-32 trailer, |
There was a problem hiding this comment.
Why do we no longer return early?
There was a problem hiding this comment.
In my first CgBI PR I had added the noHeader parameter as a workaround for streams without headers that came from the CgBI images. It worked fine but I didn't like that it mixed concerns in ZLibInflateStream.
This PR now separates out the concerns so ZlibInflateStream always handles streams with headers, and ChunkedReadStream can be used for those without.
So that early exist logic now lives in PngDecoderCore instead of imposing on ZlibInflateStream:
https://github.com/SixLabors/ImageSharp/pull/3137/changes/BASE..4869c344b3e5c7df9e8b696b6cd6433230178a65#diff-e626c30d90afb58e53b15876a4ee894c9581334c8ddbb2fa2be18759a60aea59R780
There was a problem hiding this comment.
That means ChunkedReadStream is now a misnomer as it’s still reading zlib compressed stream is it not?
There was a problem hiding this comment.
It's just a stream wrapper and is fairly generic so in theory it could be used for other cases. But yes, right now it's only being used for zlib
| /// 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?
|
See this commit: e6f09d6 |
2 participants
Prerequisites
Description
Following on from #3136 , this attempts to streamline and improve the related code as well as improve test coverage.