image-png
image-png copied to clipboard
Performance: Add an internal `skip_frame_decoding` flag to `DecodeOptions`.
PTAL?
This PR improves the performance of scenarios where Reader.next_frame_info is used to skip N frames (such scenarios may arise from the way how seeking to an arbitrary frame is implemented currently in SkPngRustCodec - see my earlier comment here and https://crbug.com/371060427).
It's not entirely clear to me how often the scenarios above occur in practice (i.e. what is the practical performance impact of this PR), but the PR is relatively simple and improves such scenarios by 90%, so it seems worth it?
Ooops... I can repro the fuzzing problem locally. Let me convert this PR into a "draft" for now. I'll ping if/when I am able to fix this. Sorry about the fuss.
Let me explain the problem that was caught by buf_independent fuzzer:
- The suffix of an
IDATpayload contains some bytes that are not needed to decompress the image data. 4 bytes of Adler 32 checksum are one example, but in general the payload may contain more pixels than is needed for the image. - When decompressor handles these extra bytes it may detect a
fdeflate-level format error (e.g. maybe an incorrect Adler 32 checksum, or maybe just a malformed data). - This PR stops passing these extra bytes to the decompressor. These changes the behavior of the
pngcrate (not just the internal behavior, but also the observable behavior - e.g. whether the given input results in errors or not; and the initial version of the PR meant that some chunking of input could results in errors and some other chunking would succeed).
The latest version of this PR (i.e. https://github.com/image-rs/image-png/commit/4c2e2d1e954fcf0e5907e6c11881f11967b5c5f9) makes the behavior chunking-independent by ensuring that if we started feeding a given IDAT sequence to fdeflate then we will feed all of IDAT payload. But if we call finish_decoding_image_data without extracting any image data from a given IDAT sequence, then we'll skip the decompressor. This still changes the behavior (decompressor errors are suppressed in the latter case), but does that in a more predictable way.
WDYT?
In the force push above I am tweaking this PR one more time to explain this errors-ignoring-behavior in a doc comment of next_frame_info (here). PTAL. I admit that this behavior looks a bit weird. But maybe this is all okay. WDYT?
I think this lack of checking whether the compressed data is valid is unfortunately an unavoidable consequence of not decompressing it.
In the GIF crate this has caused identical issue with detection of frames that contain too many LZW bytes. You just can't know that until you decompress it.
This PR strikes me as kind of messy. It adds state tracking at the level of ReadDecoder which is redundant with what StreamingDecoder already knows. And it uses that state to toggles settings in the underlying StreamingDecoder.
I think part of the issue is that StreamingDecoder is a public API, so we haven't been iterating on its interface. I've got a branch where I've been experimenting with adding an alternative interface that is a more direct mapping to ReadDecoder. (It is still very much WIP, but I think the code is shaping up to be cleaner and the early performance results are also promising)