Skip to content

Conversation

@richiemcilroy
Copy link
Member

@richiemcilroy richiemcilroy commented Oct 27, 2025

Microphone device selection now prefers native 48kHz configs to avoid unnecessary resampling. The audio mixer pipeline has been refactored to insert aresample filters for each input, ensuring all sources are resampled to the target format before mixing, improving compatibility and audio quality.

Summary by CodeRabbit

  • New Features
    • Microphone audio sampling now intelligently detects and uses native 48 kHz sampling rates when available, eliminating unnecessary resampling
    • Audio mixer enhanced with per-source resampling, allowing each input source to be independently resampled for more robust multi-source audio processing

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 27, 2025

Walkthrough

Changes introduce preferred 48 kHz sampling rate selection in microphone device configuration and add per-source resampling stages in the audio mixer graph. Each input now includes an aresample filter targeting the mixer's INFO rate, format, and channel layout instead of connecting directly to the mixer.

Changes

Cohort / File(s) Summary
Microphone device config selection
crates/recording/src/feeds/microphone.rs
Added 48 kHz pre-selection path in get_usable_device to prioritize configs natively supporting 48 kHz with matching sample format and range; falls back to existing select_sample_rate logic if no preferred config found.
Audio mixer per-source resampling
crates/recording/src/sources/audio_mixer.rs
Inserted aresample filter stage after each abuffer targeting mixer's INFO rate, format, and channel layout. Replaced direct abuffer-to-amix connections with abuffer→resample→amix chain. Added aformat stage before sink. Extended AudioMixer struct with new resamplers: Vec<ffmpeg::filter::Context> field. Updated graph construction and tick processing to initialize and drain resamplers.

Sequence Diagram

sequenceDiagram
    participant App as Application
    participant Dev as Device Config<br/>(microphone.rs)
    participant Graph as Audio Graph
    participant Resample as Per-Source<br/>Resamplers
    participant Amix as AMix Filter
    participant Format as AFormat Filter
    participant Sink as ABufSink

    Note over Dev: New 48kHz Pre-selection
    App->>Dev: get_usable_device()
    Dev->>Dev: Search for native 48kHz config
    alt 48kHz config found
        Dev->>App: Return config with 48kHz<br/>(no resampling needed)
    else Not found
        Dev->>Dev: Fallback to select_sample_rate()
        Dev->>App: Return selected config
    end

    Note over Graph: Updated Audio Graph Structure
    App->>Graph: Initialize mixer with inputs
    loop For each input source
        Graph->>Graph: Create abuffer
        Graph->>Resample: Create aresample<br/>(target INFO rate/format)
        Resample->>Amix: Connect resample output
    end
    Amix->>Format: Connect to aformat
    Format->>Sink: Connect to abuffersink

    Note over Resample: Processing Flow
    App->>Resample: tick() - drain buffers
    Resample->>Amix: Push resampled frames
    Amix->>Amix: Mix all inputs
    Amix->>Format: Output mixed audio
    Format->>Sink: Format to target spec
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • crates/recording/src/sources/audio_mixer.rs: Requires careful verification of filter graph wiring logic, initialization flow, and the new resamplers field integration. Pay close attention to how frames flow through the pipeline and whether the aformat stage configuration correctly derives from mixer INFO. Verify the tick/drain logic properly consumes from all resamplers and handles edge cases (empty buffers, graph state transitions).
  • crates/recording/src/feeds/microphone.rs: Review the 48 kHz pre-selection logic to ensure the fallback path is correctly implemented and that config validation (sample format matching, range coverage) is sound.

Possibly related PRs

Poem

🐰 A rabbit's tune for audio streams so fine,
Forty-eight kHz—the golden sampling line!
Each source now resamples with grace and might,
Through filters they dance, then mix just right,
No stutters, no glitches—pure audio delight! 🎵

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Add native 48kHz support and input resampling to audio mixer" directly aligns with the two major changes in the changeset. It accurately reflects the addition of preferred 48 kHz device selection in microphone.rs to avoid unnecessary resampling, and the introduction of per-source aresample filters in the audio_mixer.rs pipeline. The title is specific and concise, clearly communicating the primary objectives without vague or generic terminology, and a teammate reviewing commit history would immediately understand the core changes being made.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch new-audio-fixes

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (5)
crates/recording/src/feeds/microphone.rs (2)

132-133: Avoid hard‑coding the preferred 48 kHz in multiple places

Nice addition. To reduce drift with other selection logic (e.g., select_sample_rate’s PREFERRED_RATES), consider centralizing the preferred rate(s) or at least hoisting to a module‑level const so it’s not duplicated inline.

Example:

