RustAudio
Adopt a higher quality resampler
Currently implemented sample rate converter uses simple linear interpolation algorithm and is prone to producing high frequency noise in output. See for example #584 and #316 related to the resampler implementation. However the algorithm is fast and requires little resources, so we should keep it at least as an option.
We should integrate an existing sample rate converter and let users to pick a resampler implementation depending on their needs. A simple optional low-pass filter may be also implemented as post-processing step for existing Rodio's SampleRateConverter to improve its audio quality.
It seems that there are 2 implementations that we can use:
- Rubato
- rust-samplerate which uses C bindings to libsamplerate
- Maybe dasp-interpolate
Note that all of these libraries process only floating point samples.
A simple optional low-pass filter may be also implemented as post-processing step for existing Rodio's SampleRateConverter to improve its audio quality.
I like this idea :+1: might give a good middle ground between low quality fast and high quality but slow. Lets first see what the perf difference the different resamplers.
I've already looked at rubato and it looks great. I'm not sure about rust-samplerate. I do not like adding a C-dependency. Those complicate cross-compiling a lot.....
Instead we could try the resampler in dasp: https://docs.rs/dasp/latest/dasp/interpolate/sinc/struct.Sinc.html.
This is, indeed a issue.
In our investigation into the playback quality of Rune, our player, which used Rodio as the playback backend, the issue happened. To rule out software-related influences, we controlled all external parameters:
- We generated an audio file with a sample rate of 44100Hz, consisting of a 1000Hz mono sine wave.
- Using OBS for recording, we set the recording parameters to 44100Hz to avoid resampling issues.
- During the recording process, both the audio player's volume and the system volume were set to 100%.
This setup resulted in a clean, smooth spectrum, confirming that there are no inherent development errors within Rune affecting audio quality.
However, we observed that Rodio's resampling process has flaws, particularly when downsampling from 48000Hz to 44100Hz, leading to suboptimal signal quality.
By subtracting Rodio's decoded output from the Ground Truth signal, the flaws in the resampling parameters become evident.The period of the mixed signal is four seconds. That is to say, the output frequency is 0.25hz lower/higher.
We hope these observations could help.
Best regards.
I just looked at the investigation in the rune player and they went through quite some trouble before finding the issue is our resampler. Given rodio is a playback lib I think it can be expected it does not introduce artifacts.... So I am going to mark this a bug.
@PetrGlad do you want to pick this up or shall I?
@dvdsk You can pick it if you want. I wonder if resampler can be an option without changing API. Currently it is always a part of Sink and UniformSourceIterato.
UniformSourceIterator
and through it mixer, yeah its gonna be a challenge. I have an idea, we could do the same as I propose here: https://github.com/RustAudio/rodio/pull/646#issuecomment-2511677332, so make the sink/stream instruct its parents/children on what re-sampler to use. Though that could impact performance, but maybe not that much as most re-samplers operate on groups of samples. An alternative would be to make every source that does resampling (UniformSourceIterator and through it mixer) generic over the resampling implementation. With static dispatch (so not Box
Do you have any suggestions? Right now I am thinking of doing both and comparing the perf.
@dvdsk You have answered this question before, but now I am thinking again about introducing "uniform source" trait whose implementations do not change their stream format (sample format, sample frequency, or channels number) mid-flight. This way complicated logic can be contained only where it is really needed. I imagine having convering 'UniformSourceIterator' next to external inputs, and using simpler processing steps everywhere else, except maybe an additional conversion may be needed just before the output device. With such simplified sources, when an input is added, it is only asserted once that stream parameters match what is expected and then the code in down-stream steps can just rely on that. For example regarding this issue, number of places where sample rate conversion is necessary will be limited so the rate converter can be configured explicitly somehow. I need some time to think how the API would look like, though. Ideally I would like to let users bring in their own resamplers, but just having some limited choice is probably enough.
Ideally I would like to let users bring in their own resamplers
Could you explain why? I was thinking about having the choice of a fast lowfi resampler (what we have now) and a hifi resampler like rubato. I do support using a trait to decouple the implementation from rodio's codebase. That is usefull if the resampler ever gets abandoned and we want to swap it out for another. But I would keep that trait private to rodio.
and using simpler processing steps everywhere else
The disadvantage is that you then have two processes (one simpel, one complex) instead of just one. It might have performance benefits, but no one has complained about rodio's current performance (one or two issues in the distant past but that had to do with clear bugs if I remember correctly).
Maybe this one could be a reference?
https://github.com/librespot-org/librespot/pull/1180
@dvdsk I'd think that giving a resampler choice if implemented properly may result in code that is less coupled and easier to change afterwards. That is not a requirement, of course.
giving a resampler choice if implemented properly may result in code that is less coupled and easier to change afterwards
oh I'm all for decoupling. I just like to keep the API as small and simple as reasonable. Internally this can be a trait implemented by different resamplers.