Improve ImageDecoder trait

[fintelia]

This sketches out possible changes we may want to make to the ImageDecoder trait in the next major release:

• Makes the trait object safe
• Adds a constructor (all non-deprecated implementations had one, but it wasn't part of the trait).
• Combines ImageDecoder and ImageDecoderRect
• Removes the into_reader, *_with_progress, and scanline_bytes methods. This is the aspect I'm least sure about, but few backends had meaningful implementations of these and it cuts down on the number of methods on the trait. Some decoders will still be able to support incremental decoding by doing multiple read_rect calls

The object safety change in particular makes our internal usage of the trait much nicer. Instead of having to work with a DecoderVisitor trait, we can rely on dynamic dispatch.

For now I've held on updating the various format decoders. That'll be a bunch of work so it makes sense to settle on what the API should be first.

Edit: scaled back some of these changes and updated all the format decoders.

[sophie-h]

While working on image-rs I got the idea that this trait could go into something like an image-decoder crate.

This would allow the format-specific crates to already implement ImageDecoder, which could then be used by image-rs. That could bring more of the format-specific code into one place and would give more overall flexibility with coupling things into image-rs and having a common abstraction trait already usable when a format crate is not yet used in image-rs.

[fintelia]

Yeah, it would be nice to restructure that way. The challenge is that we have to be very confident in the trait definitions we pick, because making any backwards incompatible changes to them would cause tons of ecosystem churn

[HeroicKatora]

The impact from a breaking change in another trait could be mitigated by some choices in usage. It's technically possible to depend on multiple versions of a single crate. What's hard is using the trait only in such ways that bumping to a new version is not a breaking change downstream.

For instance, consider an opaque wrapper that happens to implement From<Box<dyn image_core::ImageDecoder>>. Then adding a new version of the trait would be akin to having to add From<Box<dyn image_core_v2::ImageDecoder>> where image_core_v2 is just an alias for the new major version of the crate. And then we can be cautious with slowly deprecating the old versions. The main costs are compiling with multiple old versions that must be managed (and this pattern interacts badly with cargo vendor which may be used in some distributions? not quite sure).

[johannesvollmer]

I think a progress callback is an essential feature for such a mature library. It's fine to not have it for small prototypes, but any real ui app or game would want to display some kind of progress. Instead, we should work on adding it to the decoders and encoders. For hdr, for example, it would be trivial to implement.

I think it's more valuable to have it, even if support is rare, than to remove it and add it later again.

Here's something we could try: Code up a wrapper around std::io::Write and std::io::Read that tracks the number of bytes being written or read. Many codecs could use this to implement the progress callback quickly, albeit imprecise. I could try to sketch out that idea

[fintelia]

Retaining read_image_with_progress with tweaks to make it object safe wouldn't be too hard. And I do agree that in principle it could be useful (that's why we added it in the first place!). The two questions I have:

1. Is the interface right? I don't have a good sense of how useful decoding progress is without the ability to get partial image contents as they are ready. Or at a more basic level, is having a Progress struct worth it over just an f32 saying indicating the percent completed? And it makes me nervous that the many of the third-party format crates we're using or planning to use don't seem provide a way to get progress callbacks (including qoi, dav1d, and zune-jpeg).
2. Is there enough implementation interest? Adding APIs is a lot easier than implementing them for all the image formats. It really isn't great to have an interface that appears to work for everything but essentially amounts to a no-op for most of them

[johannesvollmer]

About your second point: There is a way to tack-on progress on existing decoders: Observe the bytes that they read. But its only meaningful if we can make two assumptions:

• the decoders implementation reads the file bytes incrementally
• the compressed file is roughly as large as the in-memory representation

if the assumptions don't hold, the progress can still be useful, but will be imprecise

[johannesvollmer]

i sketched the idea at #2014

about your first point: i think a progress struct is nice. a percentage float would probably be enough though, but it would only be worth it, if our goal is to reduce the complexity in the repo. if we accept the code complexity of the progress struct, we can just tack on a ratio_0_to_1() ->f32 on the Progress to make it nicer to use. another advantage of f32 progress is that existing third party implementations might only expose that information, and not expose the total byte count.

maybe struct Progress { ratio: f32, order_of_magnitude: Option<u64> }?

An advantage of current & total would be that if you have multiple images, the sum of all progresses could be computed more precisely.

[johannesvollmer]

into_reader, *_with_progress, and scanline_bytes

I think this is the most interesting part. Why don't most implementations provide that functionality? How can we take away the pain of implementing those features?

Looking at the implementation of the hdr decoder, which has a loop that goes through all the scan lines.

The implementation interest is something we cannot change. But we can make it easier for the implementations to add support for the features.

The think core problem might be this: Those functions make assumptions about the internal file format layout. They can be easily implemented if the file stores scanlines from top to bottom. In any other case, they is no easy way to implement that functionality, or it would be extemely inefficient to the point of uselessness (example: to_reader reads the whole file and then just returns all the bytes at once, defeating the point of providing a reader). Many formats don't even read scanlines, but tiles.

Maybe it would make sense to relax the requirements, by allowing the reader to read contents in arbitrary order, instead of forcing scanlines top to bottom. This might be a nice decider api to work with (but still cumbersome for implementations):

let decoder = open("x.jpg")?;
let reader = decoder.reader(); // or optionally .sub_rect_reader(rect)

let mut pixel_buffer = vec![0_u8, reader.total_byte_size()];

loop {
    let progress = reader.read_next(&mut pixel_buffer)?;
   // note: the decoder can write any number of pixels anywhere into the buffer, in whatever order they appear in the file.
   // this might be scanlines or tiles or even individual pixels, or even the whole image.
   // maybe the progress object also returns the location of the pixels that were changed?
   // the intermediate buffer and pixel coordinates can be used for an incremental image display

    if progress.done() { return Ok(pixel_buffer) }
}

This API would allow us to build a lot of convenience and abstraction on top of it.

trait Decoder {
    // ...
    fn read_next(&self, pixel_buffer: &mut [u8]) -> Result<Progress>;
}

// we know the total number of pixels, so knowing the indices of the pixels that changed is enough to compute a reliable progress ratio: progress.bytes_writtyen_to_pixel_buffer.len() as f32 / total_byte_size as f32;
enum Progress {
    ProcessedImageTile (TileIndices),
    ProcessedPixelBufferBytes (Range<usize>),
    // .. possibly more
    Done,
}

This design requires the decoders to keep some state, which might be cumbersome. Maybe we can abuse async/await syntax to write code that looks like a generator function?

This design also will not work with external decoders that currently have a closure/callback mechanism for the progress.

---

Unfortunately, this question will remain: What about the decoders that have external implementations and don't support any of this?

[fintelia]

Another thing that makes it annoying to convert decoders is that you want to reasonably throttle how often you call the progress callback. Too often and you'll destroy performance. Not often enough and the user won't get progress indications. Even just checking "should I invoke the callback" too often might interfere with SIMD and slow down our fastest encoders (which can hit multiple hundred million pixels per second!)

I also realized that the current function signature won't work when trying to make the trait object safe. No methods with generic arguments can be called on a trait object. At which point, perhaps we might as well make a separate ImageDecoderProgress trait