Skip to content

feat: high-quality resampler#843

Open
roderickvd wants to merge 10 commits intomasterfrom
feat/rubato-resampler
Open

feat: high-quality resampler#843
roderickvd wants to merge 10 commits intomasterfrom
feat/rubato-resampler

Conversation

@roderickvd
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Member

yara-blue commented Feb 9, 2026

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
Copy link
Copy Markdown
Member Author

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
Copy link
Copy Markdown
Member Author

Split into different files now.

Copy link
Copy Markdown
Member

@yara-blue yara-blue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread examples/resample.rs
fn main() -> Result<(), Box<dyn Error>> {
#[cfg(debug_assertions)]
{
eprintln!("WARNING: Running in debug mode. Audio may be choppy, especially with");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should not print this when building the Resampler. Seems like a footgun similar to dropping the Sink.

Comment thread examples/resample.rs

let args: Vec<String> = std::env::args().collect();

if args.len() < 2 {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try using Clap instead.

Comment thread examples/resample.rs
player.sleep_until_end();

let playback_time = playback_start.elapsed();
println!("\nPlayback finished in {:.2}s", playback_time.as_secs_f32());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say for an example its fine to use the Debug fmt implementation for Duration

Comment thread examples/resample.rs
}

/// Parse the resampling quality from a string argument
fn parse_quality(method: &str) -> Result<ResampleConfig, Box<dyn Error>> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is worth adding these in the example it might be worth add factories for them on ResampleConfig?

Comment thread examples/resample.rs
"fast" => ResampleConfig::fast(),
"balanced" => ResampleConfig::balanced(),
"accurate" => ResampleConfig::accurate(),
_ => return Err(format!(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be part of arg parsing, (don't worry clap does that for you if you make method an enum)

channels.get() as usize,
rubato::FixedAsync::Output,
)
.map_err(|e| format!("Failed to create sinc resampler: {:?}", e))?;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same error thing as above

channels.get() as usize,
rubato::FixedSync::Output,
)
.map_err(|e| format!("Failed to create FFT resampler: {:?}", e))?;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once more :)

Comment thread src/source/chain.rs
}

// Otherwise we use the constant value.
Some(THRESHOLD)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong here (I do not have the span misery mitigations clearly in my head) but can this not get dicey when I:

  • create an empty chain and put it in a queue
  • put a 48khz mono source in the queue
  • queue calls current_span_en get Some(THRESHOLD) on that queue
  • queue calls sample_rate get 44100 on that queue
  • queue calls next on chain gets None
  • queue calls next on the mono source and gets a sample intended for 48khz but playing at 44100.

Now our queue may not call things in this order but what would happen with a user created one?

An alternative design for chain would forbid the empty state by getting the first sample in the new factory, then failing with an error if the first source was empty.

Again im pretty unsure about all this since the span misery has completely left my brain, thanks again for going through all that!

Comment thread src/queue.rs
if self.current.is_exhausted() && self.silence_samples_remaining == 0 {
if let Some((next, _)) = self.input.next_sounds.lock().unwrap().front() {
// Current source exhausted, peek at next queued source
// This prevents wrong resampling setup in UniformSourceIterator
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a subtle race condition here:

  • call sample_rate, current is exhausted
  • no next source yet so get self.current.sample_rate()
  • new source gets appended
  • next call works without silence placeholder at possibly wrong sample rate

I'd suggest "poisoning" the queue whenever the current is exhausted and we do not have something new yet.

Comment thread src/queue.rs
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I've already reviewed them so it would just get confusing but in the future please seperate out changes that can be unrelated to multiple PR's. In this case I believe the changes to these files:

  • uniform,
  • from_fn,
  • from_factory,
  • from_iter
  • chain()
    Should have been separated out, now the whole PR will be held up by their review. It also makes it harder to have the whole PR reviewed (which is part of why it took so long to get this reviewed). For me a 200 line change is something I can sneak in my day somewhere while a large PR like this needs an entire afternoon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants