Skip to content

fix(timestamp): correct mach tick conversion in Duration arithmetic + compensate mic input latency#1985

Merged
richiemcilroy merged 4 commits into
CapSoftware:mainfrom
ManthanNimodiya:fix/mach-timestamp-duration-arithmetic
Jul 7, 2026
Merged

fix(timestamp): correct mach tick conversion in Duration arithmetic + compensate mic input latency#1985
richiemcilroy merged 4 commits into
CapSoftware:mainfrom
ManthanNimodiya:fix/mach-timestamp-duration-arithmetic

Conversation

@ManthanNimodiya

@ManthanNimodiya ManthanNimodiya commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

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.

  • Timestamp arithmetic fix (macos.rs): Inverts the numer/denom ratio in Add<Duration> and Sub<Duration> for MachAbsoluteTimestamp. The old code computed ns * (numer/denom) (ticks → ns direction) instead of ns * (denom/numer) (ns → ticks), causing ~1736x offset on Apple Silicon. Three roundtrip tests verify the fix.
  • Mic capture latency compensation (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 via input_device_by_name rather 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

Filename Overview
crates/timestamp/src/macos.rs Corrects numer/denom inversion in Add/Sub for MachAbsoluteTimestamp; adds three roundtrip tests that catch the regression on Apple Silicon.
crates/audio/src/latency.rs Adds input_device_by_name helper to resolve the recording device by name instead of always using System::default_input_device(); falls back to default if no match is found.
crates/recording/src/feeds/microphone.rs Applies the measured input-pipeline latency (device + safety offset + stream latency, capped at 500ms) to each capture timestamp, correcting the systematic mic-track delay.
apps/desktop/src/routes/(window-chrome)/settings/general.tsx Adds 24 and 25 FPS options to the max framerate dropdown; backend already accepts 1–120 so no server-side changes needed.

Comments Outside Diff (1)

  1. crates/timestamp/src/macos.rs, line 8-11 (link)

    P2 The comment on the inner field says "Nanoseconds" but the value is actually Mach absolute time ticks. MachAbsoluteTimestamp::now() stores the raw result of mach_absolute_time() (ticks), and duration_since multiplies by numer/denom to 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
    This is a comment left during a code review.
    Path: crates/timestamp/src/macos.rs
    Line: 8-11
    
    Comment:
    The comment on the inner field says "Nanoseconds" but the value is actually Mach absolute time **ticks**. `MachAbsoluteTimestamp::now()` stores the raw result of `mach_absolute_time()` (ticks), and `duration_since` multiplies by `numer/denom` to 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.
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.

    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

Comment on lines +700 to +703
let latency_info = estimate_input_latency(
callback_sample_rate,
buffer_size_frames.unwrap_or(1024),
Some(&label),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 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.

@ManthanNimodiya

Copy link
Copy Markdown
Contributor Author

@greptileai

has_input_streams(device)
&& device
.name()
.is_ok_and(|name| name.to_string() == target)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Device::name() is already a String; can avoid the extra allocation here.

Suggested change
.is_ok_and(|name| name.to_string() == target)
.is_ok_and(|name| name == target)

@ManthanNimodiya ManthanNimodiya force-pushed the fix/mach-timestamp-duration-arithmetic branch from 1fbdeab to 6d5cec6 Compare July 7, 2026 12:14
// 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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Suggested change
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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same idea for sub: integer math + saturating_sub keeps the intent clear and avoids float rounding.

Suggested change
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))

@richiemcilroy richiemcilroy merged commit 7a93f19 into CapSoftware:main Jul 7, 2026
15 of 19 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