Unified PNG/APNG decoder - #3099
Open
RunDevelopment wants to merge 4 commits into
Open
Conversation
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
resolves #3038
My original plan was to slowly prepare for #3038 via a series of small PRs for PNG that would build the foundation for this more complex change. That did not work, because the first PR didn't get reviewed after being open for a month. So I got impatient and did all of this in 4 hours.
Commits
This PR is structured into 4 commits for reviewability. The first 3 prepare for the last commit which contains the main change.
Commit 1: I split the decoder and encoder logic into separate files.
png.rswas getting quite large and I wanted to make it obvious that only the decoder is changed in later commits.Commit 2: I changed the way color type/bits are parsed into
imagecolor types to make it more obvious which color types are actually supported by the decoder. SincePngDecoderandApngDecoderare tightly coupled, I also removed the check inApngDecoderthat re-verified the parsed color inPngDecoder. Both decoders support the same colors, so this check didn't do anything.Commit 3: I replaced the internal state
PngDecoder { decoder: Option<...>, reader: Option<...>, ... }with a 3-state enum. This removes the invalid statePngDecoder { decoder: Some(_), reader: Some(_) }and makes the logic ofensure_reader_and_headera lot easier to understand.Commit 4: Add the unified decoder and make the other two private.
The unified decoder
The basic idea is that the unified decoder is a wrapper around both
PngDecoder(single image) andApngDecoder(animation). It uses one of them depending on whether the underlying file is animated.Changes:
PngDecoder->BasePngDecoderimpl ImageDecoder for ApngDecoder. (It wasn't necessary anymore.)BasePngDecoderandApngDecoderprivate.PngDecoder. This has (almost) the same API as before. OnlyPngDecoder::apngwas removed.ApngDecoder::read_sequence_data->ApngDecoder::newand take an animation control struct as an argument to ensure an APNG decoder can only be constructed for animated files.num_playsinApngDecoder. This removes a possible error path inPngDecoder::animation_attributes(unified decoder).Notes:
impl ImageDecoder for BasePngDecodergot to stay, because it makes the implementation of (unified)PngDecodereasier.Open questions
PngDecoderawkward to use.ImageReader::into_frameswill work for APNG files but it will error for normal PNGs. Should sequence control work properly for single images too? (This essentially just means keeping track of whetherBasePngDecoderhas read its one and only image.) Related toImageReader::into_frameswill always error for most formats #3037PngDecoderwould read the thumbnail if present and the first frame otherwise. Now the unifiedPngDecoderwill always read the first frame. This means that it's currently impossible to read the thumbnail (if present). Given that there was no (real) way to tell whether a file had a thumbnail in the first place, the previous behavior was probably unintentional. Nonetheless, being able to read the thumbnail should be a feature IMO. This should be done with a dedicated API for it, though.