The main change is a clarification to the responsibilities; the trait is an interface from an implementor towards the image library. That is, the protocol established from its interface should allow us to drive the decoder into our buffers and our metadata. It is not optimized to be used by an external caller which should prefer the use of ImageReader and other inherent methods instead.

With this framing, I think Limits::max_image_width and Limits::max_image_height no longer need to be communicated to or handled by the ImageDecoder trait, because the external code can check ImageDecoder::dimensions() before invoking ImageDecoder::read_image(); only the memory limit (Limits::max_alloc) is essential. That being said, the current way Limits are handled by ImageDecoder isn't that awkward to implement, so to reduce migration costs keeping the current ImageDecoder::set_limits() API may be OK.

A couple thoughts...

I do like the idea of handling animation decoding with this same trait. To understand, are you thinking of "sequences" as being animations or also stuff like the multiple images stored in a TIFF file? Even just handling animation has some tricky cases though. For instance in PNG, the default image that you get if you treat the image as non-animated may be different from the first frame of the animation. We might need both aread_image and aread_frame method.

The addition of aninit method doesn't seem like it gains us much. The tricky part of our currentnew+set_limits API is that you get to look at the image dimensions and total output size in bytes when deciding what decoding limits to set. Requiringinit (and by extensionset_limits) to be called before reading the dimensions makes it basically the same as just having awith_limits constructor.

Requiring init (and by extension set_limits) to be called before reading the dimensions makes it basically the same as just having a with_limits constructor.

It's a dyn-compatible way that achieves the goal of the constructor so it is actually an abstraction.

The tricky part of our current new+set_limits API is that you get to look at the image dimensions and total output size in bytes when deciding what decoding limits to set.

What do you by this? The main problem inpng that I'm aware of is the lack of configured limits for reading the header in theImageReader path, that motivated the extra constructor in the first place. Withpng we can not modify the limits after the fact but also we don't really perform any large size-dependent allocation within the decoder.

I'm also not suggesting that callingset_limits after the layout inspection would be disallowed but obviously is decoder dependent on whether that 'frees' additional capacity. I guess if that is sufficient remains to be seen? When we allocate a buffer (with applied allocator limits)´ that allows forwarding the remaining buffer size to the decoder. Or, set aside a different buffer allowance for metadata vs. image data. Whatever change is necessary inpng just comes on top anyways, theinit flow just allows us to abstract this and thus apply it with an existingBox<dyn ImageDecoder> so we don't have to do it all before. Indeed, as the comment on size eludes to we may want to different limit structs: one user facing that we use inImageReader and one binding-facing that is passed toImageDecoder::set_limits. Then settings just need to be

@fintelia This now includes the other changes including toImageReader as a draft of what I meant in#2679 (comment). In short:

• The file guessing routines and theconstruction of the boxed decoder are split into a separate type,ImageFile, which provides the previous methods ofImageReader but also aninto_reader for the mutable, stateful interface.
• ImageReader:
  • features all accessors for metadata considering that some formats fill said metadata (or a pointer to it) after an image is decoded.
  • hasviewbox as a re-imagining of the previousImageDecoderRect trait but split into two responsibilites: the trait does theefficiency decision on an image-by-image basis with an interface that allows a partial application of the viewbox (in jpeg and tiff we would decode whole tiles); then the reader takes care of translating that into an exact layout. Note that another type of image buffer with offset+rowpitch information could do that adjustment zerocopy—I still want to get those benefits of the type erased buffer/image-canvas someday and this fits in.
• The code also retrieves the CICP from the color profile and annotates theDynamicImage with it where available. For sanity's sake themoxcms integration was rewritten to allow a smaller dependency to be used here, I'll split these off the PR if we decide to go that route.
• Conceivably there's again_map (or similar) that may be queried similar to the metadata methods. For that to be more ergonomic I'd like to seriously considerread_plane for, in tiff lingo, planar images as well as associated and non-associated mask data; and more speculatively other extra samples that are bump maps? uv? true cmyk?. While that does not necessarily all go into1.* for any output that is not quite neatly statically sorted and sized as anRgba 4-channel-homogeneous-host-order, I imagine it will bemuch simpler for a decoder to provides its data successively in multiple calls instead of a contiguous large byte slice. Similar toviewbox we'd allow this whereImageReader provides the compatibility to re-layout the image for the actual user—except where explicitly instructed. AdjustingImageReader::decode to that effect should be no problem in principle.

I can't speak about image metadata, but I really don't like the newImageDecoder interface as both an implementor of the interface and a potential user of it. Right now, it's just not clear to me at all how decoders should behave. My problems are:

1. init. This is just two-phase initialization and opens so many questions.
   • Do usershave to call it? The docs say "should" not "must" be called beforeread_image.
   • Are users allowed to call it multiple times? If so, the decoder has to keep track of whether the header has already been read.
   • Sinceinit returns a layout, what's the point ofdimensions() andcolor_type()? And what if they disagree?
   • What shoulddimensions and co do beforeinit is called?
   • Ifinit fails, what should happen to methods likedimensions andread_image? When called, should they panic, return an error, return default values?
   • After callingread_image, do you have to re-init before callingread_image again?
