fix(encode/pyrowave): refuse a frame that isn't the session's mode
ci / web (push) Successful in 54s
ci / docs-site (push) Successful in 55s
ci / bench (push) Failing after 6m37s
ci / rust (push) Failing after 6m43s
deb / build-publish-host (push) Failing after 5m44s
arch / build-publish (push) Failing after 6m45s
docker / build-push (--build-arg FEDORA_VERSION=44, ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora44-rpm) (push) Successful in 11s
docker / build-push (., web/Dockerfile, punktfunk-web) (push) Successful in 12s
decky / build-publish (push) Successful in 18s
docker / build-push (ci, ci/rust-ci-noble.Dockerfile, punktfunk-rust-ci-noble) (push) Successful in 8s
docker / build-push (ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora-rpm) (push) Successful in 9s
docker / build-push (ci, ci/rust-ci.Dockerfile, punktfunk-rust-ci) (push) Successful in 9s
docker / build-push (docs-site, docs-site/Dockerfile, punktfunk-docs) (push) Successful in 7s
docker / build-push-arm64cross (push) Successful in 9s
docker / deploy-docs (push) Successful in 23s
ci / rust-arm64 (push) Successful in 9m47s
deb / build-publish (push) Successful in 9m9s
android / android (push) Successful in 11m59s
deb / build-publish-client-arm64 (push) Successful in 8m44s
rpm / build-publish (44, fedora-44, punktfunk-fedora44-rpm) (push) Failing after 8m27s
windows-host / package (push) Successful in 17m35s
rpm / build-publish (43, bazzite, punktfunk-fedora-rpm) (push) Successful in 16m58s
apple / swift (push) Successful in 6m51s
apple / screenshots (push) Successful in 26m43s
ci / web (push) Successful in 54s
ci / docs-site (push) Successful in 55s
ci / bench (push) Failing after 6m37s
ci / rust (push) Failing after 6m43s
deb / build-publish-host (push) Failing after 5m44s
arch / build-publish (push) Failing after 6m45s
docker / build-push (--build-arg FEDORA_VERSION=44, ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora44-rpm) (push) Successful in 11s
docker / build-push (., web/Dockerfile, punktfunk-web) (push) Successful in 12s
decky / build-publish (push) Successful in 18s
docker / build-push (ci, ci/rust-ci-noble.Dockerfile, punktfunk-rust-ci-noble) (push) Successful in 8s
docker / build-push (ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora-rpm) (push) Successful in 9s
docker / build-push (ci, ci/rust-ci.Dockerfile, punktfunk-rust-ci) (push) Successful in 9s
docker / build-push (docs-site, docs-site/Dockerfile, punktfunk-docs) (push) Successful in 7s
docker / build-push-arm64cross (push) Successful in 9s
docker / deploy-docs (push) Successful in 23s
ci / rust-arm64 (push) Successful in 9m47s
deb / build-publish (push) Successful in 9m9s
android / android (push) Successful in 11m59s
deb / build-publish-client-arm64 (push) Successful in 8m44s
rpm / build-publish (44, fedora-44, punktfunk-fedora44-rpm) (push) Failing after 8m27s
windows-host / package (push) Successful in 17m35s
rpm / build-publish (43, bazzite, punktfunk-fedora-rpm) (push) Successful in 16m58s
apple / swift (push) Successful in 6m51s
apple / screenshots (push) Successful in 26m43s
PyroWave never checked frame dimensions against the session, and it applies no alignment — `width`/`height` are the negotiated mode verbatim — so a mismatched frame was encoded edge-smeared or cropped, silently, forever. Every other Linux backend already refuses exactly this, with this shape, in `submit`: libav-NVENC (`linux/mod.rs`), VAAPI (`vaapi.rs`) and openh264 (`sw.rs`) all carry the same `ensure!`. PyroWave was the only one that didn't. That is the justification; an earlier draft cited `vulkan_video.rs` instead, which is the weaker precedent — it bails in its Dmabuf arms only, and its CSC path and CPU arm have no dimension guard at all. Mostly this is a wrong-picture bug and not a memory-safety one: `rgb2yuv.comp` clamps every fetch with `min(p, textureSize - 1)` and the CPU arm uploads `min(len, need)` into a session-sized image. But it also closes a narrow real hazard that was not in the filing: `import_cached` keys on `(st_dev, st_ino)` and returns the cached `VkImage` on a hit WITHOUT rechecking the extent, and unlike the capture side it is never cleared on a renegotiation — so a dmabuf inode recycled across a shrinking renegotiation would hand the encoder an image sized for the old, larger allocation. This check closes that route. ⚠ Recorded at the code because it changes the failure mode, not just the detection: a mismatch is NOT always transient. A compositor-initiated PipeWire renegotiation updates the capturer's size in place and signals nothing the encode loop reads, so it can be a permanent new steady state — and `reset()` reopens at the same dimensions by construction, so the host's five-reset budget cannot recover (~3.1 s of frozen stream, then the session ends) where before it would have streamed on with a wrong picture. For a real mode change that trade is clearly right; for a 16-row KWin mismatch it is not, and the proper fix is for the host to classify this error as a PIPELINE rebuild rather than an encoder reset. Filed, not done here. The device-selection half of WP4.5 was written, reviewed and WITHDRAWN — see the handoff doc. Matching the selected render GPU regresses the hybrid Intel-compositor + NVIDIA-present topology this project has a live field report for, because `selected_gpu()` answers NVIDIA whenever `/dev/nvidiactl` exists regardless of where capture actually runs. WP4.5 (dimension half). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -917,6 +917,40 @@ impl PyroWaveEncoder {
|
||||
);
|
||||
let dev = self.device.clone();
|
||||
let (w, h) = (self.width, self.height);
|
||||
// The frame must be exactly the session's mode (WP4.5). PyroWave applies NO alignment —
|
||||
// `width`/`height` are the negotiated mode verbatim — so any mismatch is a bug somewhere,
|
||||
// and until now it was a SILENT one: the frame was encoded edge-smeared or cropped.
|
||||
//
|
||||
// Every other Linux backend already refuses exactly this, with this shape, in `submit`:
|
||||
// libav-NVENC (`linux/mod.rs`), VAAPI (`linux/vaapi.rs`) and openh264 (`sw.rs`) all carry
|
||||
// the same `ensure!`. PyroWave was the only one that didn't. (`vulkan_video.rs` bails in
|
||||
// its Dmabuf arms only — its CSC path and its CPU arm do not — so it is the weaker
|
||||
// precedent, not the model.)
|
||||
//
|
||||
// Mostly this is a wrong-picture bug rather than a memory-safety one: `rgb2yuv.comp`
|
||||
// clamps every fetch with `min(p, textureSize - 1)` and the CPU arm uploads
|
||||
// `min(len, need)` into a session-sized image. But it also closes a narrow real hazard —
|
||||
// `import_cached` keys on `(st_dev, st_ino)` and returns the cached `VkImage` on a hit
|
||||
// WITHOUT rechecking the extent, and unlike the capture side it is never cleared on a
|
||||
// renegotiation. A dmabuf inode recycled across a SHRINKING renegotiation would hand the
|
||||
// encoder an image sized for the old, larger allocation. After this check every frame
|
||||
// reaching that cache has the session's dimensions, so the size-change route is closed.
|
||||
//
|
||||
// ⚠ A mismatch is NOT always transient. A compositor-initiated PipeWire renegotiation
|
||||
// updates the capturer's size in place and signals nothing the encode loop reads, so this
|
||||
// can be a permanent new steady state — and `reset()` reopens at the SAME dimensions by
|
||||
// construction, so the host's five-reset budget cannot fix it (≈3.1 s of frozen stream,
|
||||
// then the session ends). That is still better than shipping a smeared picture forever,
|
||||
// but the real fix is for the host to treat this error as a PIPELINE rebuild rather than
|
||||
// an encoder reset. Filed; not this commit.
|
||||
if frame.width != w || frame.height != h {
|
||||
bail!(
|
||||
"pyrowave: frame {}x{} != session mode {w}x{h} — refusing a mismatched encode \
|
||||
source",
|
||||
frame.width,
|
||||
frame.height
|
||||
);
|
||||
}
|
||||
// Everything from `begin` through `queue_submit` runs in one closure whose error arm
|
||||
// resets `self.cmd`. On every arm inside it the reset is LEGAL: a mid-recording failure
|
||||
// leaves RECORDING, a failed `end` leaves INVALID, a failed `queue_submit` enqueued
|
||||
@@ -1687,6 +1721,48 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// WP4.5: a frame that is not the session's mode must be REFUSED, not encoded. PyroWave
|
||||
/// applies no alignment, so a mismatch can only be a stale frame from a renegotiated mode —
|
||||
/// and the failure is silent without this check (`rgb2yuv.comp` clamps its fetches and the CPU
|
||||
/// arm uploads `min(len, need)`), so it ships an edge-smeared or cropped picture rather than
|
||||
/// erroring. Both directions, since undersized and oversized smear in opposite ways. The
|
||||
/// session must still be usable afterwards: the refusal happens before anything is recorded,
|
||||
/// so a correctly-sized frame right after must encode normally.
|
||||
#[test]
|
||||
#[ignore = "needs a real Vulkan 1.3 compute device (run on a GPU host, not the build box)"]
|
||||
fn pyrowave_refuses_a_frame_that_is_not_the_mode() {
|
||||
let (w, h) = (256u32, 256u32);
|
||||
let mut enc =
|
||||
PyroWaveEncoder::open(w, h, 60, 40_000_000, crate::ChromaFormat::Yuv420).expect("open");
|
||||
for (fw, fh) in [(w - 2, h), (w, h - 2), (w + 2, h + 2)] {
|
||||
let err = enc
|
||||
.submit(&cpu_frame(fw, fh, 0, [200, 40, 40, 255]))
|
||||
.expect_err("a frame that is not the mode must be refused");
|
||||
let msg = format!("{err:#}");
|
||||
assert!(
|
||||
msg.contains("session mode"),
|
||||
"the refusal must name the mismatch, got: {msg}"
|
||||
);
|
||||
}
|
||||
// A refusal must enqueue NOTHING. Checked before the good frame, because `pending` is a
|
||||
// queue: asserting `is_some()` after a successful submit would pass even if the three
|
||||
// refusals had each pushed an AU, and would then be measuring the wrong one.
|
||||
assert!(
|
||||
enc.poll().expect("poll").is_none(),
|
||||
"a refused frame must not enqueue an AU"
|
||||
);
|
||||
enc.submit(&cpu_frame(w, h, 16_666_667, [40, 200, 40, 255]))
|
||||
.expect("a correctly-sized frame after a refusal must still encode");
|
||||
assert!(
|
||||
enc.poll().expect("poll").is_some(),
|
||||
"the session must survive the refusals"
|
||||
);
|
||||
assert!(
|
||||
enc.poll().expect("poll").is_none(),
|
||||
"exactly one AU for one accepted frame"
|
||||
);
|
||||
}
|
||||
|
||||
/// WP5.1: a failed dmabuf import must leak neither the dup'd fd nor the VkImage. Drives
|
||||
/// `vk_util::import_rgb_dmabuf` DIRECTLY — not `import_cached` — so the deliberate failures
|
||||
/// cannot feed pf-zerocopy's raw-dmabuf degrade latch (which never un-latches by design).
|
||||
|
||||
Reference in New Issue
Block a user