From e25e83843c3f07ee1d8cd3283d619a01a98be799 Mon Sep 17 00:00:00 2001 From: tstoltz Date: Sun, 19 Apr 2026 12:26:39 +0200 Subject: [PATCH 1/6] fix(linux): use two-level Frame::Video(VideoFrame::...) enum + SystemTime display_time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Frame enum was refactored into a two-level shape (Frame::Audio / Frame::Video(VideoFrame::*)) and the per-format structs' display_time field was changed from u64 to SystemTime, but the Linux PipeWire engine in src/capturer/engine/linux/mod.rs was not updated to match. Result: 8 compile errors on Linux, no crates.io release compiles there. This commit: - Wraps the four emit sites (RGBx / RGB / xBGR / BGRx) in Frame::Video(VideoFrame::...(...)) to match frame::Frame's current two-level shape. - Replaces `display_time: timestamp as u64` with `display_time: SystemTime::now()`, matching what the macOS and Windows engines already do. The raw PipeWire pts from spa_meta_header is monotonic ns since an arbitrary reference, not wall-clock, so it cannot be converted losslessly to SystemTime. Relative frame ordering survives via channel-send order. - Adds SystemTime to the std::time imports and VideoFrame to the crate::frame imports. `cargo check` on Linux (Ubuntu 24, pipewire 1.0, via nix develop) now succeeds — 16 warnings remaining, all pre-existing lifetime- elision nits unrelated to this fix. --- src/capturer/engine/linux/mod.rs | 37 ++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/capturer/engine/linux/mod.rs b/src/capturer/engine/linux/mod.rs index 07967fb3..471458aa 100644 --- a/src/capturer/engine/linux/mod.rs +++ b/src/capturer/engine/linux/mod.rs @@ -5,7 +5,7 @@ use std::{ mpsc::{self, sync_channel, SyncSender}, }, thread::JoinHandle, - time::Duration, + time::{Duration, SystemTime}, }; use pipewire as pw; @@ -31,7 +31,7 @@ use pw::{ use crate::{ capturer::Options, - frame::{BGRxFrame, Frame, RGBFrame, RGBxFrame, XBGRFrame}, + frame::{BGRxFrame, Frame, RGBFrame, RGBxFrame, VideoFrame, XBGRFrame}, }; use self::{error::LinCapError, portal::ScreenCastPortal}; @@ -133,31 +133,40 @@ fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) { .to_vec() }; + // `timestamp` (from spa_meta_header.pts) is a PipeWire monotonic + // nanosecond count since an arbitrary reference — not wall-clock. + // display_time's SystemTime contract is wall-clock, so we use + // SystemTime::now() here (matches what the macOS and Windows + // engines do today). Relative frame ordering survives via + // channel-send order; sub-millisecond buffer timing is lost. + let _ = timestamp; // suppress "unused" warning until we wire pts elsewhere + let display_time = SystemTime::now(); + if let Err(e) = match user_data.format.format() { - VideoFormat::RGBx => user_data.tx.send(Frame::RGBx(RGBxFrame { - display_time: timestamp as u64, + VideoFormat::RGBx => user_data.tx.send(Frame::Video(VideoFrame::RGBx(RGBxFrame { + display_time, width: frame_size.width as i32, height: frame_size.height as i32, data: frame_data, - })), - VideoFormat::RGB => user_data.tx.send(Frame::RGB(RGBFrame { - display_time: timestamp as u64, + }))), + VideoFormat::RGB => user_data.tx.send(Frame::Video(VideoFrame::RGB(RGBFrame { + display_time, width: frame_size.width as i32, height: frame_size.height as i32, data: frame_data, - })), - VideoFormat::xBGR => user_data.tx.send(Frame::XBGR(XBGRFrame { - display_time: timestamp as u64, + }))), + VideoFormat::xBGR => user_data.tx.send(Frame::Video(VideoFrame::XBGR(XBGRFrame { + display_time, width: frame_size.width as i32, height: frame_size.height as i32, data: frame_data, - })), - VideoFormat::BGRx => user_data.tx.send(Frame::BGRx(BGRxFrame { - display_time: timestamp as u64, + }))), + VideoFormat::BGRx => user_data.tx.send(Frame::Video(VideoFrame::BGRx(BGRxFrame { + display_time, width: frame_size.width as i32, height: frame_size.height as i32, data: frame_data, - })), + }))), _ => panic!("Unsupported frame format received"), } { eprintln!("{e}"); From 51a303db849cac55700d9f6c4a8c0b4cf8ce7a48 Mon Sep 17 00:00:00 2001 From: tstoltz Date: Wed, 29 Jul 2026 15:02:10 +0200 Subject: [PATCH 2/6] fix(linux): advertise only formats the engine can actually decode The PipeWire stream advertised RGB, RGBA, RGBx and BGRx, while the frame callback handled RGB, RGBx, xBGR and BGRx. Two independently-maintained sets, disagreeing in both directions: * RGBA was offered to the compositor but had no decode arm, so a compositor selecting it reached `panic!("Unsupported frame format received")` -- inside a PipeWire callback, despite scap itself having offered that format; * xBGR had a decode arm but was never advertised, so it could not be negotiated at all. Fixes the advertisement to match the dispatch, and makes the dangerous direction mechanically checkable: * SUPPORTED_VIDEO_FORMATS becomes the authoritative advertisement set, and stream_params is built from it rather than a second hand-written list; * the per-format dispatch moves into video_frame_for() -> Option so it can be exercised without a live PipeWire stream; * three tests assert the invariant, the specific RGBA regression, and that xBGR stays both decodable and advertised. What the tests do and do not guarantee, stated precisely in the code because the two directions are NOT equally covered: * `advertised => decodable` IS enforced generally, for every entry, by every_advertised_format_has_a_decode_arm. This is the direction that matters -- advertising a format the engine cannot decode is what caused the panic. * `decodable => advertised` is NOT enforced generally. Only the historical xBGR omission is pinned. Adding a new arm to video_frame_for without adding it here would leave that format simply unnegotiable -- harmless, but silent. Closing the second direction would mean generating both the array and the dispatch from one declarative table, or enumerating every VideoFormat. Neither seemed warranted for four formats; noted in the code if the set grows. The now-unreachable fallback reports the offending format instead of panicking: it runs inside an FFI-driven callback, where unwinding is not something the C caller is prepared for. Verified in both directions rather than only the happy path. With the fix, 6/6 lib tests pass and `cargo fmt --check` is clean. Reintroducing the original bug -- putting RGBA back into the advertised list -- fails two of the new tests with their intended diagnostics ("advertised format VideoFormat::RGBA has no decode arm in video_frame_for"), so the guard demonstrably guards. --- src/capturer/engine/linux/mod.rs | 182 +++++++++++++++++++++++++------ 1 file changed, 150 insertions(+), 32 deletions(-) diff --git a/src/capturer/engine/linux/mod.rs b/src/capturer/engine/linux/mod.rs index 471458aa..152d22c3 100644 --- a/src/capturer/engine/linux/mod.rs +++ b/src/capturer/engine/linux/mod.rs @@ -39,6 +39,82 @@ use self::{error::LinCapError, portal::ScreenCastPortal}; mod error; mod portal; +/// The authoritative set of video formats advertised to PipeWire in +/// `stream_params()`. Every entry must have a decode arm in +/// [`video_frame_for`]. +/// +/// Previously the advertised and decodable sets were maintained independently +/// and disagreed in both directions: `RGBA` was advertised but had no decode +/// arm, while `xBGR` had a decode arm but was never advertised. A compositor +/// that selected `RGBA` therefore reached the fallback and panicked, despite +/// scap having offered that format itself. +/// +/// **What the tests below mechanically guarantee**, stated precisely because +/// the two directions are not equally covered: +/// +/// - `advertised => decodable` is enforced generally, for every entry here, +/// by `every_advertised_format_has_a_decode_arm`. This is the direction +/// that matters: advertising a format the engine cannot decode is what +/// caused the panic. +/// - `decodable => advertised` is **not** enforced generally. Only the +/// historical `xBGR` omission is pinned, by +/// `xbgr_is_both_decodable_and_advertised`. Adding a new arm to +/// [`video_frame_for`] without adding it here would leave that format +/// simply unnegotiable — harmless, but silent. +/// +/// Closing the second direction properly would mean generating both this +/// array and the dispatch from one declarative table, or enumerating every +/// `VideoFormat`. Neither is warranted for the four formats this engine +/// supports; add it here if the set grows. +const SUPPORTED_VIDEO_FORMATS: [VideoFormat; 4] = [ + VideoFormat::RGB, + VideoFormat::RGBx, + VideoFormat::xBGR, + VideoFormat::BGRx, +]; + +/// Build the [`VideoFrame`] for a negotiated `format`, or `None` when this +/// engine has no decode arm for it. +/// +/// Split out of the `on_process` callback so the advertised list above can be +/// checked against the dispatch without a live PipeWire stream. See that +/// list's docs for exactly which direction the tests enforce. +fn video_frame_for( + format: VideoFormat, + display_time: SystemTime, + width: i32, + height: i32, + data: Vec, +) -> Option { + match format { + VideoFormat::RGBx => Some(VideoFrame::RGBx(RGBxFrame { + display_time, + width, + height, + data, + })), + VideoFormat::RGB => Some(VideoFrame::RGB(RGBFrame { + display_time, + width, + height, + data, + })), + VideoFormat::xBGR => Some(VideoFrame::XBGR(XBGRFrame { + display_time, + width, + height, + data, + })), + VideoFormat::BGRx => Some(VideoFrame::BGRx(BGRxFrame { + display_time, + width, + height, + data, + })), + _ => None, + } +} + static CAPTURER_STATE: AtomicU8 = AtomicU8::new(0); static STREAM_STATE_CHANGED_TO_ERROR: AtomicBool = AtomicBool::new(false); @@ -142,34 +218,30 @@ fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) { let _ = timestamp; // suppress "unused" warning until we wire pts elsewhere let display_time = SystemTime::now(); - if let Err(e) = match user_data.format.format() { - VideoFormat::RGBx => user_data.tx.send(Frame::Video(VideoFrame::RGBx(RGBxFrame { - display_time, - width: frame_size.width as i32, - height: frame_size.height as i32, - data: frame_data, - }))), - VideoFormat::RGB => user_data.tx.send(Frame::Video(VideoFrame::RGB(RGBFrame { - display_time, - width: frame_size.width as i32, - height: frame_size.height as i32, - data: frame_data, - }))), - VideoFormat::xBGR => user_data.tx.send(Frame::Video(VideoFrame::XBGR(XBGRFrame { - display_time, - width: frame_size.width as i32, - height: frame_size.height as i32, - data: frame_data, - }))), - VideoFormat::BGRx => user_data.tx.send(Frame::Video(VideoFrame::BGRx(BGRxFrame { - display_time, - width: frame_size.width as i32, - height: frame_size.height as i32, - data: frame_data, - }))), - _ => panic!("Unsupported frame format received"), - } { - eprintln!("{e}"); + match video_frame_for( + user_data.format.format(), + display_time, + frame_size.width as i32, + frame_size.height as i32, + frame_data, + ) { + Some(video_frame) => { + if let Err(e) = user_data.tx.send(Frame::Video(video_frame)) { + eprintln!("{e}"); + } + } + None => { + // Unreachable by construction: PipeWire can only negotiate + // a format we advertised, and everything in + // SUPPORTED_VIDEO_FORMATS has a decode arm. Reported + // rather than panicking because this runs inside an + // FFI-driven callback, where unwinding is not something + // the C caller is prepared for. + eprintln!( + "Unsupported frame format received: {:?}", + user_data.format.format() + ); + } } } } else { @@ -224,10 +296,10 @@ fn pipewire_capturer( Choice, Enum, Id, - pw::spa::param::video::VideoFormat::RGB, - pw::spa::param::video::VideoFormat::RGBA, - pw::spa::param::video::VideoFormat::RGBx, - pw::spa::param::video::VideoFormat::BGRx, + SUPPORTED_VIDEO_FORMATS[0], + SUPPORTED_VIDEO_FORMATS[1], + SUPPORTED_VIDEO_FORMATS[2], + SUPPORTED_VIDEO_FORMATS[3], ), pw::spa::pod::property!( FormatProperties::VideoSize, @@ -376,3 +448,49 @@ impl LinuxCapturer { pub fn create_capturer(options: &Options, tx: mpsc::Sender) -> LinuxCapturer { LinuxCapturer::new(options, tx) } + +#[cfg(test)] +mod format_negotiation_tests { + use super::*; + + fn sample(format: VideoFormat) -> Option { + video_frame_for(format, SystemTime::UNIX_EPOCH, 1, 1, vec![0u8; 4]) + } + + /// Anything offered to PipeWire must be something this engine can decode. + /// Adding a format to `SUPPORTED_VIDEO_FORMATS` without a matching arm in + /// `video_frame_for` fails here rather than at runtime on a user's + /// compositor. + #[test] + fn every_advertised_format_has_a_decode_arm() { + for format in SUPPORTED_VIDEO_FORMATS { + assert!( + sample(format).is_some(), + "advertised format {format:?} has no decode arm in video_frame_for" + ); + } + } + + /// Pins the specific regression: `RGBA` was advertised with no decode arm. + #[test] + fn rgba_is_not_advertised_while_undecodable() { + assert!( + sample(VideoFormat::RGBA).is_none(), + "video_frame_for gained an RGBA arm -- add RGBA to \ + SUPPORTED_VIDEO_FORMATS and delete this test" + ); + assert!( + !SUPPORTED_VIDEO_FORMATS.contains(&VideoFormat::RGBA), + "RGBA is advertised but video_frame_for cannot decode it" + ); + } + + /// `xBGR` had a decode arm but was never advertised, so it could never be + /// negotiated. Pins that one historical omission -- it does NOT generalize + /// to "every decodable format is advertised", which no test here checks. + #[test] + fn xbgr_is_both_decodable_and_advertised() { + assert!(sample(VideoFormat::xBGR).is_some()); + assert!(SUPPORTED_VIDEO_FORMATS.contains(&VideoFormat::xBGR)); + } +} From 577a977962918494cdad3f2b63bbad2384ba75a6 Mon Sep 17 00:00:00 2001 From: tstoltz Date: Wed, 29 Jul 2026 17:42:37 +0200 Subject: [PATCH 3/6] review: destructure the advertised set, name the dropped PTS binding Both from review on #187. 1. `let _ = timestamp;` dropped the meaning of the value. Renamed to `_pts_ns` with a TODO, so the intent to plumb PipeWire PTS through frame metadata later is visible rather than implied. 2. `stream_params` indexed SUPPORTED_VIDEO_FORMATS[0..3], which derives the VALUES from the array but leaves its ARITY hand-maintained -- growing the set to five would silently keep advertising only the first four. Now destructured (`let [fmt0, fmt1, fmt2, fmt3] = SUPPORTED_VIDEO_FORMATS;`) so the compiler rejects that callsite when the set changes. This is a genuine third drift direction the original patch missed: it claimed deriving the advertisement from the array removed drift, but only the values were derived. Verified the new guard actually fires rather than assuming it: temporarily growing the array to 5 entries (duplicating an already-decodable format, so the decode-arm test still passes and the destructuring is isolated as the thing under test) fails with exactly one error -- error[E0527]: pattern requires 4 elements but array has 5 Restored, 6/6 lib tests pass, cargo fmt --check clean. The const's docs are updated accordingly: growing the array can no longer silently under-advertise, and that is compiler-enforced rather than test-enforced. --- src/capturer/engine/linux/mod.rs | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/capturer/engine/linux/mod.rs b/src/capturer/engine/linux/mod.rs index 152d22c3..759541ec 100644 --- a/src/capturer/engine/linux/mod.rs +++ b/src/capturer/engine/linux/mod.rs @@ -49,23 +49,27 @@ mod portal; /// that selected `RGBA` therefore reached the fallback and panicked, despite /// scap having offered that format itself. /// -/// **What the tests below mechanically guarantee**, stated precisely because -/// the two directions are not equally covered: +/// **What is mechanically guaranteed**, stated precisely because the +/// directions are not equally covered: /// /// - `advertised => decodable` is enforced generally, for every entry here, /// by `every_advertised_format_has_a_decode_arm`. This is the direction /// that matters: advertising a format the engine cannot decode is what /// caused the panic. +/// - **Growing this array cannot silently under-advertise.** `stream_params` +/// destructures it (`let [fmt0, fmt1, fmt2, fmt3] = ...`) rather than +/// indexing, so adding a fifth entry fails to compile at that callsite +/// instead of quietly continuing to offer only the first four. /// - `decodable => advertised` is **not** enforced generally. Only the /// historical `xBGR` omission is pinned, by /// `xbgr_is_both_decodable_and_advertised`. Adding a new arm to /// [`video_frame_for`] without adding it here would leave that format /// simply unnegotiable — harmless, but silent. /// -/// Closing the second direction properly would mean generating both this -/// array and the dispatch from one declarative table, or enumerating every -/// `VideoFormat`. Neither is warranted for the four formats this engine -/// supports; add it here if the set grows. +/// Closing that last direction would mean generating both this array and the +/// dispatch from one declarative table, or enumerating every `VideoFormat`. +/// Neither is warranted for the four formats this engine supports; add it +/// here if the set grows. const SUPPORTED_VIDEO_FORMATS: [VideoFormat; 4] = [ VideoFormat::RGB, VideoFormat::RGBx, @@ -215,7 +219,7 @@ fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) { // SystemTime::now() here (matches what the macOS and Windows // engines do today). Relative frame ordering survives via // channel-send order; sub-millisecond buffer timing is lost. - let _ = timestamp; // suppress "unused" warning until we wire pts elsewhere + let _pts_ns = timestamp; // TODO: plumb PipeWire PTS through frame metadata let display_time = SystemTime::now(); match video_frame_for( @@ -286,6 +290,11 @@ fn pipewire_capturer( .process(process_callback) .register()?; + // Destructured rather than indexed: if SUPPORTED_VIDEO_FORMATS gains a + // fifth entry, this fails to compile instead of silently continuing to + // advertise only the first four. Suggested by review on #187. + let [fmt0, fmt1, fmt2, fmt3] = SUPPORTED_VIDEO_FORMATS; + let obj = pw::spa::pod::object!( pw::spa::utils::SpaTypes::ObjectParamFormat, pw::spa::param::ParamType::EnumFormat, @@ -296,10 +305,10 @@ fn pipewire_capturer( Choice, Enum, Id, - SUPPORTED_VIDEO_FORMATS[0], - SUPPORTED_VIDEO_FORMATS[1], - SUPPORTED_VIDEO_FORMATS[2], - SUPPORTED_VIDEO_FORMATS[3], + fmt0, + fmt1, + fmt2, + fmt3, ), pw::spa::pod::property!( FormatProperties::VideoSize, From 29b799422d31ba4bd06845cee38b3ed759e1e927 Mon Sep 17 00:00:00 2001 From: tstoltz Date: Wed, 29 Jul 2026 18:00:15 +0200 Subject: [PATCH 4/6] fix(linux): keep RGBA support -- dropping it would regress COSMIC Corrects the approach of the previous commit. It resolved the advertised-vs-decodable mismatch by REMOVING RGBA from the advertised set, which fixes the panic but is the wrong direction: RGBA was advertised for a reason. COSMIC negotiates RGBA where GNOME and KDE negotiate BGRx. That is exactly what #153 reported ("I am using COSMIC DE, which uses VideoFormat::RGBA instead of VideoFormat::BGRx ... so it leads to a panic") and what the still -open #169 proposes fixing. Dropping RGBA from the offer would either leave COSMIC without a compatible format or, if a portal supplies it anyway, silently discard every frame. RGBA now keeps its place in SUPPORTED_VIDEO_FORMATS and gains a decode arm mapping onto RGBx -- same byte order and width, differing only in whether the fourth byte is alpha or undefined padding. VideoFrame has no RGBA variant and scap does not expose alpha semantics, so this matches the mapping #169 proposes. Growing the set to five made the destructuring guard added in the previous commit fire immediately, which is the guard working as intended. Also in this commit: * The timestamp comment claimed SystemTime::now() "matches what the macOS and Windows engines do" and that only sub-millisecond precision is lost. Both are wrong. Windows anchors a SystemTime/performance-counter origin and derives each frame's display_time from the capture delta; and discarding PTS loses the source capture clock and inter-frame timing entirely, not just precision. Reworded to say what it actually does. * The unsupported-format fallback logged once per frame, so a portal delivering an undecodable format could emit dozens of lines per second indefinitely while copying and dropping each buffer. Now reported once per negotiated format via `unsupported_format_reported`, reset on renegotiation. * "Unreachable by construction" overstated it -- #153 is direct evidence that portal behaviour is not a construction guarantee. Now "should be unreachable when the portal honours the negotiated format set", retained as a defensive fallback. * The general test only asserted `is_some()`, so it would have passed if every format decoded as RGB. It now pins the exact VideoFrame variant per format, plus a test that dimensions, timestamp and payload survive the dispatch unchanged. * The RGBA test was written to be deleted when RGBA support arrived. It is now an equality -- decode support and advertisement must change together -- so it evolves with the implementation instead. Mutation-tested both directions of the regression this guards: * removing the RGBA decode arm (the original upstream bug) fails with "advertised format VideoFormat::RGBA has no decode arm in video_frame_for"; * removing RGBA from the advertised set (this PR's earlier mistake) fails with "RGBA decode support and RGBA advertisement must change together". 7/7 lib tests pass, cargo fmt --check clean. --- src/capturer/engine/linux/mod.rs | 188 +++++++++++++++++++++++-------- 1 file changed, 138 insertions(+), 50 deletions(-) diff --git a/src/capturer/engine/linux/mod.rs b/src/capturer/engine/linux/mod.rs index 759541ec..21eb10b1 100644 --- a/src/capturer/engine/linux/mod.rs +++ b/src/capturer/engine/linux/mod.rs @@ -43,35 +43,38 @@ mod portal; /// `stream_params()`. Every entry must have a decode arm in /// [`video_frame_for`]. /// -/// Previously the advertised and decodable sets were maintained independently -/// and disagreed in both directions: `RGBA` was advertised but had no decode -/// arm, while `xBGR` had a decode arm but was never advertised. A compositor -/// that selected `RGBA` therefore reached the fallback and panicked, despite -/// scap having offered that format itself. +/// The advertised and decodable sets used to be maintained independently and +/// disagreed in both directions: `RGBA` was advertised but had no decode arm, +/// while `xBGR` had a decode arm but was never advertised. A compositor that +/// selected `RGBA` therefore reached the fallback and panicked -- reported +/// against COSMIC in #153 and #169, which negotiates `RGBA` where GNOME and +/// KDE negotiate `BGRx`. +/// +/// `RGBA` is kept advertised and given a decode arm rather than dropped, +/// precisely so that case keeps working. /// /// **What is mechanically guaranteed**, stated precisely because the /// directions are not equally covered: /// /// - `advertised => decodable` is enforced generally, for every entry here, -/// by `every_advertised_format_has_a_decode_arm`. This is the direction -/// that matters: advertising a format the engine cannot decode is what -/// caused the panic. +/// by `every_advertised_format_has_a_decode_arm`, which also pins the exact +/// [`VideoFrame`] variant each one maps to. /// - **Growing this array cannot silently under-advertise.** `stream_params` -/// destructures it (`let [fmt0, fmt1, fmt2, fmt3] = ...`) rather than -/// indexing, so adding a fifth entry fails to compile at that callsite -/// instead of quietly continuing to offer only the first four. -/// - `decodable => advertised` is **not** enforced generally. Only the -/// historical `xBGR` omission is pinned, by -/// `xbgr_is_both_decodable_and_advertised`. Adding a new arm to -/// [`video_frame_for`] without adding it here would leave that format -/// simply unnegotiable — harmless, but silent. +/// destructures it rather than indexing, so adding an entry fails to +/// compile at that callsite instead of quietly continuing to offer only the +/// previous set. +/// - `decodable => advertised` is **not** enforced generally. Adding a new arm +/// to [`video_frame_for`] without adding it here leaves that format simply +/// unnegotiable. That is quieter than the panic above, but -- as #153 shows +/// -- "a compositor wants a format we do not offer" is a real failure, not a +/// harmless one. /// /// Closing that last direction would mean generating both this array and the /// dispatch from one declarative table, or enumerating every `VideoFormat`. -/// Neither is warranted for the four formats this engine supports; add it -/// here if the set grows. -const SUPPORTED_VIDEO_FORMATS: [VideoFormat; 4] = [ +/// Neither seemed warranted at this size; add it here if the set grows. +const SUPPORTED_VIDEO_FORMATS: [VideoFormat; 5] = [ VideoFormat::RGB, + VideoFormat::RGBA, VideoFormat::RGBx, VideoFormat::xBGR, VideoFormat::BGRx, @@ -103,6 +106,17 @@ fn video_frame_for( height, data, })), + // RGBA has the same byte order and width as RGBx; the two differ only + // in whether the fourth byte carries alpha or is undefined padding. + // `VideoFrame` has no RGBA variant and scap does not currently expose + // alpha semantics, so RGBA is represented as RGBx and the fourth byte + // is treated as unused. Same mapping proposed in #153 and #169. + VideoFormat::RGBA => Some(VideoFrame::RGBx(RGBxFrame { + display_time, + width, + height, + data, + })), VideoFormat::xBGR => Some(VideoFrame::XBGR(XBGRFrame { display_time, width, @@ -126,6 +140,14 @@ static STREAM_STATE_CHANGED_TO_ERROR: AtomicBool = AtomicBool::new(false); struct ListenerUserData { pub tx: mpsc::Sender, pub format: spa::param::video::VideoInfoRaw, + /// Whether an unsupported negotiated format has already been reported. + /// + /// Without this the fallback in `process_callback` would log once per + /// captured frame -- potentially dozens of lines per second, indefinitely + /// -- if a portal delivers a format outside the negotiated set. Reset in + /// `param_changed_callback` so a genuinely new negotiation is reported + /// again. + pub unsupported_format_reported: bool, } fn param_changed_callback( @@ -154,6 +176,10 @@ fn param_changed_callback( .parse(param) // TODO: Tell library user of the error .expect("Failed to parse format parameter"); + + // A new format was negotiated, so allow the unsupported-format warning to + // fire once more if this one also turns out to be undecodable. + user_data.unsupported_format_reported = false; } fn state_changed_callback( @@ -213,13 +239,20 @@ fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) { .to_vec() }; - // `timestamp` (from spa_meta_header.pts) is a PipeWire monotonic - // nanosecond count since an arbitrary reference — not wall-clock. - // display_time's SystemTime contract is wall-clock, so we use - // SystemTime::now() here (matches what the macOS and Windows - // engines do today). Relative frame ordering survives via - // channel-send order; sub-millisecond buffer timing is lost. - let _pts_ns = timestamp; // TODO: plumb PipeWire PTS through frame metadata + // `timestamp` (spa_meta_header.pts) is a monotonic nanosecond + // count from an unspecified origin, so it cannot be represented + // directly as `display_time`'s `SystemTime`. + // + // `SystemTime::now()` is the minimal compile repair: it records + // when this callback processed the frame, NOT when the source + // captured it. That discards the source capture clock and its + // inter-frame timing, not merely sub-millisecond precision -- + // callback delivery can be delayed or bursty. Note this is weaker + // than the Windows engine, which anchors a SystemTime/performance- + // counter origin and derives each frame's display_time from the + // capture timestamp delta. Doing the same here needs a PTS origin + // to anchor against, which is out of scope for a compile fix. + let _pipewire_pts_ns = timestamp; let display_time = SystemTime::now(); match video_frame_for( @@ -235,16 +268,24 @@ fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) { } } None => { - // Unreachable by construction: PipeWire can only negotiate - // a format we advertised, and everything in - // SUPPORTED_VIDEO_FORMATS has a decode arm. Reported - // rather than panicking because this runs inside an - // FFI-driven callback, where unwinding is not something - // the C caller is prepared for. - eprintln!( - "Unsupported frame format received: {:?}", - user_data.format.format() - ); + // Should be unreachable when the portal honours the + // negotiated format set, since everything in + // SUPPORTED_VIDEO_FORMATS has a decode arm -- but portal + // behaviour is not a construction guarantee (see #153, + // where a compositor's choice reached this path), so it is + // retained as a defensive fallback. + // + // Reported rather than panicking because this runs inside + // an FFI-driven callback, where unwinding is not something + // the C caller is prepared for. Logged once per negotiated + // format rather than once per frame. + if !user_data.unsupported_format_reported { + user_data.unsupported_format_reported = true; + eprintln!( + "Unsupported frame format received: {:?}", + user_data.format.format() + ); + } } } } @@ -271,6 +312,7 @@ fn pipewire_capturer( let user_data = ListenerUserData { tx, format: Default::default(), + unsupported_format_reported: false, }; let stream = pw::stream::Stream::new( @@ -293,7 +335,7 @@ fn pipewire_capturer( // Destructured rather than indexed: if SUPPORTED_VIDEO_FORMATS gains a // fifth entry, this fails to compile instead of silently continuing to // advertise only the first four. Suggested by review on #187. - let [fmt0, fmt1, fmt2, fmt3] = SUPPORTED_VIDEO_FORMATS; + let [fmt0, fmt1, fmt2, fmt3, fmt4] = SUPPORTED_VIDEO_FORMATS; let obj = pw::spa::pod::object!( pw::spa::utils::SpaTypes::ObjectParamFormat, @@ -309,6 +351,7 @@ fn pipewire_capturer( fmt1, fmt2, fmt3, + fmt4, ), pw::spa::pod::property!( FormatProperties::VideoSize, @@ -462,14 +505,17 @@ pub fn create_capturer(options: &Options, tx: mpsc::Sender) -> LinuxCaptu mod format_negotiation_tests { use super::*; + const W: i32 = 3; + const H: i32 = 2; + fn sample(format: VideoFormat) -> Option { - video_frame_for(format, SystemTime::UNIX_EPOCH, 1, 1, vec![0u8; 4]) + video_frame_for(format, SystemTime::UNIX_EPOCH, W, H, vec![7u8; 24]) } - /// Anything offered to PipeWire must be something this engine can decode. - /// Adding a format to `SUPPORTED_VIDEO_FORMATS` without a matching arm in - /// `video_frame_for` fails here rather than at runtime on a user's - /// compositor. + /// Anything offered to PipeWire must be something this engine can decode, + /// AND must map to the variant callers expect. `is_some()` alone would + /// still pass if every format accidentally decoded as `RGB`, so each + /// mapping is pinned explicitly. #[test] fn every_advertised_format_has_a_decode_arm() { for format in SUPPORTED_VIDEO_FORMATS { @@ -478,19 +524,61 @@ mod format_negotiation_tests { "advertised format {format:?} has no decode arm in video_frame_for" ); } + + assert!(matches!(sample(VideoFormat::RGB), Some(VideoFrame::RGB(_)))); + assert!(matches!( + sample(VideoFormat::RGBx), + Some(VideoFrame::RGBx(_)) + )); + assert!(matches!( + sample(VideoFormat::xBGR), + Some(VideoFrame::XBGR(_)) + )); + assert!(matches!( + sample(VideoFormat::BGRx), + Some(VideoFrame::BGRx(_)) + )); + // RGBA is deliberately represented as RGBx -- same byte order and + // width, alpha ignored. See video_frame_for. + assert!(matches!( + sample(VideoFormat::RGBA), + Some(VideoFrame::RGBx(_)) + )); } - /// Pins the specific regression: `RGBA` was advertised with no decode arm. + /// The frame payload must survive the dispatch unchanged -- a decode arm + /// that mapped to the right variant but dropped or reordered the buffer + /// would still satisfy the mapping assertions above. #[test] - fn rgba_is_not_advertised_while_undecodable() { - assert!( - sample(VideoFormat::RGBA).is_none(), - "video_frame_for gained an RGBA arm -- add RGBA to \ - SUPPORTED_VIDEO_FORMATS and delete this test" + fn decoding_preserves_dimensions_timestamp_and_data() { + let Some(VideoFrame::BGRx(frame)) = sample(VideoFormat::BGRx) else { + panic!("BGRx did not decode to a BGRx frame"); + }; + assert_eq!(frame.width, W); + assert_eq!(frame.height, H); + assert_eq!(frame.display_time, SystemTime::UNIX_EPOCH); + assert_eq!(frame.data, vec![7u8; 24]); + } + + /// `RGBA` and its advertisement must change together. + /// + /// COSMIC negotiates `RGBA` where GNOME/KDE negotiate `BGRx` (#153, #169), + /// so dropping either half silently breaks that desktop: removing the + /// decode arm reintroduces the original panic, and removing it from the + /// advertised set leaves COSMIC without a compatible offer. + /// + /// Written as an equality rather than "must be absent" so it stays + /// meaningful if the representation changes, instead of needing deletion. + #[test] + fn rgba_is_decodable_and_advertised_together() { + assert_eq!( + sample(VideoFormat::RGBA).is_some(), + SUPPORTED_VIDEO_FORMATS.contains(&VideoFormat::RGBA), + "RGBA decode support and RGBA advertisement must change together" ); assert!( - !SUPPORTED_VIDEO_FORMATS.contains(&VideoFormat::RGBA), - "RGBA is advertised but video_frame_for cannot decode it" + sample(VideoFormat::RGBA).is_some(), + "RGBA support was removed -- this regresses COSMIC, see #153/#169" ); } From d9d50ab54b936f0d7dd19f6e72da4e790ebb9144 Mon Sep 17 00:00:00 2001 From: tstoltz Date: Wed, 29 Jul 2026 18:01:54 +0200 Subject: [PATCH 5/6] review: size test buffers per format instead of a fixed length From review on #187. `sample()` handed every format a 4-bytes-per-pixel buffer, including RGB, which is packed 3-byte. Harmless today because video_frame_for does not inspect the buffer, but if it ever validates size the RGB cases would fail for the wrong reason -- masking or faking the result the test actually exists to check. Buffer length is now derived per format via bytes_per_pixel(). The payload-preservation assertion derives its expected value from the same helper rather than repeating a literal, so the two cannot drift. --- src/capturer/engine/linux/mod.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/capturer/engine/linux/mod.rs b/src/capturer/engine/linux/mod.rs index 21eb10b1..2d6030ef 100644 --- a/src/capturer/engine/linux/mod.rs +++ b/src/capturer/engine/linux/mod.rs @@ -508,8 +508,27 @@ mod format_negotiation_tests { const W: i32 = 3; const H: i32 = 2; + /// Bytes per pixel for a given negotiated format. RGB is packed 3-byte; + /// every other format this engine decodes is 4-byte. + fn bytes_per_pixel(format: VideoFormat) -> usize { + match format { + VideoFormat::RGB => 3, + _ => 4, + } + } + + /// A correctly-sized sample buffer for `format`. + /// + /// Sized per format rather than a fixed length so that if + /// `video_frame_for` ever validates buffer size, these tests fail for the + /// reason under test rather than because RGB was handed a 4-byte-per-pixel + /// buffer. + fn sample_data(format: VideoFormat) -> Vec { + vec![7u8; (W * H) as usize * bytes_per_pixel(format)] + } + fn sample(format: VideoFormat) -> Option { - video_frame_for(format, SystemTime::UNIX_EPOCH, W, H, vec![7u8; 24]) + video_frame_for(format, SystemTime::UNIX_EPOCH, W, H, sample_data(format)) } /// Anything offered to PipeWire must be something this engine can decode, @@ -557,7 +576,7 @@ mod format_negotiation_tests { assert_eq!(frame.width, W); assert_eq!(frame.height, H); assert_eq!(frame.display_time, SystemTime::UNIX_EPOCH); - assert_eq!(frame.data, vec![7u8; 24]); + assert_eq!(frame.data, sample_data(VideoFormat::BGRx)); } /// `RGBA` and its advertisement must change together. From d0a222d29c12b0ec4850d6e034a1158c477002eb Mon Sep 17 00:00:00 2001 From: tstoltz Date: Wed, 29 Jul 2026 18:24:01 +0200 Subject: [PATCH 6/6] review: reject unsupported formats before copying; fix stale docs From review on #187. Runtime fix, not cleanup: process_callback copied the whole frame buffer before dispatching on format, so the one-shot flag added earlier bounded the LOGGING while leaving the copying unbounded. A noncompliant portal could still make scap to_vec() and discard every frame indefinitely -- real memory bandwidth at high resolution. The negotiated format is now checked against SUPPORTED_VIDEO_FORMATS before the copy, and `break 'outside` requeues the PipeWire buffer exactly like the other early exits. The remaining None arm can now only be reached if SUPPORTED_VIDEO_FORMATS contains something video_frame_for cannot decode -- which every_advertised_format_has_a_decode_arm exists to prevent -- so its message says that rather than repeating "unsupported format". Kept as a report rather than unreachable!() because this runs inside an FFI-driven callback. Documentation fixes: * Two doc comments referenced functions that DO NOT EXIST: `stream_params()` and `on_process`. The real ones are pipewire_capturer's format-negotiation block and process_callback. Corrected. * Removed the `// TODO: Format negotiation` marker directly above pipewire_capturer, since this PR implements exactly that. * The destructuring comment still said "fifth entry"/"first four" after the set grew to five. Now "sixth"/"first five", and re-verified against a real compiler run rather than edited by assumption: growing the array to six fails with error[E0527]: pattern requires 5 elements but array has 6 Test fixes: * decoding_preserves_dimensions_timestamp_and_data only covered BGRx while its name and docs implied all formats. Renamed to bgrx_dispatch_preserves_dimensions_timestamp_and_data and states it is a representative branch -- the exact variant mapping for every format is already asserted separately. * bytes_per_pixel used a `_ => 4` catch-all, which would silently hand a future 3-byte format a 4-byte fixture: precisely the false test signal the helper was introduced to prevent. Now exhaustive over the supported set, with a panic for anything undeclared. 7/7 lib tests pass, cargo fmt --check clean. --- src/capturer/engine/linux/mod.rs | 74 ++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 28 deletions(-) diff --git a/src/capturer/engine/linux/mod.rs b/src/capturer/engine/linux/mod.rs index 2d6030ef..ee8a32ce 100644 --- a/src/capturer/engine/linux/mod.rs +++ b/src/capturer/engine/linux/mod.rs @@ -40,8 +40,8 @@ mod error; mod portal; /// The authoritative set of video formats advertised to PipeWire in -/// `stream_params()`. Every entry must have a decode arm in -/// [`video_frame_for`]. +/// [`pipewire_capturer`]'s format-negotiation block. Every entry must have a +/// decode arm in [`video_frame_for`]. /// /// The advertised and decodable sets used to be maintained independently and /// disagreed in both directions: `RGBA` was advertised but had no decode arm, @@ -59,8 +59,8 @@ mod portal; /// - `advertised => decodable` is enforced generally, for every entry here, /// by `every_advertised_format_has_a_decode_arm`, which also pins the exact /// [`VideoFrame`] variant each one maps to. -/// - **Growing this array cannot silently under-advertise.** `stream_params` -/// destructures it rather than indexing, so adding an entry fails to +/// - **Growing this array cannot silently under-advertise.** The negotiation +/// block destructures it rather than indexing, so adding an entry fails to /// compile at that callsite instead of quietly continuing to offer only the /// previous set. /// - `decodable => advertised` is **not** enforced generally. Adding a new arm @@ -83,7 +83,7 @@ const SUPPORTED_VIDEO_FORMATS: [VideoFormat; 5] = [ /// Build the [`VideoFrame`] for a negotiated `format`, or `None` when this /// engine has no decode arm for it. /// -/// Split out of the `on_process` callback so the advertised list above can be +/// Split out of [`process_callback`] so the advertised list above can be /// checked against the dispatch without a live PipeWire stream. See that /// list's docs for exactly which direction the tests enforce. fn video_frame_for( @@ -230,6 +230,22 @@ fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) { if n_datas < 1 { return; } + // Check the negotiated format BEFORE copying the buffer. The + // one-shot flag below bounds the *logging*, but without this the + // callback would still `to_vec()` and then discard every frame a + // noncompliant portal delivers -- bounded logs, unbounded wasted + // copying, which at high resolution is real memory bandwidth. + // `break 'outside` requeues the PipeWire buffer at the end of the + // function, same as every other early exit here. + let negotiated_format = user_data.format.format(); + if !SUPPORTED_VIDEO_FORMATS.contains(&negotiated_format) { + if !user_data.unsupported_format_reported { + user_data.unsupported_format_reported = true; + eprintln!("Unsupported frame format received: {negotiated_format:?}"); + } + break 'outside; + } + let frame_size = user_data.format.size(); let frame_data: Vec = unsafe { std::slice::from_raw_parts( @@ -256,7 +272,7 @@ fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) { let display_time = SystemTime::now(); match video_frame_for( - user_data.format.format(), + negotiated_format, display_time, frame_size.width as i32, frame_size.height as i32, @@ -268,23 +284,16 @@ fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) { } } None => { - // Should be unreachable when the portal honours the - // negotiated format set, since everything in - // SUPPORTED_VIDEO_FORMATS has a decode arm -- but portal - // behaviour is not a construction guarantee (see #153, - // where a compositor's choice reached this path), so it is - // retained as a defensive fallback. - // - // Reported rather than panicking because this runs inside - // an FFI-driven callback, where unwinding is not something - // the C caller is prepared for. Logged once per negotiated - // format rather than once per frame. + // Formats outside the advertised set are already rejected + // above, so reaching here means SUPPORTED_VIDEO_FORMATS + // contains something `video_frame_for` cannot decode -- + // which `every_advertised_format_has_a_decode_arm` exists + // to prevent. Retained rather than made `unreachable!()` + // because this runs inside an FFI-driven callback, where + // unwinding is not something the C caller is prepared for. if !user_data.unsupported_format_reported { user_data.unsupported_format_reported = true; - eprintln!( - "Unsupported frame format received: {:?}", - user_data.format.format() - ); + eprintln!("Advertised format {negotiated_format:?} has no decode arm"); } } } @@ -296,7 +305,6 @@ fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) { unsafe { stream.queue_raw_buffer(buffer) }; } -// TODO: Format negotiation fn pipewire_capturer( options: Options, tx: mpsc::Sender, @@ -333,8 +341,8 @@ fn pipewire_capturer( .register()?; // Destructured rather than indexed: if SUPPORTED_VIDEO_FORMATS gains a - // fifth entry, this fails to compile instead of silently continuing to - // advertise only the first four. Suggested by review on #187. + // sixth entry, this fails to compile instead of silently continuing to + // advertise only the first five. Suggested by review on #187. let [fmt0, fmt1, fmt2, fmt3, fmt4] = SUPPORTED_VIDEO_FORMATS; let obj = pw::spa::pod::object!( @@ -508,12 +516,17 @@ mod format_negotiation_tests { const W: i32 = 3; const H: i32 = 2; - /// Bytes per pixel for a given negotiated format. RGB is packed 3-byte; - /// every other format this engine decodes is 4-byte. + /// Bytes per pixel for a given negotiated format. + /// + /// Deliberately exhaustive over the supported set rather than falling back + /// to 4: a catch-all would silently hand a future 3-byte format a 4-byte + /// fixture, which is exactly the false test signal this helper exists to + /// prevent. Adding a format requires consciously declaring its layout. fn bytes_per_pixel(format: VideoFormat) -> usize { match format { VideoFormat::RGB => 3, - _ => 4, + VideoFormat::RGBA | VideoFormat::RGBx | VideoFormat::xBGR | VideoFormat::BGRx => 4, + other => panic!("no test fixture size defined for {other:?}"), } } @@ -568,8 +581,13 @@ mod format_negotiation_tests { /// The frame payload must survive the dispatch unchanged -- a decode arm /// that mapped to the right variant but dropped or reordered the buffer /// would still satisfy the mapping assertions above. + /// + /// Covers `BGRx` as a representative branch, not all five: the exact + /// variant mapping for every format is already asserted in + /// `every_advertised_format_has_a_decode_arm`, and all arms construct + /// their frame identically. #[test] - fn decoding_preserves_dimensions_timestamp_and_data() { + fn bgrx_dispatch_preserves_dimensions_timestamp_and_data() { let Some(VideoFrame::BGRx(frame)) = sample(VideoFormat::BGRx) else { panic!("BGRx did not decode to a BGRx frame"); };