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

Document the interaction betweeen Frames::collect_frames() and memory limits #2109

Open
Shnatsel opened this issue Jan 16, 2024 · 1 comment

Comments

@Shnatsel
Copy link
Contributor

image::Frames::collect_frames() currently does not honor memory limits.

I am modifying the underlying decoders to honor memory limits. GIF has landed in #2090, and I am working on PNG now. The limits in these decoders are enforced for the intermediate buffers plus one output frame at a time. However, collect_frames() accumulates the frames in memory and can easily exceed the memory limit.

We should at least document that this is the case.

Refactoring it to actually enforce the limits could potentially be possible, but that would be a longer-term project since it requires changes to the animation decoder trait, and would interact with the highly dynamic memory usage of the underlying decoders in non-tirival ways that I don't see a good way to implement off the top of my head.

Thoughts?

@fintelia
Copy link
Contributor

Yeah, that all sounds reasonable. The animation decoding API has always gotten less attention than other parts of the crate, so it could certainly be improved.

A big motivation for memory limits has been to ensure that the crate can actually by fuzz tested effectively. If you can trivially craft an image that triggers an OOM, then the fuzzer is going to find it almost instantly and you won't make much progress trying actually interesting inputs. As long as we can fuzz the underlying decoding logic, I'm less worried about having some convenience methods that potentially allocate unbounded amounts of memory (but ideally with documentation saying that!)

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

No branches or pull requests

2 participants