2. viewbox makes it more difficult to implement decoders.
   • Now they always have to internally keep track of the viewbox rect if rect decoding is supported.
   • After callingviewbox, what shoulddimensions be? If they should be the viewbox size, should they reflect the new viewbox even before callinginit?
   • It's not clear what should happen ifviewbox returns ok, butinit errors.
   • What should happen if users supply a viewbox outside the bounds of the image?
3. When callingviewbox, is the offset of the rect relative to the (0,0) of the full image or the last set viewbox?
4. What should happen ifread_image is called twice? Should it read the same image again, error, read the next image in the sequence? The docs don't say.
   • If the intended behavior is "read the same image again", then those semantics would force all decoders to requireSeek for the reader (or keep an in memory copy of the image for subsequent reads). Not an unreasonable requirement, but it should be explicitly documented.

Regarding rectangle decoding, I think it would be better if we force decoders to support arbitrary rects. That's because the current interface is actually less efficient by allowing decoder to support only certain rects. To read a specific rect that is not supported as is,ImageReader has to read a too-large rect and then crop the read image, allocating the memory for the too-large image only to throw it away. It isforced to do this, because of the API.

However, most image formats are based on lines of block (macro pixels). So we can do a trick. Decode a line according to the too-large rect, and then only copy the pixels in the real rect to the output buffer. This reduces the memory overhead for unsupported rects fromO(width*height) toO(width*block_height). Supported rects don't need this dance and can decode into the output buffer directly. I.e. that's kinda what DDS does.

And if a format can't do the line-based trick for unsupported rects, then decoders should just allocate a temp buffer for the too-large rect and then crop (=copy what is needed). This is still just as efficient as the bestImageReader can do.

For use cases where users can use rowpitch to ignore the exccess parts of the too-large rect, we could just have a method that gives back a preferred rect, which can be decoded very efficiently.

So the API could look like this:

traitImageDecoder{// .../// Returns a viewbox that contains all pixels of the given rect but can potentially be decoded more efficiently./// If rect decoding is not supported or no more-efficient rect exists, the given rect is returned as is.fnpreferred_viewbox(&self,viewbox:Rect) ->Rect{        viewbox// default impl}fnread_image_rect(&mutself,buf,viewbox) ->ImageResult{Err(ImageError::Decoding(Decoding::RectDecodingNotSupported))// or similar}

This API should make rect decoding easier to use, easier to implement, and allow for more efficient implementations.

1. init. This is just two-phase initialization and opens so many questions.

That was one of the open questions, the argument you're presenting makes it clear it should return the layout and that's it. Renamed tonext_layout accordingly. I'd like toremove the existingdimensions()/color_type methods from the trait as well. There's no point using separate method calls for communicating them.

• For use cases where users can use rowpitch, […]

  That is ultimately the crux of the problem. I'd say it's pretty much the only problem even though that does not appreciate the complexity. A lot of what you put forth is overly specific to solving one instance of it, obviously focusing on DDS. That's not bad but take a step back to the larger picture. There's no good way to communicate all kinds of layouts that the callercould handle: tiled, planar, depths, sample types …. With the information being exchanged right now, no-one can find a best-match between the requirements ofimage's data types (andLimits) and what the decoder can provide. This won't be solved by moving complexity into the decoders, we need to get structured information out of them primarily, then make that decision / handling the resulting byte data inimage's code.

• 2. viewbox makes it more difficult to implement decoders.

  The point of the default implementation in this PR is that it is purely opt-in. Don't implement the method for decoders that can not provide viewbox decoding and everything workscorrectly. The documentation seems to be confusing, point taken. We're always going to have inefficiencies, I'm for working through thedistinct alternative layouts that allow an optimization one-by-one. More importantly for this PR immediately is what outcome a caller may want and what interface would give it to them—in this case I've worked on the use-case of extracting part of an atlas.

• However, most image formats are based on lines of block (macro pixels). So we can do a trick.

  I'mnot designing anything in this interface around a singular "trick", that's the wrong way around. That is how we got here. That's precisely what createdImageDecoderRect, almost to the dot. Falsehoods programmer's assume about image decoding will lead that to this breaking down and to be horrible to maintain. The trick you mention should live in the decoder's trait impl and nowhere else and we can bring it back where appropriate and possible. (Note that if you do it for a specific format, some formats will be even more efficient and not require you decode anything line-by-line but skip ahead, do tiles, … That's just to drive home the point that you donot want to do this above the decoder abstraction but below it in theImageDecoder impl).

• It is forced to do this, because of the API.

  The decoder impls is onlyforced to do anything if we force it via an interface—this PR does not;read_image_rect(&mut self, buf, viewbox) does force a decoder to be able to handle all possible viewboxes—this PR does not. I'm definitely taking worse short-term efficiency over code maintenance problems—the latter won't get us efficiency in the long run either.

When calling viewbox, is the offset of the rect relative to the (0,0) of the full image or the last set viewbox?

It's suppose to be to the full image. Yeah, that needs more documentation and pointers to the proper implementation.

Resolving the naming question aspeek_layout, hopefully satisfactory for now.