feat: high-quality resampler

[roderickvd]

Q: How hard could it be to add a high-quality Resample source using Rubato?
A: Almost four weeks of evening work, that's how hard!

It's been surprisingly hard to get the details down. Way more than "just" creating a Rubato instance and processing some buffer. Very grateful for the test cases that were already there - without them, I'd never have gotten this down.

Features

• Builder pattern:

// Quick presets
let config = ResampleConfig::fast();
let config = ResampleConfig::balanced();  // same as default()
let config = ResampleConfig::accurate();

// Full customization
let config = ResampleConfig::sinc()
    .sinc_len(nz!(256))
    .interpolation(Sinc::Cubic)
    .window(WindowFunction::BlackmanHarris2)
    .chunk_size(nz!(512))
    .build();

source.resample(target_rate, config);

• Polynomial and sinc interpolation: including support for FFT-based resampling for fixed ratios.

• Fixed-ratio optimization: auto-detects and switches sinc resamplers to FFT-based or nearest-neighbor processing when the ratio allows. This lowers CPU usage while providing the highest quality.

• Span boundary handing: specialized implementation of fix: correct Source trait semantics and span tracking bugs #831 to recreate the resampler on the fly when parameters change at span boundaries, even after seeking.

Performance

Through refactoring of UniformSourceIterator and friends, the performance hit on cargo bench is now 7-8% - which is about the same as the span boundary detection from #831. Considering this PR also adds span boundary detection for the resampler, that means the improved resampling is virtually free.

Initially I had made SampleRateConverter a simple wrapper around the new Resample source. This caused a 20-25% performance hit, and led to my refactoring described next.

Changes

• The UniformSourceIterator now is optimized for passthrough, channel count conversion, sample rate conversion, or both. It also inlines the former Take struct (private; not to be confused with TakeDuration) and works with Resample directly.

• For UniformSourceIterator to work with Resample directly, it needs to convert an Iterator into a Source. That's from_iter right? Wrong in v0.21: there FromIter is actually something that chains sources together. That seemed counter-intuitive to me. So to bring it in line with Rust idioms:

  • FromIter was renamed to Chain (chains sources from an iterator)
  • FromFactory was renamed to FromFn (chains sources from a function)
  • FromIter now wraps an Iterator<Item = Sample> into a Source

• SourcesQueueOutput was copied from fix: correct Source trait semantics and span tracking bugs #831 because it contains peeking fixes necessary to make UniformSourceIterator detect the sample rate correctly when transitioning out of silence.

• SampleRateConverter is tagged as deprecated.

• ChannelCountConverter now implements Source while keeping a minimum surface for just I: Iterator.

• Minor opportunistic refactoring.

Questions

• Should we move and rename Resample from src/source/resample.rs to SampleRateConverter in src/conversions/sample_rate.rs and basically replace it?
  • This would be breaking due to requiring that I: Source instead of I: Iterator.
  • Resample::new could live besides new_poly, new_sinc and the builder.

Related

Supersedes #670

[yara-blue]

Q: How hard could it be to add a high-quality Resample source using Rubato? A: Almost four weeks of evening work, that's how hard!

Oops, I've had this for a while in rodio experiments https://github.com/RustAudio/rodio-experiments/blob/main/src/conversions/resampler/variable_input.rs

but hey that's nice we can compare :)

and its probably not as extensive (for example I said hell no to seeking :))

[roderickvd]

Yes the hard work started when I thought I was done and started merging the tests from the existing SampleRateConverter 😆

Beyond padding blocks to satisfy Rubato’s chunks there is also output delay skipping, cutting off of mirror images after the last sample (too many samples) and flushing the last signal energy from Rubato’s filter (not enough samples).

This complexity required some degree of complexity to address it - if you see opportunities to simplify then do let me know. The test suite quickly reveals what you cannot take out…

[roderickvd]

Split into different files now.

[yara-blue]

Thank you I see why this was sooo much work there are a lot of subtleties!

I've found a few rough edges that are worth sanding down. It's mostly code quality enhancements.

Due to it's size it took me a bit to find the time to review it. By now the PR needs a rebase, I propose we first address all the comments and only then rebase. Otherwise we'd lose where we are.

