fix(linux): repair the PipeWire engine build, and advertise only decodable formats - #187
Open
Tristan-Stoltz-ERC wants to merge 6 commits into
Conversation
…Time display_time 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.
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<VideoFrame> 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.
Tristan-Stoltz-ERC
force-pushed
the
fix/linux-engine-compile-and-format-negotiation
branch
from
July 29, 2026 13:14
b6eb7b8 to
51a303d
Compare
Tristan-Stoltz-ERC
added a commit
to Luminous-Dynamics/scap
that referenced
this pull request
Jul 29, 2026
The doc comments claimed "single source of truth", "must stay in lockstep", and that the tests guard "the two drifting apart". That overclaims: the general test iterates SUPPORTED_VIDEO_FORMATS, so it proves `advertised => decodable` only. The reverse direction is pinned for xBGR alone -- adding a new arm to video_frame_for without adding it to the array would leave that format unnegotiable and fail nothing. Comments only, no behaviour change. Corrects the same wording already fixed in the upstream resubmission (CapSoftware/scap#187) so the two copies do not disagree, and so a claim now known to be false does not sit in the code. The asymmetry is acceptable and now documented as such: advertising an undecodable format is what panicked; the reverse is harmless but silent. Closing it properly would need a declarative table generating both the array and the dispatch, which is not warranted for four formats. Verified: cargo fmt --check clean, 3/3 format_negotiation tests pass.
Both from review on CapSoftware#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.
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 CapSoftware#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 CapSoftware#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 CapSoftware#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 -- CapSoftware#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.
From review on CapSoftware#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.
From review on CapSoftware#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent Linux-engine fixes, both small and self-contained. This replaces #183, which became structurally orphaned (details at the bottom) — apologies for the churn.
Related open PRs: #176 and #178 also touch the Linux frame/timestamp migration, and #169 proposes the RGBA handling below. This PR deliberately isolates the minimal compile repair plus the format-negotiation correction, without their broader or unrelated changes. Happy to rebase onto or defer to any of them if you'd rather take one of those first.
1. The Linux engine does not compile against the current frame enum
src/capturer/engine/linux/mod.rsstill constructs the single-levelFrame::RGBx(...)variants, butFramenow nests video variants underFrame::Video(VideoFrame::...). It also assigns ani64PipeWire timestamp todisplay_time, which is aSystemTime.Repaired to the two-level enum, with
display_time = SystemTime::now().On the timestamp, stated precisely —
spa_meta_header.ptsis a monotonic nanosecond count from an unspecified origin, so it cannot be represented directly as aSystemTime.SystemTime::now()is the minimal compile repair: it records when the 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.This is deliberately weaker than the Windows engine, which anchors a
SystemTime/performance-counter origin and derives each frame'sdisplay_timefrom the capture delta. Doing the same here needs a PTS origin to anchor against, which felt out of scope for a compile fix — but I'm glad to implement it, or exposeptsas a separate field, if you'd prefer.2. The engine advertises a format it cannot decode
The stream advertises
RGB, RGBA, RGBx, BGRx; the callback handlesRGB, RGBx, xBGR, BGRx. Two hand-maintained sets, disagreeing both ways:RGBAis advertised but has no decode arm — a compositor selecting it hitspanic!("Unsupported frame format received")inside a PipeWire callback, for a format scap itself offered.xBGRhas a decode arm but is never advertised, so it can never be negotiated.This is independent of the compile repair and predates it.
RGBA is kept, not dropped
An earlier revision of this PR fixed the mismatch by removing
RGBAfrom the advertised set. That was the wrong direction — RGBA is advertised for a reason. COSMIC negotiatesRGBAwhere GNOME and KDE negotiateBGRx, which is precisely what #153 reported and what #169 proposes fixing. Removing it would either leave COSMIC without a compatible offer, or silently discard every frame if a portal supplied it anyway.So
RGBAkeeps its place and gains a decode arm mapping ontoRGBx— same byte order and width, differing only in whether the fourth byte is alpha or undefined padding.VideoFramehas noRGBAvariant and scap doesn't currently expose alpha semantics, so this matches the mapping in #169.SUPPORTED_VIDEO_FORMATSis now the authoritative advertisement set,pipewire_capturer's negotiation block destructures it rather than indexing (so growing it fails to compile at that callsite instead of silently under-advertising), and the dispatch moved intovideo_frame_for() -> Option<VideoFrame>so it can be checked without a live PipeWire stream.What the tests guarantee — and what they don't
advertised => decodableis enforced generally, for every entry, and each format's exactVideoFramevariant is pinned. A separate test asserts dimensions, timestamp and payload survive the dispatch unchanged forBGRxas a representative branch (all arms construct their frame identically).decodable => advertisedis not enforced generally. Adding an arm tovideo_frame_forwithout adding it to the array leaves that format unnegotiable. Quieter than a panic, but fix: Add handling for missing VideoFormat on Linux #153 shows "a compositor wants a format we don't offer" is a real failure, not a harmless one.Closing that last direction would need a declarative table generating both the array and the dispatch. That seemed disproportionate at this size — happy to add it if you'd prefer.
Unsupported formats are rejected before the frame is copied
The unsupported-format path previously logged once per frame and copied the entire frame buffer before discarding it. So a portal delivering an undecodable format could emit dozens of lines per second indefinitely while also burning real memory bandwidth at high resolution.
The negotiated format is now checked against
SUPPORTED_VIDEO_FORMATSbefore theto_vec(), and reported once per negotiated format (reset on renegotiation). The early exit requeues the PipeWire buffer exactly like the other early exits in that callback.I've also stopped describing it as "unreachable by construction" — #153 is direct evidence that portal behaviour isn't a construction guarantee. It's a defensive fallback that reports rather than panicking, since this runs inside an FFI-driven callback where unwinding isn't something the C caller is prepared for.
Verification
cargo test --lib: 7/7 pass;cargo fmt --checkclean.advertised format VideoFormat::RGBA has no decode arm in video_frame_for;RGBA decode support and RGBA advertisement must change together;error[E0527]: pattern requires 5 elements but array has 6.scap-backendjob (lib + example + tests,--all-targets --no-run), and has been exercised live against a real KDE-Wayland session at 1920×1080.Linux only. I have not compiled Windows or macOS and have deliberately not touched them here. I don't have a COSMIC system, so the RGBA path is reasoned from #153/#169 and unit-tested, not verified live — if a COSMIC user can confirm, that would be worth having.
Deliberately not included
Two further changes exist in a downstream fork but are not here, because they change observable behaviour and deserve separate review:
mpsc::channel()is unbounded and grows without limit under downstream backpressure (heaptrack measured ~1.44 GB of 1.47 GB in a 25.76 s repro). Converting tosync_channel+try_sendchanges delivery semantics cross-platform, and audio differs from stale video there.I'll submit those once I can show real Windows and macOS compilation evidence. I'd rather not hand you patches nobody has built.
I also noticed some pre-existing buffer-handling issues adjacent to this code (a
n_datas < 1early return that skips re-queueing, and a null-buffer path). They're outside this PR's scope and I haven't touched them — happy to open a separate issue if useful.What happened to #183
#183 was opened from
Luminous-Dynamics/scap, which left this repository's fork network at some point. GitHub closed it automatically when a later push forced it to recompute the comparison, and it can't be reopened (state cannot be changed. The repository may be missing relevant data). No review content was lost — #183 remains readable, and its discussion (thanks @tembo for the review there) informed this smaller resubmission.This PR is from a genuine fork and deliberately narrower: the compile repair plus the negotiation correction, rather than the original 18-commit history.