Generic API for encoder options#3048
Conversation
|
Defining types local to Have you considered an alternatives? The single downcast is quite strange, you'd need to match the type on both sides with no typing in between. Also, there's no general case. The standard |
|
Thanks for taking a look. I'm also not 100% happy with the design yet. I just started with the high-level API I wanted ( Regarding not exposing more than we can maintain: Options (at least right now) don't expose anything new. It's just the options encoders already have packaged differently. Aside: Speaking of packaging, I'm also thinking about using this to unify the way encoders are configured. Right now, it's mess of API styles. Saying "all formats have an I also want to say that I wanted this options format to be extensible to third-party encoders à la #2916. I also wanted it to be extensible with other metadata too (e.g. XMP) but that didn't work out too well.
With the goal of providing a simple high-level API ( I also looked at other libraries.
I don't think these APIs translate well for us. However, one commonality is that they all support a lot of options. Probably because it's useful to users.
There is some typing with We could also add something like
Promising for what? We could attach bad options, but that's about it. Also nightly. |
Very nice, quite a wide selection of styles that would each not fit Rust's idiomatic implementation well. Yes, I'd also very much avoid OpenCV. The PIL dismissal stands out though, the PR also has "untyped" parameters but they are not named. While true that we can not name arguments in the call it is instead more feasible to use the type itself, which would be awkward in Python i.e. quality is a float and newtype wrapers are not idiomatic there. Also, the implication here of overloading everything at the top-level is that many options can be and are shared between multiple encoders, their name being convention as to meaning, while only some are in format-specific ones that require domain knowledge on the specific target. The flat hierarchy has some design consequences.
I think the general shape is fine! The main design problem is that the lack of clear motivating example. The usage you're talking about works only in a monomorphic setting: the caller needs to know it is jpeg so that the jpeg's option struct can be constructed, yet the main mechanism is one of making the type information runtime to the library. That's a strange contrast. Would it not be nicer to have the JpegOptions construct the encoder, if such value must exist anyways at that point? That's one less import and the well-established builder pattern. I don't think the implementation is wrong but that motivation does not provide design validation.
I'll preface this with a caution. Please don't read the below as inviting complexity. If it can't be done in a simple way, the current API is probably better :)
Yes, we'd bring our own struct here that could maybe verify if some option type was used. There's nothing special or even unsafe about it. As a solution to providing multiple options values of different types. I'd hoped that for a generic high-level option struct where mainly compression is defined roughly in terms of perception metric in the CQP sense that is used with much more depth in video (industry used of this was confirmed to me at RustWeek; there's special hardware with acceleration for choosing such parameters in video but we won't need that). Other shared parameters, such as "comment" blocks shared by multiple formats, could be similarly abstracted. Now, the high-level API would be amenable to this. Just attempt a downcast into different types afterall. But maybe we could do a little better in terms of API if this were not an afterthought? The decoder could then fetch those general options in addition to format specific ones. This would allow control over encoders even when the format choice is a runtime one. Then of course you could provide format specific options in addition. If the options were an owned parameter then Tiff could retain them and pass them on to the inner jpeg/webp encoder. |
Not really. As the user, you're supposed to know what format you're encoding with. Names are shared by necessity, not for semantics. E.g. Plus, all options require knowing the format. I.e.
I really like that. (I initially wanted to write that this would be impossible, because it requires GAT over types but those were stabilized a while ago. I'm even using return position impl trait in traits. The future is now!) This also solves the problem that options couldn't move data into the encoder, since options had to be passed around by reference. But if the options construct the encoder, then we can pass around options by value. This also solved the JPEG quality issue. I'm also thinking that this would make for a nice path toward a unified options API style for encoders. I'm imagining that all encoders would just have |
The story for saving images with custom encoding options isn't great. Encoders are a patchwork of different API styles and capabilities, and the trait governing them has fundamental limitations too (#2556).
In this PR, I hope to improve the situation a little by adding an API for universal encoding options. Example usage:
Instead of having to use encoders and their (arguably) low-level APIs directly, users can now provide a struct of encoding options instead. This makes it easy to configure encoders in a generic way.
Formats that currently support encoding options are PNG, JPEG, AVIF, PNM, and TGA.
Changes:
EncodingOptionstrait.{DynamicImage,ImageBuffer}::save_with_optionsandsave_buffer_with_options.TODO:
jpeg-encoder(Add quality getter and setter toEncodervstroebel/jpeg-encoder#36) or changing our encoder. For now, I just excluded quality form the options that can be set.Should help with #2810