fix(timestamp): correct mach tick conversion in Duration arithmetic + compensate mic input latency#1985
Conversation
| let latency_info = estimate_input_latency( | ||
| callback_sample_rate, | ||
| buffer_size_frames.unwrap_or(1024), | ||
| Some(&label), |
There was a problem hiding this comment.
macOS ignores device name, always queries system-default input
estimate_input_latency on macOS internally calls System::default_input_device() and ignores the device_name argument entirely (the parameter is named _device_name in crates/audio/src/latency.rs:577). If the user has selected a non-default microphone — say a USB audio interface — while their system default input is a Bluetooth headset, the latency returned will be ≥120ms (the wireless floor) even though the actual wired device latency is ~10ms, overcompensating by ~100ms and making the mic track appear to start 100ms too early. The previous behavior (no compensation) was better in that case.
The fix requires resolving the macOS device by name (or device UID) instead of defaulting to System::default_input_device().
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/feeds/microphone.rs
Line: 700-703
Comment:
**macOS ignores device name, always queries system-default input**
`estimate_input_latency` on macOS internally calls `System::default_input_device()` and ignores the `device_name` argument entirely (the parameter is named `_device_name` in `crates/audio/src/latency.rs:577`). If the user has selected a non-default microphone — say a USB audio interface — while their system default input is a Bluetooth headset, the latency returned will be ≥120ms (the wireless floor) even though the actual wired device latency is ~10ms, overcompensating by ~100ms and making the mic track appear to start 100ms too early. The previous behavior (no compensation) was better in that case.
The fix requires resolving the macOS device by name (or device UID) instead of defaulting to `System::default_input_device()`.
How can I resolve this? If you propose a fix, please make it concise.| has_input_streams(device) | ||
| && device | ||
| .name() | ||
| .is_ok_and(|name| name.to_string() == target) |
There was a problem hiding this comment.
Device::name() is already a String; can avoid the extra allocation here.
| .is_ok_and(|name| name.to_string() == target) | |
| .is_ok_and(|name| name == target) |
… MachAbsoluteTimestamp add/sub
1fbdeab to
6d5cec6
Compare
| // ns = ticks * numer / denom, so ticks = ns * denom / numer. | ||
| // On Apple Silicon (numer/denom = 125/3) multiplying by numer/denom | ||
| // instead would add ~1736x the intended duration. | ||
| let ticks = rhs.as_nanos() as f64 * info.denom as f64 / info.numer as f64; |
There was a problem hiding this comment.
Minor nit: since this was a correctness bug, it might be worth avoiding the f64 conversion here (precision loss for large Durations) and doing the ns→ticks conversion in integer space.
| let ticks = rhs.as_nanos() as f64 * info.denom as f64 / info.numer as f64; | |
| let ticks = rhs | |
| .as_nanos() | |
| .saturating_mul(info.denom as u128) | |
| / info.numer as u128; | |
| Self(self.0.saturating_add(ticks.min(u64::MAX as u128) as u64)) |
| fn sub(self, rhs: Duration) -> Self::Output { | ||
| let info = TimeBaseInfo::new(); | ||
| let freq = info.numer as f64 / info.denom as f64; | ||
| let ticks = rhs.as_nanos() as f64 * info.denom as f64 / info.numer as f64; |
There was a problem hiding this comment.
Same idea for sub: integer math + saturating_sub keeps the intent clear and avoids float rounding.
| let ticks = rhs.as_nanos() as f64 * info.denom as f64 / info.numer as f64; | |
| let ticks = rhs | |
| .as_nanos() | |
| .saturating_mul(info.denom as u128) | |
| / info.numer as u128; | |
| Self(self.0.saturating_sub(ticks.min(u64::MAX as u128) as u64)) |
Bug 1: Duration arithmetic ~1736x off on Apple Silicon
Add/Sub for MachAbsoluteTimestamp multiplied by numer/denom when converting ns→ticks instead of dividing. Harmless on Intel (1/1), but on Apple Silicon (125/3) every Timestamp + Duration applied ~1736x the intended offset.
This made #1966's frame.timestamp + trim_duration push mic_start_time forward by seconds-to-minutes on M-series Macs in 0.5.4, desyncing the mic track.
Added roundtrip tests that fail against the old code on Apple Silicon.
Bug 2: mic timestamps use HAL delivery time, not capture time
cpal's capture timestamp marks when samples left the audio HAL, not when sound hit the mic. That gap (device latency + safety offset + stream latency) was already measured by estimate_input_latency but never applied, so mic tracks ran 10–50ms late on wired mics, 100ms+ on Bluetooth.
Timestamps are now shifted back by the measured device latency (clamped 0–500ms, buffer latency excluded).
Feature: 24/25 FPS options
Added to the max framerate dropdown. Backend already clamps 1–120, UI-only.
Greptile Summary
This PR fixes two correctness bugs in the Mac audio pipeline and adds 24/25 FPS options to the settings UI. The changes are well-scoped and include tests for the arithmetic fix.
macos.rs): Inverts thenumer/denomratio inAdd<Duration>andSub<Duration>forMachAbsoluteTimestamp. The old code computedns * (numer/denom)(ticks → ns direction) instead ofns * (denom/numer)(ns → ticks), causing ~1736x offset on Apple Silicon. Three roundtrip tests verify the fix.microphone.rs,latency.rs): Shifts each callback's capture timestamp back by the measured input-pipeline latency (device_latency + safety_offset + stream_latency, capped at 500ms, buffer excluded) to compensate for the gap between HAL delivery time and actual microphone capture. Device name lookup now resolves the named device viainput_device_by_namerather than falling back to the system-default input unconditionally.Confidence Score: 5/5
Safe to merge; both fixes are arithmetically correct, device fallback is preserved, and the latency cap prevents wild overcorrection.
The timestamp arithmetic inversion is verified by three new roundtrip tests. The latency compensation touches only the mic capture timestamp, is clamped to 500ms, and excludes buffer latency correctly. Device name resolution properly falls back to the system default when no match is found. No data paths are left unguarded.
No files require special attention.
Important Files Changed
Comments Outside Diff (1)
crates/timestamp/src/macos.rs, line 8-11 (link)MachAbsoluteTimestamp::now()stores the raw result ofmach_absolute_time()(ticks), andduration_sincemultiplies bynumer/denomto convert ticks → nanoseconds. The misleading comment likely contributed to the original numer/denom inversion bug. Fixing it here avoids the same mistake creeping back in.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (2): Last reviewed commit: "fix(audio): resolve macOS input latency ..." | Re-trigger Greptile