API changes required for supporting limits in `image`

[Shnatsel]

We have a long-standing ugly API and ugly workarounds in the code of image to compensate for missing error reporting in png API: image-rs/image#2084

The upcoming semver-breaking release v0.18 is an excellent opportunity to fix that.

[fintelia]

The PngDecoder struct was changed to returning ImageResult<bool> in 0.25.0 so that blocker mentioned in the linked issue has been resolved.

Unfortunately, there's another problem. The ImageDecoder trait in the image crate requires that the color type and output buffer size to be available before setting the decoding limits (and changing that would make the API much less pleasant to use). But in PNG that is actually somewhat inconvenient because if the IHDR chunk indicates "truecolor" as the color type, then the image is either RGB or RGBA depending on whether there is a tRNS chunk anywhere before the first IDAT. To determine the absence of a tRNS chunk you thus need to consume all the image metadata up to the first IDAT. However, one key reason we wanted decoding limits was to prevent the images with huge amounts of metadata to pull all that data into memory and trigger an OOM.

There's a few ways we could address this, but they pretty much all center around adding a Seek bound (which we already did in #558).

[Shnatsel]

So all changes to png crate necessary for setting limits have been made, and I can go ahead and close this?

[fintelia]

Thinking about it, the other thing we probably need to change is how the ICC profile, EXIF chunk, and text chunks are accessed. Right now they're public fields on the Info object, so the API requires them to be eagerly loaded with the rest of the metadata. Other than those three sorts of chunks the rest of the image metadata can be loaded in a constant amount of memory so it is fine to load them before the limits are known.

There can be only one ICC profile and EXIF chunk, so storing their location/size shouldn't be an issue. For text chunks, we already pass set_ignore_text_chunk so I guess that handles it.

[fintelia]

One puzzle will be how to handle Encoder::with_info. Currently it only takes an Info struct, so it wouldn't have access to the ICC or EXIF profile if those were switched to loading lazily

[197g]

If the Limits are an issue for ICC and EXIF I would propose that we (add an option / switch) to count their memory use towards the usual image data usage, instead of the much more restricted auxiliary chunks. That is, the caller should be able to decide which set of chunks is important enough for them to allocate persistently and consider part of the necessary return value. Then the eager metadata should not be much of a problem, the fault of applying them at loading time is the image trait in my opinion too.

While not a complete sketch of implementation, I do believe there is a possible implementation that preserves the API just fine and this can be closed.

[fintelia]

I didn't completely follow that. Is the idea that we would make Decoder::set_ignore_iccp_chunk save the stream position of the ICC chunk, and then add a method on Reader to seek to that location and read it? (And then seek back to the old position so other methods aren't impacted)