Add more Interpolation types for the `rate::Converter` (the current implementation is only linear)

[mitchmindtree]

The linear interpolation used by rate::Converter is fast but not loses information, acting as a poor low-pass filter for many conversions. It would be useful to implement more interpolation types so that a user has more control over the performance/quality trade-off.

[andrewcsmith]

Just pushed a commit to vox_box that includes sinc interpolation of a given value. Pretty hacky, definitely needs further work, but perhaps this could be an impetus for adding some interpolation types.

I saw something in the docs about the Converter API needing stabilization. Once the API is set, I'm happy to port this sinc interpolation function over.

[andrewcsmith]

ping @mitchmindtree? I'm working on something now that could use a better sample interpolation algorithm, and would be able to implement it if the API is going to settle any time soon.

[mitchmindtree]

Hey @andrewcsmith, sorry about the delayed reply, this issue had slipped my mind!

At the moment, the linear interpolation is baked right into the Converter type. I was thinking we could do something along these lines:

• Abstract the concept of interpolation into something like an Interpolate trait.
• Abstract the existing linear interpolation logic into an implementation of Interpolate for a new Linear type.
• Do something similar for a new Sinc type using your algorithm.
• Perhaps we could implement all this in a new lerp.rs module?
• Finally, we could add a new type parameter to the Converter which allows for being generic over the type of interpolation.

Let me know your thoughts on this :+1: I'm definitely keen on the idea of generalising the interpolation and getting some higher quality options in there!

[andrewcsmith]

Okay, excellent. I'm not quite clear on what Interpolate and Converter would do differently. The various types of interpolation will have different memory buffer requirements. Here's how I see it:

• Converter trait: helper methods for what Converter currently does, such as helper methods for sample rate construction. impl Iterator for everything that impls Converter.
• Linear struct: impls Converter, comprising all of what Converter currently does.
• Sinc struct: impls Converter, probably using a ring buffer as storage for (ideally) both the left and the right sides.

It would also be perhaps preferable to scrap Converter entirely and just call the trait Interpolator, to be more specific about what is happening. Basically, I think we can probably get this done with one trait for the sample rate-related helper methods, and one struct for each type of interpolation.

Bikeshed, I also think lerp.rs implies linear interpolation, and that interpolate.rs would be more clear.

If that's all cool, I can get on it. I would be quite happy to get these methods out of my own voice processing crate and into sample, where they seem like they belong.

Edit—I think maybe I get your original intention, thinking about it more. Is the idea to have the Converter struct deal with the Iterator responsibilities, such as incrementing the interpolation value with each frame, and then have Interpolator as a trait that the various types Linear, Sinc, Parabolic, etc., would need to implement? Indeed, that might be a cleaner separation of duties.

[mitchmindtree]

I also think lerp.rs implies linear interpolation, and that interpolate.rs would be more clear.

Sounds good!

If that's all cool, I can get on it

Awesome! I'd love to see these land and would be more than happy to review and test them when you're ready :)

Re splitting Converter and Interpolator- Yeah your edit is pretty much what I've had in mind. I'm open to any other ideas you come up with as you go too of course! I haven't given it too much thought just yet.

[andrewcsmith]

Since it seems this is the tracking issue for sample rate conversion, I'm posting my comment here. If you think the API is settled I'll get cracking on this Sinc situation.

Here's my reasoning behind making next_source_frame take Option<Self::Frame>: if the Interpolator takes Self::Frame directly, then our unwrap_or logic to find a default occurs in the Converter. This means that an end-user has to write a new Converter in order to change the tapering behavior. In my implementation, the end-user only has to write a new Interpolator, which is a much smaller task.

Down the line, perhaps we can provide a impl Interpolator for LinearWithTaper struct that has an additional value to taper toward. But we can improve that later, without changing my proposed Converter and Interpolator API.

So—essentially—I'm trying to minimize the possibility for future breaking changes.

[andrewcsmith]

Should be fixed by #47

[andrewcsmith]

@mitchmindtree wondering if perhaps we can close this one as well. The sinc interpolation I added is pretty excellent, and parabolic interpolation can be used by just setting the sinc depth to 1. (Perhaps useful for control signals.)

[mitchmindtree]

Sounds good! We'd probably do best to add new issues for specific Interpolators in the future, rather than trying to keep this umbrella issue going which could get messy.