Proper resampling is the only way to get out of the span mess making this essential to the future of Rodio. Therefore let me thank you again for figuring out how this works and getting a solid implementation done.

[roderickvd]

Addressed review points in 97f5701 and rebased - sorry, I already force-pushed without thinking about it.

This required backporting a fix from #786.

I'll investigate a bit more why the Hydrogen SRC test shows that we're missing samples. This PR took great care to be sample-exact, so I don't yet understand. That should not hold up merging.

[roderickvd]

What about the open questions from the PR body?

[roderickvd]

Upon further investigation, it turns out that the sample delay depends on Rubato's sinc settings. With something as extreme as this, the sample delay is 0 as it should be:

ResampleConfig::Sinc {
    sinc_len: 2048,
    oversampling_factor: 1024,
    interpolation: Sinc::Cubic,
    window: WindowFunction::BlackmanHarris2,
    f_cutoff: 0.9945994235, // 0.5^(16/sinc_len)
    chunk_size: 2048,
    sub_chunks: 1,
}

Note that that f_cutoff is higher than what Rubato's calculate_cutoff would return. I believe this is because Rubato optimizes for an Fs of 20 kHz, while the HydrogenAudio SRC test compares against Fs/2 or 22.05 kHz. Higher f_cutoff come at a cost of greater phase shift.

Finally, I can confirm that the spectrogram is clean when compiled with the 64bit feature.

In all, I think this is good to go.

[phayes]

I think this can read past span edges. There's no checking in this while loop of span boundaries, so we can read past into a new inner span with different channel counts and sample-rates.

[phayes]

If tracing feature is enabled, should this logged as an error before returing None to end the stream?

[phayes]

Same thing here, perhaps log this error if tracing is enabled?

[phayes]

I think perhaps this should be resampler.output_remaining(), not resampler.output_len().

While they might often be the same, when we're zooming through initial delay samples we use Buffer::skip() which advances the read marker but doesn't change the buffer length.

Since we're never emitting those initial dummy samples, I think using resampler.output_len() will overreport in this situation.

[phayes]

Thinking more about this. This could be a problem even outside of initial sample delay skipping.

We use this outout_len value here in current_span_len():

// When the ratio contains a fraction, we cannot choose the floor or ceiling
// arbitrarily, because the resampler may produce either based on its internal state
if output_has_samples {
    // Running state: we are iterating over our buffer with resampled samples
    Some(output_len)
} 

If someone queries our current_span_len() mid-chunk, I think we want the amount remaining in the current buffer, not the total buffer size.

[phayes]

I have two more general comments on this:

Amortization

Resampling can be very bursty. A single outer next() does both of these things all at once in a single next_sample():

1. Pull many samples from self.input.next() until we have enough to do resampling work.
2. Run one expensive process_into_buffer(...) call.

It might make sense to try to amortize this over more calls to next_sample(). There's nothing we can do to amortize process_into_buffer(), but we could try to spread the calls to inner next() over many next_sample(), which could help when inner next() is also occationally expensive. The idea would be to estimate the number of inner pulls we'll need before the next process_into_buffer() and pull those into the input buffer on every next_sample() (or every other next_sample() depending on ratio etc). Then fill_input_buffer() would only need to pull the balance.

It's quite possible that I'm way over thinking this, and buffer sizes in CPAL make careful amortization of inner next() calls over-engineering. But I wanted to leave the thought here for you to consider.

Resampler Pool

I notice that we're allocating a new resampler on span changes. It might make sense to introduce the idea of a "resampler pool" where we can park used resamplers, reset them with Resampler::reset(&mut self), then draw from the pool without allocating if there's an already allocated and compatible resampler available.

Check out https://github.com/phayes/ardftsrc-rs/blob/master/ardftsrc/src/realtime.rs#L615, where I've implemented this for ardfcsrc. It's not quite the same idea since my architecture around handling span boundaries and buffers is pretty different, but it could be worth thinking about.

Both of these ideas could be "future extensions" and are definitely niche optimizations, but I thought it worth sharing my thoughts.