-    let preferred_rate = cpal::SampleRate(48_000);
+    const PREFERRED_RATE: cpal::SampleRate = cpal::SampleRate(48_000);
+    let preferred_rate = PREFERRED_RATE;

154-163: Prefer best 48 kHz config when multiple exist

This early 48 kHz path is great. If multiple configs match, you currently take the first by prior sort (sample size, then max rate). Consider explicitly preferring formats we encode best (e.g., F32 over integer) and/or higher channel counts.

Sketch:

-            if let Some(config) = configs.iter().find(|config| {
+            if let Some(config) = configs
+                .iter()
+                .filter(|config| {
                     ffmpeg_sample_format_for(config.sample_format()).is_some()
                         && config.min_sample_rate().0 <= preferred_rate.0
                         && config.max_sample_rate().0 >= preferred_rate.0
-            }) {
+                })
+                .max_by_key(|c| {
+                    let fmt_rank = match c.sample_format() {
+                        cpal::SampleFormat::F32 => 3,
+                        cpal::SampleFormat::I32 | cpal::SampleFormat::I16 | cpal::SampleFormat::U16 => 2,
+                        _ => 1,
+                    };
+                    (fmt_rank, c.channels())
+                }) {
                 return Some(config.clone().with_sample_rate(preferred_rate));
             }

Please confirm the set of SampleFormat variants your CPAL version exposes for inputs so we can fine‑tune fmt_rank ordering accordingly.

crates/recording/src/sources/audio_mixer.rs (3)

112-119: Derive amix inputs from resamplers, not abuffers

Both lengths match today, but tying inputs to resamplers is more robust against future refactors.

-        let mut amix = filter_graph.add(
+        let mut amix = filter_graph.add(
             &ffmpeg::filter::find("amix").expect("Failed to find amix filter"),
             "amix",
-            &format!(
-                "inputs={}:duration=first:dropout_transition=0",
-                abuffers.len()
-            ),
+            &format!(
+                "inputs={}:duration=first:dropout_transition=0",
+                resamplers.len()
+            ),
         )?;

121-130: aformat args align with INFO; consider making this a helper

Arguments mirror INFO (fmt/rate/layout). Consider a small helper to build these strings consistently if other graphs need them later.

No change required.


138-140: Linking resamplers → amix is correct; consider optional quality flags

Current defaults are fine. If you want higher quality when converting from 44.1 → 48 kHz, consider enabling soxr where available or tuning aresample:

  • aresample=out_sample_rate=...,out_sample_fmt=...,out_chlayout=...,filter_size=64,cutoff=0.95
  • or resampler=soxr:precision=20 (if FFmpeg built with soxr)

This is optional and depends on your CPU budget.

If you’d like, I can draft benchmarks comparing default aresample vs soxr on your typical input mix.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c83f852 and 991225c.

📒 Files selected for processing (2)
  • crates/recording/src/feeds/microphone.rs (2 hunks)
  • crates/recording/src/sources/audio_mixer.rs (5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Format Rust code using rustfmt and ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.

Files:

  • crates/recording/src/sources/audio_mixer.rs
  • crates/recording/src/feeds/microphone.rs
crates/*/src/**/*

📄 CodeRabbit inference engine (AGENTS.md)

Rust crates should place tests within the src/ and/or a sibling tests/ directory for each crate inside crates/*.

Files:

  • crates/recording/src/sources/audio_mixer.rs
  • crates/recording/src/feeds/microphone.rs
🧬 Code graph analysis (2)
crates/recording/src/sources/audio_mixer.rs (2)
crates/editor/src/audio.rs (5)
  • new (44-56)
  • new (84-93)
  • new (243-262)
  • new (346-366)
  • info (80-82)
crates/media-info/src/lib.rs (3)
  • new (29-45)
  • rate (125-127)
  • channel_layout (117-119)
crates/recording/src/feeds/microphone.rs (1)
crates/media-info/src/lib.rs (1)
  • ffmpeg_sample_format_for (284-294)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Clippy
  • GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
  • GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
  • GitHub Check: Analyze (rust)
🔇 Additional comments (2)
crates/recording/src/sources/audio_mixer.rs (2)

71-110: Per‑source resampling topology looks correct

abuffer → aresample → amix is the right shape; args derived from the mixer target are consistent. Good separation by keeping both abuffers and resamplers vectors for indexing.

Please run a quick sanity check with mixed input rates (e.g., 44.1 kHz + 48 kHz) and verify amix doesn’t report format negotiation warnings in FFmpeg logs.


227-235: New resamplers field: good state exposure

Keeping resamplers in the struct is helpful for future introspection or dynamic graph changes. No issues spotted.

@richiemcilroy richiemcilroy merged commit 0cbd594 into main Oct 27, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants