Assorted optimizations for GStreamer's image-rs plugin#2907
Conversation
|
|
||
| /// Return the ICC profile's name as embedded in the image. | ||
| /// | ||
| /// For formats that don't store embedded profiles with separate names this function should always return `Ok(None)`. |
There was a problem hiding this comment.
Are there formats other than PNG which do store names for the embedded profiles?
There was a problem hiding this comment.
That's the only one. I introduced this API, however, because I didn't know how to downcast from impl ImageDecoder to PngDecoder (and so access a decoder specific API).
There was a problem hiding this comment.
We may need a novel mechanism for this. Downcast ties it to the png version which conflicts with the requirement of being able to bump it (through breaking changes). On the other hand a new API on the trait is also rather awkward. Maybe something similar to Error::provide would fit the bill in letting us translate types provided by version-encumbered crates to stable types defined in image directly (that we then only let include the 'stable' options / data from the formats). The converse holds for encoder, another unsatisfied user ask.
There was a problem hiding this comment.
You're trying to cast between a Box<dyn ImageDecoder> and a image::codecs::png::PngDecoder, right?
For that, I think we might just be able switch to a Box<dyn ImageDecoder + Any> or make Any a super-trait of ImageDecoder?
There was a problem hiding this comment.
Any does not really solve the problem. It only makes the dependency problem a runtime error but I'd consider that worse than a breaking change everytime we bump dependencies. There's no reasonable way to use Any::downcast right here for downstream—they'd have to add fallback cases for potentially unpublished versions of our dependencies preemptively to avoid functionality breaking and that just pollutes the dependency graph greatly.
There was a problem hiding this comment.
No, if I'm understanding correctly, the goal isn't to downcast to the decoder struct from the png crate but rather to cast to the PngDecoder struct in this crate
There was a problem hiding this comment.
No, if I'm understanding correctly, the goal isn't to downcast to the decoder struct from the png crate but rather to cast to the PngDecoder struct in this crate
Correct, I already have to do something similar when dealing with image-extras formats that do not support guessing. It is also required when accessing the animation API IIRC, because only explicitly typed instances of AnimationDecoder allow calling into_frames.
There was a problem hiding this comment.
make Any a super-trait of ImageDecoder?
Any requires Self: 'static. Decoders are generic over readers and some useful readers (e.g. &[u8]) aren't 'static. So ImageDecoder: Any would be a major limitation.
It's also unclear to me how the downcast would even work. Say we did require ImageDecoder: Any, you can't downcast unless you know the exact reader type. E.g. image_decoder.downcast_ref::<images::codecs::PngDecoder<Reader>>(). Would that even be useful?
only explicitly typed instances of AnimationDecoder allow calling into_frames.
In 0.25.x, yes, but this will change soon with the new decoding API. ImageDecoder will handle single images, sequences, and animations. #2672
There was a problem hiding this comment.
Hm, yeah the 'static requirement does make that less workable
b28450e to
76ddbc3
Compare
Hi,
This is a MR to add a few things for feature parity in my GStreamer reader plugin:
iCCPchunk's profile nameread_imageBoth require upstream work, so marking as draft until those are landed: image-rs/image-png#679, mozilla/mp4parse-rust#446