From 6237e3d0a3f1ea170db2e458d996216756c518ba Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Thu, 13 Aug 2026 15:26:35 +0200 Subject: [PATCH] =?UTF-8?q?fix(pf-vdisplay):=20the=20KWin=20=E2=89=A460=20?= =?UTF-8?q?Hz=20path=20never=20checked=20what=20KWin=20actually=20built,?= =?UTF-8?q?=20and=20the=20log=20reported=20the=20request=20as=20if=20it=20?= =?UTF-8?q?were=20a=20readback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 4K60 GameStream session captured 1920x1080. `create()` asked KWin for 3840x2160, KWin built something else, and nothing compared the two: only the >60 Hz arm read anything back, and it gets that for free because it installs a custom mode. The ≤60 Hz arm installs nothing, which is exactly why it never noticed. The line that should have caught it was the one that hid it. `spawn_vout` returns a node id, never a size, so tracing::info!(node_id, width, height, "KWin virtual output ready") was echoing the REQUEST — the field log stated 3840x2160 while the output was 1080p, and the first pass at diagnosing this was done against that number. It now logs `requested_w`/`requested_h`, and the readback sits under it. Unverified, the mismatch was silent and total. `final_dims` carried the request forward, so `apply_topology`, `clear_replication_source` and `resolve_kscreen_addr` — all of which resolve by dims — quietly missed their own output, leaving the stream neither primary nor de-mirrored; and the encoder opened at the captured size, handing the client a bitstream that disagreed with the resolution it had configured its decoder from. Suspected trigger is KWin restoring per-output mode/scale from kwinoutputconfig.json, which is keyed by output NAME — and ours is deliberately stable across sessions so KDE reapplies that client's scaling (Stage 3). The feature and the failure are the same mechanism. - `kwin_output_mgmt::actual_dims()` reads the output's real mode + scale. Resolution is by name alone, so it declines unless EXACTLY one output carries our prefix: two means a supersede is in flight, and the dims filter is the only thing that can tell the replacement from the predecessor whose name it reuses. Failing closed keeps this a pure addition. - On a mismatch, re-assert the requested mode through the same `set_custom_mode` install+select the sacrificial birth already uses (an output at a size we don't want, moved to one we do) and arm `expect_exact_dims` so the capturer holds frames until the screencast renegotiates. 60 Hz is requested, not `mode.refresh_hz`: only the size is wrong here, and asking for the client's rate would install a 30 Hz mode for a 30 fps client. - If KWin refuses the correction, report the size that is REALLY there rather than the request, so the dims-keyed resolves and the encoder key on reality, and say in the log how to clear the stored entry. - Scale is logged, never corrected — a non-unity scale here is the Stage 3 feature working, not a fault. - `mode_satisfies()` extracts the acceptance predicate both arms now share, so they cannot drift into disagreeing about what "we got what we asked for" means. Tested: a restored 1080p does not pass for a 4K request, a CVT-aligned width does, and the slack is bounded, one-sided and width-only. The stream-side warning is reworded but deliberately still NOT fatal: mirroring a pinned monitor streams a size the client never negotiated BY DESIGN (§7.3 — a panel runs at the mode its owner set and the client scales), so refusing the mismatch would break every mirror session. It now names both causes and states what the client actually does with the stream. Does not claim to close the Xbox Moonlight disconnect it was found through: that client's IDR storm begins ~4.6 s after the first frame, which a decoder simply unable to handle the size would not do. The 1080p-instead-of-4K is a real defect on its own terms and is what this fixes. --- crates/pf-vdisplay/src/vdisplay/linux/kwin.rs | 183 +++++++++++++++++- .../src/vdisplay/linux/kwin_output_mgmt.rs | 38 ++++ .../punktfunk-host/src/gamestream/stream.rs | 23 ++- 3 files changed, 237 insertions(+), 7 deletions(-) diff --git a/crates/pf-vdisplay/src/vdisplay/linux/kwin.rs b/crates/pf-vdisplay/src/vdisplay/linux/kwin.rs index 0eab03a4..817aa0c6 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/kwin.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/kwin.rs @@ -302,10 +302,14 @@ impl VirtualDisplay for KwinDisplay { let want_high = mode.refresh_hz > 60; let birth_h = if want_high { height + 16 } else { height }; let (mut node_id, mut stop) = spawn_vout(width, birth_h)?; + // `requested_*`, NOT `width`/`height`: `spawn_vout` hands back a node id, never a size, so + // every number on this line is what we ASKED for. Logged as `width=… height=…` it read like + // a readback of what KWin built, and a field report where KWin had actually built a 1080p + // output was diagnosed against a log line stating 3840x2160. The readback is below. tracing::info!( node_id, - width, - height, + requested_w = width, + requested_h = height, birth_h, embedded_pointer = !self.hw_cursor, "KWin virtual output ready" @@ -346,9 +350,7 @@ impl VirtualDisplay for KwinDisplay { // width at or just below the request (a CVT alignment). That also proves the output // left the sacrificial birth size, so the recording stream will renegotiate to it. match active { - Some((aw, ah, ahz)) - if ah == height && aw <= width && width - aw < CVT_H_GRANULARITY => - { + Some((aw, ah, ahz)) if mode_satisfies((aw, ah), width, height) => { expect_exact_dims = true; final_dims = (aw, ah); ahz @@ -381,6 +383,125 @@ impl VirtualDisplay for KwinDisplay { } } } else { + // ≤60 Hz installs no mode, so nothing here ever learned what KWin actually built — and + // KWin does not necessarily build what it was asked for. `OutputConfigurationStore` + // restores per-output mode AND scale from `kwinoutputconfig.json` keyed by output NAME, + // and ours is stable across sessions by design (Stage 3, so KDE reapplies that client's + // scaling) — so a slot that last ran at 1080p gets 1080p put back on top of the 4K we + // just requested. The >60 Hz arm above is immune only incidentally: it installs a mode, + // so it gets a readback for free. + // + // Unverified, that mismatch is silent and total. The capture builds at KWin's size, the + // encoder opens against it, and Moonlight — which configured its decoder for the size it + // negotiated over RTSP — receives a bitstream it cannot decode, asks for a keyframe + // every ~50 ms, and drops the session. Meanwhile every dims-keyed resolve below + // (`apply_topology`, `clear_replication_source`, `resolve_kscreen_addr`) is looking for + // an output at the requested size and quietly finding nothing, so the stream isn't even + // made primary or de-mirrored. + match crate::kwin_output_mgmt::actual_dims(&our_prefix) { + // KWin honoured the request — the overwhelmingly common case. No configuration is + // built and nothing is applied: byte-for-byte the behaviour this arm always had. + // + // The scale is recorded rather than corrected. A non-1.0 scale here is NOT a fault + // to repair: the stable output name exists precisely so KDE reapplies this client's + // scaling on reconnect (Stage 3), so forcing the 1.0 we asked `stream_virtual_output` + // for would undo a feature. It is logged because it is the other half of the stored + // per-output config, and because the pixel-vs-logical question it raises is exactly + // what a future "the size is right but the capture is halved" report will turn on — + // KWin's output screencast streams the source's PIXEL size, so a scale should not + // move the captured dimensions, and a report showing otherwise would be the evidence + // that assumption is wrong on some KWin version. + Some((aw, ah, _, scale)) if (aw, ah) == (width, height) => { + if scale != 1.0 { + tracing::debug!( + width, + height, + scale, + "KWin virtual output verified at the requested size, carrying a stored \ + non-unity scale (per-client scaling — capture is unaffected)" + ); + } + } + Some((aw, ah, _, scale)) => { + tracing::warn!( + actual_w = aw, + actual_h = ah, + requested_w = width, + requested_h = height, + stored_scale = scale, + our_prefix, + "KWin built our virtual output at a DIFFERENT size than requested (a stored \ + kwinoutputconfig.json mode/scale for this output name) — re-asserting the \ + requested mode so the stream matches what the client negotiated" + ); + // Re-assert the requested size through the SAME install+select the sacrificial + // birth uses above: an output sitting at a size we don't want, moved to the one + // we do, with the screencast stream renegotiating to it on the first buffers + // recorded after the consumer connects. `aw`/`ah` play the birth size — that is + // literally what they are here, just not deliberately. + // + // 60 Hz, NOT `mode.refresh_hz`: this arm is ≤60 Hz by construction and only the + // SIZE is wrong. Asking for the client's rate would install a 30 Hz mode for a + // 30 fps client and throttle the compositor to it — a behaviour change fixing a + // size has no business making. KWin's virtual outputs are 60 Hz natively and + // `achieved_hz` below stays the client's rate exactly as before. + match crate::kwin_output_mgmt::set_custom_mode( + &our_prefix, + aw, + ah, + width, + height, + 60, + ) { + // Same acceptance test as the high-refresh arm — literally, so the two can + // never drift. That the mode moved at all also proves the screencast will + // renegotiate, which is what `expect_exact_dims` then waits for. + Some((cw, ch, _)) if mode_satisfies((cw, ch), width, height) => { + expect_exact_dims = true; + final_dims = (cw, ch); + tracing::info!( + active_w = cw, + active_h = ch, + "KWin virtual output corrected to the requested size" + ); + } + other => { + // Correction refused (pre-6.6 KWin has no `set_custom_modes`, or the + // compositor didn't answer). Report the size that is REALLY there, not + // the one we asked for: the dims-keyed resolves below and the encoder + // all key on `final_dims`, and carrying the request forward is what + // made this a silent failure rather than a degraded one. The session + // still runs, at KWin's size: the stream layer warns that the client is + // decoding something other than what it negotiated but does NOT refuse + // it, because a monitor mirror legitimately streams a size the client + // never asked for (§7.3) and failing here would break every one. + tracing::warn!( + active = ?other, + actual_w = aw, + actual_h = ah, + requested_w = width, + requested_h = height, + "KWin would not re-assert the requested mode — the output is STUCK \ + at its stored size. Clear this output's entry from \ + kwinoutputconfig.json (or set it to the streamed resolution in \ + System Settings → Display) and reconnect" + ); + final_dims = (aw, ah); + } + } + } + // Management unavailable, or two outputs share our name (a supersede in flight, the + // one case only a dims-keyed resolve can disambiguate). Nothing verifiable to act + // on, so carry on exactly as this arm always did rather than reconfigure an output + // we cannot identify. + None => { + tracing::debug!( + our_prefix, + "KWin: could not read back the virtual output's actual mode (management \ + unavailable or a same-named supersede in flight) — proceeding unverified" + ); + } + } mode.refresh_hz }; // Display-management topology (Stage 2): `Extend` leaves the streamed output an extension; @@ -733,6 +854,25 @@ fn monitors_from_kscreen_json(doc: &serde_json::Value) -> Vec bool { + let (aw, ah) = active; + ah == want_h && aw <= want_w && want_w - aw < CVT_H_GRANULARITY +} + /// One row of an output's mode list, as parsed from `kscreen-doctor -j`. #[derive(Clone, Debug, PartialEq)] struct KModeRow { @@ -1851,9 +1991,40 @@ fn await_created( #[cfg(test)] mod tests { use super::{ - modes_from_json, monitors_from_kscreen_json, pick_custom_mode, KModeRow, MANAGED_PREFIX, + mode_satisfies, modes_from_json, monitors_from_kscreen_json, pick_custom_mode, KModeRow, + MANAGED_PREFIX, }; + /// The field failure this predicate now guards, in the shape the log reported it: a client + /// negotiated 3840x2160, KWin restored a stored 1920x1080 for the output name, and nothing + /// compared the two — so the session captured 1080p, encoded 1080p, and shipped it to a client + /// that had configured its decoder for 4K. Half the requested size is not an alignment. + #[test] + fn a_restored_stored_mode_does_not_pass_for_the_requested_one() { + assert!(!mode_satisfies((1920, 1080), 3840, 2160)); + } + + /// The case the predicate must NOT reject, and the reason it can't just test equality: libxcvt + /// rounds a width down to the 8-px cell grain, so the mode KWin builds for a 2868-wide request + /// really is 2864 wide. Rejecting it would strand the output on its birth mode. + #[test] + fn a_cvt_aligned_width_still_satisfies_the_request() { + assert!(mode_satisfies((2864, 1320), 2868, 1320)); + assert!(mode_satisfies((3840, 2160), 3840, 2160)); // exact is the common case + } + + /// The alignment slack is bounded and one-sided. A width 8+ px short is a different mode, not a + /// rounding of ours; a width ABOVE the request is somebody else's mode entirely (and is what + /// would underflow the subtraction if the `<=` guard were ever dropped); and the height is + /// never rounded, so it must match exactly. + #[test] + fn the_alignment_slack_is_bounded_one_sided_and_width_only() { + assert!(mode_satisfies((3833, 2160), 3840, 2160)); // 7 short — inside the grain + assert!(!mode_satisfies((3832, 2160), 3840, 2160)); // 8 short — a different mode + assert!(!mode_satisfies((3848, 2160), 3840, 2160)); // wider than asked + assert!(!mode_satisfies((3840, 2159), 3840, 2160)); // height is never aligned + } + fn row(id: &str, w: u32, h: u32, hz: f64) -> KModeRow { KModeRow { id: id.to_string(), diff --git a/crates/pf-vdisplay/src/vdisplay/linux/kwin_output_mgmt.rs b/crates/pf-vdisplay/src/vdisplay/linux/kwin_output_mgmt.rs index 92b84b5f..66be6235 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/kwin_output_mgmt.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/kwin_output_mgmt.rs @@ -1030,6 +1030,44 @@ pub(crate) fn clear_replication_source(our_prefix: &str, our_w: u32, our_h: u32) } } +/// The size + scale our just-created virtual output ACTUALLY landed at, read only. +/// +/// [`resolve_ours`] keys on the size we asked KWin for, which answers "is our output there?" but +/// can never answer "did KWin give us what we asked for?" — a miss is indistinguishable from an +/// output that simply hasn't appeared. That gap is not theoretical: KWin restores per-output config +/// (mode AND scale) from `kwinoutputconfig.json` keyed by output NAME, and ours is deliberately +/// stable across sessions (see the note on [`is_mirroring`]), so a stored 1080p mode left by an +/// earlier session is re-applied on top of the 4K we just requested. Every dims-keyed caller then +/// silently misses — topology, de-mirror, position — and the capture pipeline builds at a size the +/// client never negotiated. +/// +/// Resolution is by NAME ALONE, so it deliberately declines (`None`) unless EXACTLY ONE output +/// carries our prefix. Two matches means a supersede is in flight, and the dims filter is the only +/// thing that can tell the replacement from the predecessor it reuses the name of — picking wrong +/// here would hand the caller the doomed output's size and, worse, invite it to reconfigure the +/// output that is about to disappear. Failing closed leaves today's behaviour untouched; the +/// verification is an addition, never a new way to get it wrong. (A prefix that is also a prefix of +/// a sibling slot's name — `-7` vs `-70` — reads as ambiguous and declines for the same reason.) +/// +/// Returns `(width, height, refresh_mHz, scale)`. Scale is reported for the log rather than acted +/// on: KWin's output screencast streams the source's PIXEL size, so a restored scale shifts the +/// desktop's logical layout without changing what we capture — but it is the other half of the +/// stored config, and naming it in the log is what turns "why is this 1080p" into one glance. +pub(crate) fn actual_dims(our_prefix: &str) -> Option<(u32, u32, u32, f64)> { + let sess = Session::open("verify_dims").ok()?; + let mut matches = sess.state.devices.values().filter(|d| { + // `seen_done`: a device mid-announce has no coherent current_mode to read, and reading one + // anyway is how you get a "KWin gave us 0x0" correction that stomps a healthy output. + d.seen_done && d.name.as_deref().is_some_and(|n| n.starts_with(our_prefix)) + }); + let ours = matches.next()?; + if matches.next().is_some() { + return None; + } + let (w, h, mhz) = sess.current_dims(ours)?; + Some((w, h, mhz, ours.scale.filter(|s| *s > 0.0).unwrap_or(1.0))) +} + /// Install + select a `want_w`×`want_h`@`want_hz` custom mode on the just-created virtual output /// (name starts with `our_prefix`, currently at its sacrificial birth size `birth_w`×`birth_h`) — /// entirely over `kde_output_management_v2`, the in-process replacement for the `kscreen-doctor` diff --git a/crates/punktfunk-host/src/gamestream/stream.rs b/crates/punktfunk-host/src/gamestream/stream.rs index 49c8ef3c..af4f857f 100644 --- a/crates/punktfunk-host/src/gamestream/stream.rs +++ b/crates/punktfunk-host/src/gamestream/stream.rs @@ -1095,10 +1095,31 @@ fn stream_body( // The first frame establishes the authoritative size/format for the encoder. let mut frame = capturer.next_frame().context("capture first frame")?; if frame.width != cfg.width || frame.height != cfg.height { + // Deliberately NOT fatal, and deliberately not "resize the output" (which read as an + // instruction nothing was carrying out). A mismatch has two very different causes and only + // one of them is a fault: + // + // * Mirroring a pinned monitor — EXPECTED. §7.3: a panel runs at the mode its owner set + // and the client scales, so `open_gs_mirror_source` passes the client's mode purely to + // keep the argument honest and the backend ignores it. Failing here would break every + // mirror session. + // * A virtual display — a FAULT. That output was created at the client's negotiated size + // precisely so this can't happen, so a mismatch means the backend did not build what it + // was asked for. The KWin backend now reads its output back and re-asserts the mode, so + // look for its `KWin built our virtual output at a DIFFERENT size` / `would not + // re-assert` lines just above this one — they carry the cause and the remedy. + // + // What the client does with it is the part worth stating plainly: the encoder opens at the + // CAPTURED size below, so Moonlight receives a bitstream that disagrees with the resolution + // it configured its decoder from. Tolerant decoders re-init off the SPS and scale; strict + // ones (Media Foundation on Xbox) may instead produce nothing and ask for a keyframe every + // ~50 ms until the client gives up and drops the session. tracing::warn!( captured = ?(frame.width, frame.height), negotiated = ?(cfg.width, cfg.height), - "captured size != negotiated size — Moonlight expects the negotiated size; resize the output" + "captured size != negotiated size — the client decodes a stream that disagrees with \ + what it negotiated (expected when mirroring a monitor; a virtual-display backend fault \ + otherwise — see the vdisplay lines above)" ); } let mut enc = encode::open_video( -- 2.54.0