Parallelize startup and harden playback/export A/V sync#1893
Conversation
|
Paragon Review Skipped Hi @richiemcilroy! Your Polarity credit balance is insufficient to complete this review. Please visit https://app.paragon.run to finish your review. |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| self.total_overlap_trimmed = self.total_overlap_trimmed.saturating_add(overlap); | ||
| if dropped_whole_frame { | ||
| self.overlap_dropped_frames += 1; | ||
| if frame_count <= STARTUP_OVERLAP_DROP_FRAME_LIMIT { |
There was a problem hiding this comment.
Minor off-by-one risk: frame_count looks 0-based (incremented after processing), so <= STARTUP_OVERLAP_DROP_FRAME_LIMIT counts 4 frames when the limit is 3. If you meant “first 3 frames”, consider switching to < (or renaming the constant/param to make the indexing explicit).
| if frame_count <= STARTUP_OVERLAP_DROP_FRAME_LIMIT { | |
| if frame_count < STARTUP_OVERLAP_DROP_FRAME_LIMIT { | |
| self.startup_overlap_drops += 1; | |
| } |
| return 0.0; | ||
| }; | ||
|
|
||
| if summary.startup_overlap_drops < MIN_STALE_STARTUP_DROPS |
There was a problem hiding this comment.
audio_timing_repair_offset applies total_overlap_trimmed_ms (whole recording). If there are any whole-frame overlap drops outside the startup window, this could over-shift audio. One cheap guard is to only apply when all drops were in startup.
| if summary.startup_overlap_drops < MIN_STALE_STARTUP_DROPS | |
| if summary.startup_overlap_drops < MIN_STALE_STARTUP_DROPS | |
| || summary.overlap_dropped_frames < MIN_STALE_STARTUP_DROPS | |
| || summary.startup_overlap_drops != summary.overlap_dropped_frames | |
| || !(MIN_STALE_STARTUP_TRIMMED_MS..=MAX_STALE_STARTUP_REPAIR_MS) | |
| .contains(&summary.total_overlap_trimmed_ms) | |
| { | |
| return 0.0; | |
| } |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn audio_timing_repair_offset(summary: Option<&cap_project::AudioGapSummary>) -> f32 { | ||
| let Some(summary) = summary else { | ||
| return 0.0; | ||
| }; | ||
|
|
||
| if summary.startup_overlap_drops < MIN_STALE_STARTUP_DROPS | ||
| || summary.overlap_dropped_frames < MIN_STALE_STARTUP_DROPS | ||
| || !(MIN_STALE_STARTUP_TRIMMED_MS..=MAX_STALE_STARTUP_REPAIR_MS) | ||
| .contains(&summary.total_overlap_trimmed_ms) | ||
| { | ||
| return 0.0; | ||
| } | ||
|
|
||
| -(summary.total_overlap_trimmed_ms as f32 / 1_000.0) |
There was a problem hiding this comment.
total_overlap_trimmed_ms may include mid-recording trims, overcorrecting silence insertion
audio_timing_repair_offset uses summary.total_overlap_trimmed_ms — which accumulates ALL overlap trims throughout the entire recording — as the correction amount. The startup_overlap_drops >= 3 guard confirms that a stale-startup burst occurred, but the silence inserted equals the full lifetime total, not just the startup portion. A recording that has, say, 200 ms of startup drops and 800 ms of unrelated mid-recording trims would insert 1 000 ms of leading silence, shifting audio 800 ms too late and creating new A/V desync instead of fixing it. Adding a startup_overlap_trimmed_ms field alongside startup_overlap_drops in AudioGapSummary would allow the correction to be scoped to the actual startup trim.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/editor/src/editor_instance.rs
Line: 646-659
Comment:
**`total_overlap_trimmed_ms` may include mid-recording trims, overcorrecting silence insertion**
`audio_timing_repair_offset` uses `summary.total_overlap_trimmed_ms` — which accumulates ALL overlap trims throughout the entire recording — as the correction amount. The `startup_overlap_drops >= 3` guard confirms that a stale-startup burst occurred, but the silence inserted equals the full lifetime total, not just the startup portion. A recording that has, say, 200 ms of startup drops and 800 ms of unrelated mid-recording trims would insert 1 000 ms of leading silence, shifting audio 800 ms too late and creating new A/V desync instead of fixing it. Adding a `startup_overlap_trimmed_ms` field alongside `startup_overlap_drops` in `AudioGapSummary` would allow the correction to be scoped to the actual startup trim.
How can I resolve this? If you propose a fix, please make it concise.| /// How long the caller waits for the audio thread's ready verdict. This MUST stay strictly larger | ||
| /// than [`AUDIO_FIRST_CALLBACK_TIMEOUT`] plus the thread's setup time so the thread's verdict | ||
| /// always wins the race. The setup includes a synchronous full pre-render of the recording, which | ||
| /// scales with its length, hence the generous headroom. If the caller times out first it concludes | ||
| /// "no audio" and stops feeding the playhead while the audio thread may still start a now-unsynced | ||
| /// stream — the desync this guards against. The thread reports failures immediately, so this only | ||
| /// bounds a genuinely wedged device and is never waited out in normal operation. | ||
| const AUDIO_READY_TIMEOUT: Duration = Duration::from_secs(15); |
There was a problem hiding this comment.
AUDIO_READY_TIMEOUT, causing UI freeze then unsynced audio
On Windows, create_stream_prerendered builds a PrerenderedAudioBuffer entirely in memory before the stream is created — this work is O(recording length). For a multi-hour recording on modest hardware, the pre-render can take well over 15 seconds. When ready_rx.recv_timeout(AUDIO_READY_TIMEOUT) fires first, spawn() returns false, the video starts playing with has_audio = false, and start = Instant::now() is set. The audio thread eventually finishes pre-rendering, builds and plays the stream, and calls handle.block_on(stop_rx.changed()) — by which point the video playhead has already advanced by the overshoot time, so audio plays unsynced for the rest of the session. The desync scenario described in the comment is live for Windows users on long recordings.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/editor/src/playback.rs
Line: 1180-1187
Comment:
**Windows pre-render can exceed `AUDIO_READY_TIMEOUT`, causing UI freeze then unsynced audio**
On Windows, `create_stream_prerendered` builds a `PrerenderedAudioBuffer` entirely in memory before the stream is created — this work is O(recording length). For a multi-hour recording on modest hardware, the pre-render can take well over 15 seconds. When `ready_rx.recv_timeout(AUDIO_READY_TIMEOUT)` fires first, `spawn()` returns `false`, the video starts playing with `has_audio = false`, and `start = Instant::now()` is set. The audio thread eventually finishes pre-rendering, builds and plays the stream, and calls `handle.block_on(stop_rx.changed())` — by which point the video playhead has already advanced by the overshoot time, so audio plays unsynced for the rest of the session. The desync scenario described in the comment is live for Windows users on long recordings.
How can I resolve this? If you propose a fix, please make it concise.
|
hey @greptileai please re-review the PR |
|
hey @greptileai please re-review the PR |
| if summary.startup_overlap_drops < MIN_STALE_STARTUP_DROPS | ||
| || summary.overlap_dropped_frames < MIN_STALE_STARTUP_DROPS | ||
| || !(MIN_STALE_STARTUP_TRIMMED_MS..=MAX_STALE_STARTUP_REPAIR_MS) | ||
| .contains(&summary.startup_overlap_trimmed_ms) | ||
| { | ||
| return 0.0; | ||
| } |
There was a problem hiding this comment.
The overlap_dropped_frames check looks redundant here (startup drops are a subset of total dropped frames), so this second guard shouldn’t ever gate a case the first didn’t already handle.
| if summary.startup_overlap_drops < MIN_STALE_STARTUP_DROPS | |
| || summary.overlap_dropped_frames < MIN_STALE_STARTUP_DROPS | |
| || !(MIN_STALE_STARTUP_TRIMMED_MS..=MAX_STALE_STARTUP_REPAIR_MS) | |
| .contains(&summary.startup_overlap_trimmed_ms) | |
| { | |
| return 0.0; | |
| } | |
| if summary.startup_overlap_drops < MIN_STALE_STARTUP_DROPS | |
| || !(MIN_STALE_STARTUP_TRIMMED_MS..=MAX_STALE_STARTUP_REPAIR_MS) | |
| .contains(&summary.startup_overlap_trimmed_ms) | |
| { | |
| return 0.0; | |
| } |
| }); | ||
|
|
||
| true | ||
| loop { |
There was a problem hiding this comment.
Minor edge case: this wait loop has no upper bound that includes the pre-render phase. For very long recordings, PrerenderedAudioBuffer::new() can take a while, and the UI will look like it’s stuck until it completes. Might be worth adding an overall timeout/cancel path (or reporting progress) so play can fail fast / remain responsive.
| export type AudioGapSummary = { | ||
| /** | ||
| * Total audio trimmed from overlapping frames over the whole recording, in milliseconds. | ||
| */ | ||
| total_overlap_trimmed_ms: number; | ||
| /** | ||
| * Number of whole audio frames dropped because they fully overlapped the committed timeline. | ||
| */ | ||
| overlap_dropped_frames: number; | ||
| /** | ||
| * Subset of `overlap_dropped_frames` that dropped within the first few frames — the | ||
| * signature of a stale buffered burst at capture start. | ||
| */ | ||
| startup_overlap_drops: number } |
There was a problem hiding this comment.
AudioGapSummary here seems to be missing startup_overlap_trimmed_ms (it exists on the Rust type, and the editor relies on it). Also the formatting is pretty different from the rest of this file.
If this file is generated, I’d fix this at the source (specta/tauri typegen) and re-generate. If it isn’t, the minimal fix would be:
| export type AudioGapSummary = { | |
| /** | |
| * Total audio trimmed from overlapping frames over the whole recording, in milliseconds. | |
| */ | |
| total_overlap_trimmed_ms: number; | |
| /** | |
| * Number of whole audio frames dropped because they fully overlapped the committed timeline. | |
| */ | |
| overlap_dropped_frames: number; | |
| /** | |
| * Subset of `overlap_dropped_frames` that dropped within the first few frames — the | |
| * signature of a stale buffered burst at capture start. | |
| */ | |
| startup_overlap_drops: number } | |
| export type AudioGapSummary = { total_overlap_trimmed_ms: number; startup_overlap_trimmed_ms: number; overlap_dropped_frames: number; startup_overlap_drops: number } |
|
|
||
| true | ||
| loop { | ||
| match ready_rx.recv_timeout(AUDIO_READY_POLL_INTERVAL) { | ||
| Ok(ready) => return ready, | ||
| Err(std_mpsc::RecvTimeoutError::Timeout) => { | ||
| if *stop_rx_before_ready.borrow_and_update() { | ||
| cancelled.store(true, Ordering::Release); | ||
| return false; | ||
| } | ||
| } | ||
| Err(std_mpsc::RecvTimeoutError::Disconnected) => return false, | ||
| } | ||
| } |
There was a problem hiding this comment.
This wait loop can now block indefinitely unless stop_rx flips. If the audio thread wedges before it can send (e.g. device/hardware hang), startup could stall forever. Consider adding an upper bound that also flips cancelled so a late audio thread doesn't start unsynced.
| true | |
| loop { | |
| match ready_rx.recv_timeout(AUDIO_READY_POLL_INTERVAL) { | |
| Ok(ready) => return ready, | |
| Err(std_mpsc::RecvTimeoutError::Timeout) => { | |
| if *stop_rx_before_ready.borrow_and_update() { | |
| cancelled.store(true, Ordering::Release); | |
| return false; | |
| } | |
| } | |
| Err(std_mpsc::RecvTimeoutError::Disconnected) => return false, | |
| } | |
| } | |
| let wait_started = Instant::now(); | |
| loop { | |
| match ready_rx.recv_timeout(AUDIO_READY_POLL_INTERVAL) { | |
| Ok(ready) => return ready, | |
| Err(std_mpsc::RecvTimeoutError::Timeout) => { | |
| if wait_started.elapsed() > Duration::from_secs(120) { | |
| cancelled.store(true, Ordering::Release); | |
| error!("Audio playback thread did not report readiness in time"); | |
| return false; | |
| } | |
| if *stop_rx_before_ready.borrow_and_update() { | |
| cancelled.store(true, Ordering::Release); | |
| return false; | |
| } | |
| } | |
| Err(std_mpsc::RecvTimeoutError::Disconnected) => return false, | |
| } | |
| } |
| if summary.startup_overlap_drops < MIN_STALE_STARTUP_DROPS | ||
| || summary.overlap_dropped_frames < MIN_STALE_STARTUP_DROPS | ||
| || !(MIN_STALE_STARTUP_TRIMMED_MS..=MAX_STALE_STARTUP_REPAIR_MS) | ||
| .contains(&summary.startup_overlap_trimmed_ms) |
There was a problem hiding this comment.
summary.overlap_dropped_frames < MIN_STALE_STARTUP_DROPS looks redundant since startup_overlap_drops <= overlap_dropped_frames by construction, so it can’t filter anything the first guard didn’t already.
| .contains(&summary.startup_overlap_trimmed_ms) | |
| if summary.startup_overlap_drops < MIN_STALE_STARTUP_DROPS | |
| || !(MIN_STALE_STARTUP_TRIMMED_MS..=MAX_STALE_STARTUP_REPAIR_MS) | |
| .contains(&summary.startup_overlap_trimmed_ms) | |
| { | |
| return 0.0; | |
| } |
| loop { | ||
| match ready_rx.recv_timeout(AUDIO_READY_POLL_INTERVAL) { | ||
| Ok(ready) => return ready, | ||
| Err(std_mpsc::RecvTimeoutError::Timeout) => { | ||
| if *stop_rx_before_ready.borrow_and_update() { | ||
| cancelled.store(true, Ordering::Release); | ||
| return false; | ||
| } | ||
| } | ||
| Err(std_mpsc::RecvTimeoutError::Disconnected) => return false, | ||
| } | ||
| } |
There was a problem hiding this comment.
spawn() blocks until audio is “ready”, but the ready signal only happens after pre-render + stream.play() + first callback. For long recordings, pre-render can take a while, so this can look like a hung play action with no upper bound.
A small outer timeout here (starting before pre-render) would prevent indefinite stalls and fall back to video-only playback.
| loop { | |
| match ready_rx.recv_timeout(AUDIO_READY_POLL_INTERVAL) { | |
| Ok(ready) => return ready, | |
| Err(std_mpsc::RecvTimeoutError::Timeout) => { | |
| if *stop_rx_before_ready.borrow_and_update() { | |
| cancelled.store(true, Ordering::Release); | |
| return false; | |
| } | |
| } | |
| Err(std_mpsc::RecvTimeoutError::Disconnected) => return false, | |
| } | |
| } | |
| let ready_wait_started = Instant::now(); | |
| loop { | |
| match ready_rx.recv_timeout(AUDIO_READY_POLL_INTERVAL) { | |
| Ok(ready) => return ready, | |
| Err(std_mpsc::RecvTimeoutError::Timeout) => { | |
| if *stop_rx_before_ready.borrow_and_update() { | |
| cancelled.store(true, Ordering::Release); | |
| return false; | |
| } | |
| if ready_wait_started.elapsed() > Duration::from_secs(15) { | |
| cancelled.store(true, Ordering::Release); | |
| return false; | |
| } | |
| } | |
| Err(std_mpsc::RecvTimeoutError::Disconnected) => return false, | |
| } | |
| } |
| export type AudioGapSummary = { | ||
| /** | ||
| * Total audio trimmed from overlapping frames over the whole recording, in milliseconds. | ||
| */ | ||
| total_overlap_trimmed_ms: number; | ||
| /** | ||
| * Number of whole audio frames dropped because they fully overlapped the committed timeline. | ||
| */ | ||
| overlap_dropped_frames: number; | ||
| /** | ||
| * Subset of `overlap_dropped_frames` that dropped within the first few frames — the | ||
| * signature of a stale buffered burst at capture start. | ||
| */ | ||
| startup_overlap_drops: number } |
There was a problem hiding this comment.
apps/desktop/src/utils/tauri.ts looks like a generated bindings file (also matches the repo’s “don’t edit **/tauri.ts” rule). If this is generated, it’d be safer to regenerate rather than patch by hand.
Also, AudioGapSummary here is missing startup_overlap_trimmed_ms, so it doesn’t match the Rust cap_project::AudioGapSummary shape.
| export type AudioGapSummary = { | |
| /** | |
| * Total audio trimmed from overlapping frames over the whole recording, in milliseconds. | |
| */ | |
| total_overlap_trimmed_ms: number; | |
| /** | |
| * Number of whole audio frames dropped because they fully overlapped the committed timeline. | |
| */ | |
| overlap_dropped_frames: number; | |
| /** | |
| * Subset of `overlap_dropped_frames` that dropped within the first few frames — the | |
| * signature of a stale buffered burst at capture start. | |
| */ | |
| startup_overlap_drops: number } | |
| export type AudioGapSummary = { | |
| /** | |
| * Total audio trimmed from overlapping frames over the whole recording, in milliseconds. | |
| */ | |
| total_overlap_trimmed_ms: number; | |
| /** | |
| * Startup-window trim used for stale-startup repair, excluding mid-recording trims. | |
| */ | |
| startup_overlap_trimmed_ms: number; | |
| /** | |
| * Number of whole audio frames dropped because they fully overlapped the committed timeline. | |
| */ | |
| overlap_dropped_frames: number; | |
| /** | |
| * Subset of `overlap_dropped_frames` that dropped within the first few frames — the | |
| * signature of a stale buffered burst at capture start. | |
| */ | |
| startup_overlap_drops: number; | |
| } |
| .min(MAX_AUDIO_TAIL_PADDING) | ||
| } | ||
|
|
||
| const STARTUP_OVERLAP_DROP_FRAME_COUNT: u64 = 3; |
There was a problem hiding this comment.
STARTUP_OVERLAP_DROP_FRAME_COUNT also exists on the editor side (and the repair threshold is tied to it). Might be worth centralizing this in a shared crate (e.g. cap_project next to AudioGapSummary) so the recorder’s “startup window” and the editor’s repair heuristic can’t silently drift.
1 participant
Greptile Summary
This PR hardens A/V sync across playback and export through several coordinated changes: structured
AudioGapSummarymetadata replaces log-scraping for startup-offset repair, GPU layer warmup is parallelised with decoder creation, the video clock is now stamped only after the audio device fires its first callback, and a default timeline is synthesised from encoded frame counts at recording stop so un-edited recordings are immediately exportable.start = Instant::now()moved afterAudioPlayback::spawn()returns, which blocks on the first device callback via a new channel handshake; initial latency offset switched frominitial_compensation_secs(2× safety multiplier) to rawinitial_output_latency_secs; silent audio frame substituted forNonerenders during export to prevent gaps.AudioGapSummarypipeline: structuredstartup_overlap_trimmed_ms/startup_overlap_dropscounters are accumulated inAudioGapTracker, stored in aOnceLockonFinishedOutputPipeline, written intoAudioMeta.gap_summary, and consumed byaudio_timing_repair_offsetin the editor—replacing fragile log-scraping while keeping a legacy fallback for old recordings.i128intermediates andusize::try_frominstead ofisizeandwrapping_add_signedto prevent index overflow on large negative track offsets.Confidence Score: 4/5
Safe to merge; the changes are well-tested and correctness-focused, with one open concern around startup latency for very long recordings.
The audio timing-repair rewrite is thorough and well-covered by new unit tests. The
start = Instant::now()move correctly ties video to the audio device's first callback, fixing the sync regression. The one remaining concern is thatAudioPlayback::spawn()has no upper-bound timeout for thePrerenderedAudioBuffer::new()pre-render phase: for a multi-hour recording the polling loop can silently stall for tens of seconds before playback begins.crates/editor/src/playback.rs — the spawn() polling loop should have an outer timeout covering the pre-render phase, not just AUDIO_FIRST_CALLBACK_TIMEOUT which starts after stream.play().
Important Files Changed
start = Instant::now()afteraudio_playback.spawn()to synchronize A/V start; adds first-callback handshake withAUDIO_FIRST_CALLBACK_TIMEOUT; hardensprefetch_durationanddurationagainst invalid timelines; changes initial latency frominitial_compensation_secs(2× multiplier) to rawinitial_output_latency_secs.AudioGapSummarymetadata; addsLegacyAudioTimingRepairfallback for old recordings; parallelises audio I/O viaspawn_audio_load; removes 5 s hardcoded fallback duration; minor redundant guard inaudio_timing_repair_offset.AudioGapSummarystruct and per-event startup/total accounting toAudioGapTracker; passesframe_counttorecord_overlap; stores summary inOnceLockonOutputPipelineand surfaces it inFinishedOutputPipeline.TimelineConfigurationandClipConfigurationfrom encoded frame_count/fps at recording stop; propagatesgap_summaryintoAudioMeta; persists populated project config so un-edited recordings have a valid timeline for export/playback.usize::try_fromto prevent overflow on large offsets; fixes stereo base-index calculation; adds regression test for negative offset (leading-silence) behaviour.start_renderer_layers_creationinto start +finish_renderer_layers_creationso GPU layer warmup overlaps decoder/segment creation; removes now-unusedtotal_framesfield.normalize_pending_timestampsto correct stale startup burst timestamps before they are enqueued, preventing phantom overlap events that would otherwise inflate the gap summary.minimum_fallback_framefrom pending requests and skips decode work for frames below that bound, reducing unnecessary decode overhead during seek.Comments Outside Diff (1)
crates/editor/src/editor_instance.rs, line 176-180 (link)When both
Video::newandget_video_duration_fallbackfail (e.g., partially written or corrupt display file), the synthesizedTimelineSegmentusesend: 5.0. A 30-second recording whose display file is unreadable would export with a 5-second timeline, silently truncating content. Since this fallback is exercised for real on first open (before recovery or remux), the project config is then written with the wrongendvalue and persisted. ReturningNonefrom the filter in theMultipleSegmentsarm (same as the explicitduration <= 0.0guard just below) and warning the user would be safer than silently synthesising an arbitrary duration.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "Add test for negative timing offset cons..." | Re-trigger Greptile