forked from unom/punktfunk
fix(clients): the settings you chose reach the stream again, and four more that never did
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) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -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));
|
||||
|
||||
@@ -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();
|
||||
|
||||
+140
-41
@@ -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::<String>);
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<String>,
|
||||
one_off_profile: Option<String>,
|
||||
) -> 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<std::path::PathBuf> {
|
||||
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)?;
|
||||
|
||||
Reference in New Issue
Block a user