-
Notifications
You must be signed in to change notification settings - Fork 145
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
base: master
Are you sure you want to change the base?
Performance: Add an internal skip_frame_decoding
flag to DecodeOptions
.
#533
Conversation
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. |
2ffbcb0
to
4c2e2d1
Compare
Let me explain the problem that was caught by
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 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)
4c2e2d1
to
84b2d37
Compare
In the force push above I am tweaking this PR one more time to explain this errors-ignoring-behavior in a doc comment of |
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 I think part of the issue is that |
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 inSkPngRustCodec
- 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?