Dynamic sample rate handling in speaker stream#1423
Conversation
📝 WalkthroughWalkthroughAdds a new async resampler wrapper and exposes it from the audio crate. Tracks macOS speaker sample rate via a shared atomic. Replaces ad-hoc resampling in the listener plugin with the wrapper, adds a resampling helper script and new data paths, and switches notification spawn to Tauri’s runtime. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Consumer
participant R as ResampledAsyncSource
participant S as Inner AsyncSource
participant L as LinearResampler
C->>R: poll_next()
alt Need source samples
R->>S: poll_next()
S-->>R: Item(f32)/Pending/None
opt On Item
R->>L: push source sample(s)
end
end
alt Source ended
R-->>C: None
else Source pending
R-->>C: Pending
else Have data
R->>L: interpolate(sample_position)
L-->>R: f32 sample
R-->>C: Item(f32)
R->>R: advance sample_position by (src_rate/target_rate)
R->>R: update last_source_rate if changed
end
sequenceDiagram
autonumber
participant AU as AudioUnit Callback
participant Ctx as Ctx
participant A as Atomic current_sample_rate
participant SS as SpeakerStream
AU->>Ctx: process(buffer)
AU->>A: store(device.actual_sample_rate or ctx.format.sample_rate) (Relaxed)
Note over A,SS: Cross-thread visibility of current output rate
SS->>A: load() (Relaxed)
A-->>SS: u32 sample_rate
SS-->>SS: sample_rate() returns loaded value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
crates/data/scripts/resamples.sh (3)
11-12: Don’t assume .wav extension when deriving BASENAMEIf a non-wav is passed, BASENAME includes the extension. Make this robust.
- B A S E N A M E=$(basename "$INPUT_FILE" .wav) + EXT="${INPUT_FILE##*.}" + BASENAME=$(basename "$INPUT_FILE" ."$EXT")
31-31: Seek faster by moving -ss before -i (optional)Placing -ss before -i speeds up splitting on large files.
- ffmpeg -i "$INPUT_FILE" -ss ${START} -t ${PART_DURATION} -ar ${RATE} "$OUTPUT_FILE" -y -loglevel error + ffmpeg -ss "$START" -t "$PART_DURATION" -i "$INPUT_FILE" -ar "$RATE" "$OUTPUT_FILE" -y -loglevel error
20-31: Edge-case: last segment duration driftFloating math can make the last cut a tad short/long. Consider letting the last part run to EOF by omitting -t on the final iteration.
-for i in "${!SAMPLE_RATES[@]}"; do +for i in "${!SAMPLE_RATES[@]}"; do RATE=${SAMPLE_RATES[$i]} PART_NUM=$((i + 1)) START=$(echo "$i * $PART_DURATION" | bc -l) OUTPUT_FILE="${DIR}/${BASENAME}_part${PART_NUM}_${RATE}hz.wav" - echo "Creating part ${PART_NUM}: ${START}s-$(echo "$START + $PART_DURATION" | bc -l)s at ${RATE}Hz" - ffmpeg -i "$INPUT_FILE" -ss ${START} -t ${PART_DURATION} -ar ${RATE} "$OUTPUT_FILE" -y -loglevel error + if [ "$PART_NUM" -eq "${NUM_PARTS}" ]; then + echo "Creating part ${PART_NUM}: ${START}s-EOF at ${RATE}Hz" + ffmpeg -ss "$START" -i "$INPUT_FILE" -ar "$RATE" "$OUTPUT_FILE" -y -loglevel error + else + echo "Creating part ${PART_NUM}: ${START}s-$(echo "$START + $PART_DURATION" | bc -l)s at ${RATE}Hz" + ffmpeg -ss "$START" -t "$PART_DURATION" -i "$INPUT_FILE" -ar "$RATE" "$OUTPUT_FILE" -y -loglevel error + ficrates/data/src/english_1/mod.rs (1)
4-27: Optional: gate these paths behind cfg(test) if only used by testsIf production code doesn’t use these constants, guard them to avoid bundling dev-only paths into the public API.
- pub const AUDIO_PART1_8000HZ_PATH: &str = concat!(/* … */); +#[cfg(test)] +pub const AUDIO_PART1_8000HZ_PATH: &str = concat!(/* … */); # …repeat for the other fivecrates/audio/src/resampler.rs (2)
15-22: Initial seeding may output leading zeros for up/down-samplingWith sample_position set to rate ratio, the first 1–2 outputs can be zeros because the interpolator isn’t primed with two frames. Consider priming two frames on first poll or tracking a primed flag.
pub fn new(source: S, target_sample_rate: u32) -> Self { let initial_rate = source.sample_rate(); Self { source, target_sample_rate, - sample_position: initial_rate as f64 / target_sample_rate as f64, + // Start at >= 2.0 so we fetch at least two frames before first output. + sample_position: 2.0, resampler: dasp::interpolate::linear::Linear::new(0.0, 0.0), last_source_rate: initial_rate, } }Alternative: add a primed: bool field and seed two frames once. I can provide that patch if you prefer.
35-38: last_source_rate is tracked but unusedEither use this to handle dynamic rate transitions (e.g., flush/prime on change) or remove it.
- last_source_rate: u32, + // last_source_rate can be leveraged to re-prime when the source rate changes. + last_source_rate: u32,If not needed, consider dropping the field and the update block.
plugins/listener/src/fsm.rs (1)
312-315: AEC failure fallback is safe; consider downgrading log level.Falling back to
mic_chunk_rawmaintains continuity. To avoid log spam during transient AEC hiccups, considertracing::warn!here.- tracing::error!("aec_error: {:?}", e); + tracing::warn!("aec_error: {:?}", e);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
crates/data/src/english_1/audio_part1_8000hz.wavis excluded by!**/*.wavcrates/data/src/english_1/audio_part2_16000hz.wavis excluded by!**/*.wavcrates/data/src/english_1/audio_part3_22050hz.wavis excluded by!**/*.wavcrates/data/src/english_1/audio_part4_32000hz.wavis excluded by!**/*.wavcrates/data/src/english_1/audio_part5_44100hz.wavis excluded by!**/*.wavcrates/data/src/english_1/audio_part6_48000hz.wavis excluded by!**/*.wav
📒 Files selected for processing (7)
crates/audio/src/lib.rs(1 hunks)crates/audio/src/resampler.rs(1 hunks)crates/audio/src/speaker/macos.rs(4 hunks)crates/data/scripts/resamples.sh(1 hunks)crates/data/src/english_1/mod.rs(1 hunks)plugins/listener/src/fsm.rs(3 hunks)plugins/notification/src/handler.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
plugins/notification/src/handler.rscrates/data/src/english_1/mod.rscrates/audio/src/lib.rscrates/audio/src/resampler.rsplugins/listener/src/fsm.rscrates/audio/src/speaker/macos.rs
🧬 Code graph analysis (3)
crates/audio/src/resampler.rs (1)
crates/audio/src/lib.rs (3)
poll_next(159-183)as_stream(187-189)sample_rate(191-197)
plugins/listener/src/fsm.rs (3)
crates/audio/src/resampler.rs (2)
new(14-23)new(101-107)crates/audio/src/mic.rs (1)
new(34-66)crates/audio/src/lib.rs (1)
from_speaker(108-115)
crates/audio/src/speaker/macos.rs (3)
crates/audio/src/lib.rs (1)
sample_rate(191-197)crates/audio/src/mic.rs (1)
sample_rate(188-190)crates/audio/src/speaker/mod.rs (2)
sample_rate(89-91)sample_rate(94-96)
⏰ 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). (2)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (9)
plugins/notification/src/handler.rs (1)
81-83: Good switch to Tauri runtime spawnUsing tauri::async_runtime::spawn inside a std::thread avoids “no reactor running” issues that tokio::spawn would hit off-runtime. The clone + await usage is correct.
crates/data/src/english_1/mod.rs (1)
4-27: Audio assets verified
All six referenced WAV files are present in crates/data/src/english_1, so no changes are needed.crates/audio/src/resampler.rs (1)
151-205: Include data crate WAV assets in CI
Tests incrates/audio/src/resampler.rs(lines 151–205) depend on six WAV files incrates/data/src/english_1; confirm your CI setup checks out the data crate and thataudio_part1_8000hz.wavthroughaudio_part6_48000hz.wavare committed or generated so the tests won’t panic.crates/audio/src/lib.rs (2)
5-5: Module wiring looks goodAdding mod resampler integrates the new file cleanly.
12-12: Public re-export is appropriatepub use resampler::*; exposes the new wrapper at the crate root for downstream use.
plugins/listener/src/fsm.rs (2)
12-12: Good move to the shared resampler wrapper.Importing
ResampledAsyncSourcecentralizes rate handling and removes bespoke resampling paths.
252-257: Migrate remaining.resample()and confirm chunk sizes
.resample()still in
- owhisper/owhisper-server/src/commands/run/realtime.rs:137
- crates/audio/src/resampler.rs:173
- crates/audio-utils/src/lib.rs:21
ensure these aren’t on the AEC path or replace withResampledAsyncSource- Only
fsm.rsuses.chunks(hypr_aec::BLOCK_SIZE); verifychunk_sizeincrates/audio-utilsmatcheshypr_aec::BLOCK_SIZEwhere it feeds AECcrates/audio/src/speaker/macos.rs (2)
1-1: Atomic sample-rate plumbing is clean.Using
Arc<AtomicU32>with relaxed ops for read-mostly SR propagation is appropriate and avoids mutex contention.Also applies to: 33-39, 46-47
165-172: SR initialization and wiring look correct.Seeding from
asbd.sample_rateand sharing into bothCtxandSpeakerStreamensures sane defaults before the first callback update.Also applies to: 176-183
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/audio/src/speaker/macos.rs (1)
115-121: Resolved prior issue: SR now read from the device each callback.This addresses the earlier bug about writing a static rate; nice fix.
🧹 Nitpick comments (2)
crates/audio/src/speaker/macos.rs (2)
115-121: Reduce churn: round and only store on change.Prevents noisy updates (float fuzz) and unnecessary atomic writes.
Apply:
- ctx.current_sample_rate.store( - device - .actual_sample_rate() - .unwrap_or(ctx.format.absd().sample_rate) as u32, - Ordering::Relaxed, - ); + let sr = device + .actual_sample_rate() + .unwrap_or(ctx.format.absd().sample_rate); + let sr_u32 = sr.round() as u32; // avoid truncation drift + if sr_u32 != ctx.current_sample_rate.load(Ordering::Relaxed) { + ctx.current_sample_rate.store(sr_u32, Ordering::Relaxed); + }
169-169: Init with rounded SR to avoid off-by-one truncation.Avoids 44099/47999 from float imprecision.
- let current_sample_rate = Arc::new(AtomicU32::new(asbd.sample_rate as u32)); + let current_sample_rate = Arc::new(AtomicU32::new(asbd.sample_rate.round() as u32));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
crates/audio/src/resampler.rs(1 hunks)crates/audio/src/speaker/macos.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/audio/src/resampler.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
crates/audio/src/speaker/macos.rs
🧬 Code graph analysis (1)
crates/audio/src/speaker/macos.rs (4)
crates/audio/src/resampler.rs (2)
sample_rate(69-71)sample_rate(115-121)crates/audio/src/lib.rs (1)
sample_rate(191-197)crates/audio/src/mic.rs (1)
sample_rate(188-190)crates/audio/src/speaker/mod.rs (2)
sample_rate(89-91)sample_rate(94-96)
⏰ 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). (2)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (7)
crates/audio/src/speaker/macos.rs (7)
1-1: Atomic for cross-thread SR tracking: good call.Appropriate choice; Ordering::Relaxed is sufficient for a standalone config value.
33-34: Stream now holds shared SR atomic.Plumbs the state where it’s needed. Looks good.
37-39: Dynamic SR exposure via atomic load is correct.Cheap read path; matches intended cross-thread visibility.
46-47: Ctx carries SR atomic for callback updates.Right ownership and sharing semantics.
105-105: Usedevicein IO proc.Renaming from
_devicetodeviceremoves the unused param and enables live SR reads.
175-175: Ctx wiring looks correct.Shared SR is propagated properly.
186-186: Stream exposes SR atomic correctly.Keeps the read path simple for callers.
1 participant
No description provided.