fix(moq-boy): align audio and video PTS to a single wall-clock reference#1211
fix(moq-boy): align audio and video PTS to a single wall-clock reference#1211
Conversation
Audio timestamps were computed from a frame counter (frame_count * frame_duration) while video used start.elapsed(), causing desync after pause/resume cycles. Now both use the same `elapsed` Duration captured once per emulator tick. For audio, when multiple frames are produced in one push, back-date the first frame so they're evenly spaced leading up to the current timestamp. https://claude.ai/code/session_015U6uGPsF5nuazT1oUDXpD5
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAudioEncoder::push_samples now takes an additional 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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.
🧹 Nitpick comments (1)
rs/moq-boy/src/audio.rs (1)
85-111: LGTM! Clean implementation of wall-clock based audio timestamps.The back-dating logic correctly spaces frames so the last emitted frame aligns with the current
elapsedtime, ensuring audio stays synchronized with video. The math is sound:
base_ts = elapsed - (pending_frames - 1) * frame_duration_us- Each subsequent frame increments by
frame_duration_usOne minor defensive consideration: the subtraction at line 100-101 could theoretically underflow if
elapsedis very small at startup. In practice this is unlikely since samples accumulate over multiple ticks before a full frame is ready, but usingsaturating_subwould be more defensive:🛡️ Optional defensive fix
- let base_ts = elapsed.as_micros() as u64 - - pending_frames.saturating_sub(1) as u64 * frame_duration_us; + let back_offset = pending_frames.saturating_sub(1) as u64 * frame_duration_us; + let base_ts = (elapsed.as_micros() as u64).saturating_sub(back_offset);,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-boy/src/audio.rs` around lines 85 - 111, The subtraction used to compute base_ts in push_samples can underflow when elapsed.as_micros() is smaller than the back-offset; change the calculation to use saturating_sub on the elapsed microseconds value (i.e., compute elapsed.as_micros() as u64 and call .saturating_sub(pending_frames.saturating_sub(1) as u64 * frame_duration_us)) so base_ts cannot wrap, keeping the existing pending_frames and frame_duration_us logic and leaving encode_frame/frame_idx logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rs/moq-boy/src/audio.rs`:
- Around line 85-111: The subtraction used to compute base_ts in push_samples
can underflow when elapsed.as_micros() is smaller than the back-offset; change
the calculation to use saturating_sub on the elapsed microseconds value (i.e.,
compute elapsed.as_micros() as u64 and call
.saturating_sub(pending_frames.saturating_sub(1) as u64 * frame_duration_us)) so
base_ts cannot wrap, keeping the existing pending_frames and frame_duration_us
logic and leaving encode_frame/frame_idx logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1564ed8e-7e28-4ada-a020-4add6a5f43f6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
rs/moq-boy/src/audio.rsrs/moq-boy/src/main.rs
Merge with main and fix formatting to pass CI checks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-boy/src/audio.rs`:
- Around line 96-107: The base timestamp calculation can underflow when
elapsed.as_micros() is smaller than the computed offset; update the computation
of base_ts (the line that sets base_ts using elapsed.as_micros() and
pending_frames * frame_duration_us) to use a saturating subtraction so the
result clamps at zero instead of wrapping (apply saturating_sub to the outer
subtraction involving elapsed.as_micros() and pending_frames.saturating_sub(1) *
frame_duration_us); keep the rest of the logic (pending_frames,
frame_duration_us, frame_idx, encode_frame) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5aade3ef-a84b-42f6-b0cf-9464ecf1322f
📒 Files selected for processing (2)
rs/moq-boy/src/audio.rsrs/moq-boy/src/main.rs
…into claude/fix-pts-alignment-gspHq
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/moq-boy/src/index.ts (1)
327-330: Consider consolidating the twoinputevent listeners for the jitter slider.The slider has two separate
inputlisteners—one in#buildControls()(line 439) for the label, and one in the constructor (line 328) for the signal. While functionally correct, consolidating them would reduce duplication.♻️ Optional: Consolidate listeners
Move the signal update into
#buildControls()by accepting the signal as a parameter, or move the label update into the constructor where the signal listener is wired:// Wire up jitter slider to sync. jitterSlider.addEventListener("input", () => { jitter.set(Number.parseInt(jitterSlider.value, 10) as Moq.Time.Milli); + // Update label here if you want to consolidate });Also applies to: 439-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/moq-boy/src/index.ts` around lines 327 - 330, There are two separate "input" listeners for jitterSlider (one in the constructor wiring the signal via jitter.set and another in `#buildControls`() updating the label); consolidate them into a single listener to avoid duplication by moving the jitter.set call into `#buildControls`() (accept the jitter signal/object as a parameter) or, alternatively, move the label update into the constructor where the signal is wired so the single listener on jitterSlider updates both the label and calls jitter.set; update references to jitterSlider, jitter.set, and `#buildControls`() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-boy/src/audio.rs`:
- Around line 115-118: The epoch computation can underflow if buffered_us >
elapsed.as_micros(); update the block in the method that sets self.epoch (the
branch checking self.epoch.is_none() && self.encode_buffer.len() >=
samples_per_frame) to compute buffered_us as before but assign self.epoch using
a saturating subtraction of elapsed.as_micros() (cast to u64) and buffered_us
(e.g., use elapsed.as_micros() as u64 .saturating_sub(buffered_us)) so the
timestamp never wraps to a huge u64; keep the same units (OPUS_SAMPLE_RATE and
CHANNELS) and types when computing buffered_us.
---
Nitpick comments:
In `@js/moq-boy/src/index.ts`:
- Around line 327-330: There are two separate "input" listeners for jitterSlider
(one in the constructor wiring the signal via jitter.set and another in
`#buildControls`() updating the label); consolidate them into a single listener to
avoid duplication by moving the jitter.set call into `#buildControls`() (accept
the jitter signal/object as a parameter) or, alternatively, move the label
update into the constructor where the signal is wired so the single listener on
jitterSlider updates both the label and calls jitter.set; update references to
jitterSlider, jitter.set, and `#buildControls`() accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27800469-7609-4c38-b74b-ffb7897ef8f3
📒 Files selected for processing (4)
js/moq-boy/src/index.tsjs/moq-boy/src/styles.tsjs/watch/src/sync.tsrs/moq-boy/src/audio.rs
✅ Files skipped from review due to trivial changes (1)
- js/moq-boy/src/styles.ts
After a pause/resume, video timestamps jump forward (they use wall-clock elapsed time), but audio timestamps continued linearly from frame_count. Now reset_epoch() clears the epoch and stale buffers on resume, so the next push_samples re-anchors to the current wall clock. This ensures audio and video PTS stay aligned across pause cycles. https://claude.ai/code/session_015U6uGPsF5nuazT1oUDXpD5
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-boy/src/audio.rs`:
- Around line 100-104: reset_epoch() clears epoch, input_buffer, and
encode_buffer but leaves frame_count, so later code in the packet timestamp
recomputation (uses elapsed, frame_count, and frame_duration_us) re-adds
pre-reset frames causing a timestamp jump; fix by resetting the frame counter
(frame_count = 0) when calling reset_epoch(), or alternatively change the
timestamp recomputation in the packetization logic that uses elapsed, epoch,
frame_count, and frame_duration_us to subtract prior frames (i.e., anchor epoch
to current time without adding previous frame_count * frame_duration_us). Update
references: reset_epoch(), frame_count, frame_duration_us, epoch, input_buffer,
encode_buffer, and the timestamp recomputation block that uses elapsed to ensure
post-reset packets are re-anchored to current wall clock.
In `@rs/moq-boy/src/main.rs`:
- Around line 224-225: Detect the audio-only resume transition (audio_active
going from false to true) and call audio_encoder.reset_epoch() on that first
transition so audio PTS is re-anchored when audio viewers reconnect;
specifically, add the reset_epoch() call inside the audio_active false→true
branch (the same logical area that later calls push_samples() and currently
drops samples) so you reset before discarding/advancing samples, while keeping
the existing reset_epoch() call that runs when the whole emulator unpauses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fedc6a95-0edc-4e67-819a-4fd380dfe0bf
📒 Files selected for processing (2)
rs/moq-boy/src/audio.rsrs/moq-boy/src/main.rs
…resume Two fixes: 1. reset_epoch() now also resets frame_count to 0. Without this, after re-anchoring epoch to the current wall clock, the old frame_count would add stale offset (e.g. 500 * 20ms = 10s) to the fresh epoch, producing timestamps far ahead of actual time. 2. Detect audio false→true transition in the emulator loop and call reset_epoch() so audio PTS re-anchors even when only the audio track resumes (video was already active, so the full-emulator pause block never triggers). https://claude.ai/code/session_015U6uGPsF5nuazT1oUDXpD5
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-boy/src/audio.rs`:
- Around line 96-105: reset_epoch currently clears internal buffers but does not
clear the ffmpeg resampler state, allowing buffered samples to leak after pause;
update reset_epoch (and/or wherever you manage the resampler Context) to call
resampler.flush() (or recreate the ffmpeg_next::software::resampling::Context)
so the resampler's internal delay is cleared; ensure this happens before
returning from reset_epoch so subsequent push_samples() cannot read and emit
pre-pause samples from resampler.delay()/resampler.run().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ebb35d0-df80-45a4-b2e0-128f7f34d10b
📒 Files selected for processing (2)
rs/moq-boy/src/audio.rsrs/moq-boy/src/main.rs
# Conflicts: # js/moq-boy/src/index.ts # rs/moq-boy/src/main.rs
- Use saturating_sub when computing epoch to prevent underflow if buffered samples exceed elapsed time (CodeRabbit review) - Flush the ffmpeg resampler's internal delay buffer in reset_epoch() so pre-pause samples don't leak into post-pause audio (CodeRabbit review) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Audio timestamps were computed from a frame counter (frame_count * frame_duration)
while video used start.elapsed(), causing desync after pause/resume cycles.
Now both use the same
elapsedDuration captured once per emulator tick.For audio, when multiple frames are produced in one push, back-date the first
frame so they're evenly spaced leading up to the current timestamp.
https://claude.ai/code/session_015U6uGPsF5nuazT1oUDXpD5