From 58485ee2cbceec3f0d9bcc31c3d5ad7a33871988 Mon Sep 17 00:00:00 2001 From: Benjamin Freeman Date: Thu, 27 Aug 2026 18:00:53 +0200 Subject: [PATCH 1/5] fix(capture): stamp Linux video frames with wall-clock PTS to stop time-compression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Linux screen encoder wrote constant-frame-rate H.264 with PTS = a running frame index, and the clock-driven catch-up meant to backfill missed 60fps ticks was capped (MAX_CATCHUP_FRAMES = 8 per advance) and only ran from two starved event-loop arms. Under load `next_index` — which was simultaneously the PTS and the frame counter — fell permanently behind the wall clock, so `file duration == frames_encoded / fps` silently dropped real time: a 61 s session came out as a 55.2 s video that played ~10% fast and drifted ahead of audio, webcam and the cursor overlay, which are all wall-clock based. Stamp each frame's PTS with the wall clock's current frame index instead of a counter, and mux variable-rate: when ticks are missed the next write jumps its PTS to the real index and the container records the gap as that frame's duration, so file length always equals real elapsed time and a stall costs one held frame rather than a deleted span (or an unbounded catch-up burst). The editor and compositor already seek/play by decoded PTS — the same path the already-VFR webcam takes — so playback is unaffected. Report duration from the timeline (next_index) not the encoded count, add a final tail stamp in finish() so a quiet ending is not short, and emit a `timeline-divergence` warning when the file's duration and measured wall-clock time disagree beyond ~100 ms so this cannot regress silently. Rewrite the catch-up tests around the wall-clock invariant and add a sparse-wakeup regression that reproduced the original compression. Fixes getopenscreen/openscreen#511 Co-Authored-By: Claude Opus 4.8 --- .../native/pipewire-capture/src/capture.rs | 204 ++++++++++++++---- .../native/pipewire-capture/src/events.rs | 5 +- electron/native/pipewire-capture/src/main.rs | 18 ++ 3 files changed, 182 insertions(+), 45 deletions(-) diff --git a/electron/native/pipewire-capture/src/capture.rs b/electron/native/pipewire-capture/src/capture.rs index 17c42f801..ac7733279 100644 --- a/electron/native/pipewire-capture/src/capture.rs +++ b/electron/native/pipewire-capture/src/capture.rs @@ -9,15 +9,27 @@ //! recording of anything. //! //! So the output rate comes from a monotonic clock instead. [`Capture::advance`] -//! asks what frame index the wall clock is on and encodes forward to it, holding -//! the last staged picture across the gap. That is why [`crate::encoder`] splits -//! conversion from encoding: a held frame costs an upload and an encode (1.4 ms -//! here) but not the colour conversion (3.6 ms), which is the expensive part. +//! asks what frame index the wall clock is on and stamps the staged picture with +//! THAT index as its PTS, holding the last picture across a gap. That is why +//! [`crate::encoder`] splits conversion from encoding: a held frame costs an +//! upload and an encode (1.4 ms here) but not the colour conversion (3.6 ms), +//! which is the expensive part. +//! +//! WALL-CLOCK PTS, NOT A FRAME COUNTER. The PTS is the clock's frame index, not +//! a running count of frames written — and the two stop agreeing the moment a +//! tick is missed. If the loop is starved under load, an encode runs long, or the +//! screen sits static between wakeups, the next write JUMPS its PTS to the real +//! index and the container stores the skipped slots as that frame's duration. So +//! the file's length always equals real elapsed time: a dropped frame becomes one +//! longer-held frame, never a deleted slice of the timeline. Encoding a running +//! counter instead silently time-compressed recordings under load and desynced +//! the screen from audio, webcam and the cursor overlay (issue #511). Playback is +//! variable-rate, which the editor and compositor already seek/play by decoded +//! PTS (the same path the webcam, itself VFR, has always taken). //! //! The clock is ours, not the compositor's. `SPA_META_Header.pts` is more precise -//! per frame, but pause/resume and — once Stage 2's audio lands — the audio -//! epoch all live on this process's monotonic clock, and quantising to 1/60 s -//! makes the difference between the two immaterial. +//! per frame, but pause/resume and the audio epoch all live on this process's +//! monotonic clock, and quantising to 1/fps makes the difference immaterial. use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -147,14 +159,6 @@ impl AudioMix { } } -/// Frames encoded in one `advance` before returning to the event loop. -/// -/// Without a bound, a long stall would be paid back in a single burst that also -/// blocks `stop` for as long as it takes. At the measured 1.4 ms per held frame -/// this is ~11 ms of work per wakeup, which still catches up eight times faster -/// than real time while leaving the loop responsive. -const MAX_CATCHUP_FRAMES: u32 = 8; - pub struct Selection { pub backend: Backend, /// One line per backend the ladder tried and refused, in order. @@ -185,8 +189,16 @@ fn default_bitrate(width: i32, height: i32, fps: i32) -> i64 { pub struct Summary { pub path: PathBuf, + /// The video timeline's length: the last PTS + 1 frame, in ms. With + /// wall-clock PTS this tracks real elapsed time even when frames were + /// dropped, so it — not the encoded frame count — is what the file lasts. pub duration_ms: u64, + /// Frames actually encoded. Under variable-rate output this can be FEWER than + /// `duration_ms * fps`: a stall is one held frame spanning many slots. pub frames: u64, + /// Real wall-clock time the recording ran, excluding paused spans. Compared + /// against `duration_ms` to flag a regression to time-compression (#511). + pub wall_clock_ms: u64, pub stats: EncodeStats, } @@ -364,27 +376,37 @@ impl Capture { self.epoch.is_some() } - /// Encodes forward to the current clock position. Returns how many frames - /// were written. + /// Stamps the staged picture at the wall clock's current frame index and + /// encodes it. Returns 1 if a frame was written this call, 0 otherwise. + /// + /// ONE ENCODE PER CALL, STAMPED FROM THE CLOCK. The PTS is `current_index()` + /// — the frame index real time is on — not a running counter. When the loop + /// is serviced every tick the indices come out consecutive and the file + /// looks constant-rate; when ticks were missed (loop starved, slow encode, + /// static screen) `next_index` JUMPS past the skipped slots and the container + /// records them as this frame's duration. That jump is what keeps the file's + /// length equal to real elapsed time under drops, with no unbounded catch-up + /// burst to block `stop` (issue #511). Several arrivals inside one 1/fps slot + /// collapse to one write, which is the correct quantisation. pub fn advance(&mut self) -> Result { if self.paused_at.is_some() || !self.encoder.has_staged_frame() { return Ok(0); } let target = self.current_index(); - let mut written = 0; let Some(muxer) = self.muxer.as_mut() else { return Ok(0); }; - while self.next_index <= target && written < MAX_CATCHUP_FRAMES { + let mut written = 0; + if target >= self.next_index { let track = self.video_track; self.encoder - .encode_staged(self.next_index, |packet| muxer.write(track, packet))?; - self.next_index += 1; + .encode_staged(target, |packet| muxer.write(track, packet))?; + self.next_index = target + 1; self.frames_written += 1; - written += 1; + written = 1; } - // Audio is NOT bounded the way video is. A held video frame can be + // Audio is NOT quantised the way video is. A held video frame can be // recreated at any time; a missed audio sample cannot, and the ring // drops the oldest once it fills. Draining every wakeup keeps it far // from that cap — at 48 kHz a 16 ms tick carries about 768 samples. @@ -452,6 +474,21 @@ impl Capture { .take() .ok_or_else(|| "capture was already finished".to_owned())?; + // Close the tail. Stamp one final held frame at the current wall-clock + // index so the file's last PTS reflects real elapsed time even when the + // loop's final heartbeat landed a few ticks before stop — otherwise a + // recording that ended during a quiet spell would be short by that gap. + if self.paused_at.is_none() && self.encoder.has_staged_frame() { + let target = self.current_index(); + if target >= self.next_index { + let track = self.video_track; + self.encoder + .encode_staged(target, |packet| muxer.write(track, packet))?; + self.next_index = target + 1; + self.frames_written += 1; + } + } + // Audio first: whatever is still in the rings is real recorded sound, // and draining it before the video flush keeps both ending at roughly // the same timestamp. `flush` lets the mix take unevenly-filled inputs, @@ -467,28 +504,44 @@ impl Capture { .finish(|packet| muxer.write(video_track, packet))?; muxer.finish()?; + let wall_clock_ms = self + .elapsed_active() + .map(|elapsed| elapsed.as_millis() as u64) + .unwrap_or(0); Ok(Summary { path: self.path.clone(), - // From the frames actually written, not from the clock: those are - // the same number only when the machine kept up, and the file's real - // duration is the one the app should be told about. - duration_ms: (self.frames_written as u64 * 1000) / self.fps.max(1) as u64, + // From the timeline, not the frame count: the last PTS is + // `next_index - 1`, so the presentation spans `next_index` frames. + // With wall-clock PTS this equals real elapsed time even when frames + // were dropped — which the old `frames_written / fps` did not, and is + // the bug being fixed (#511). + duration_ms: (self.next_index as u64 * 1000) / self.fps.max(1) as u64, frames: self.frames_written, + wall_clock_ms, stats: self.encoder.stats(), }) } /// Output frame index the wall clock is currently on, excluding paused time. + /// `-1` before the first frame is staged, so `advance` writes nothing. fn current_index(&self) -> i64 { - let Some(epoch) = self.epoch else { - return -1; - }; - let mut elapsed = epoch.elapsed(); - elapsed = elapsed.saturating_sub(self.paused_total); + match self.elapsed_active() { + Some(elapsed) => (elapsed.as_nanos() as i64 * self.fps as i64) / 1_000_000_000, + None => -1, + } + } + + /// Wall-clock time since the first staged frame, with paused spans removed. + /// `None` until the timeline has started. The single source of both the PTS + /// clock (`current_index`) and the divergence telemetry (`wall_clock_ms`), so + /// the two cannot drift apart by construction. + fn elapsed_active(&self) -> Option { + let epoch = self.epoch?; + let mut elapsed = epoch.elapsed().saturating_sub(self.paused_total); if let Some(since) = self.paused_at { elapsed = elapsed.saturating_sub(since.elapsed()); } - (elapsed.as_nanos() as i64 * self.fps as i64) / 1_000_000_000 + Some(elapsed) } } @@ -604,9 +657,11 @@ mod tests { } #[test] - fn a_static_screen_still_produces_frames() { + fn a_static_screen_still_produces_frames_and_tracks_wall_clock() { // The whole reason the clock drives the output: one staged frame, no - // further arrivals, and the file must still fill with frames. + // further arrivals, and the file's DURATION must still track real time. + // Under variable-rate output a long static gap is one held frame, so the + // event loop's heartbeat is simulated — a tick every ~30 ms. let output = std::env::temp_dir().join("openscreen-capture-static.mp4"); let (mut capture, _) = Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new()) @@ -615,12 +670,18 @@ mod tests { .stage(&frame(320, 240, shim::constants().video_format_bgrx)) .expect("stage"); - std::thread::sleep(Duration::from_millis(150)); - let written = capture.advance().expect("advance"); - assert!(written >= 3, "150 ms at 30 fps should hold at least 3 frames, wrote {written}"); + for _ in 0..5 { + std::thread::sleep(Duration::from_millis(30)); + capture.advance().expect("advance"); + } let summary = capture.finish().expect("finish"); - assert_eq!(summary.frames, written as u64); + assert!(summary.frames >= 1, "a static screen must still produce frames, got {}", summary.frames); + assert!( + summary.duration_ms >= 130, + "the timeline must track the ~150 ms elapsed, got {} ms", + summary.duration_ms + ); let _ = std::fs::remove_file(&output); } @@ -654,7 +715,7 @@ mod tests { assert!(written >= 1, "the cropped picture should have been encoded, wrote {written}"); let summary = capture.finish().expect("finish"); - assert_eq!(summary.frames, written as u64); + assert!(summary.frames >= 1, "the cropped picture must reach the file, got {}", summary.frames); let _ = std::fs::remove_file(&output); } @@ -971,7 +1032,11 @@ mod tests { } #[test] - fn catch_up_is_bounded_so_a_stall_cannot_block_stop() { + fn a_long_stall_is_one_jump_not_a_burst_or_a_deleted_span() { + // A stall (the loop starved under load) must not be paid back as an + // unbounded catch-up burst that blocks `stop`, nor — the #511 bug — as a + // deleted slice of the timeline. Wall-clock PTS represents it as a single + // held frame whose PTS jumps to real time: one encode, honest duration. let output = std::env::temp_dir().join("openscreen-capture-catchup.mp4"); let (mut capture, _) = Capture::start(&output, 320, 240, 60, Some(1_000_000), Some(Backend::Software), Vec::new()) @@ -980,10 +1045,63 @@ mod tests { .stage(&frame(320, 240, shim::constants().video_format_bgrx)) .expect("stage"); - // 500 ms at 60 fps is 30 frames due; one advance must not write them all. + // 500 ms at 60 fps is 30 slots due; a single advance writes ONE frame + // stamped at the real index, not 30 duplicates. std::thread::sleep(Duration::from_millis(500)); let written = capture.advance().expect("advance"); - assert_eq!(written, MAX_CATCHUP_FRAMES); + assert_eq!(written, 1, "a stall is one time-stamped frame, not a burst"); + + let summary = capture.finish().expect("finish"); + assert!( + summary.duration_ms >= 480, + "duration must track the ~500 ms elapsed, got {} ms", + summary.duration_ms + ); + assert!( + summary.frames <= 3, + "the stall must not be backfilled with duplicates, encoded {} frames", + summary.frames + ); + let _ = std::fs::remove_file(&output); + } + + #[test] + fn sparse_wakeups_do_not_compress_the_timeline() { + // THE regression guard for #511. advance() is serviced only a couple of + // times, far less often than the frame rate, as if the event loop were + // starved under load. The old frame-index PTS produced a file shorter + // than real time (55.2 s for 61 s in the field report); wall-clock PTS + // keeps duration ~= elapsed, and the wall-clock telemetry agrees with it. + let output = std::env::temp_dir().join("openscreen-capture-sparse.mp4"); + let (mut capture, _) = + Capture::start(&output, 320, 240, 60, Some(1_000_000), Some(Backend::Software), Vec::new()) + .expect("start"); + capture + .stage(&frame(320, 240, shim::constants().video_format_bgrx)) + .expect("stage"); + + std::thread::sleep(Duration::from_millis(200)); + capture.advance().expect("advance"); + std::thread::sleep(Duration::from_millis(200)); + capture.advance().expect("advance"); + + let summary = capture.finish().expect("finish"); + // ~400 ms of real time, honoured despite only a couple of encoded frames. + assert!( + summary.duration_ms >= 360, + "the timeline compressed: {} ms for ~400 ms of capture", + summary.duration_ms + ); + // Duration and measured wall-clock must agree within a small epsilon — + // the invariant the `timeline-divergence` warning (main.rs) watches. + let skew = (summary.duration_ms as i64 - summary.wall_clock_ms as i64).abs(); + assert!( + skew <= 60, + "duration {} ms and wall-clock {} ms diverged by {} ms", + summary.duration_ms, summary.wall_clock_ms, skew + ); + // Variable-rate, not a duplicate burst: far fewer than 400 ms × 60 fps. + assert!(summary.frames < 10, "expected a handful of held frames, got {}", summary.frames); let _ = std::fs::remove_file(&output); } } diff --git a/electron/native/pipewire-capture/src/events.rs b/electron/native/pipewire-capture/src/events.rs index 085ea56aa..36234ba11 100644 --- a/electron/native/pipewire-capture/src/events.rs +++ b/electron/native/pipewire-capture/src/events.rs @@ -143,8 +143,9 @@ pub enum Event { timestamp_ms: u64, path: String, duration_ms: u64, - /// Frames written to the file, including any duplicated to hold the - /// constant frame rate. + /// Frames actually encoded. Output is variable-rate (each frame stamped + /// with its wall-clock PTS), so under drops this can be FEWER than + /// `duration_ms * fps` — a stall is one held frame spanning many slots. frames: u64, /// Frames the compositor delivered that the encoder never saw, because a /// newer one replaced them in the mailbox first. A non-zero value here diff --git a/electron/native/pipewire-capture/src/main.rs b/electron/native/pipewire-capture/src/main.rs index 8ec4078c2..806a6aa19 100644 --- a/electron/native/pipewire-capture/src/main.rs +++ b/electron/native/pipewire-capture/src/main.rs @@ -1127,6 +1127,24 @@ fn finish_capture( ), }); } + // The video timeline should equal real elapsed wall-clock time. With + // wall-clock PTS it does by construction; a divergence beyond a frame + // or two means frames are being stamped off the clock again (#511) — + // surface it loudly rather than shipping a silently time-compressed + // file that plays ahead of audio, webcam and the cursor overlay. + const TIMELINE_SKEW_EPSILON_MS: i64 = 100; + let skew = summary.duration_ms as i64 - summary.wall_clock_ms as i64; + if skew.abs() > TIMELINE_SKEW_EPSILON_MS { + let _ = emitter.emit(&Event::Warning { + code: "timeline-divergence".to_owned(), + message: format!( + "the recorded video timeline is {} ms but the recording ran {} ms of \ + wall-clock time (off by {} ms); the screen track may be out of sync \ + with audio, webcam and the cursor overlay. See issue #511.", + summary.duration_ms, summary.wall_clock_ms, skew.abs() + ), + }); + } let _ = emitter.emit(&Event::CaptureStopped { timestamp_ms: timestamp_ms(), path: summary.path.display().to_string(), From 281bda8e3ea702f52fce7c505bc671a407c50901 Mon Sep 17 00:00:00 2001 From: Benjamin Freeman Date: Thu, 27 Aug 2026 18:18:06 +0200 Subject: [PATCH 2/5] fix(capture): write the tail frame even when stopped while paused (CodeRabbit #512) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit finish() guarded the final held-frame write on `paused_at.is_none()`, so a stop that arrived while paused skipped it and left next_index at the last heartbeat — dropping the active time between that heartbeat and the pause from the timeline, the same compression this PR fixes. current_index() already freezes at the pause boundary, so the tail write is correct while paused. Add a regression that stages, lets active time pass unserviced, pauses, and finishes without resuming, asserting duration_ms tracks wall_clock_ms. Co-Authored-By: Claude Opus 4.8 --- .../native/pipewire-capture/src/capture.rs | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/electron/native/pipewire-capture/src/capture.rs b/electron/native/pipewire-capture/src/capture.rs index ac7733279..c00e2a289 100644 --- a/electron/native/pipewire-capture/src/capture.rs +++ b/electron/native/pipewire-capture/src/capture.rs @@ -478,7 +478,10 @@ impl Capture { // index so the file's last PTS reflects real elapsed time even when the // loop's final heartbeat landed a few ticks before stop — otherwise a // recording that ended during a quiet spell would be short by that gap. - if self.paused_at.is_none() && self.encoder.has_staged_frame() { + // Runs even when stopped while PAUSED: `current_index()` freezes at the + // pause boundary, so the active time up to the pause still reaches the + // timeline (a stop can follow a pause with no resume in between). + if self.encoder.has_staged_frame() { let target = self.current_index(); if target >= self.next_index { let track = self.video_track; @@ -1065,6 +1068,42 @@ mod tests { let _ = std::fs::remove_file(&output); } + #[test] + fn finishing_while_paused_still_records_the_active_time_before_the_pause() { + // Stop can arrive while paused — the user pauses, then decides to stop + // without resuming. The active time between the last heartbeat and the + // pause must still reach the timeline; dropping it would compress the + // file and desync the screen from audio (#511), the same class of bug. + // current_index() freezes at the pause boundary, so the tail write is + // both safe and required while paused. + let output = std::env::temp_dir().join("openscreen-capture-pause-finish.mp4"); + let (mut capture, _) = + Capture::start(&output, 320, 240, 60, Some(1_000_000), Some(Backend::Software), Vec::new()) + .expect("start"); + capture + .stage(&frame(320, 240, shim::constants().video_format_bgrx)) + .expect("stage"); + + // ~200 ms of active time passes WITHOUT a heartbeat servicing it (loop + // starved), then the user pauses and stops. + std::thread::sleep(Duration::from_millis(200)); + capture.pause(); + let summary = capture.finish().expect("finish"); + + assert!( + summary.duration_ms >= 180, + "the ~200 ms active before the pause must reach the timeline, got {} ms", + summary.duration_ms + ); + let skew = (summary.duration_ms as i64 - summary.wall_clock_ms as i64).abs(); + assert!( + skew <= 60, + "duration {} ms and wall-clock {} ms diverged by {} ms", + summary.duration_ms, summary.wall_clock_ms, skew + ); + let _ = std::fs::remove_file(&output); + } + #[test] fn sparse_wakeups_do_not_compress_the_timeline() { // THE regression guard for #511. advance() is serviced only a couple of From 932d94f61f2b9651cc32d7b522a8b177db66bb39 Mon Sep 17 00:00:00 2001 From: Benjamin Freeman Date: Thu, 27 Aug 2026 18:38:48 +0200 Subject: [PATCH 3/5] fix(capture): freeze staging while paused and snapshot wall-clock before flush (CodeRabbit #512) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Writing the tail frame while paused (previous commit) surfaced two issues in CodeRabbit's re-review: - Privacy: the compositor keeps streaming while the app is paused, so a frame arriving during the pause was still staged, and finish()'s tail write could then encode that POST-pause content into the file when a stop followed a pause with no resume. Gate `stage()` on `paused_at`: a paused recording ingests no new pixels, so the held picture — and the tail frame — is the last pre-pause one. Add a regression asserting a frame received while paused is not staged and never reaches the file. - False telemetry: `wall_clock_ms` was read after the audio/encoder/mp4 flush, which on a long recording keeps the active clock ticking for tens of ms and could trip the `timeline-divergence` warning on a slow flush alone. Snapshot it right after the tail write, where the video timeline is already frozen, so the two are compared at the same instant. Co-Authored-By: Claude Opus 4.8 --- .../native/pipewire-capture/src/capture.rs | 53 +++++++++++++++++-- 1 file changed, 48 insertions(+), 5 deletions(-) diff --git a/electron/native/pipewire-capture/src/capture.rs b/electron/native/pipewire-capture/src/capture.rs index c00e2a289..960be4a0f 100644 --- a/electron/native/pipewire-capture/src/capture.rs +++ b/electron/native/pipewire-capture/src/capture.rs @@ -335,6 +335,17 @@ impl Capture { /// Converts a captured frame into the encoder's staging buffer. Nothing is /// written until [`Self::advance`] runs. pub fn stage(&mut self, frame: &shim::Frame) -> Result<(), String> { + // A paused recording must not ingest new pixels. The compositor keeps + // streaming while the app is paused — pause is app-side — so frames still + // arrive here; staging one would move the held picture to POST-pause + // content, which the tail write in `finish` would then encode into the + // file if a stop follows a pause with no resume. The user expects pause + // to hold that privacy boundary, so the staged picture is frozen at the + // pause instant instead. A no-op, not an error: the frame is simply + // dropped, exactly as a mid-recording drop would be. + if self.paused_at.is_some() { + return Ok(()); + } let format = pixel_format(frame.video_format)?; // Address the crop by moving the START of the slice, and hand swscale the @@ -480,7 +491,9 @@ impl Capture { // recording that ended during a quiet spell would be short by that gap. // Runs even when stopped while PAUSED: `current_index()` freezes at the // pause boundary, so the active time up to the pause still reaches the - // timeline (a stop can follow a pause with no resume in between). + // timeline (a stop can follow a pause with no resume in between). The + // staged picture is the last PRE-pause frame — `stage` is gated on pause — + // so this cannot leak post-pause content into the file. if self.encoder.has_staged_frame() { let target = self.current_index(); if target >= self.next_index { @@ -492,6 +505,17 @@ impl Capture { } } + // Snapshot the active wall-clock time HERE, before the flush below. + // Draining audio, the encoder and the mp4 trailer can take tens of ms on + // a long recording, and `elapsed_active` keeps ticking through it; reading + // it afterwards would make a slow flush look like a timeline divergence + // (issue #512). The video timeline (`next_index`) is already frozen at the + // tail write above, so this is the moment the two are meant to agree. + let wall_clock_ms = self + .elapsed_active() + .map(|elapsed| elapsed.as_millis() as u64) + .unwrap_or(0); + // Audio first: whatever is still in the rings is real recorded sound, // and draining it before the video flush keeps both ending at roughly // the same timestamp. `flush` lets the mix take unevenly-filled inputs, @@ -507,10 +531,6 @@ impl Capture { .finish(|packet| muxer.write(video_track, packet))?; muxer.finish()?; - let wall_clock_ms = self - .elapsed_active() - .map(|elapsed| elapsed.as_millis() as u64) - .unwrap_or(0); Ok(Summary { path: self.path.clone(), // From the timeline, not the frame count: the last PTS is @@ -1068,6 +1088,29 @@ mod tests { let _ = std::fs::remove_file(&output); } + #[test] + fn frames_arriving_while_paused_are_not_staged() { + // A paused recording must not ingest new pixels. The compositor keeps + // streaming while the app is paused, so frames still arrive; staging one + // would move the held picture to post-pause content, which the tail write + // in finish() could then encode when a stop follows a pause (#512). Pause + // must hold that privacy boundary — the staged picture freezes. + let output = std::env::temp_dir().join("openscreen-capture-pause-stage.mp4"); + let (mut capture, _) = + Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new()) + .expect("start"); + + // Pause BEFORE any frame is staged, then a frame arrives during the pause. + capture.pause(); + let staged = capture.stage(&frame(320, 240, shim::constants().video_format_bgrx)); + assert!(staged.is_ok(), "staging while paused is a no-op, not an error: {staged:?}"); + assert!(!capture.started(), "a frame received while paused must not start the timeline"); + + let summary = capture.finish().expect("finish"); + assert_eq!(summary.frames, 0, "nothing captured while paused may reach the file"); + let _ = std::fs::remove_file(&output); + } + #[test] fn finishing_while_paused_still_records_the_active_time_before_the_pause() { // Stop can arrive while paused — the user pauses, then decides to stop From 8ba4787e98b23fa5dfec837fe4da8b092576f2fd Mon Sep 17 00:00:00 2001 From: Benjamin Freeman Date: Sun, 30 Aug 2026 20:49:22 +0200 Subject: [PATCH 4/5] refactor(capture): give a paused frame its own StageOutcome::Frozen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pause guard returned `StageOutcome::Staged`, which is a safe white lie — a pause is not a `Dropped` import failure (that would abort past MAX_CONSECUTIVE_IMPORT_FAILURES) — but it hides a real distinction: nothing was staged. Anything that later reasons about `Staged` (counting encoded frames, import health) would silently fold the pause case in, and with `Staged` overloaded the compiler can't flag it. Add `StageOutcome::Frozen` so a pause-freeze is its own outcome. The exhaustive match in `main` now names it explicitly (grouped with `Staged` — both end an import-failure run), so any future change to that logic must decide what a freeze means rather than inherit `Staged`'s behaviour by accident. Behaviour is unchanged. The pause test now pins `Frozen`. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../native/pipewire-capture/src/capture.rs | 26 ++++++++++++++----- electron/native/pipewire-capture/src/main.rs | 5 +++- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/electron/native/pipewire-capture/src/capture.rs b/electron/native/pipewire-capture/src/capture.rs index b235f51b3..e4c0dd2ab 100644 --- a/electron/native/pipewire-capture/src/capture.rs +++ b/electron/native/pipewire-capture/src/capture.rs @@ -244,6 +244,13 @@ pub struct Capture { pub enum StageOutcome { /// A new frame was staged; `advance` will encode it. Staged, + /// The recording is paused, so the incoming frame was deliberately ignored and + /// the held picture kept frozen at the pause instant (pause is app-side, but the + /// compositor keeps streaming). A distinct outcome on purpose: it is NOT + /// `Staged` — nothing new was staged, so a caller counting encoded frames must + /// not tally it — and NOT `Dropped` — nothing failed, so it must not count + /// toward the import-failure budget that ends a recording. + Frozen, /// A recoverable per-frame failure — one dmabuf the GPU could not map, or a /// transient EAGAIN. The frame is skipped and `advance` holds the previously /// staged one forward, so a single bad frame costs one frame, not the whole @@ -400,16 +407,19 @@ impl Capture { // to hold that privacy boundary, so the staged picture is frozen at the // pause instant instead. // - // Reported as `Staged`, not `Dropped`: `Dropped` is the GPU-import - // failure signal, which warns per frame and ends the recording past - // MAX_CONSECUTIVE_IMPORT_FAILURES — a pause longer than that many frames - // would abort the file. Nothing failed here. + // Its own `Frozen`, not `Dropped` and not `Staged`. Not `Dropped`: that is + // the GPU-import failure signal, which warns per frame and ends the + // recording past MAX_CONSECUTIVE_IMPORT_FAILURES, so a pause longer than + // that many frames would abort the file — nothing failed here. Not `Staged` + // either: nothing was staged, so anything downstream that counts encoded + // frames or reasons about import health must be able to tell a freeze from + // a real frame rather than have it hidden behind `Staged`. // // Ahead of the dmabuf path so the freeze covers the zero-copy route as // well, and so `mark_started` stays untouched: a pause that arrives // before the first frame must leave the capture unstarted. if self.paused_at.is_some() { - return Ok(StageOutcome::Staged); + return Ok(StageOutcome::Frozen); } // Zero-copy dmabuf path: import the tiled GPU buffer into an NV12 VAAPI @@ -1222,7 +1232,11 @@ mod tests { // Pause BEFORE any frame is staged, then a frame arrives during the pause. capture.pause(); let staged = capture.stage(&frame(320, 240, shim::constants().video_format_bgrx)); - assert!(staged.is_ok(), "staging while paused is a no-op, not an error: {staged:?}"); + assert_eq!( + staged.expect("staging while paused is a no-op, not an error"), + StageOutcome::Frozen, + "a frame arriving while paused is Frozen — not Staged (nothing staged) nor Dropped (nothing failed)", + ); assert!(!capture.started(), "a frame received while paused must not start the timeline"); let summary = capture.finish().expect("finish"); diff --git a/electron/native/pipewire-capture/src/main.rs b/electron/native/pipewire-capture/src/main.rs index 90093b26c..07d4b673e 100644 --- a/electron/native/pipewire-capture/src/main.rs +++ b/electron/native/pipewire-capture/src/main.rs @@ -858,7 +858,10 @@ fn run( let (width, height) = (frame.crop.width, frame.crop.height); mailbox.recycle(frame.pixels); match staged { - Ok(capture::StageOutcome::Staged) => { + // A real frame, or a pause-freeze — neither is an import + // failure, so end any run of them. (A freeze holds the last + // picture; `advance` is a no-op while paused.) + Ok(capture::StageOutcome::Staged | capture::StageOutcome::Frozen) => { consecutive_drops = 0; } // Recoverable: one frame the GPU could not import. Warn so it From 07d3739fbba82f11e7cf0684aa7c931152b46d94 Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Thu, 3 Sep 2026 17:46:43 +0200 Subject: [PATCH 5/5] fix(preview): hold the frame a VFR seek lands on, not the one after it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `decode_at` decoded forward until `pts >= target` and returned THAT frame — the first one at or after the target, rather than the one actually on screen at that instant. The invariant the rest of the crate follows is the opposite: at time t you show the LAST frame whose pts is <= t, never a frame from the future. `timeline_walk::frame_step` already applies it to the sequential pump, and its doc records that this exact bug was fixed there once. The seek path never was. On a constant-rate source the two differ by one frame period — 16 ms at 60 fps, invisible, which is why it survived. On a variable-rate source they differ by the whole gap. The webcam (`MediaRecorder`) and macOS captures are already VFR, and this branch makes the Linux screen capture VFR too, so a seek landing in a held-frame gap returned an image from the future. Measured on a VFR file with a 520 ms hold (frames at 1.000 s and 1.520 s), seeking to 1.219 s: before, the frame at 1.520 s — 301 ms ahead; after, the held frame at 1.000 s. The PR description lists this as an accepted caveat, on the grounds that a gap means a static screen so the two frames are near-identical. That holds when the compositor sent nothing, but not for the case this branch exists to fix: there the gap is frames DROPPED under load, and the content moved across it. The decision is extracted as `seek_step`, mirroring `timeline_walk::frame_step`, so the boundary cases are testable without ffmpeg or a fixture. A frame landing ON the target stops the search rather than continuing. That is not an optimisation: `decode_at` does not only return a picture, it leaves the stream positioned for the sequential pump that follows (`next_frame` / `peek_next_time_sec`). Decoding one frame past the target advanced that position, which shifted everything downstream — a first version of this commit did exactly that and changed 47 of 3600 frames in a real export. The equality is judged within a microsecond, because the target is a reconstructed float and the pts an integer times the timebase, so the two never coincide bit-for-bit. Constant-rate sources are therefore untouched, verified rather than assumed: a full S4 export (3600 frames, 1080p60, webcam, cursor, three zooms) is byte-identical to the same export built from this branch's merge-base — md5 4e74ab24873e9c76ed02ddebaf99f121 both ways — at 30.02 s against 30.07 s for the control. The longer walk now runs only in the VFR-gap case, which is the only case that needs it. --- crates/compositor/src/linux_decode.rs | 181 +++++++++++++++++++++++++- 1 file changed, 175 insertions(+), 6 deletions(-) diff --git a/crates/compositor/src/linux_decode.rs b/crates/compositor/src/linux_decode.rs index 1255898ee..6e1a96f3c 100644 --- a/crates/compositor/src/linux_decode.rs +++ b/crates/compositor/src/linux_decode.rs @@ -40,6 +40,62 @@ extern "C" { /// `timestamp` comme un timestamp absolu (AV_TIME_BASE = microsecondes). const SEEK_SET: i32 = 0; +/// Ce que la boucle d'avance de `decode_at` doit faire de la frame qu'elle vient +/// de décoder. +#[derive(Debug, Clone, Copy, PartialEq)] +pub(crate) enum SeekStep { + /// La frame est due : l'adopter, puis continuer à chercher mieux. + Adopt, + /// La frame dépasse la cible et on tient déjà la frame due : s'arrêter sans + /// l'adopter. + Stop, + /// La frame dépasse la cible mais on ne tient rien — la cible précède la + /// première frame du flux. L'adopter en repli et s'arrêter. + AdoptAndStop, +} + +/// Tolérance d'égalité entre un pts et la cible, en secondes. +/// +/// La cible est reconstruite en flottant (`idx / fps`, via un aller-retour par les +/// microsecondes) et le pts est un entier multiplié par la timebase : deux frames +/// « au même instant » ne tombent donc pas sur le même `f64`. Une microseconde est +/// quatre ordres de grandeur sous la période la plus courte qu'on rencontre +/// (1/240 s) et dix ordres au-dessus de l'erreur de représentation, donc elle +/// sépare « la même frame » de « la frame d'après » sans ambiguïté. +const PTS_EPSILON_SEC: f64 = 1e-6; + +/// Décision pure de la sémantique de hold pour le chemin de SEEK — pendant de +/// `timeline_walk::frame_step`, qui l'applique au pompage séquentiel. Extraite pour +/// la même raison : c'est ici que vivent les cas limites, et les tester ne doit +/// demander ni ffmpeg ni fichier. +/// +/// L'invariant est celui de tout le crate : à l'instant t on affiche la DERNIÈRE +/// frame dont le pts est ≤ t, jamais une frame encore à venir. +/// +/// S'ARRÊTER SUR LA CIBLE, PAS APRÈS. Une frame qui tombe SUR la cible est due, et +/// rien de plus tard ne peut faire mieux : on l'adopte et on s'arrête. Continuer +/// à décoder « au cas où » coûterait une frame de plus, et surtout laisserait le +/// décodeur UNE FRAME PLUS LOIN — or `decode_at` ne fait pas que rendre une image, +/// il positionne le flux pour le pompage séquentiel qui suit +/// (`next_frame`/`peek_next_time_sec`). Un seul appel avançant d'une frame de trop +/// décalait la suite de l'export : mesuré 47 frames modifiées sur 3600. +pub(crate) fn seek_step(pts_sec: f64, target_sec: f64, have_candidate: bool) -> SeekStep { + // Au-delà de la cible : c'est une frame du futur. On tient déjà la frame due, + // sauf si la cible précède la première frame du flux — alors celle-ci est le + // meilleur choix disponible. + if pts_sec > target_sec + PTS_EPSILON_SEC { + return if have_candidate { SeekStep::Stop } else { SeekStep::AdoptAndStop }; + } + // Sur la cible (à l'epsilon près) : due, et rien de mieux ne viendra. + if pts_sec >= target_sec - PTS_EPSILON_SEC { + return SeekStep::AdoptAndStop; + } + // Avant la cible : due pour l'instant, mais une frame plus tardive peut encore + // l'être aussi — c'est ce parcours qui, sur une source à cadence variable, + // finit par tenir la bonne frame au lieu de sauter à celle d'après. + SeekStep::Adopt +} + /// Cherche la vidéo du fichier, ouvre le décodeur, et rend un état prêt à /// décoder. La struct expose `decode_at(frame_idx)` qui seek + décode jusqu'à /// la frame `frame_idx` (0-indexée depuis le début du flux). @@ -307,8 +363,10 @@ impl SwDecoder { } /// Seek vers la keyframe la plus proche AVANT `frame_idx`, puis décode - /// jusqu'à atteindre la frame demandée. Le seek est résolu par - /// `av_seek_frame` avec `SEEK_SET | BACKWARD` (cherche le keyframe + /// jusqu'à la frame DUE à cet instant : la dernière dont le pts est ≤ la + /// cible, jamais une frame encore à venir (même invariant que + /// `timeline_walk::frame_step`, cf. la boucle plus bas). Le seek est résolu + /// par `av_seek_frame` avec `SEEK_SET | BACKWARD` (cherche le keyframe /// précédent le timestamp demandé). Renvoie une `AVFrame` allouée par /// `av_frame_alloc` que le caller doit libérer via `free_frame` — /// ou laisser `vk_frames::VkFrames::present` consommer (qui réécrit @@ -334,9 +392,10 @@ impl SwDecoder { // // Sans BACKWARD, ffmpeg se cale sur la première position indexée AU NIVEAU OU // APRÈS la cible, au lieu de la keyframe qui la précède. La boucle d'avance - // ci-dessous s'arrête dès que `pts >= target`, condition alors satisfaite par la - // toute première frame décodée : elle ne fait plus rien et `decode_at` rend la - // keyframe SUIVANTE. L'erreur est d'un GOP entier. + // ci-dessous n'a alors plus rien à avancer — la toute première frame décodée + // dépasse déjà la cible, et faute de frame antérieure à tenir elle est rendue + // telle quelle : `decode_at` rend la keyframe SUIVANTE. L'erreur est d'un GOP + // entier. // // D'où le symptôme asymétrique signalé : l'écran porte une keyframe toutes les // ~1,78 s, la webcam toutes les ~6,73 s, donc l'écart y est ~4x plus grand. Et @@ -419,6 +478,22 @@ impl SwDecoder { loop { let recv_r = avcodec_receive_frame(self.dec, frame); if recv_r == 0 { + let pts_sec = (*frame).best_effort_timestamp as f64 * self.stream_timebase; + // SÉMANTIQUE DE HOLD, cf. `seek_step`. L'ancienne condition + // (`>= cible`, testée APRÈS adoption) rendait la première frame + // AU-DELÀ de la cible. Sur une source à cadence constante + // l'écart est d'une période (16 ms à 60 fps) et ne se voit pas ; + // sur une source à cadence VARIABLE il vaut tout le trou. La + // webcam (`MediaRecorder`) et les captures macOS sont déjà VFR, + // et la capture Linux le devient — un seek tombant dans un trou + // rendait une image du FUTUR, jusqu'à plusieurs centaines de ms + // en avance. + let step = seek_step(pts_sec, target_ts_seconds, !found.is_null()); + // On tient déjà la frame due : s'arrêter sans adopter. Le + // nettoyage après la boucle libère `frame`. + if step == SeekStep::Stop { + break 'outer; + } if found.is_null() { found = av_frame_alloc(); if found.is_null() { @@ -442,7 +517,9 @@ impl SwDecoder { av_frame_unref(found); av_frame_move_ref(found, frame); av_frame_unref(frame); - if (*found).best_effort_timestamp as f64 * self.stream_timebase >= target_ts_seconds { + // Repli : la frame adoptée dépasse déjà la cible (celle-ci + // précède la première frame du flux), rien de mieux ne viendra. + if step == SeekStep::AdoptAndStop { break 'outer; } } else if recv_r == -11 { @@ -518,4 +595,96 @@ mod tests { let r = SwDecoder::open("Z:/does/not/exist.mp4"); assert!(r.is_err()); } + + /// LA garde anti-régression du seek en cadence variable. + /// + /// Un trou de 500 ms — une frame tenue à 10,0 s, la suivante seulement à + /// 10,5 s, ce que produit une capture qui a perdu des frames sous charge. + /// Chercher au milieu du trou doit rendre la frame TENUE, pas celle d'après : + /// à 10,2 s c'est bien l'image de 10,0 s qui est à l'écran. + /// + /// L'ancienne condition (`pts >= cible`) rendait ici la frame de 10,5 s, soit + /// 300 ms de futur. Invisible en cadence constante (l'écart y vaut une + /// période), franc dès que la source a des trous. + #[test] + fn un_seek_dans_un_trou_rend_la_frame_tenue_pas_celle_du_futur() { + assert_eq!(seek_step(10.0, 10.2, false), SeekStep::Adopt); + assert_eq!(seek_step(10.5, 10.2, true), SeekStep::Stop); + } + + /// Une frame qui tombe SUR la cible est due, et on s'arrête là. + /// + /// C'est le cas NORMAL, pas un cas limite : `Decoder::seek_to` quantifie la + /// cible en index de frame (`round(secondes * fps)`), donc sur une source à + /// cadence constante chaque seek tombe pile sur un pts. + #[test] + fn une_frame_pile_sur_la_cible_est_due_et_arrete_la_recherche() { + assert_eq!(seek_step(0.4, 0.4, false), SeekStep::AdoptAndStop); + assert_eq!(seek_step(0.4, 0.4, true), SeekStep::AdoptAndStop); + } + + /// L'égalité se juge à l'epsilon près, sinon elle ne se produit jamais. + /// + /// La cible est reconstruite en flottant et le pts est un entier fois la + /// timebase : ils encadrent le même instant sans jamais coïncider au bit près. + /// Sans tolérance, une frame un cheveu SOUS la cible repart pour un tour et le + /// décodeur finit une frame trop loin — le défaut qui a modifié 47 frames sur + /// 3600 dans un export réel avant que ce test n'existe. + #[test] + fn l_egalite_avec_la_cible_tolere_l_erreur_de_representation() { + let target = 3.0 / 60.0; + for delta in [-1e-12, -1e-9, 0.0, 1e-12, 1e-9] { + assert_eq!( + seek_step(target + delta, target, true), + SeekStep::AdoptAndStop, + "un ecart de {delta:e} s doit compter comme « sur la cible »" + ); + } + } + + /// Cadence constante : on s'arrête EXACTEMENT sur la frame cible, comme avant + /// la sémantique de hold. Ce n'est pas qu'une question d'image rendue — + /// `decode_at` positionne aussi le flux pour le pompage séquentiel qui suit, + /// donc s'arrêter un cran plus loin décalerait tout l'export. C'est ce qui + /// garantit qu'aucun export existant ne bouge d'un octet. + #[test] + fn en_cadence_constante_on_s_arrete_pile_sur_la_cible() { + let target = 3.0 / 60.0; + let mut adopted: Option = None; + let mut decoded = 0; + for i in 0..10 { + let pts = i as f64 / 60.0; + decoded += 1; + match seek_step(pts, target, adopted.is_some()) { + SeekStep::Adopt => adopted = Some(pts), + SeekStep::AdoptAndStop => { + adopted = Some(pts); + break; + } + SeekStep::Stop => break, + } + } + assert_eq!(adopted, Some(target), "la frame due est celle qui porte la cible"); + assert_eq!(decoded, 4, "4 frames decodees (0..=3), pas une de plus"); + } + + /// Une cible antérieure à la première frame du flux ne doit pas rendre `Err` : + /// il n'y a pas de frame due, la première disponible est le meilleur choix. + /// C'est le seul cas où l'on adopte une frame du futur, faute de mieux. + #[test] + fn une_cible_avant_la_premiere_frame_rend_la_premiere_frame() { + assert_eq!(seek_step(5.0, 0.0, false), SeekStep::AdoptAndStop); + } + + /// Un flux sans pts exploitable donne `best_effort_timestamp == i64::MIN`, qui + /// converti en secondes est très négatif — donc toujours « due », donc adopté + /// jusqu'à l'EOF, et `decode_at` rend la dernière frame. Comportement + /// inchangé, vérifié ici pour qu'un futur remaniement ne le casse pas en + /// silence. + #[test] + fn un_flux_sans_pts_continue_de_rendre_la_derniere_frame() { + let sans_pts = i64::MIN as f64 * 1e-6; + assert_eq!(seek_step(sans_pts, 0.0, true), SeekStep::Adopt); + assert_eq!(seek_step(sans_pts, 1_000.0, true), SeekStep::Adopt); + } }