fix(recording): keep system audio on the recording timeline when delivery is intermittent#1980
Conversation
…very is intermittent WASAPI loopback only produces packets while something renders audio, so a system-audio track could start at the first sound (not the recording start), lose long silent stretches to the 1s gap-insertion cap, and end short of the stop point due to a frame-of-reference bug in tail padding. A late first packet also became the latest start_time, anchoring playback past the head of every other track. - Anchor system-audio pipelines at the recording epoch (AudioAnchor::PipelineEpoch): head silence is synthesized from the epoch to the first packet, start_time is reported as ~0, and a track that never receives a packet still spans the recording. Mic/camera keep first-frame (device-ready) anchoring. - Insert the full wall-clock-validated gap instead of truncating to 1s; long fills are chunked into <=1s frames. Gap health events are only emitted for device-backed sources where a gap is a real anomaly. - Fix tail padding to compare the epoch-relative target against the track's own timeline (subtract the start offset) and fill the true trailing gap instead of capping at 300ms. - Keep a silent render stream open on the captured endpoint (Windows) so loopback delivers packets through system silence, and bump cpal to zero capture buffers flagged AUDCLNT_BUFFERFLAGS_SILENT instead of passing unspecified contents through. - Sync test coverage: sync_matrix system-audio cases (late first sound, dead zone, silent throughout, bursty notifications, concurrent mic + system audio) verifying start anchoring, silence materialization and wall-clock content positions; playback selftest fixture now records a continuous mic plus loopback-style burst-delivered system audio with recorder-faithful clip offsets; meta tests lock the anchor semantics.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| |data, _| { | ||
| data.bytes_mut().fill(0); | ||
| }, | ||
| |_| {}, |
There was a problem hiding this comment.
The keepalive output stream error callback is currently a no-op, so device-loss / stream errors will be completely silent. Might be worth logging (even at debug!) for diagnostics.
| |_| {}, | |
| |e| tracing::warn!("loopback silence keepalive stream error: {e}"), |
| &self.config | ||
| } | ||
|
|
||
| /// Whether the silent keepalive render stream is active (Windows only). |
There was a problem hiding this comment.
Nit: has_silence_keepalive() only tells you the stream was built, not that it’s actually playing (or that play() succeeded). Tweaking wording avoids confusion.
| /// Whether the silent keepalive render stream is active (Windows only). | |
| /// Whether a silent keepalive render stream is configured (Windows only). |
| async move { | ||
| let capturer = setup_result.map_err(|e| anyhow!("{e}"))?; | ||
| if capturer.has_silence_keepalive() { | ||
| info!("System audio loopback silence keepalive active"); |
There was a problem hiding this comment.
This log happens right after capturer construction (before Capturer::play()), so “active” reads a bit strong — more like “keepalive available/configured”.
| info!("System audio loopback silence keepalive active"); | |
| info!("System audio loopback silence keepalive configured"); |
| frame_ts: Timestamp, | ||
| start_samples: u64, |
There was a problem hiding this comment.
Minor readability: you already have duration_to_sample_count(), so you can avoid the custom ms math here (and the implicit rounding via as_millis()).
| frame_ts: Timestamp, | |
| start_samples: u64, | |
| let chunk_max = duration_to_sample_count(SILENCE_FRAME_MAX, sample_rate).max(1); |
| use futures::SinkExt; | ||
| for (start, end) in bursts { | ||
| let chunk_frames = (f64::from(rate) * CHUNK_SECS) as usize; | ||
| let chunks = ((end - start) / CHUNK_SECS).round() as usize; |
There was a problem hiding this comment.
Minor robustness thing: round() here can flip the chunk count if end-start isn't exactly representable as f64 (and then the burst either truncates or extends past end, which makes the span checks annoying to debug). Adding an assert around the rounding would make failures much easier to diagnose.
| let chunks = ((end - start) / CHUNK_SECS).round() as usize; | |
| let chunks_f = (end - start) / CHUNK_SECS; | |
| let chunks = chunks_f.round() as usize; | |
| debug_assert!((chunks_f - chunks as f64).abs() < 1e-6); |
| drain(&mut decoder, &mut samples, &mut rate, &mut frame); | ||
|
|
||
| if rate == 0 || samples.is_empty() { | ||
| return Err("no audio decoded".to_string()); |
There was a problem hiding this comment.
This currently drops any non-F32(Planar) decode output on the floor, so the failure mode is just no audio decoded even if ffmpeg decoded fine but chose a different output format on some platform/build. Might be worth making the error message self-describing to avoid CI archaeology.
| return Err("no audio decoded".to_string()); | |
| if rate == 0 || samples.is_empty() { | |
| return Err(format!( | |
| "no audio decoded (rate={rate}, format={:?}; expected f32 planar)", | |
| frame.format() | |
| )); | |
| } |
| async move { | ||
| let capturer = setup_result.map_err(|e| anyhow!("{e}"))?; | ||
| if capturer.has_silence_keepalive() { | ||
| info!("System audio loopback silence keepalive active"); |
There was a problem hiding this comment.
Nit: has_silence_keepalive() is really “keepalive stream is available/configured”, not “it’s actively playing” (that only happens in Capturer::play(), and even then it’s best-effort). This log reads a bit stronger than what we can guarantee.
| info!("System audio loopback silence keepalive active"); | |
| info!("System audio loopback silence keepalive available"); |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Hosted Windows runners have no audio device, so the loopback keepalive, AUDCLNT_BUFFERFLAGS_SILENT zeroing and QPC capture timestamps could never run in CI. Install the Scream virtual sound card in the sync-tests Windows job and add an endpoint integration test that asserts packets flow through engine silence (keepalive), silence decodes as digital zeros (SILENT flag handling), capture timestamps track real time, and a tone played on the endpoint is captured. Skips loudly on machines with no endpoint; CAP_REQUIRE_AUDIO_ENDPOINT=1 (set in CI) makes a missing endpoint a hard failure so a broken driver install cannot silently skip.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
| if: runner.os == 'Windows' | ||
| shell: pwsh | ||
| run: | | ||
| Invoke-WebRequest -Uri https://github.com/duncanthrax/scream/releases/download/4.0/Scream4.0.zip -OutFile Scream4.0.zip |
There was a problem hiding this comment.
CI workflow downloads and installs unaudited third-party kernel driver without integrity verification
Downloads and installs a third-party kernel driver without verifying its integrity.
Add SHA-256 verification of the downloaded zip before extraction and driver installation.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name=".github/workflows/sync-tests.yml">
<violation number="1" location=".github/workflows/sync-tests.yml:93">
<priority>P1</priority>
<title>CI workflow downloads and installs unaudited third-party kernel driver without integrity verification</title>
<evidence>The workflow step downloads Scream4.0.zip from duncanthrax/scream GitHub releases, extracts Scream.sys, adds its Authenticode certificate to the LocalMachine TrustedPublisher store, and installs the driver via devcon-x64.exe. No SHA-256 or other integrity check is performed on the downloaded artifact.</evidence>
<recommendation>Add a SHA-256 hash verification of the downloaded Scream4.0.zip before extraction, or pin the release artifact to a verified checksum. Consider whether a kernel driver installation is necessary in CI, or replace with a software-only virtual audio device alternative.</recommendation>
</violation>
</file>
superagent-security
Bot
added
the
pr:flagged
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
| run: | | ||
| Invoke-WebRequest -Uri https://github.com/duncanthrax/scream/releases/download/4.0/Scream4.0.zip -OutFile Scream4.0.zip | ||
| Expand-Archive -Path Scream4.0.zip -DestinationPath Scream | ||
| $cert = (Get-AuthenticodeSignature Scream\Install\driver\x64\Scream.sys).SignerCertificate |
There was a problem hiding this comment.
Tiny hardening for the driver install step: it’s worth validating the Authenticode signature status before importing whatever cert is in the downloaded zip into TrustedPublisher.
| $cert = (Get-AuthenticodeSignature Scream\Install\driver\x64\Scream.sys).SignerCertificate | |
| $sig = Get-AuthenticodeSignature Scream\Install\driver\x64\Scream.sys | |
| if ($sig.Status -ne 'Valid' -or -not $sig.SignerCertificate) { throw "Scream.sys signature invalid: $($sig.Status)" } | |
| $cert = $sig.SignerCertificate |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
superagent-security
Bot
removed
the
pr:flagged
| @@ -603,6 +602,359 @@ fn bytemuck_cast_f32(data: &mut [u8]) -> &mut [f32] { | |||
| unsafe { std::slice::from_raw_parts_mut(data.as_mut_ptr().cast::<f32>(), len) } | |||
There was a problem hiding this comment.
bytemuck_cast_f32() is doing an u8 -> f32 cast without any alignment/length checks, which is UB if the backing buffer isn’t properly aligned.
| unsafe { std::slice::from_raw_parts_mut(data.as_mut_ptr().cast::<f32>(), len) } | |
| fn bytemuck_cast_f32(data: &mut [u8]) -> &mut [f32] { | |
| let (prefix, samples, suffix) = data.align_to_mut::<f32>(); | |
| debug_assert!(prefix.is_empty() && suffix.is_empty()); | |
| samples | |
| } |
| frame.set_rate(AUDIO_RATE); | ||
| let data = frame.data_mut(0); | ||
| let samples = unsafe { | ||
| std::slice::from_raw_parts_mut(data.as_mut_ptr().cast::<f32>(), data.len() / 4) |
There was a problem hiding this comment.
Same deal here: from_raw_parts_mut(...cast::<f32>()) can be UB if the buffer isn’t aligned. Using align_to_mut keeps it safe (and the debug assert makes failures obvious).
| std::slice::from_raw_parts_mut(data.as_mut_ptr().cast::<f32>(), data.len() / 4) | |
| let (prefix, samples, suffix) = data.align_to_mut::<f32>(); | |
| debug_assert!(prefix.is_empty() && suffix.is_empty()); |
… step 3.8/4.0 driver signatures postdate the 2021 kernel cross-signing deprecation; PnP blocks their install on an interactive consent prompt, which hangs a headless runner. 3.6's signature chain installs silently. A step timeout turns any future hang into a fast failure.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
| $actual = (Get-FileHash -Algorithm SHA256 Scream3.6.zip).Hash.ToLowerInvariant() | ||
| if ($actual -ne $expected) { throw "Scream3.6.zip SHA-256 mismatch: got $actual, expected $expected" } | ||
| Expand-Archive -Path Scream3.6.zip -DestinationPath Scream | ||
| $cert = (Get-AuthenticodeSignature Scream\Install\driver\Scream.sys).SignerCertificate |
There was a problem hiding this comment.
We’re importing a cert from the downloaded zip straight into LocalMachine\\TrustedPublisher. It’d be good to sanity-check the Authenticode signature first (at least ensure it’s actually signed and not hash-mismatched) before trusting the signer cert.
| $cert = (Get-AuthenticodeSignature Scream\Install\driver\Scream.sys).SignerCertificate | |
| $sig = Get-AuthenticodeSignature Scream\Install\driver\Scream.sys | |
| if (-not $sig.SignerCertificate -or $sig.Status -in @('NotSigned','HashMismatch')) { throw "Scream.sys signature invalid: $($sig.Status)" } | |
| $cert = $sig.SignerCertificate |
1 participant
Context
A Windows 0.5.4 user reported "microphone audio is delayed and lags behind the video when system audio is recorded" (Discord, with an attached .cap). Deep analysis of that recording showed the recorded tracks, persisted clip offsets, and export were all correctly aligned (mic click transients match cursor.json click timestamps at ~0ms; a full export re-measure confirmed ±2ms) — the perceived lag is the mic acoustically picking up system sounds ~330ms late through their VoiceMeeter + speaker output chain, which is faithfully recorded and only audible as an echo when the system-audio track exists for comparison.
The investigation did surface four real latent defects in how intermittent (WASAPI-loopback-style) system audio interacts with the sync architecture. This PR fixes all of them and extends the sync test infrastructure with realistic system-audio delivery shapes.
Defects fixed
system_audio.start_timewas the time of the first sound, not source-ready. Because playback anchors at the latest trackstart_time, a recording where the first sound comes N seconds in would cut the first N seconds of video and mic. System-audio pipelines are now anchored at the recording epoch (AudioAnchor::PipelineEpoch): head silence is synthesized from the epoch to the first packet andstart_timeis reported as ~0.0, so the track can never become the alignment anchor. Mic/camera keep first-frame (device-ready) anchoring, and legacy recordings keep their historical alignment (no meta/editor semantics changed — locked by tests).detect_gapcapped each silence insertion at 1s even though the gap is already wall-clock-validated, so audio resuming after a long silent stretch was placed up to the truncated amount too early while repeated insertions converged. The full gap is now inserted, chunked into ≤1s frames.scap-cpalnow keeps a silent render stream open on the captured endpoint (the OBS workaround) so loopback delivers packets through system silence, and the cpal fork is bumped to zero capture buffers flaggedAUDCLNT_BUFFERFLAGS_SILENTinstead of passing unspecified buffer contents through (CapSoftware/cpalfix/wasapi-honor-silent-flag).Gap health events are now only emitted for device-backed sources, so normal loopback silence no longer surfaces "Audio gap detected" warnings.
Test coverage added
sync_matrixsystem-audio cases driving the real pipeline with loopback-style delivery:late-first-sound,dead-zone,silent-throughout,bursty-notifications, andwith-mic(concurrent mic + system-audio pipelines sharing the recording epoch — the reported scenario). Each verifies epoch anchoring, silence materialization, full-span duration, and that every burst decodes at its true wall-clock position via RMS-envelope span checks.cap selftest playback) now records a continuous mic (starting 0.3s in, device-warmup style) plus burst-only system audio with coincident beeps and recorder-faithful clip offsets, so the headless editor-playback and export measurements exercise the whole chain.latest_start_time/calculate_audio_offsetsanchor semantics for both new and legacy recordings.Verification
cap-recording201/201 unit tests,cap-project30/30,cap-editor18/18,cap-enc-ffmpeg38/38.cap selftest playback: PASS with the new two-audio-track fixture (7/7 events, drift −12ms, mad 9ms, export leg matches).cap selftest av-syncinfrastructure: recorded system_audio.ogg spans epoch→stop (24.11s) withstart_time: 0.0while display starts at 0.082s.sync-tests.ymlon this PR (macOS dev machine cannot cross-build the Windows C deps).Greptile Summary
This PR keeps intermittent system audio aligned with the recording timeline. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (1): Last reviewed commit: "chore: cargo fmt" | Re-trigger Greptile
Context used: