Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance: Add an internal skip_frame_decoding flag to DecodeOptions. #533

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

anforowicz
Copy link
Contributor

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?

@anforowicz
Copy link
Contributor Author

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.

@anforowicz anforowicz marked this pull request as draft November 14, 2024 21:33
@anforowicz anforowicz force-pushed the skip-frame-decoding-flag-in-streaming-decoder branch from 2ffbcb0 to 4c2e2d1 Compare November 15, 2024 00:48
@anforowicz
Copy link
Contributor Author

Let me explain the problem that was caught by buf_independent fuzzer:

  • The suffix of an IDAT payload 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 png crate (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. 4c2e2d1) 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?

This commit adds an internal `DecodeOptions::skip_frame_decoding` flag
and sets it to true from `ReadDecoder::finish_decoding_image_data`.
This results in the following performance gains when using the
`next_frame_info` public API to skip frames: `change: [-94.186% -94.170%
-94.157%] (p = 0.00 < 0.05)`

This commit is mainly motivated by https://crbug.com/371060427
and image-rs#510.  Hat tip
to @kornelski for suggesting this approach in
image-rs#510 (comment)
@anforowicz anforowicz force-pushed the skip-frame-decoding-flag-in-streaming-decoder branch from 4c2e2d1 to 84b2d37 Compare November 15, 2024 01:02
@anforowicz
Copy link
Contributor Author

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?

@anforowicz anforowicz marked this pull request as ready for review November 15, 2024 01:07
@kornelski
Copy link
Contributor

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.

@fintelia
Copy link
Contributor

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants