diff --git a/docs/screenshare-audio-exclusion-plan.md b/docs/screenshare-audio-exclusion-plan.md index 223c74a..44d1295 100644 --- a/docs/screenshare-audio-exclusion-plan.md +++ b/docs/screenshare-audio-exclusion-plan.md @@ -1,7 +1,7 @@ # Design v3: whole-desktop screen-share audio without self-echo -**Status:** v3.2 — round-5 measurements applied; **awaiting round-6 re-review** (§14). -**Date:** 2026-07-21 (v1: 07-19 · v2: 07-20 · Option C 07-20 · v3.1 round 4 · v3.2 round 5) +**Status:** v3.3 — round-6 blockers resolved; **awaiting round-7 ratification** (§14). +**Date:** 2026-07-21 (v1: 07-19 · v2: 07-20 · Option C 07-20 · v3.1 r4 · v3.2 r5 · v3.3 r6) **Origin:** Joe's suggestion — "whitelist all audio except audio coming from peerspeak." **Scope:** a new capture mode in pixelpass (`src/host/pipeline.rs`, `src/host/audio.rs`), playback tagging + AEC-identity export + teardown-ordering invariants in peerspeak. @@ -353,12 +353,58 @@ So the signal graph needs **three** edge types: `app → sink → monitor-reader` with no synthetic edge. A **port**-granular walk *does* need an explicit rule (inputs feed `port.monitor=true` outputs); a node-granular one does not. **Use node granularity for taint; use ports only for link creation.** -3. **Owner-bridge edges** — the intra-process hop the graph cannot see. Bridge on - `node.link-group` when present, else `client.id`. ✅ Measured: `client.id` is shared - by the forwarder's two legs (`107`) and is distinct per application (Firefox `96`, - Steam `82`, loopback `107`), so it is a usable fallback where `node.link-group` is - absent — which is the case for ordinary apps (OBS, browsers, recorders) that read a - monitor and re-emit, since only PipeWire *modules* set `link-group`. + ⚠️ **Accepted cost — duplex over-taint.** Node granularity smears taint across + multi-role nodes: an `Audio/Duplex` node whose playback side is tainted will have its + capture side treated as tainted too. That is **fail-closed** (over-exclusion, not a + leak) and is accepted for v1, but it can contradict the "Firefox with a microphone + stays shareable" promise in §6.1.1 on a duplex device. Port-modelling `Audio/Duplex` + and unknown-role nodes is the fix if this bites in field testing. +3. **Owner-bridge edges** — the intra-process hop the graph cannot see. This one is + genuinely hard; see §6.1.2. A single key does **not** work. + +### 6.1.2 ⚠️ The owner bridge — no single key works (measured refutation) + +**I claimed `client.id` was the fallback bridge key for apps without a `link-group`. +I disproved my own claim; Codex independently reached the same verdict.** + +Test: one `gst-launch-1.0 pulsesrc device=fable_src.monitor ! audioconvert ! pulsesink +device=fable_dst` process — one PID, an input leg and an output leg, the exact +OBS/recorder/forwarder shape, and the same framework pixelpass itself uses. + +| leg | `client.id` | `node.link-group` | `application.process.id` | +| --- | --- | --- | --- | +| `Stream/Input/Audio` | **209** | null | 20172 | +| `Stream/Output/Audio` | **210** | null | 20172 | + +**Two distinct client objects for one process**, both named `gst-launch-1.0`, both with +`pipewire.sec.pid=2541`. So `client.id` bridges *a client connection*, not *an owner* — +an app is free to open one connection per stream, and GStreamer does exactly that. The +earlier "libcanberra appears as clients 74 and 77" observation was the same signal and I +under-weighted it. + +**Replacement: a conservative union of owner keys, strongest first**, taking the first +that resolves: + +| # | key | scope | notes | +| --- | --- | --- | --- | +| 1 | `node.link-group` | per module/filter instance | precise; set by PipeWire modules (loopback, echo-cancel, filter-chain) | +| 2 | `pulse.module.id` | per pactl module | precise; module-created streams only | +| 3 | `client.id` | per **connection** | correct when an app uses one connection; insufficient alone | +| 4 | `application.process.id` (on the **Node**) | per process | ✅ the only key that bridged the gst-launch legs (`20172` on both) | +| — | else | — | **unresolved ⇒ fail closed** (exclude the output leg) | + +⚠️ **Key 4 has a trap that must be coded explicitly.** For *module-created* streams, +`application.process.id` is **pipewire-pulse's own PID** (§5.2 correction 5). Bridging on +it would fuse *every* Pulse module's legs into one owner, so a single tainted module input +would exclude every module-created stream on the box — mass over-exclusion, and exactly +the failure correction 2 warned about (a tunnel/RTP module may be the sole carrier of +audio the user wants shared). **So: never bridge on key 4 when the value equals the +pipewire-pulse PID.** Those cases are already covered precisely by keys 1 and 2. + +Note the irony worth recording: PID returns to the design, but in a different *role*. +It is unusable as **identity** ("is this peerspeak's audio?") and workable as +**correlation** ("are these two legs the same app?") — and in the correlation role a +wrong answer fails closed rather than leaking. ### 6.1.1 ⚠️ Bridge taint must be conditional, or it over-excludes badly @@ -385,9 +431,45 @@ Result, both cases correct: | Firefox screen-sharing *with desktop audio* (input leg on a tainted monitor) | **excluded** | | EasyEffects / loopback / combine-sink forwarding a mix containing peerspeak | **excluded** | -Where a client has a tainted input leg and an output leg with **no** resolvable bridge, -fail closed and exclude the output leg. Note this only ever engages for clients actually -reading a tainted monitor, so the blast radius is small. +Where an owner has a tainted input leg and an output leg with **no** resolvable bridge +(§6.1.2), fail closed and exclude the output leg. This only ever engages for owners +actually reading a tainted monitor, so the blast radius is small. + +### 6.1.3 ⚠️ Taint must be STICKY — current topology is not enough + +**Conceded to Codex in round 6; my "conditional bridge" was correct about topology and +wrong about time.** Taint computed from the *current* graph forgets **buffered audio**. + +Failure: an app reads a tainted monitor, buffers or delays (a recorder with a 5 s ring +buffer, a DAW with latency compensation, anything doing lookahead), and then its input leg +untaints or disappears — the user stops sharing to it, the stream corks, the app closes +the capture. A purely topological recompute now sees a clean input, un-taints the owner, +and pixelpass **relinks the output leg while it is still emitting peerspeak audio from +the buffer.** No graph event marks the moment the buffer drains. + +**Rule: taint is sticky per owner for the duration of the share.** Once an owner is +tainted it stays tainted until its client/output nodes disappear. Re-eligibility requires +teardown, not a topology change. A timed drain is strictly weaker and would need +measurement to justify; do not ship one in v1. + +The cost is that an app which *once* read a tainted monitor stays unshared for the rest of +the share. That is the right trade — it is silence for one app, versus echo for everyone. + +### 6.1.4 The transition window is real but bounded + +Taint can also *arrive*: a peer joins and speaks, or the AEC loads, mid-share. Recompute +on every graph event and dropping links for newly tainted nodes is **necessary but not +zero-leak**, because a topology change races audio already in flight. + +Order of magnitude on this box: a graph quantum is ~`512/48000`–`1024/48000` ⇒ **≈10.6–21.3 +ms**, plus main-loop scheduling. App-internal buffers are unbounded in principle. So the +worst case is a sub-quantum-to-tens-of-ms audible sliver at the transition. + +⚠️ Mitigating structure: if peerspeak's playback **roots already exist** in the graph and +merely become non-silent (the common case — the AEC and call playback nodes are created at +join, long before anyone speaks), then topological taint has **already** excluded +everything downstream and there is no window at all. The window exists only when a taint +root is *newly created* mid-share. That makes AEC-load-mid-share the case to test. This means pixelpass must **track Node, Port and Link globals** — not just node global-add, which is all the existing router does (`pixelpass/src/host/audio.rs:523-584` @@ -438,17 +520,23 @@ An add-only listener never sees it. Required: `pipewire` crate **0.9.2** (`pixelpass/Cargo.toml:48`, `Cargo.lock:4121`). -- **Endpoint props fast path.** Registry `Link` globals carry all four endpoint keys - (`/usr/include/pipewire-0.3/pipewire/keys.h:256`), and they were observed present on - every live Link. **Residual doubt:** the observation was via `pw-dump`, which binds - objects — so "present on the object" is measured, "always present in the *global-add* - props" is not strictly proven. Cheap resolution: log the raw global-add props once in - the implementation spike. -- **Fallback if a prop is missing:** bind that Link, install an info listener, read the - endpoint ids off `LinkInfoRef` (`pipewire-0.9.2/src/link.rs:153` exposes all four - without params), then **drop the observer proxy**. Retained proxies are needed only for - *our own* fan-out links, where dropping is the ownership/lifecycle mechanism (§4.2) — - not for external links we merely observe. +- **Endpoint props are an OPTIMIZATION, not the correctness path** (settled in round 6). + Registry `Link` globals carry all four endpoint keys + (`/usr/include/pipewire-0.3/pipewire/keys.h:256`) and they were observed present on + every live Link — but the observation was via `pw-dump`, which binds objects, and + neither the headers nor the crate prove those keys are *always* present in the + **global-add** props. So: use them when present, and **rely on the bind fallback for + correctness.** A raw global-add log in the implementation spike settles current + behaviour, but the fallback must exist regardless. +- **Fallback:** bind that Link, install an info listener, read the endpoint ids off + `LinkInfoRef` (`pipewire-0.9.2/src/link.rs:153` exposes all four without params), then + **drop the observer proxy**. Retained proxies are needed only for *our own* fan-out + links, where dropping is the ownership/lifecycle mechanism (§4.2) — not for external + links we merely observe. +- **Readiness epoch.** Start fan-out only when: initial `core.sync`/`done` received **and** + every required Link/Port/Client/Node observer fallback has resolved or hit a fail-closed + timeout. Then **revalidate in the same graph epoch immediately before creating each + owned link** — a stale decision from a previous epoch is not a decision. - **Full recompute per graph event is fine for v1.** Rebuild taint by BFS/DFS from the hazard roots, O(V+E). A typical desktop graph is ~40 nodes / ~120 links; this is sub-millisecond-class in Rust. No incremental dirty-set in v1; coalesce bursts only if