diff --git a/docs/screenshare-audio-exclusion-plan.md b/docs/screenshare-audio-exclusion-plan.md new file mode 100644 index 0000000..83565d7 --- /dev/null +++ b/docs/screenshare-audio-exclusion-plan.md @@ -0,0 +1,461 @@ +# Design v2: whole-desktop screen-share audio without self-echo + +**Status:** proposal, not implemented. v2 supersedes v1 after adversarial review. +**Date:** 2026-07-20 (v1: 2026-07-19) +**Origin:** Joe's suggestion — "whitelist all audio except audio coming from peerspeak." +**Review history:** v1 reviewed by Codex (gpt-5.6-sol, high) — +`~/Documents/handoff-docs/Codex/peerspeak/review-2026-07-19-audio-exclusion-design.md`. +Verdict: "not sound enough to implement as written," 4 release blockers, all +independently verified against source and all correct. v2 is a redesign, not a patch. +**Scope:** new capture mode in pixelpass `src/host/pipeline.rs` + `src/host/audio.rs`; +playback tagging and a graceful-stop protocol in peerspeak. + +> **⚠️ This document is still gated on empirical data.** Section 10 lists the +> observations that must exist before any of this is implemented. Every claim below +> marked **OPEN-Q** is a guess with a named experiment attached, not a decision. +> No `pw-dump` of this machine's graph has been captured yet. + +--- + +## 1. The problem + +A sharer using whole-desktop audio sends their entire output mix to viewers. That mix +necessarily includes peerspeak's own playback — remote peers' voices, remote +screen-share audio — so viewers hear themselves. + +`--strict-audio` solved this for *per-app* mode by refusing to mirror the desktop at +all. Whole-desktop mode has no equivalent, so a sharer today chooses between echo and +sharing exactly one app's audio. + +Joe's ask is the missing third option: **share the desktop, minus peerspeak.** + +## 2. Feasibility, and the correct framing + +PipeWire has no capture-side filter. A monitor port carries an already-summed signal; +once peerspeak's output is in that mix it cannot be subtracted out. What PipeWire does +provide is **routing**: every playback stream is a node whose sink target can be +reassigned at runtime. + +So the feature is built by constructing a sink that only eligible streams feed, and +capturing that. Same observable behaviour, entirely different mechanism. + +This is **not** echo cancellation. `src/audio/echo_cancel.rs` addresses the microphone +path, where coupling is acoustic and needs adaptive cancellation. Here the signals +never need to be mixed at all. + +**Correction from v1:** v1 called the separation "exact and lossless." That was an +overclaim. The *membership* of the constructed mix is exact — peerspeak's audio is +never summed in. The *delivered audio* is not lossless: pixelpass downmixes to 48 kHz +stereo and AAC-encodes at 128 kbps (`pixelpass/src/host/pipeline.rs:303-324`), and the +null-sink/loopback stages may resample streams whose layout or clock differs. + +## 3. What actually exists today (v1 got this wrong) + +**v1's premise was false.** v1 claimed whole-desktop capture already loads +`@DEFAULT_SINK@.monitor → pixelpass_capture_` and that the feature is "the +existing per-app path with an inverted predicate." It is not. + +`setup_audio` (`pixelpass/src/host/pipeline.rs:121-140`) activates `Routing` **only** +when `--app` is set or `PIXELPASS_AUDIO_VIA_NULL_SINK` is set (a dogfood path, per its +own comment). peerspeak's no-app argv is exactly `--host --output json` +(`peerspeak/src/screenshare/mod.rs:135-165`). Normal whole-desktop capture therefore +**bypasses `Routing` entirely** and hands the real default monitor to `pulsesrc`. + +What `Routing::start` actually does (`pixelpass/src/host/audio.rs:65-212`): + +| Piece | Condition | +| --- | --- | +| `module-null-sink pixelpass_capture_` | always, when `Routing` runs at all | +| `module-loopback @DEFAULT_SINK@.monitor → capture` | skipped only when `--app` **and** `--strict-audio` | +| `StreamRouter` (libpipewire thread) | only when `opts.app` is `Some` | +| local-monitor `capture.monitor → @DEFAULT_SINK@` | loaded on `FirstRoutedStream`, unloaded on `LastRoutedStreamGone` | + +So this feature needs a **new capture mode in `pipeline.rs`**, plus a substantially +new router. It is not a predicate inversion. v1's "every primitive already exists" +claim is withdrawn. + +Two further corrections to v1's model of the existing router: + +- `StreamRouter` subscribes to registry global-add/global-remove only + (`audio.rs:523-560`). It does not bind nodes or watch properties. v1's risk 4 + ("pavucontrol overrides get re-fought") is **backwards** — a manual move generates no + new global-add, so the override currently wins until the node is destroyed. If the + new mode must enforce routing continuously, that is new behaviour with new loop- + prevention requirements. +- `try_flush` records *intent*, not success (`audio.rs:642-655`): it ignores + `Metadata::set_property`'s return and appends every pending ID to `routed_node_ids` + regardless. There is no link-state confirmation. Any status this feature surfaces to + the user inherits that dishonesty unless it is fixed. + +## 4. The core redesign: ownership by inherited tag, not by PID + +v1 identified peerspeak's audio by `application.process.id`. Codex showed that fails in +three separate ways, and it fails for a common reason: **PID is a property of a process, +but what we need to identify is a *stream's owner*, and peerspeak's audio is emitted by +processes and modules it does not run inside.** + +Concretely, PID-matching misses: + +1. **The AEC playback leg.** `module-echo-cancel` is loaded via pactl + (`peerspeak/src/audio/echo_cancel.rs:83-94`) and its virtual-sink→speaker playback + stream lives in `pipewire-pulse`, not peerspeak. An inverted predicate would route + *remote call audio* into the capture — the exact thing the feature removes. +2. **The media player.** `spawn_player` launches mpv/VLC as a detached child with + `kill_on_drop(false)` (`peerspeak/src/screenshare/mod.rs:761`). Excluding the + pixelpass *viewer* PID excludes a tunnel process that plays nothing. +3. **Notification sounds.** `pw-play`/`paplay`/`aplay` children (`src/notify.rs:265`). + +Plus PID is not authoritative in general: PipeWire reports `application.process.id` as +the **pipewire-pulse PID** for Pulse-emulated clients, and the property can be absent or +overridden. + +### 4.1 The proposal + +**peerspeak tags every stream it owns, at creation, and the router excludes by tag.** + +The lever that makes this cheap: every peerspeak-owned playback path is a +`Command::new(…)` spawn, so a tag can be *inherited through the environment*. + +| Owner | Mechanism | +| --- | --- | +| native call playback (`src/audio/pipewire_impl.rs:374-388`) | set the tag directly in the stream's property dict | +| mpv / VLC (`src/screenshare/mod.rs:761`) | `.env("PIPEWIRE_PROPS", …)` / `PULSE_PROP` on the `Command` | +| `pw-play` / `paplay` / `aplay` (`src/notify.rs:265`) | same | +| `module-echo-cancel` legs | pass properties at `pactl load-module`, **OPEN-Q 3** | + +This dissolves blockers 2 and 3 instead of patching them. No live PID channel, no +PID-reuse hazard, no re-evaluation of already-existing nodes when a PID is retired. + +**Proposed tag:** a single stable key, e.g. `peerspeak.owned=1`. Naming is the user's +call (§11). Verified working on this machine for all three spawned-child cases — +see §10.1. + +**The tag is a correctness mechanism, not a security boundary.** Any same-user client +can set `peerspeak.owned=1` and opt itself out of capture; a child can also sanitize its +environment. PipeWire is explicit that only `pipewire.*` properties are suitable for +security decisions. That is acceptable here — the threat model is "don't echo the +user's own call back at them," not "defend against a hostile local process." It must be +stated rather than assumed. A related hazard worth handling: a stable boolean **leaks +into grandchildren**, so anything mpv or VLC itself spawns inherits the exemption. + +### 4.2 Fail closed: eligibility, not exclusion + +Even with perfect tagging, "route everything that isn't ours" sweeps in third-party +infrastructure: other apps' loopbacks, filter-chains, virtual-sink forwarders, tunnel +sinks, combine-sinks, DSP bridges. Moving those can create cycles, bypass DSP, or break +an unrelated application's graph. + +**So the predicate is an allowlist, not a denylist.** Route a node only if it is +positively identified as ordinary end-user application playback. Anything unrecognized +is **left alone and reported as not-captured**, never optimistically moved. + +That inverts the failure mode from "we broke your audio graph" to "that app's audio +didn't make it into the share," which is recoverable and visible. + +The exact property set defining "ordinary application playback" **cannot be written +today** — it depends on what real nodes look like on this machine. Candidates to +evaluate from a `pw-dump`: `node.passive`, `node.virtual`, `media.class`, `media.role`, +`client.id` → Client object ownership, presence of a `pipewire.sec.pid`, +`node.dont-move`. **OPEN-Q 1.** + +### 4.3 The self-match, corrected + +pixelpass's own local-monitor loopback (`capture.monitor → @DEFAULT_SINK@`) has a +playback half that appears as a `Stream/Output/Audio` node. An unqualified predicate +selects it and points it back at the capture sink — an immediate feedback topology. + +v1 proposed tagging it via `source_output_properties=`. **That is the wrong half.** +`source_output_properties` applies to the capture/source-output leg; the self-matching +node is the sink-input/playback leg. Correct form: + +``` +sink_input_properties=node.name=pixelpass_local_monitor_ sink_dont_move=true +``` + +`sink_dont_move=true` is defense in depth: session policy refuses the move even if the +predicate regresses. Do **not** fall back to matching a generic `loopback.*` shape — +that either catches unrelated loopbacks or misses a second pixelpass host's. + +Whether `sink_input_properties=node.name=…` actually surfaces on the registry node on +the deployed PipeWire build is **OPEN-Q 2**. Whether WirePlumber rejects the cycle or +permits audible runaway feedback if the predicate does regress is **OPEN-Q 5**. + +## 5. Topology (Option A, still preferred — for a corrected reason) + +New host mode. Name TBD (§11); referred to here as *routed-desktop*. + +``` + eligible app streams ────► pixelpass_capture_ ──► gst capture ──► viewers + (routed by StreamRouter) │ + └─ local-monitor loopback ──► @DEFAULT_SINK@ ──► speakers + ▲ + peerspeak-tagged playback ──────────────────────────────────────────────┘ + (never routed; AEC, call, mpv, notifications) +``` + +- Capture null-sink as today. +- **No** `@DEFAULT_SINK@.monitor → capture` loopback, ever, in this mode — that + loopback *is* the echo. Unlike best-effort per-app mode there is no + fallback-to-desktop path to oscillate. +- Local monitor created **synchronously, before the first target move**, tagged and + `sink_dont_move` per §4.3. + +**Option B (make a null-sink the system default) is still rejected — but v1's reason +was wrong.** v1 argued a pixelpass crash leaves the desktop silent. Codex is right that +this is unproven: pactl modules are owned by `pipewire-pulse` and *persist* past a +pixelpass crash rather than vanishing, and WirePlumber selects a best-available sink +when a configured default is absent. + +The real reason to reject B on this machine: it hijacks the global default sink, which +directly conflicts with the user's `Ctrl+Meta+F` / `Ctrl+Meta+S` output-switch scripts. +B would have to either fight the user's device choice or let new streams silently bypass +the share. It also mutates system-wide state for unrelated clients and needs +saved-default restoration plus idempotent crash repair. + +B's genuine advantages, recorded honestly: new apps land in the mix automatically, no +registry-add race, and no sweeping up of arbitrary module playback legs. + +## 5.1 Option C — copy instead of move (Codex's proposal; now the leading candidate) + +Codex's round-2 review made the strongest architectural point either review has +produced: **the design is overbuilt because it *moves* audio that pixelpass only needs +to *copy*.** + +PipeWire permits a node's output ports to link to more than one sink. So instead of +reassigning `target.object` and dragging every desktop stream off the user's speakers, +pixelpass creates a *second*, pixelpass-owned link from each eligible playback stream to +the capture sink, leaving the existing speaker link untouched: + +``` + app output ──────────────────► existing hardware / filter path (untouched) + └── pixelpass-owned link ──► capture sink ──► viewers +``` + +If those links are created with `object.linger=false`, they are owned by the pixelpass +connection and **vanish when the process dies** — SIGKILL stops being catastrophic. + +This deletes, rather than solves, most of §6–§8: + +| Problem in the move-based design | Under copy | +| --- | --- | +| local-monitor loopback | not needed — audio never leaves the speakers | +| added playback latency for the sharer | gone | +| output-device switch mid-share (§8.2) | gone | +| prior-target capture/restore (§7) | gone — nothing is retargeted | +| pavucontrol conflicts | gone | +| two hosts fighting over `target.object` (§8.3) | gone — links are independent | +| stranded desktop audio after SIGKILL (§6) | gone with `object.linger=false` | + +It does **not** remove the eligibility/ownership problem (§4.2) — pixelpass still has to +decide which streams to fan out. But the failure mode of a wrong decision drops from +"we rewired or stranded your desktop" to "that stream wasn't captured," which is exactly +the posture §4.2 is reaching for. + +**Known risks:** `port.exclusive` and passthrough streams may refuse fan-out; and the +open question is whether **WirePlumber tears down foreign links** it did not create as +part of its own policy management. That is the one thing that would kill Option C, and +it is cheap to test. + +**Recommendation: a direct-link feasibility spike before any further work on Option A.** +If fan-out holds on this graph, Option C supersedes Option A outright. + +## 6. Graceful stop is a prerequisite, not a test case + +**Stop Share is currently `SIGKILL`.** `peerspeak/src/core/mod.rs:698` and `:3479` call +Tokio `Child::kill()`, which on Unix is SIGKILL-plus-wait. So `Routing::cleanup`, +`Cmd::Shutdown`, and `Drop` **never run on the normal stop path** — v1 treated this as +an exotic crash scenario when it is the ordinary button. + +Today that strands one app. Under routed-desktop it would strand *every desktop stream* +pointing at an orphan capture sink, on every single stop. This alone makes the feature +unshippable without a lifecycle change. + +Required, in order: + +1. **A graceful control path in peerspeak** — SIGTERM (or a stdin command) with a + bounded timeout, escalating to SIGKILL only on failure to exit. +2. **pixelpass handling that signal** to run `Routing::cleanup` — currently there is no + signal handler on this path. **OPEN-Q 7.** +3. **Cleanup that confirms rather than assumes** — the current path writes + `target.object` clears without inspecting `set_property`'s result or waiting for a + core sync/done before quitting (`audio.rs:494-512`). +4. **`--repair` extended** to understand this mode. Its current scope is dead-PID + modules only (`pixelpass/src/repair.rs:15-63`); it must also clear or restore + routing state and stay safe with a second live host. + +## 7. Restore, don't clear + +The router sets `target.object` to `None` on shutdown, which returns a stream to +following the default. Any stream the user had *deliberately pinned* to another device +loses that pin. In per-app mode that was one app; here it is the whole desktop. + +The prior value must be captured before the move and restored on cleanup. This is state +the current router does not keep. + +Related: `node.dont-move=true` streams will not move at all, and `node.dont-reconnect` / +`node.dont-fallback` streams may error or die rather than fall back when the capture +sink vanishes. The predicate must detect these and report per-node outcome instead of +counting requested moves as routed. **OPEN-Q 4.** + +## 8. Remaining risks + +1. **Latency now applies to everything.** All desktop audio traverses + capture-sink → loopback → hardware at `latency_msec=20`. That is a real regression + for rhythm games and monitoring, paid by the sharer. Whether 20 ms is right for this + mode, or whether `node.latency` should be driven lower, is open. +2. **Output-device switching mid-share — release-blocking.** The local monitor resolves + `@DEFAULT_SINK@` once at module load (`audio.rs:139-151`) with no default-metadata + listener. Switching output mid-share strands the sharer's *entire routed desktop* on + the old device. **This is a genuine pre-existing defect that already hurts per-app + mode**; this feature raises its blast radius from one app to everything. Must ship + with a tested retarget/reload — or an enforced "restart the share after switching + output" limitation. +3. **Two hosts cannot coexist.** Both routers write the same `target.object` key on + every eligible stream; last writer wins, and stopping one can clear the other's + target. Either serialize routed-desktop hosts behind a per-user lock with a clear + error, or build ownership/generation tokens. The exclusion namespace must cover + *all* pixelpass-owned loopbacks, not just this host's. +4. **"What the sharer hears" is not what viewers get.** The capture sink sees stream + volume but not the hardware sink's mute/volume, per-device DSP, or spatial + processing. A sharer can mute their speakers and viewers still receive full-level + audio. UX wording must be "eligible desktop application audio," not "everything you + hear." +5. **Startup race.** `try_flush` acts only once both sink serial and `default` metadata + are bound; before that, streams are unrouted — viewers get silence, which is the safe + direction, but the front-end should say so. Note `Routing::start` returns before any + readiness acknowledgement and router-thread failure is only logged + (`audio.rs:439-446`), so the host can report success while viewers get permanent + silence. +6. **Idle nodes are not gone nodes.** `handle_global_remove` fires only on global + destruction (`audio.rs:601-620`), so a paused app stays counted as routed. State + events based on it are approximate. +7. **Stale prerequisites.** The router cannot rebind `sink_serial` / `default_metadata` + if either disappears, so a pipewire-pulse or WirePlumber restart leaves it alive and + inert. +8. **Pre-existing type bug.** `object.serial` is 64-bit in PipeWire but parsed as `u32` + (`audio.rs:534-540`). Global IDs are reused; serial is the recommended stable + identifier. Worth fixing before the router grows. + +## 9. Capability negotiation + +v1 cited `src/core/messages.rs:503` as capability negotiation. It is not — that file +only defines the `AudioAppsListed` UI event. Actual detection is a `pixelpass --help` +substring probe (`src/screenshare/mod.rs:245-278`, invoked at `src/core/mod.rs:3368`). + +A help-token probe is an acceptable continuation of the existing pattern, but this +feature must **not** overload `app_audio_supported: bool`. Strict per-app and +routed-desktop are independent capabilities; a pixelpass build may have one and not the +other. Model them separately (enum or bitset), name the exact new probe token, and +define the fallback UX when it is absent. + +## 10. What must be measured before implementing + +**Nothing in §4.2's predicate can be finalized without this.** A `pw-dump` of this +machine with peerspeak in a call (AEC on), mpv playing a viewed share, a notification +sound firing, and representative browser/game streams: + +| # | Question | Resolves | +| --- | --- | --- | +| OPEN-Q 1 | What properties distinguish ordinary app playback from infrastructure legs? | §4.2 predicate | +| OPEN-Q 2 | Does `sink_input_properties=node.name=…` surface on the registry node? | §4.3 tag | +| OPEN-Q 3 | Can `module-echo-cancel` legs carry a tag from `pactl load-module`? If not, what identifies them? (node names are PID-stamped: `echo_cancel.rs:80-82`) | §4.1 row 4 | +| OPEN-Q 4 | Real fallback behaviour per stream class when the capture sink vanishes — graceful vs SIGKILL | §7 | +| OPEN-Q 5 | Does WirePlumber reject the capture-monitor cycle, or is it audible feedback? | §4.3 | +| OPEN-Q 6 | Does `PIPEWIRE_PROPS` / `PULSE_PROP` env tagging actually land on mpv/VLC/pw-play nodes? | **✅ RESOLVED POSITIVELY — measured, see below** | +| OPEN-Q 7 | Does pixelpass currently handle SIGTERM at all on the host path? | **✅ RESOLVED: no** — only `tokio::signal::ctrl_c()` (SIGINT), `pixelpass/src/common/signal.rs:6`. Use SIGINT, not SIGTERM, for the graceful path in §6 | +| OPEN-Q 8 | `@DEFAULT_SINK@` behaviour with the user's real hotkey scripts, old sink still present | §8.2 | + +### 10.1 OPEN-Q 6 — MEASURED 2026-07-20 on this machine + +Codex's round-2 review asserted this was "resolved negatively — environment inheritance +is not a backend-independent ownership contract; the redesign collapses as written," +on the grounds that `PULSE_PROP` is undocumented and "absent from the installed libpulse +binaries." + +**That is factually wrong and the experiment disproves it.** `PULSE_PROP` and +`PULSE_PROP_OVERRIDE` are both present in `/usr/lib/pulseaudio/libpulsecommon-17.0.so` +(where `pa_proplist_update_from_environment` lives; `libpulse.so.0` links it), and the +tag reaches the graph for every player peerspeak actually spawns: + +| Client | Backend | Env used | `peerspeak.owned` on the node | +| --- | --- | --- | --- | +| `paplay` | Pulse-emulated | `PULSE_PROP` | **`1`** ✅ | +| `mpv` | default ao | `PULSE_PROP` + `PIPEWIRE_PROPS` | **`1`** ✅ | +| `vlc` (LibVLC 3.0.23) | `client.api=pipewire-pulse` | `PULSE_PROP` | **`1`** ✅ | + +Method: play a 12 s 48 kHz tone under each client with the env set, then +`pw-dump | jq` the `Stream/Output/Audio` nodes. VLC on this box has no native PipeWire +aout (`/usr/lib/vlc/plugins/audio_output/` = dummy, file, alsa, amem, pulse) — Codex is +right about that — but it is irrelevant, because its Pulse output goes through libpulse +and `PULSE_PROP` is honoured there. + +**So §4.1's ownership model stands for the three spawned-child cases.** Codex's *other* +objection to it — that the tag is client-controlled and spoofable, so it is a +correctness mechanism and explicitly **not** a security boundary — is correct and is +now stated as such in §4.1. + +**Also measured, and it corrects both prior reviews:** `application.process.id` on the +`paplay` node was **paplay's own PID (192571)**, not pipewire-pulse's. Codex asserted +the pipewire-pulse claim in round 1, then retracted it in round 2; the retraction is +right. The server-PID warning belongs to `pipewire.sec.pid` (which was *absent* on +these nodes). PID remains untrusted and may be missing or overridden, so it stays a +fallback signal — but it was dismissed too aggressively in v1 and v2. + +**Still genuinely open: OPEN-Q 3, the AEC playback leg**, which is the one owned-audio +path that cannot inherit an environment tag because pactl loads it inside +`pipewire-pulse`. Codex reports the Pulse-compat `module-echo-cancel` exposes only +`sink_properties` (populating the virtual sink, not the playback stream), so the options +are: load the *native* module instead, correlate the four legs by module/group identity, +or redesign the AEC target. **Not yet verified on this machine** — verifying it requires +loading the module on the live graph. + +## 11. Naming — the user's call + +Everything here is a placeholder. Candidates for the pixelpass mode: +`--audio-mode=desktop-routed`, `--audio-mode=desktop-excluding`. Internal selector: +`--exclude-audio-tag=`. Do **not** surface `--exclude-pid` — it names an +unreliable mechanism and reads like process control. + +peerspeak picker wording: something like "System audio except peerspeak," with a note +that call and watched-share playback are excluded. Whether "All system audio" stays +alongside it (with a stated echo risk) or is replaced when the capability is present is +a product decision. + +## 12. Testing + +Pure and unit-testable, PipeWire at the edges, per house style: + +- `fn eligibility(node_props, &Ownership) -> Eligibility` — allowlist semantics, with + `NotEligible { reason }` so status can explain itself. Cases: peerspeak tag present; + AEC leg; own local-monitor node name; another host's local monitor; `dont-move`; + non-`Stream/Output/Audio`; missing props; the capture sink itself. +- Pure module-argument construction for both loopbacks, asserting the §4.3 tag and + `sink_dont_move`. +- Pure prior-target capture/restore logic (§7). +- Pure capability-probe parsing (§9). + +Field tests — the only thing that can prove a viewer does not hear themselves: + +1. Sharer in a call while sharing, AEC on **and** off. +2. Sharer simultaneously *viewing* another share while sharing. +3. Lifecycle, each separately: Stop button, room leave, UI crash, pixelpass panic, + SIGTERM, SIGINT, SIGKILL, last-viewer disconnect, pipewire-pulse restart, + `pixelpass --repair`. +4. Output-device switch mid-share via the real hotkey scripts. +5. Two concurrent hosts (second pixelpass CLI, second peerspeak instance). +6. A notification sound firing mid-share. +7. Sample-rate/channel/passthrough behaviour on real sinks; CPU and latency cost of + routing the whole desktop. + +## 13. Open decisions for the reviewer + +1. Is tag-inheritance (§4.1) sound as the ownership contract, and is OPEN-Q 6 the right + thing to gate the whole design on? +2. Is allowlist-eligibility (§4.2) the right risk posture, given it means some app audio + silently won't be shared until its shape is recognized? +3. Is the graceful-stop work (§6) properly a prerequisite commit, or does it land inside + this feature? +4. Should the two pre-existing defects (§8.2 output switching, §8.8 `object.serial`) + be split into their own tasks ahead of this? +5. Is there a materially simpler design that meets Joe's ask that both reviews have + missed?