From 9243a770bce116f869e268860060e1a7bafe7823 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Wed, 29 Jul 2026 01:26:22 +0200 Subject: [PATCH] fix(host/encode): an Automatic bitrate follows the pixels actually encoded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `build_pipeline` opens the encoder at the DELIVERED frame size but was handed a bitrate resolved from the NEGOTIATED mode. Those agree for a virtual display, which is created at the mode that was asked for. They do not agree for a monitor mirror: `MirrorDisplay::create` ignores the requested mode by design (a physical head runs at the mode its owner set), so a client asking for 1080p and mirroring a 4K panel encoded four times the pixels at the 1080p rate — a quarter of the bits per pixel the codec was sized for, which arrives as a soft, stuttering picture rather than as the mismatch it actually is. This is the reported "laggy mirror". Re-resolve for what will really be encoded, and thread the applied rate back out so the session adopts it: `bitrate_kbps` is what the ABR controller climbs from, what the console samples, and what a `SetBitrate` ack is measured against, so letting it disagree with the live encoder makes each of those reason about a stream that does not exist. The mid-stream rebuild paths (mode switch, compositor switch, capture-loss) adopt it too — each can land on a different source than it left. Automatic only. An explicit client rate is the operator's statement about their own link and is never second-guessed. Co-Authored-By: Claude Opus 5 (1M context) --- crates/punktfunk-host/src/native/handshake.rs | 5 + crates/punktfunk-host/src/native/stream.rs | 150 ++++++++++++++++-- 2 files changed, 140 insertions(+), 15 deletions(-) diff --git a/crates/punktfunk-host/src/native/handshake.rs b/crates/punktfunk-host/src/native/handshake.rs index 967d4b1d..a7146ccb 100644 --- a/crates/punktfunk-host/src/native/handshake.rs +++ b/crates/punktfunk-host/src/native/handshake.rs @@ -577,6 +577,10 @@ pub(super) async fn negotiate( // prepared display and the session wiring cannot disagree with it. let cursor_fw = welcome.host_caps & punktfunk_core::quic::HOST_CAP_CURSOR != 0; let (mode, shard_payload) = (hello.mode, welcome.shard_payload); + // "Automatic" — `bitrate_kbps` above is the host's own answer for `mode`, so the build + // may re-resolve it if the source turns out to deliver a different size. Sampled here + // rather than in the thread body so the closure doesn't have to capture `hello`. + let bitrate_auto = hello.bitrate_kbps == 0; let trace = bringup.clone(); std::thread::Builder::new() .name("punktfunk1-stream".into()) @@ -588,6 +592,7 @@ pub(super) async fn negotiate( client_hdr, cursor_fw, bitrate_kbps, + bitrate_auto, bit_depth, chroma, codec, diff --git a/crates/punktfunk-host/src/native/stream.rs b/crates/punktfunk-host/src/native/stream.rs index c1f610e5..081ffb93 100644 --- a/crates/punktfunk-host/src/native/stream.rs +++ b/crates/punktfunk-host/src/native/stream.rs @@ -1232,6 +1232,7 @@ pub(super) fn virtual_stream(ctx: SessionContext, prepared: Option { // Replace the pipeline first (drops the old capturer → old PipeWire stream + // virtual output), then the factory (drops e.g. the old KWin connection). @@ -1642,6 +1662,10 @@ pub(super) fn virtual_stream(ctx: SessionContext, prepared: Option, + // The bitrate the encoder was ACTUALLY opened at (kbps). Normally the one asked for; different + // when an Automatic rate was re-resolved because the source delivers a size the session did not + // negotiate (a monitor mirror). The caller adopts it so the ABR controller, the console sample + // and every later `SetBitrate` resolve against what the encoder is really doing. + u32, ); /// The in-place resize fast path (latency plan P2.3, Windows IDD-push): the manager mode-sets the @@ -3197,6 +3246,8 @@ pub(super) fn prepare_display( client_hdr: Option, cursor_forward: bool, bitrate_kbps: u32, + // Passed through to [`build_pipeline`] — see its parameter of the same name. + bitrate_auto: bool, bit_depth: u8, chroma: crate::encode::ChromaFormat, codec: crate::encode::Codec, @@ -3245,6 +3296,7 @@ pub(super) fn prepare_display( &mut vd, mode, bitrate_kbps, + bitrate_auto, bit_depth, plan, quit, @@ -3270,6 +3322,8 @@ fn build_pipeline_with_retry( vd: &mut Box, mode: punktfunk_core::Mode, bitrate_kbps: u32, + // Passed through to [`build_pipeline`] — see its parameter of the same name. + bitrate_auto: bool, bit_depth: u8, plan: crate::session_plan::SessionPlan, quit: &Arc, @@ -3333,6 +3387,7 @@ fn build_pipeline_with_retry( vd, mode, bitrate_kbps, + bitrate_auto, bit_depth, plan, quit, @@ -3427,6 +3482,25 @@ fn pacing_hz(session_hz: u32, achieved_hz: u32) -> u32 { achieved_hz.min(session_hz).max(1) } +/// Adopt the rate a freshly built pipeline's encoder was actually opened at. +/// +/// The session's own `bitrate_kbps` is the number every later decision reads — the ABR controller's +/// climb base, the console's sample, what a `SetBitrate` ack is measured against — so letting it +/// disagree with the live encoder means each of those reasons about a stream that doesn't exist. +/// Silent when nothing changed, which is the overwhelmingly common case. +fn adopt_built_bitrate(current: &mut u32, built: u32, live: &Arc) { + if built == *current { + return; + } + tracing::info!( + from_kbps = *current, + to_kbps = built, + "adopted the rebuilt pipeline's bitrate (re-resolved for what it actually encodes)" + ); + *current = built; + live.store(built, Ordering::Relaxed); +} + /// Encode-stall recovery: rebuild the encoder in place (keeping capture + the session up) and /// discard the owed in-flight frame records — their AUs died with the old encoder instance. /// Returns `false` when the backend has no in-place rebuild ([`crate::encode::Encoder::reset`]'s @@ -3450,6 +3524,10 @@ fn build_pipeline( vd: &mut Box, mode: punktfunk_core::Mode, bitrate_kbps: u32, + // The client asked for "Automatic", so `bitrate_kbps` is the host's own codec-aware answer for + // `mode` — and may be re-resolved below when the source delivers a different size than `mode`. + // An explicit client rate is left exactly as given. + bitrate_auto: bool, bit_depth: u8, plan: crate::session_plan::SessionPlan, quit: &Arc, @@ -3568,6 +3646,40 @@ fn build_pipeline( if let Some(t) = trace { t.mark("first_frame"); } + // A source may deliver a different frame size than the session negotiated, and the encoder below + // is opened at the DELIVERED one. The case that matters is a monitor MIRROR: `MirrorDisplay` + // ignores the requested mode by design (a physical head runs at the mode its owner set — + // design/per-monitor-portal-capture.md §7.3), so a client that asked for 1080p and mirrors a 4K + // panel encodes four times the pixels. An Automatic bitrate resolved from the *negotiated* mode + // then hands the codec a quarter of the bits per pixel it was sized for, which arrives as a soft, + // stuttering picture rather than as the mismatch it actually is. Re-resolve for what we will + // really encode. + // + // Automatic ONLY. An explicit client rate is the operator's statement about their own link and is + // never second-guessed — the same contract `resolve_bitrate_kbps_for` keeps everywhere else. The + // refresh is left at the negotiated one so this changes exactly one input (the pixel count) to a + // formula that is otherwise untouched. + let bitrate_kbps = if bitrate_auto && (frame.width, frame.height) != (mode.width, mode.height) { + let delivered = punktfunk_core::Mode { + width: frame.width, + height: frame.height, + ..mode + }; + let re = resolve_bitrate_kbps_for(plan.codec, 0, &delivered, plan.chroma, bit_depth); + if re != bitrate_kbps { + tracing::info!( + negotiated = %format!("{}x{}", mode.width, mode.height), + delivered = %format!("{}x{}", frame.width, frame.height), + from_kbps = bitrate_kbps, + to_kbps = re, + "the source delivers a different size than the session negotiated — re-resolved the \ + Automatic bitrate for the pixels actually being encoded" + ); + } + re + } else { + bitrate_kbps + }; // `bit_depth` is the handshake-negotiated value (8, or 10 = HEVC Main10 when the client // advertised VIDEO_CAP_10BIT and the host opted in). Threaded down from the Welcome. let mut enc = crate::encode::open_video( @@ -3605,7 +3717,15 @@ fn build_pipeline( ); } let interval = std::time::Duration::from_secs_f64(1.0 / effective_hz.max(1) as f64); - Ok((capturer, enc, frame, interval, node_id, pool_gen)) + Ok(( + capturer, + enc, + frame, + interval, + node_id, + pool_gen, + bitrate_kbps, + )) } #[cfg(test)]