fix: eliminate “helicopter” mic stutter by aligning mixer timebase #1310
Conversation
WalkthroughThe pull request refactors timestamp computation in the audio mixer by using platform-specific clock sources instead of a universal approach. macOS uses MachAbsoluteTime, Windows uses PerformanceCounter, and other platforms fall back to Instant. This timestamp is then passed to Changes
Sequence DiagramsequenceDiagram
participant Code as Audio Mixer Loop
participant OS as Platform Layer
participant Mixer as mixer.tick()
Code->>OS: Check target platform
alt macOS
OS->>OS: Use MachAbsoluteTime
else Windows
OS->>OS: Use PerformanceCounter
else Other
OS->>OS: Use Instant::now()
end
OS->>Code: Return timestamp
Code->>Mixer: mixer.tick(start, now)
alt Success
Mixer->>Code: Ok(...)
Code->>Code: Continue loop
else Error
Mixer->>Code: Err(())
Code->>Code: Break
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/recording/src/sources/audio_mixer.rs (2)
171-179: Align anchors: use one Timestamps base to avoid subtle drift between buffering and emit paths.Buffering compares against
self.timestamps(captured inbuild()), whiletick()builds output timestamps fromstart(captured separately inrun()). Unify by anchoringstarttomixer.timestampsso both paths share the same base.Apply this change:
- let start = Timestamps::now(); - let mut mixer = match self.build(output) { Ok(mixer) => mixer, Err(e) => { tracing::error!("Failed to build audio mixer: {}", e); let _ = ready_tx.send(Err(e.into())); return; } }; let _ = ready_tx.send(Ok(())); + // Use the same anchor as the mixer to keep buffer math and emit timestamps aligned. + let start = mixer.timestamps;Also, make the cfg predicates consistent and symmetric for readability:
- #[cfg(target_os = "macos")] + #[cfg(target_os = "macos")] let now = Timestamp::MachAbsoluteTime(cap_timestamp::MachAbsoluteTimestamp::now()); - #[cfg(windows)] - let now = Timestamp::PerformanceCounter(cap_timestamp::PerformanceCounterTimestamp::now()); - #[cfg(not(any(target_os = "macos", windows)))] + #[cfg(target_os = "windows")] + let now = Timestamp::PerformanceCounter( + cap_timestamp::PerformanceCounterTimestamp::now() + ); + #[cfg(not(any(target_os = "macos", target_os = "windows")))] let now = Timestamp::Instant(Instant::now());
- Confirm that iOS is not a supported target; if it is, consider adding an
#[cfg(target_os = "ios")]branch using the Mach timebase as on macOS.Also applies to: 188-196, 196-203
196-203: Add a regression test that exercises non-Instanttimebases.Current tests use only
Timestamp::Instant. Add macOS/Windows-guarded tests that enqueue frames timestamped withMachAbsoluteTime/PerformanceCounterand assert no gap-induced silence is inserted whennowuses the same timebase.I can draft a minimal
#[cfg(target_os = "macos")]/#[cfg(target_os = "windows")]test mirroringsource_buffer::frame_gap.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/recording/src/sources/audio_mixer.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand 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/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/recording/src/sources/audio_mixer.rs
🧬 Code graph analysis (1)
crates/recording/src/sources/audio_mixer.rs (3)
crates/timestamp/src/macos.rs (1)
now(18-20)crates/timestamp/src/win.rs (1)
now(49-53)crates/timestamp/src/lib.rs (1)
now(103-112)
⏰ 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). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/recording/src/sources/audio_mixer.rs (1)
196-202: LGTM! Platform-specific timestamp selection correctly aligns mixer timebase.The platform-specific timestamp selection properly addresses the root cause described in the PR: aligning the mixer's clock with the OS-native clocks used by microphone frame timestamps. This prevents false gap detection that caused the "helicopter" stutter effect.
Consider adding a brief comment explaining the rationale:
+ // Use the same timestamp source as microphone frames to prevent false gap detection. + // Mic frames use OS-native clocks (Mach absolute time on macOS, QueryPerformanceCounter + // on Windows), so the mixer must use the same timebase for accurate comparison. #[cfg(target_os = "macos")] let now = Timestamp::MachAbsoluteTime(cap_timestamp::MachAbsoluteTimestamp::now());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/recording/src/sources/audio_mixer.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand 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/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/recording/src/sources/audio_mixer.rs
🧬 Code graph analysis (1)
crates/recording/src/sources/audio_mixer.rs (3)
crates/timestamp/src/macos.rs (1)
now(18-20)crates/timestamp/src/win.rs (1)
now(49-53)crates/timestamp/src/lib.rs (1)
now(103-112)
⏰ 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). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
crates/recording/src/sources/audio_mixer.rs (1)
204-204: Correct usage of platform-specific timestamp in tick call.The updated tick call correctly passes the platform-specific
nowtimestamp, which is used for accurate gap detection inbuffer_sources. The output timestamps (line 426) continue to useInstantfor downstream consumers, maintaining compatibility while fixing the gap detection issue.
Aligns the audio mixer’s clock with microphone frame timestamps to prevent false gap detection and silence insertion that caused pulsing/stutter on playback.
Root cause: Mic frames are timestamped using OS-native clocks (e.g., Mach absolute time on macOS), but the mixer compared them against
Instant::now(). The mismatched timebases made the mixer think frames were late, so it periodically inserted silence, heard as a “helicopter” effect.Summary by CodeRabbit