image icon indicating copy to clipboard operation
image copied to clipboard

Expose encoder_for_format() publicly

Open Shnatsel opened this issue 4 months ago • 3 comments

In wondermagick I want to be able to call set_icc_profile and set_exif_metadata on an encoder without a match that enumerates every single format. That requires a public API to go from a format to an impl Encoder. This PR adds such an API.

TODO: trait ImageEncoderBoxed isn't public, so this is not shippable as-is. Any ideas on how best to handle that?

Shnatsel avatar Aug 24 '25 22:08 Shnatsel

What about a strong opaque type around Box<dyn ImageEncoderBoxed>? It could implement a deref(-mut) into the dyn ImageEncoder even. (Edit: and a constructor from T: ImageEncoder if you feel like that is necessary for usability).

197g avatar Aug 25 '25 21:08 197g

My initial reaction was that we might want to have an encoding analog of ImageReader rather than just a method on IamgeFormat, but maybe that's not necessary?

As far as return type, is it possible to make the method return impl ImageEncoder and return an opaque wrapper struct?

And if we're not tracking it anywhere, we should make a note to have ImageEncoder be dyn-compatible in 1.0

fintelia avatar Aug 25 '25 23:08 fintelia

I'm not completely certain we want the trait to be dyn-compatible. The main purpose is to facilitate writing bindings correctly for which the consumption is required. The use of those encoders requires them to be boxed but to me that is separate from the purpose of facilitating bindings. It's not completely unreasonable to leave it dyn-incompatible as long as we have an internal conversion.

Afterall, Box<dyn _> is one language-internal way of making a trait usable with type erasure. The way we've implemented it now is another. As long as implementation costs of the approach remain low, I don't see too much reason to change it. There's certainly encoders for which writing non-destructively would be weird to implement, it may be better to have ImageEncoder deal with it. How to interact with an AnimationEncoder seems the harder question. Maybe there's an argument for dyn compatibility found there? Optional downcasting is specific to the type erasure, i.e. if we have a trait method to_animation(self: Box<Self>) -> Result<Box<dyn AnimationEncoder>, Self> we can only downcast Box<dyn AnimationEncoder>, nothing else.

197g avatar Aug 26 '25 09:08 197g