From 9c5af8d7e117c29a35217c0a0f35c2043c5ce3f7 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Fri, 31 Jul 2026 19:30:40 +0200 Subject: [PATCH] fix(clients): the settings you chose reach the stream again, and four more that never did MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since spec mode (`f32c3aaa`, shipped in 0.22.0) the GTK shell has handed the session a `--resolved-spec` built from `Settings::default()`. A session running from a spec performs ZERO store reads by design, so every stream since has run at the defaults: `bitrate_kbps: 0` reaches the host, which reads it as its 20 Mbps fallback — the field report this starts from, "my bitrate seems to be stuck at 20 MBPS" on 0.22.3 — and with it the resolution, refresh, render scale, codec, decoder, HDR, 4:4:4, audio channels, mic, touch/mouse mode, invert scroll, stats tier, match-window and gamepad type. Profiles never applied at all: the spec's `profile` was None, and spec mode ignores `--profile`. The tell in the wild is that the GPU and audio-device pickers kept working — the session reads those three off disk into env vars before it reads the spec. The plan carried a comment explaining that `settings` "carries only what the argv needs (the fullscreen flag)". That was true when it was written and stopped being true one commit later, in another file. `ConnectPlan::for_target` answers that shape of bug: a front-end holding its own request type resolves through the same helper the session's compat path uses, so there is no hand-built `Settings` left to go stale. The GTK shell's `fullscreen_on_stream` parameter goes with it — that is a tier-P field, and a shell-read global was beating a profile that set it. The Windows shell was never affected (it spawns with no spec) but had the same fullscreen-vs-profile bug, so it moves onto the resolver too. The audit that followed found four more settings stored, rendered in two UIs, and read by nothing: - `enable_444` never became `VIDEO_CAP_444`. "Full chroma (4:4:4)" is announced in the 0.22.0 notes for Linux and Windows and did nothing on either; only Apple advertised the bit. The host already gates it on its own policy, its capturer, HEVC and a real GPU probe, and answers the resolved chroma in the Welcome before we build a decoder — the client only has to ask. There is no desktop decode probe (Apple has `Stage444Probe`), but the presenter bails cleanly on a plane format it doesn't know and demotes to software, so the downside is a slow decode, not a broken one. - `session_params` picked the HEIGHT fallback off `settings.width == 0`. - `cli::exec_session` dropped `--profile` from its forward list, so `punktfunk --connect … --profile Work` from a script or a Decky wrapper streamed with the host's binding instead. - `ResolvedSpec::write_temp` named the spec by the SPAWNER's pid alone, so a cancelled connect and the retry behind it shared one path — and the first child's exit deleted the file the second was still starting up to read. The regression test asserts the plan carries the profile-overlaid bitrate and that the spec equals it. On the old code both are 0. Co-Authored-By: Claude Opus 5 (1M context) --- clients/linux/src/app.rs | 14 +- clients/linux/src/cli.rs | 4 + clients/linux/src/spawn.rs | 181 ++++++++++++++++++----- clients/session/src/main.rs | 13 +- clients/windows/src/app/connect.rs | 11 +- crates/pf-client-core/src/orchestrate.rs | 52 ++++++- 6 files changed, 222 insertions(+), 53 deletions(-) diff --git a/clients/linux/src/app.rs b/clients/linux/src/app.rs index eb3cb4f2..374f8309 100644 --- a/clients/linux/src/app.rs +++ b/clients/linux/src/app.rs @@ -417,15 +417,11 @@ impl SimpleComponent for AppModel { self.hosts.emit(HostsMsg::ClearError); self.hosts .emit(HostsMsg::SetConnecting(Some(req.card_key()))); - let fullscreen = self.settings.borrow().fullscreen_on_stream; - if let Err(e) = spawn::spawn_session( - sender.input_sender().clone(), - req, - fp_hex, - tofu, - fullscreen, - opts, - ) { + // No settings ride along: the spawner resolves this host's effective ones + // (globals + its profile) for both the argv and the child's spec. + if let Err(e) = + spawn::spawn_session(sender.input_sender().clone(), req, fp_hex, tofu, opts) + { self.busy = false; self.hosts.emit(HostsMsg::SetConnecting(None)); self.hosts.emit(HostsMsg::ShowError(e)); diff --git a/clients/linux/src/cli.rs b/clients/linux/src/cli.rs index 181cbf36..ac29b7c0 100644 --- a/clients/linux/src/cli.rs +++ b/clients/linux/src/cli.rs @@ -80,6 +80,10 @@ pub fn exec_session() -> glib::ExitCode { "--launch", "--mgmt", "--connect-timeout", + // A one-off profile pick, same grammar the session documents (`--profile ""` forces + // the global defaults). Without it here, `punktfunk --connect … --profile Work` from a + // script or a Decky wrapper streamed with the host's binding instead. + "--profile", ]; let mut cmd = std::process::Command::new(crate::spawn::session_binary()); let mut args = std::env::args().skip(1).peekable(); diff --git a/clients/linux/src/spawn.rs b/clients/linux/src/spawn.rs index 3530745f..f74be5ff 100644 --- a/clients/linux/src/spawn.rs +++ b/clients/linux/src/spawn.rs @@ -14,7 +14,6 @@ use crate::app::AppMsg; use crate::ui_hosts::ConnectRequest; use pf_client_core::orchestrate::{self, ConnectPlan, HostTarget, SessionEvent}; -use pf_client_core::trust::Settings; /// Spawn tunables beyond a plain connect. #[derive(Debug, Default)] @@ -32,11 +31,49 @@ pub struct SpawnOpts { pub use orchestrate::{session_binary, CancelHandle}; +/// The plan one card click resolves to. `for_target` does ALL of the resolving — this +/// device's settings with the host's bound profile (or the "Connect with ▸" one-off) overlaid, +/// and the per-host clipboard decision — through the same helpers the session's own compat path +/// uses, so the two cannot disagree. +/// +/// Resolving is not optional: `orchestrate::spawn_session` writes `plan.settings` into the +/// child's `--resolved-spec`, and a session running from a spec reads NO stores. This used to +/// build the plan with `..Settings::default()`, on the theory that only the argv read it — +/// which meant every stream since the arch-split ran at the DEFAULTS: the host's fallback +/// bitrate (20 Mbps), the native resolution, `auto` codec, stereo, and no profile. +fn plan_for(req: &ConnectRequest, fp_hex: &str, tofu: bool, opts: &SpawnOpts) -> ConnectPlan { + let mut plan = ConnectPlan::for_target( + HostTarget { + name: req.name.clone(), + addr: req.addr.clone(), + port: req.port, + fp_hex: Some(fp_hex.to_string()), + mac: req.mac.clone(), + id: None, + }, + req.launch.as_ref().map(|(id, _)| id.clone()), + // A plain card click carries no one-off: the resolver honors the host's own binding + // (design/client-settings-profiles.md §4.6). Only a "Connect with ▸" pick (or a URL's + // `profile=`) sets one, and it applies to this session alone. + req.profile.clone(), + ); + // This shell still runs its own dial-first wake fallback in `app.rs`, so the plan's own + // wake stays off (it becomes real when the GTK connect path moves onto + // `ConnectOrchestrator`, arch-split C0). + plan.wake = false; + plan.connect_timeout_secs = opts.connect_timeout_secs; + plan.tofu = tofu; + plan +} + /// Spawn the session binary for a connect with `fp_hex` pinned and translate its /// lifecycle into [`AppMsg`]s. `tofu` = the fingerprint came from the host's advert /// rather than the store — the app persists it once the child reports ready (the child /// connects pinned to it, so ready proves the host really holds that identity). /// +/// Settings are NOT a parameter: the plan resolves them (including the fullscreen policy, +/// which is a profileable field — a shell-read global would override a profile that set it). +/// /// The caller has already taken `busy`; [`AppMsg::SessionExited`] releases it. `Err` = /// the spawn itself failed (binary missing?) — surfaced as a connect error. pub fn spawn_session( @@ -44,48 +81,9 @@ pub fn spawn_session( req: ConnectRequest, fp_hex: String, tofu: bool, - fullscreen_on_stream: bool, opts: SpawnOpts, ) -> Result<(), String> { - // The plan this connect resolves to. A plain card click carries no `--profile`: it honors - // the host's own binding, which the session resolves through the same helper this shell - // would have used (design/client-settings-profiles.md §4.6) — passing it here would be a - // second source of truth for one decision. Only a "Connect with ▸" pick (or a URL's - // `profile=`) sets one, and it applies to this session alone. - // - // Two fields are deliberately not the shell's state yet, because nothing in the spawn path - // reads them: `settings` carries only what the argv needs (the fullscreen flag), and `wake` - // is false because this shell still runs its own dial-first wake fallback in `app.rs`. Both - // become real when the GTK connect path moves onto `ConnectOrchestrator` (arch-split C0). - let plan = ConnectPlan { - host: HostTarget { - name: req.name.clone(), - addr: req.addr.clone(), - port: req.port, - fp_hex: Some(fp_hex.clone()), - mac: req.mac.clone(), - id: None, - }, - launch: req.launch.as_ref().map(|(id, _)| id.clone()), - profile: None, - // A one-off pick rides the flag; without one the session resolves the host's own - // binding through the same helper this shell would have used. - profile_override: req.profile.clone(), - settings: Settings { - fullscreen_on_stream, - ..Default::default() - }, - wake: false, - connect_timeout_secs: opts.connect_timeout_secs, - tofu, - // The per-host clipboard decision, resolved here so the child doesn't look it up - // again — matched by address, the way every other per-host lookup matches. - clipboard: pf_client_core::trust::KnownHosts::load() - .hosts - .iter() - .any(|h| h.addr == req.addr && h.port == req.port && h.clipboard_sync), - }; - + let plan = plan_for(&req, &fp_hex, tofu, &opts); let persist_paired = opts.persist_paired; let (mut error, mut ended) = (None::<(String, bool)>, None::); orchestrate::spawn_session(&plan, opts.cancel, move |ev| match ev { @@ -116,3 +114,104 @@ pub fn spawn_session( })?; Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + /// The plan a card click builds carries THIS DEVICE'S settings, with the host's bound + /// profile overlaid — not `Settings::default()`. + /// + /// This is the 0.22.x regression, and it was invisible because the defaults are all + /// plausible: the spec's `bitrate_kbps: 0` means "host default", which the host reads as + /// its 20 Mbps fallback. A stream that ignores every setting looks exactly like a stream + /// that is merely capped. One test, one `HOME` — the stores are read from it, so this + /// deliberately does not split into several that would race over the same env var. + #[test] + fn the_plan_carries_resolved_settings_not_defaults() { + use pf_client_core::profiles::{ProfilesFile, SettingsOverlay, StreamProfile}; + use pf_client_core::trust::{KnownHost, KnownHosts, Settings}; + + let home = std::env::temp_dir().join(format!("pf-spawn-test-{}", std::process::id())); + let cfg = home.join(".config/punktfunk"); + std::fs::create_dir_all(&cfg).unwrap(); + std::env::set_var("HOME", &home); + + // A device whose owner has set a bitrate, and a host bound to a profile that raises it + // further — the two layers the spec has to carry. + let globals = Settings { + bitrate_kbps: 200_000, + width: 2560, + height: 1440, + codec: "av1".into(), + fullscreen_on_stream: false, + ..Default::default() + }; + globals.save(); + let mut catalog = ProfilesFile { + version: 1, + profiles: vec![StreamProfile { + id: "aaaaaaaaaaaa".into(), + name: "Game".into(), + overrides: SettingsOverlay { + bitrate_kbps: Some(750_000), + ..Default::default() + }, + ..StreamProfile::new("") + }], + }; + catalog.save().unwrap(); + let known = KnownHosts { + hosts: vec![KnownHost { + name: "Desk".into(), + addr: "192.168.1.50".into(), + port: 9777, + fp_hex: "a".repeat(64), + paired: true, + profile_id: Some("aaaaaaaaaaaa".into()), + clipboard_sync: true, + ..Default::default() + }], + }; + known.save().unwrap(); + + let req = ConnectRequest { + name: "Desk".into(), + addr: "192.168.1.50".into(), + port: 9777, + fp_hex: Some("a".repeat(64)), + pair_optional: false, + launch: None, + mac: vec![], + profile: None, + }; + let opts = SpawnOpts::default(); + + // A card click: globals, with the BOUND profile's overrides on top. + let plan = plan_for(&req, &"a".repeat(64), false, &opts); + assert_eq!(plan.settings.bitrate_kbps, 750_000, "profile override"); + assert_eq!((plan.settings.width, plan.settings.height), (2560, 1440)); + assert_eq!(plan.settings.codec, "av1"); + assert_eq!(plan.profile.as_ref().map(|p| p.name.as_str()), Some("Game")); + assert!(plan.clipboard, "the host's own opt-in"); + // …and the spec the child actually runs from is those same settings. + assert_eq!(plan.spec(plan.clipboard).settings, plan.settings); + // The fullscreen policy rides the argv from the resolved settings, not a shell global. + assert!(!plan.session_args().contains(&"--fullscreen".to_string())); + + // "Connect with ▸ Default settings" (`Some("")`) drops back to the globals, and the + // flag still rides so the session agrees about which layer it is on. + let defaults_req = ConnectRequest { + profile: Some(String::new()), + ..req.clone() + }; + let plan = plan_for(&defaults_req, &"a".repeat(64), false, &opts); + assert_eq!(plan.settings.bitrate_kbps, 200_000, "back to the globals"); + assert_eq!(plan.profile, None); + let args = plan.session_args(); + let i = args.iter().position(|a| a == "--profile").unwrap(); + assert_eq!(args[i + 1], ""); + + let _ = std::fs::remove_dir_all(&home); + } +} diff --git a/clients/session/src/main.rs b/clients/session/src/main.rs index 46dcef79..dbb8ace3 100644 --- a/clients/session/src/main.rs +++ b/clients/session/src/main.rs @@ -193,7 +193,7 @@ mod session_main { } else { settings.width }, - height: if settings.width == 0 { + height: if settings.height == 0 { native.height } else { settings.height @@ -245,11 +245,22 @@ mod session_main { // slice NALs, so the host may keep its multi-slice low-latency default (§7 LN1). // The mobile/TV embedders must NOT copy this blindly — Amlogic MediaCodec wedges // on multi-slice AUs (see `VIDEO_CAP_MULTI_SLICE`), so they advertise per-decoder. + // 4:4:4 is opt-in and off by default (Settings "Full chroma"): the bit only says + // "upgrade me if you can" — the host still gates on its own policy, its capturer, + // HEVC, and a real GPU 4:4:4 encode probe, and answers the resolved chroma in the + // Welcome BEFORE we build a decoder. Advertised unconditionally when the user asks + // for it because every decode path here can produce it: the hardware ones where the + // driver decodes RExt, and swscale for the rest (the decoder demotes on its own). video_caps: punktfunk_core::quic::VIDEO_CAP_MULTI_SLICE | if settings.hdr_enabled { punktfunk_core::quic::VIDEO_CAP_10BIT | punktfunk_core::quic::VIDEO_CAP_HDR } else { 0 + } + | if settings.enable_444 { + punktfunk_core::quic::VIDEO_CAP_444 + } else { + 0 }, // No portable Wayland/X11 display-volume query yet, so the host keeps its EDID // defaults for Linux clients; `PUNKTFUNK_CLIENT_PEAK_NITS` (read in the session diff --git a/clients/windows/src/app/connect.rs b/clients/windows/src/app/connect.rs index 9ba9daf5..b8d534c4 100644 --- a/clients/windows/src/app/connect.rs +++ b/clients/windows/src/app/connect.rs @@ -222,7 +222,16 @@ fn connect_spawn( *ctx.shared.session.lock().unwrap() = child.clone(); ctx.shared.stats_line.lock().unwrap().clear(); ctx.shared.browse.store(false, Ordering::SeqCst); - let fullscreen = ctx.settings.lock().unwrap().fullscreen_on_stream; + // Through the same resolver the session uses, not the raw globals: "Start streams + // fullscreen" is a profileable (tier-P) field, so a host bound to a windowed profile has to + // win here too — the child takes this decision from the argv, not from its own settings. + let fullscreen = pf_client_core::trust::effective_settings( + &target.addr, + target.port, + target.profile.as_deref(), + ) + .0 + .fullscreen_on_stream; set_status.call(String::new()); set_screen.call(if opts.awaiting_approval { Screen::RequestAccess diff --git a/crates/pf-client-core/src/orchestrate.rs b/crates/pf-client-core/src/orchestrate.rs index 643b2c08..2bef7dfc 100644 --- a/crates/pf-client-core/src/orchestrate.rs +++ b/crates/pf-client-core/src/orchestrate.rs @@ -110,6 +110,48 @@ impl ConnectPlan { } } + /// The plan for a host a front-end carries in its OWN request type rather than as a stored + /// [`KnownHost`] (the GTK shell's `ConnectRequest`, a resolved link target). Settings, + /// profile and the clipboard decision are resolved here, through the same helpers + /// [`ConnectPlan::for_host`] uses; the caller then sets only what is genuinely its own + /// (`wake` when it runs its own wake fallback, `tofu`, `connect_timeout_secs`). + /// + /// It exists because hand-building the struct is a trap. [`spawn_session`] writes + /// `settings` into the child's `--resolved-spec`, and a session running from a spec reads + /// NO stores — so a `..Settings::default()` in a front-end does not fall back to the + /// user's settings, it silently streams at every default: the host's fallback bitrate, the + /// native resolution, `auto` codec, stereo audio. That shipped on the GTK shell (fixed + /// 2026-07-31) and presented as "my bitrate is stuck at 20 Mbps". + pub fn for_target( + host: HostTarget, + launch: Option, + one_off_profile: Option, + ) -> ConnectPlan { + let known = KnownHosts::load(); + // No record yet — a first connect straight off an advert. A default record says + // exactly the right thing: no profile binding, no clipboard opt-in. + let fallback = KnownHost::default(); + let stored = known + .hosts + .iter() + .find(|h| h.addr == host.addr && h.port == host.port) + .unwrap_or(&fallback); + let mut plan = ConnectPlan::resolve( + stored, + launch.as_deref(), + one_off_profile.as_deref(), + &ProfilesFile::load(), + &Settings::load(), + ); + // The CALLER's target wins over the stored record's: its fingerprint can be one the + // host just advertised and we haven't persisted (trust on first use), and its name/MAC + // come from the same discovery snapshot the card was drawn from — so `wake` has to be + // re-decided against that MAC rather than the record's. + plan.wake = plan.settings.auto_wake && !host.mac.is_empty(); + plan.host = host; + plan + } + /// The same plan, built from stores the caller already has — no disk, no clock, no /// environment. This is the form the URL router uses: a front-end loads the three stores /// once per event and every decision below is a pure function of them. @@ -492,8 +534,16 @@ impl ResolvedSpec { /// than a pipe because the session already takes a file path elsewhere and a crashed /// spawner leaves something inspectable; the name carries the pid so concurrent launches /// (a shell and a CLI, or two Decky invocations) never overwrite each other's spec. + /// + /// The pid alone was not enough: it is the SPAWNER's, so two launches from one shell — a + /// cancelled connect and the retry right behind it — shared a single path, and the first + /// child's exit deleted the file the second was still starting up to read ("resolved spec: + /// No such file"). A per-launch counter separates them. pub fn write_temp(&self) -> std::io::Result { - let path = std::env::temp_dir().join(format!("punktfunk-spec-{}.json", std::process::id())); + static LAUNCH: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let n = LAUNCH.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let path = + std::env::temp_dir().join(format!("punktfunk-spec-{}-{n}.json", std::process::id())); let json = serde_json::to_vec_pretty(self) .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; std::fs::write(&path, json)?;