Skip to content

Conversation

@Shnatsel
Copy link
Member

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 Shnatsel requested a review from fintelia August 24, 2025 22:58
@197g
Copy link
Member

197g commented Aug 25, 2025

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).

@fintelia
Copy link
Contributor

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

@197g
Copy link
Member

197g commented Aug 26, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants