Repository navigation
Improve API design for Codecs #26
Description
Activity
- added this to the milestone
on Nov 29, 2016 I did a little bit of digging through the code on this, and had some thoughts and questions.
So, if I understand correctly, we want to have an options class - for encoders let's say for now - so
EncoderOptions.These options would be used in all
IImageEncoders inEncode, possibly like thispublic void Encode<TColor, TPacked>(Image<TColor, TPacked> image, Stream stream, IEncoderOptions encoderOptions)There seems to be some overlap in the options between encoders but encoders also have options that are specific to them, and therefore doesn't make sense to have in the generic
EncoderOptions- for example theBmpEncoderCoreusesBmpBitsPerPixel.One idea to solve this is to have a generic
IEncoderOptionswhich has all the common options - if any, and then have derived options for any other format specific ones.I tried this out in a branch for
BmpEncoder- it works, but I think it sucks a little bit because of the casting to the concrete type inBmpEncoder. Also, it would be good if the core encoders could actually beIImageEncoders and share an interface - which using this they could do if theIEncoderOptionsis added to the Encode interface, but then the cast would have to move inside the Core object. Which maybe is OK.Thoughts?
@olivif I am working on some changes to the encoders. I used different naming but I will use yours to explain what I did. I added an
IEncoderOptionsargument to the Encode method and added andIBmpEncoderOptionsargument in the SaveAsBmp method but your idea got me thinking. The encoder method has two overloads. One with IEncoderOptions and one with IBmpEncoderOptions. I could try to cast the object that is passed in to the IEncoderOptions overload and call the IBmpEncoderOptions overload if that works. Then someone could creat a single object that implements the various encoder options. The only issue with that is that you have some kind of hidden functionality. I would rather make it so people can see it in the API.The encoder method has two overloads. One with IEncoderOptions and one with IBmpEncoderOptions.
Will the
IBmpEncoderOptionsoverload be in theIEncoderinterface? If it is, then it would be a bit odd to have format specific stuff in there. If it's not, it would also be kind of odd since you wouldn't be enforcing the specific encoding method on let's say a new type of encoder.Then someone could create a single object that implements the various encoder options.
Hmm this got me thinking as well. We are trying to solve this using an approach that involves passing in the options at "encoding time", so when calling encode. Here's another idea, what if we push the options initialization into the constructor? Right now even with this
Encodeapproach it's half in ctor half in the method, since you have some defaults anyway. So, we push the responsibility of initializing the encoder with the correct configuration in the ctor. Whoever knows the concrete type will know they need to pass in a Bmp specific options and that seems a little less weird. The only challenge with this is that you need to create an encoder per encode operation, if you want different options. Not sure if that works with the other parts of the stack, haven't dug much into the upper layers.The
IBmpEncoderOptionswill only be available in the BmpEncoder. Moving it from a property to the constructor is prob a good idea and I will try and make that work. But that won't really change much for the rest of the API. The Save method:public Image<TColor> Save(Stream stream, IEncoderOptions options)will have the same signature.Yep, that sounds great to me! Clean interface and specific implementation options hidden in the implementation. Perfection ❤️
- added a commit that references this issue
on Jan 18, 2020
At present we have to pass many options to the codecs as parameters. There is also ambiguity over what exactly "Quality" means - Sometimes it's color count, sometimes compression.
We need to:
Qualityproperty fromImage<TColor, TPacked>Image<TColor, TPacked>that allows passing a specific decoder or decoders with options.