Support limits in PNG animation decoding #2112
Conversation
…dd TODOs where updates to the limits on the underlying reader should go
…ed. This is consistent with other decoders. The limits don't do much right now anyway due to image-rs/image-png#286
| let info = self.reader.info(); | ||
| limits.check_dimensions(info.width, info.height)?; | ||
| self.limits = limits; | ||
| // TODO: add `png::Reader::change_limits()` and call it here |
There was a problem hiding this comment.
We can't really implement a change_limits method on the png side.
The problem is that many of the allocations in that crate happen when you create the png::Reader by calling png::Decoder::read_info because that's where the image metadata is read and saved into the inner Info struct. Ironically, the crate allocates relatively little memory for decoding pixel data, because it is quite good at decoding that data in a streaming fashion.
Rather, the current plan is to switch to calling Decoder::set_limits in this method, and only later call read_info when the user queries metadata beyond what's stored in the IHDR.
An alternative possibility would be for the png to actually not store metadata chunks as it reads them, but rather to store the offsets of those chunks, and then Seek past them. This would bound the memory used by the decoder while still enabling the metadata to be loaded later. In that case, the limits could safely be set later. Adding the Seek bound would, however, be a breaking change both here and in the png crate. (An added complication is that BufReader's seek() implementation has some performance gotchas. It should be possible to work around them once Seek::seek_relative is stabilized, but I don't know if/when that'll happen.)
There was a problem hiding this comment.
The idea is that change_limits() would look at the amount of memory already allocated by the png crate and report an error if the crate is already over budget. This would let us easily bail out of the total memory usage limit is exceeded (including all the metadata).
I don't think the Seek bound is a good idea (too limiting, largely defeats streaming decoding use cases) but a refactor to move the compositing logic to png and track the allocations there could be viable.
|
I think that at some point we should probably move the frame compositing logic into the |
|
Then let's go ahead and merge this. I plan to do some more work on the |
Implements memory limit for animation decoding and dimensions limits for individual animation frames, similar to how
it's done for GIF.
Notable changes:
reserve_bufferonLimitsto be reused in PNG and GIF animation decodersApngDecodernew()to default to no limits (consistently with other decoders) now that the limits are actually enforcedOut of scope of this PR:
pngcrate for it to limit its own allocations in animation decoding because the required APIs for that are missing frompngpngcrate actually enforcing the limits it's passed: Zlib decompression may exceed allocation limits image-png#286 (rendering the previous bullet point moot)is_apngis allowed to return an error (unless we're OK with it just returningfalseon an error or something?), seePngDecodersupports setting limits viawith_limitsbut not viaset_limits#2084Open questions:
pngcrate allocates large amounts of memory when reading just the headers, but I don't know if it does.output_buffer_size()function that may overflow #2111 be fixed in this PR or separately?