diff --git a/docs/screenshare-audio-exclusion-plan.md b/docs/screenshare-audio-exclusion-plan.md index 44d1295..ab3b156 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.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) +**Status:** 🟢 **v3.4 — CONVERGED after 7 review rounds. Ready for implementation planning; NOT approved for merge.** +**Date:** 2026-07-21 (v1: 07-19 · v2: 07-20 · Option C 07-20 · v3.1 r4 · v3.2 r5 · v3.3 r6 · v3.4 r7) **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. @@ -302,7 +302,45 @@ If any node re-emits audio it received, the re-emitting node is a fresh, *untagg `Stream/Output/Audio` carrying the mix — including peerspeak's playback and the AEC leg, both of which were correctly excluded one hop earlier. -The canonical instance: **EasyEffects.** When it is running, applications play into +### 6.1.0 🔴 The hazard is LIVE on this machine right now — not hypothetical + +Discovered 2026-07-21 while measuring something else. **The user's current default sink is +not hardware.** `pactl info` reports: + +``` +Default Sink: sink-sunshine-stereo ← factory.name = support.null-audio-sink +``` + +All three real hardware sinks (`Arctis_1_Wireless`, `pci-…analog-stereo`, +`…hdmi-stereo-extra3`) are **SUSPENDED**; the only `RUNNING` sink is Sunshine's virtual +one. The live graph is: + +``` +Firefox:output_{FL,FR} ──► sink-sunshine-stereo:playback_{FL,FR} (a null sink) + sink-sunshine-stereo:monitor_{FL,FR} ──► sunshine:input_{FL,FR} +``` + +That is **exactly the forwarder shape** this section is about, active in the default audio +path, with no EasyEffects involved. Consequences: + +1. **Reachability is settled.** Earlier rounds argued from "EasyEffects is installed"; + the real machine is already running a virtual-sink forwarder topology full time. + peerspeak's own playback would land in `sink-sunshine-stereo` and its monitor. +2. **§6.5's "hardware-sink-only" shortcut wouldn't just leak — it would capture + NOTHING here**, because no application links to a hardware sink at all. +3. Sunshine happens to have **only** a `Stream/Input/Audio` leg (id 168) — it encodes and + sends over the network rather than re-emitting locally — so it is not itself a fan-out + leak source. The topology, not this particular app, is the point. +4. §10 item 2 (`@DEFAULT_SINK@` resolved once) is worse than described: the default sink + here is a *transient app-owned null sink* that appears and disappears with Sunshine. + +⚠️ **Separate, UNVERIFIED question this raises for peerspeak itself** (not this feature): +`echo_cancel::enable` passes `sink_master` only when the user has picked a device, else it +binds to system defaults (`echo_cancel.rs:83-94`). With the default sink being Sunshine's +null sink, what does the AEC bind to, and does call audio still reach the user's speakers? +**This is a question, not a finding — do not repeat it as a bug until it is tested.** + +The canonical *installed* instance: **EasyEffects.** When it is running, applications play into `ee_sink` and EasyEffects emits a single combined output stream to the hardware. That output leg has no `peerspeak.owned` tag and no `pulse.module.id` matching our AEC, so **both §5.1 and §5.2 pass it**, and fanning it out re-injects the entire call into the @@ -382,8 +420,15 @@ an app is free to open one connection per stream, and GStreamer does exactly tha 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: +**Replacement: a conservative union of owner keys, strongest first.** + +⚠️ **Wording trap — "resolves" means "yields a MATCH between the two legs," NOT "is the +first property present on the node."** A naive first-present implementation reproduces the +exact bug: in the gst-launch case `client.id` **is present on both legs** (209 and 210), so +first-present stops at key 3, finds the values differ, and concludes "different owners — +not bridged." The leak survives. The rule is: *try each key in order; a key resolves only +if both legs carry it and the values are equal; otherwise fall through to the next key.* +This must be an explicit unit test (§12). | # | key | scope | notes | | --- | --- | --- | --- | @@ -406,6 +451,23 @@ 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. +**How pixelpass learns the pipewire-pulse PID** (needed for the key-4 exception). It must +derive this itself; it cannot assume a value, and peerspeak can supply only a *hint*: + +- Read `pipewire.sec.pid` from the **Client** objects of Pulse-emulated streams. Measured: + it is `2541` for Firefox, Steam, KDE Connect, sunshine and libcanberra alike, while each + node's own `application.process.id` differs (Firefox `11114`, sunshine `4119`). +- Require a **single consistent** value across those clients, and validate it by reading + `/proc//comm` (or cmdline) and confirming it is `pipewire-pulse`. +- "This PID owns implausibly many unrelated streams" is a **diagnostic**, never + correctness logic. + +Failure modes: if pixelpass fails to identify the real pipewire-pulse PID, the result is +broad **over-exclusion** (annoying, safe). If it wrongly suppresses a genuine app PID, the +result is over-exclusion **for that app** — safe *only* because unresolved ancestry is +fail-closed. If unresolved ancestry were ever implemented fail-open, both of these become +leaks. That is the invariant holding this whole section up. + ### 6.1.1 ⚠️ Bridge taint must be conditional, or it over-excludes badly The owner bridge must propagate taint **only when the input leg is itself tainted.** @@ -452,6 +514,17 @@ tainted it stays tainted until its client/output nodes disappear. Re-eligibility teardown, not a topology change. A timed drain is strictly weaker and would need measurement to justify; do not ship one in v1. +⚠️ **Stickiness must be lifetime-aware, never keyed on a raw recyclable id.** `client.id`, +node ids, module indices, `node.link-group` values and PIDs **all recycle on this stack** — +measured directly for module indices and node ids (§5.2 correction 3), and the same +`link-group` string came back verbatim across an unload/reload. If sticky taint were stored +against a bare key, a later unrelated app inheriting that recycled id would inherit the +taint and be silently excluded forever. + +Store stickiness against a **live owner component** — the concrete set of node/client +objects observed to form that owner — and clear it only once **all** member objects have +disappeared. A key that reappears after full teardown is a new owner and starts clean. + 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. @@ -762,6 +835,23 @@ Pure, unit-testable seams with PipeWire at the edges, per house style: - Link bookkeeping: per-port link set, "captured" only at all-`ACTIVE`, idempotent re-enumeration, proxy retention/drop. +**⚠️ Node-local eligibility tests are NOT sufficient.** The C2/C3-class defects all live in +the graph engine, so the taint engine needs its own pure test surface, fed synthetic +Node/Port/Link/Client fixtures: + +| test | catches | +| --- | --- | +| GStreamer **split clients** (two `client.id`s, one `application.process.id`) are bridged | the C2 refutation | +| the key union **falls through** a present-but-unequal key (`client.id` differs ⇒ try key 4) | the §6.1.2 wording trap | +| the pipewire-pulse PID does **not** bridge unrelated modules | mass over-exclusion | +| a module forwarder with neither `link-group` nor `pulse.module.id` ⇒ **unresolved ⇒ excluded** | fail-closed invariant | +| sticky taint **survives** the tainted input leg unlinking/disappearing while the output leg lives | the C3 buffered-audio defect | +| sticky taint **clears** once every owner member object is gone | over-exclusion forever | +| a **recycled** `client.id` / module index / node id / `link-group` does **not** inherit taint | §6.1.3 lifetime-awareness | +| the readiness epoch blocks stale or unresolved link decisions | §6.4 | +| taint crosses `app → sink → monitor-reader` at node granularity | §6.1 edge type 2 | +| an `Audio/Duplex` node over-taints (asserted as *known accepted* behaviour, so a future fix is a deliberate change) | §6.1 caveat | + Field tests — the only thing that can prove a viewer does not hear themselves: 1. Sharer in a call while sharing, **AEC on and AEC off**. @@ -805,20 +895,45 @@ Both reviewers agree on all seven. Recorded as decided; reopen only with new evi - **D7 — no materially simpler design exists** that still meets Joe's ask. The available simplification is to *narrow v1 scope*, not to change architecture. ✅ -## 14. Readiness — 🔴 BLOCKED (round 4), unblock set +## 14. Readiness — 🟢 CONVERGED (round 7). Ready for implementation planning. -v3.0 was reviewed and blocked; v3.1 (this revision) applies the three required rewrites. -The minimum unblock set was: +**Both reviewers agree v3.4 is ready to become the implementation plan.** Seven rounds; +every blocker raised has been either fixed or refuted with evidence. This is *design* +approval — **nothing is approved for merge**, and no code has been written. -1. **§6 rewritten around transitive graph eligibility + dynamic link revalidation** — - applied in §6.1–6.3. This is the round's biggest finding: eligibility is a graph - property, and **EasyEffects is installed on the target machine**, so the leak is - reachable, not theoretical. -2. **§5.3 rewritten as a bounded AEC validation state machine with explicit `--aec`** — - applied. -3. **§7.2's field-order fix replaced with explicit shutdown + a real regression guard** — - applied. +How the blockers closed: -**Next step: a fifth round re-reviewing v3.1**, focused on whether the graph-ancestry -predicate in §6.1 is actually implementable against the `pipewire` crate's Link/Port -globals, and on the cost of the sync barrier in §6.3. Nothing here is approved for merge. +| round | blocker | outcome | +| --- | --- | --- | +| 4 | eligibility is node-local | **fixed** — §6 rewritten as graph taint | +| 4 | one-shot AEC validation | **fixed** — §5.3 bounded state machine | +| 4 | field reorder insufficient | **fixed** — §7.2 explicit shutdown + ordering test | +| 5 | Link-only walk misses forwarders | **fixed** — §6.1 three edge types, measured | +| 5 | monitor edge needs synthetic modelling | **refuted** — free at node granularity (C1) | +| 6 | `client.id` is not an owner key | **refuted my own claim** — §6.1.2 owner-key union | +| 6 | current-only taint forgets buffers | **conceded** — §6.1.3 sticky taint | +| 7 | "resolves" = first-present would still leak | **fixed** — §6.1.2 wording trap + test | +| 7 | stickiness on recyclable ids | **fixed** — §6.1.3 lifetime-aware owner components | + +### v1 scope — agreed + +Option C fan-out · explicit `--aec=off|pulse-module:` · peerspeak playback and child +tagging · exact AEC module validation · graph taint with the owner-key union and the +pipewire-pulse PID exception · sticky taint · readiness epoch · fail-closed unresolved +ancestry · owned non-lingering links · §10 items 1, 4 and 5 landed first. + +### Deliberately OUT of v1 + +Node-granular taint only (no port modelling, duplex over-taint accepted) · no timed drain · +no hot-AEC-reload epoch protocol · no native PipeWire AEC support · no incremental +dirty-set (full recompute) · no "implausibly many streams" heuristic as correctness · +pipewire-pulse/PipeWire daemon restart handled as **revoke and stop**, not seamless +recovery. + +### Next step + +Turn this into a sequenced implementation plan (prerequisites §10 items 1/4/5 first, then +the taint engine behind its unit-test surface, then the link manager, then the peerspeak +tagging and `--aec` plumbing), and **field-test §12** — the rig upgrade in §9.2 is still +owed, and nothing in this design has been tested over the real GStreamer/AAC/network path +or on two machines.