docs: v3.4 audio-exclusion — CONVERGED, ready for implementation planning
Round 7. Codex ratifies: v3.3 is ready to become the implementation plan. Seven rounds, every blocker fixed or refuted with evidence. Design approval only — nothing approved for merge, no code written. Two subtle catches from the ratification round, both applied: - The owner-key union had a wording trap that would have preserved the exact bug it was written to fix. "Resolves" must mean "yields a MATCH between the two legs", not "first property present on the node" — client.id IS present on both gst-launch legs but differs, so a first-present implementation stops at key 3, sees a mismatch, concludes "different owners" and leaks. Now specified as try-in-order-until-equal, with a dedicated test. - Sticky taint must be lifetime-aware, not keyed on raw ids. client.id, node ids, module indices, link-groups and PIDs all recycle on this stack, so a bare key would hand an unrelated future app permanent inherited taint. Stored against live owner components, cleared only when all members vanish. Also added: how pixelpass learns the pipewire-pulse PID itself (consistent pipewire.sec.pid across Pulse clients, validated against /proc/<pid>/comm), with the failure modes in both directions — safe only because unresolved ancestry is fail-closed, which is the invariant the section rests on. NEW LIVE FINDING (§6.1.0), the strongest reachability evidence yet and one Codex's sandbox could not have seen: the user's CURRENT DEFAULT SINK is sink-sunshine-stereo, a support.null-audio-sink. Every hardware sink is SUSPENDED; the only RUNNING sink is Sunshine's virtual one, with Firefox playing into it and sunshine reading its monitor. The hazardous forwarder topology is live in the default audio path full time, with no EasyEffects involved. It also means the rejected hardware-sink-only shortcut would have captured NOTHING on this machine. Flagged separately, explicitly UNVERIFIED: what module-echo-cancel binds to when the default sink is an app-owned null sink. §12 expanded with a graph-engine test surface (node-local tests cannot catch C2/C3-class defects). §14 rewritten: convergence table, agreed v1 scope, and what is deliberately out. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -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/<pid>/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:<idx>` · 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.
|
||||
|
||||
Reference in New Issue
Block a user