search

image-rs / image

Encoding and decoding images in Rust
Apache License 2.0
4.98k stars 618 forks source link

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

Open Shnatsel opened 10 months ago

Shnatsel commented 10 months ago

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 commented 9 months ago

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!)