Round 6. Codex disagreed with two of my three round-5 claims and was right about both; I had independently refuted one of them with a sharper test. C2 REFUTED (by my own measurement): client.id is NOT an owner bridge. One gst-launch process doing capture+playback produced TWO client objects (209 input, 210 output), no link-group, same application.process.id 20172. So client.id bridges a *connection*, not an owner, and GStreamer — the same framework pixelpass uses — splits them by default. Replaced with a conservative union, strongest first: node.link-group, owned pulse.module.id, client.id, node application.process.id, else fail closed. With a trap Codex did not flag: application.process.id is pipewire-pulse's PID for module-created streams, so bridging on it would fuse every Pulse module's legs into one owner and mass-exclude tunnel/RTP/loopback audio the user may legitimately want shared. Never bridge on that key when it equals the pipewire-pulse PID; keys 1-2 already cover those precisely. PID thus returns to the design in the CORRELATION role while remaining unusable in the IDENTITY role — and in that role a wrong answer fails closed. C3 CONCEDED: taint must be STICKY. Current-topology taint forgets buffered audio — an app that reads a tainted monitor, buffers, then closes its input leg would be relinked while still emitting peerspeak audio from the buffer, and no graph event marks the drain. Taint now persists per owner until its nodes disappear. Added §6.1.4 quantifying the arrival-side window (~10.6-21.3 ms quantum plus scheduling) and noting it is zero when taint roots already exist, which is the common case. C1 SUSTAINED with Codex's caveat: node-granular traversal is free for the monitor boundary, but over-taints Audio/Duplex nodes. Fail-closed, accepted for v1, documented as a known contradiction of the "Firefox with a mic stays shareable" promise on duplex devices. S1: endpoint props demoted to an optimization; bind-LinkInfo fallback is the correctness path. S2: readiness epoch + revalidate before each link creation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
825 lines
47 KiB
Markdown
825 lines
47 KiB
Markdown
# 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)
|
||
**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.
|
||
|
||
**Why v3 is a rewrite, not a patch.** v1 and v2 both described a design that *moves*
|
||
desktop streams onto a capture sink. That design is dead. v2's §5.1 adopted **Option C
|
||
(copy via a second owned link)** after a measured feasibility spike, and the AEC
|
||
identity gate — the one thing both reviewers agreed nothing could go ahead of — has
|
||
since passed. Roughly two thirds of v2 described problems Option C does not have.
|
||
Carrying that text forward as "retained for the record" made the live plan unreadable.
|
||
v1/v2 remain in git history at `88ad5a0` and `10203e1`.
|
||
|
||
**Review history**
|
||
| Round | Artifact | Verdict |
|
||
| --- | --- | --- |
|
||
| v1 review | `~/Documents/handoff-docs/Codex/peerspeak/review-2026-07-19-audio-exclusion-design.md` | not sound as written; 4 blockers, all verified correct |
|
||
| v2 review | `…/review-2026-07-20-audio-exclusion-design-v2.md` | blocked; produced Option C |
|
||
| fan-out spike | `~/Documents/handoff-docs/Claude/peerspeak/fanout-spike-results-2026-07-20.md` + Codex rounds 3/4 | **Option C adopted**, ratified |
|
||
| AEC identity gate | `~/Documents/handoff-docs/Claude/peerspeak/aec-playback-leg-identity-2026-07-20.md` | **🟢 gate passed**, both models agree after 2 adversarial rounds |
|
||
|
||
---
|
||
|
||
## 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. 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 **graph topology**: a node's output ports may link to more than one consumer.
|
||
|
||
So the feature is built by constructing a sink that only *eligible* streams feed, and
|
||
capturing that sink's monitor. Same observable behaviour as a filter, 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 summed at all.
|
||
|
||
**Fidelity is not claimed.** 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`).
|
||
Adding a second link also makes the source node participate in a second format/buffer
|
||
negotiation, which can perturb it even though the original link survives (§9.3).
|
||
|
||
## 3. What exists today
|
||
|
||
`setup_audio` (`pixelpass/src/host/pipeline.rs:123-142`) activates `Routing` **only** when
|
||
`--app` is set or `PIXELPASS_AUDIO_VIA_NULL_SINK` is set. peerspeak's no-app argv is
|
||
`--host --output json` (`peerspeak/src/screenshare/mod.rs:135-165`), so normal
|
||
whole-desktop capture **bypasses `Routing` entirely** and hands the real default monitor
|
||
to `pulsesrc`. This feature is therefore a genuinely new mode, not an inverted predicate.
|
||
|
||
`Routing::start` (`pixelpass/src/host/audio.rs:65-212`) today provides:
|
||
|
||
| Piece | Condition |
|
||
| --- | --- |
|
||
| `module-null-sink pixelpass_capture_<pid>` | 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@` | on `FirstRoutedStream`, unloaded on `LastRoutedStreamGone` |
|
||
|
||
Under Option C the null sink is reused, the monitor loopback is never loaded in this
|
||
mode, the local monitor is not needed at all, and `StreamRouter` is replaced by a
|
||
link manager rather than extended. Two existing defects in that file are inherited and
|
||
must be dealt with as prerequisites (§10).
|
||
|
||
## 4. Architecture — Option C: copy, don't move
|
||
|
||
pixelpass creates a *second*, pixelpass-owned link from each eligible playback stream's
|
||
output ports to the capture sink, and leaves the stream's existing route untouched.
|
||
|
||
```
|
||
app output ──────────────────────► existing hardware / filter path (UNTOUCHED)
|
||
└── pixelpass-owned link ────► pixelpass_capture_<pid> ──► gst pulsesrc ──► viewers
|
||
|
||
peerspeak-owned playback ────────► speakers only (never linked to capture)
|
||
AEC playback leg ────────────────► speakers only (never linked to capture)
|
||
```
|
||
|
||
### 4.1 Why this beats the move-based design
|
||
|
||
| Problem under move (Option A) | Under copy |
|
||
| --- | --- |
|
||
| local-monitor loopback needed to keep the sharer hearing audio | not needed — audio never leaves the speakers |
|
||
| added playback latency for the sharer | gone |
|
||
| output-device switch mid-share strands the desktop | gone — the app's own route is never touched |
|
||
| prior `target.object` capture/restore | gone — nothing is retargeted |
|
||
| pavucontrol conflicts | gone |
|
||
| two concurrent hosts fight over `target.object` | gone — links are independent per host |
|
||
| SIGKILL strands the whole desktop on an orphan sink | gone — see §4.2 |
|
||
|
||
**The decisive measurement (spike E2): destroying the capture sink mid-share left the
|
||
application playing to its speakers undisturbed.** Capture-side failure degrades to "not
|
||
captured," never to "the user's audio is broken." That is Option C's entire case, and it
|
||
is the reason a broader eligibility predicate is now acceptable (§6).
|
||
|
||
### 4.2 Ownership of links is the safety mechanism
|
||
|
||
In the Rust `pipewire` crate, **dropping a `Link` proxy destroys the object.** So:
|
||
|
||
- Links are created **non-lingering** and their proxies are **retained** for the life of
|
||
the share.
|
||
- Process death (including SIGKILL) destroys the connection, which destroys the links.
|
||
Measured: non-lingering links die on SIGKILL of the owner.
|
||
- A stream is counted as captured only once **every** required link reaches `ACTIVE`.
|
||
- On any link failure: leave the original route alone, report the stream as unsupported.
|
||
Never fall back to "capture the default monitor instead" — that fallback *is* the echo.
|
||
|
||
⚠️ **Rig gotchas that constrain how this is built and tested** (learned the hard way):
|
||
`pw-link --props object.linger=false` and `pw-cli create-link <props>` both **ignore**
|
||
linger and force it on; only `pw-link -m` yields a non-lingering link. `pw-cli
|
||
create-object` does not exist in 1.6.8 — the verb is `create-link <node> <port> <node>
|
||
<port>`. And links do **not** self-restore after a node or sink is recreated: a *live
|
||
owner* must re-link. That re-link loop is the pattern pixelpass has to implement, and
|
||
it is also what makes the daemon-restart case (§12) untested rather than free.
|
||
|
||
## 5. Ownership: identifying peerspeak's own audio
|
||
|
||
Every peerspeak-owned playback path is either a stream peerspeak itself creates, a
|
||
`Command::new(…)` spawn, or a pactl-loaded module. Three mechanisms cover all three.
|
||
|
||
### 5.1 Env-inherited tag, for spawned children — mechanism ✅ MEASURED, not yet built
|
||
|
||
⚠️ **This table lists IMPLEMENTATION SITES, not current behaviour.** None of these sites
|
||
tags anything today: `pipewire_impl.rs:374-388` sets no `peerspeak.owned` prop, and
|
||
neither the mpv/VLC spawn (`screenshare/mod.rs:768-775`) nor the notification spawn
|
||
(`notify.rs:265-272`) sets any env. The *mechanism* is measured working (below); the
|
||
*wiring* is work.
|
||
|
||
| Owner | Site | Mechanism to add |
|
||
| --- | --- | --- |
|
||
| native call playback | `src/audio/pipewire_impl.rs:374-388` | add the tag to the stream's property dict |
|
||
| mpv / VLC | `src/screenshare/mod.rs:768-775` | `.env("PULSE_PROP", …)` + `PIPEWIRE_PROPS` on the `Command` |
|
||
| `pw-play` / `paplay` / `aplay` | `src/notify.rs:265-272` | same |
|
||
|
||
Exact strings to be pinned at implementation time (§11), and each site needs a test:
|
||
native props asserted on the constructed dict, child sites asserted on the `Command` env.
|
||
|
||
Measured 2026-07-20 on this box (PipeWire 1.6.8): `peerspeak.owned=1` reached the graph
|
||
node for **paplay ✅, mpv ✅, VLC ✅**. `PULSE_PROP` and `PULSE_PROP_OVERRIDE` are both
|
||
present in `/usr/lib/pulseaudio/libpulsecommon-17.0.so`. (Codex asserted this mechanism
|
||
did not exist; it was disproved empirically. VLC has no native PipeWire aout on this box,
|
||
which is true and irrelevant — its Pulse path honours `PULSE_PROP`.)
|
||
|
||
**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, and a child can sanitize its own
|
||
environment. PipeWire is explicit that only `pipewire.*` properties are usable for
|
||
security decisions. Acceptable here: the threat model is "don't echo the user's own call
|
||
back at them," not "defend against a hostile local process." Stated, not assumed.
|
||
|
||
⚠️ **Known leak: a stable boolean is inherited by grandchildren.** Anything mpv or VLC
|
||
spawns is exempted too. Accepted for now; revisit if it bites.
|
||
|
||
### 5.2 The AEC playback leg — 🟢 gate passed, with five load-bearing corrections
|
||
|
||
`module-echo-cancel` is loaded via pactl (`peerspeak/src/audio/echo_cancel.rs:83-94`), so
|
||
its playback leg lives inside `pipewire-pulse` and **cannot inherit an env tag.** It is a
|
||
`Stream/Output/Audio` node linked straight to the speakers, so a broad selector *will*
|
||
pick it up unless explicitly excluded — and that is not a nicety:
|
||
|
||
> **Measured (3 arms, links verified in-graph before measuring, recorded via
|
||
> `parec -d <sink>.monitor` — the production path):** naive fan-out that includes the AEC
|
||
> leg copies remote-call audio into the share at **≈desktop level** (−45.0 dB vs the
|
||
> −44.5 dB desktop tone). Exact exclusion produces **−83.3 dB, matching the control floor
|
||
> to 0.1 dB.**
|
||
|
||
**What `module-echo-cancel` actually creates (RESULT 1):** four nodes, not two —
|
||
`Audio/Sink peerspeak_ec_sink.<pid>`, `Audio/Source peerspeak_ec_source.<pid>`,
|
||
`Stream/Output/Audio **echo-cancel-playback**` (passive, virtual), and
|
||
`Stream/Input/Audio echo-cancel-capture`. Codex's "Pulse-compat exposes only
|
||
`sink_properties`" concern is real about the *module args* and irrelevant to the *graph
|
||
props*.
|
||
|
||
**The identity (RESULT 2):** all four nodes carry `pulse.module.id` equal, byte for byte,
|
||
to the module index `pactl load-module` returned — the value peerspeak already stores in
|
||
`EchoCancelGuard::module_index` (`echo_cancel.rs:106-112`, a `String` validated as `u64`).
|
||
No PID guesswork, no name matching, no leg correlation.
|
||
|
||
```
|
||
exclude every node where pulse.module.id == <the index pactl returned to the live guard>
|
||
```
|
||
|
||
**⚠️ The five corrections. These are the design, not footnotes:**
|
||
|
||
1. **This is not an "identity contract."** It is an exact correlation *observed on
|
||
PipeWire 1.6.8*, not a documented API. No PipeWire source is installed on this box and
|
||
neither model had network, so whether the property is the *mechanism* pipewire-pulse
|
||
uses to reap module-created objects or a cosmetic mirror is **UNRESOLVED**.
|
||
⇒ **Runtime-validate and fail closed** (§5.3).
|
||
2. **"Has any `pulse.module.id`" is REJECTED as an exclusion rule.** Tunnel, RTP and
|
||
loopback modules may be the *only* carrier of audio the user legitimately wants
|
||
shared. **Exact equality with the owned live index — nothing weaker.**
|
||
3. **Module index and `node.link-group` are REUSED verbatim across unload/reload** (both
|
||
came back `536870919` / `echo-cancel-1974-13`), and **node IDs are recycled *and
|
||
reassigned across legs*** — id 136 was the playback leg on load 1 and the capture leg
|
||
on load 2. ⇒ **Never cache a node id. Never assume leg order. Never cache the module
|
||
index across an unload** — it is only trustworthy as "the index for the currently-live
|
||
guard," so pixelpass must be (re)told on every load.
|
||
4. **The `echo-cancel-` group prefix is hazard *detection*, not ownership.** Neither it
|
||
nor the fixed node name `echo-cancel-playback` identifies *peerspeak's* instance. A
|
||
foreign or second AEC is a product-policy question (§5.4). The better long-term answer
|
||
is the **native** PipeWire AEC module, which exposes `playback.props` and would let us
|
||
stamp our own random token directly on the playback leg.
|
||
5. **PID — the whole muddle, finally resolved** (third measurement, 2026-07-21, settles
|
||
four rounds of contradictory claims). Two *different properties on two different
|
||
object types* were being conflated:
|
||
|
||
| property | lives on | value for Pulse-emulated clients |
|
||
| --- | --- | --- |
|
||
| `application.process.id` | the **Node** | the app's **own** PID (Firefox `11114`, paplay's own) |
|
||
| `pipewire.sec.pid` | the **Client** | **pipewire-pulse's** PID — `2541` for *every* Pulse client on this box (Firefox, Steam, KDE Connect, libcanberra all identical) |
|
||
|
||
Module-created streams (the AEC) are the exception on the *node* side: their
|
||
`application.process.id` is pipewire-pulse's (`1974` at the time of that run), because
|
||
pipewire-pulse genuinely is the client. Earlier notes claiming `pipewire.sec.pid` was
|
||
"absent on these nodes" were right — it is absent from **nodes** because it is a
|
||
**Client** property. **PID-based AEC exclusion stays unusable**; PID is a fallback
|
||
hint, never identity. `client.id` — *not* PID — is the usable per-app correlation key
|
||
(§6.1 edge type 3).
|
||
|
||
Parse defensively: `pulse.module.id` renders as a JSON **number** in `pw-dump` but SPA
|
||
props are strings. `536870919 = 0x20000007`; pipewire-pulse indices start at `0x20000000`
|
||
so they fit u32 — but this sits right next to the `object.serial` u32-truncation bug
|
||
(§10.1), so compare as strings or as `u64`, never as `u32`.
|
||
|
||
### 5.3 Fail closed — validation and revocation
|
||
|
||
Because the identity is an observed correlation rather than a contract, pixelpass must
|
||
**verify it at runtime and refuse to run the mode when it cannot.** A one-shot
|
||
"enumerate at start" check is *not* good enough — it races, in both directions:
|
||
|
||
- peerspeak's `enable()` returns once `wait_for_nodes` sees the virtual **source and
|
||
sink** by name (`echo_cancel.rs:132-158`, polling `pactl list sources/sinks short`).
|
||
It does **not** wait for the playback `Stream/Output/Audio` hazard leg, which is the
|
||
node we actually need to exclude.
|
||
- pixelpass's capture spawns **lazily, on first viewer** (`pixelpass/src/host/mod.rs:300-304`),
|
||
so enumeration happens at a moment peerspeak does not control, and a refusal surfaces
|
||
*after* the ticket has been handed out.
|
||
|
||
**So validation is a bounded state machine, not a check:**
|
||
|
||
```
|
||
NotConfigured ──(--aec=off)──────────────► fan-out proceeds, no AEC exclusion
|
||
Validating ──(--aec=pulse-module:<i>)─► enumerate after a registry sync barrier;
|
||
wait up to a bounded deadline for ≥1 node
|
||
with pulse.module.id == i
|
||
Validated ──► fan-out permitted, excluding that identity transitively (§6.1)
|
||
Failed ──► deadline expired, identity never observed ⇒ NO FAN-OUT
|
||
Revoked ──► the live module identity disappeared ⇒ STOP FAN-OUT NOW
|
||
```
|
||
|
||
- **No fan-out occurs in `Validating`.** Silence is the safe direction; echo is not.
|
||
- **`Failed` is fail-closed**: report a capability failure, do not silently share.
|
||
Fall back to a mode with no echo risk, or an explicit user override.
|
||
- **Revocation is loss of the live *module* identity** — *all* nodes bearing the index
|
||
gone — not the transient absence of one playback leg, which can cork or relink. Getting
|
||
this wrong turns a normal cork into a spurious share-wide audio stop.
|
||
- On `Revoked`, **drop the link proxies**; do not keep the numeric index and hope. The
|
||
index is reused (§5.2 correction 3), so a retained stale index can alias onto an
|
||
unrelated future module.
|
||
- **Never** infer exclusion from `node.name == "echo-cancel-playback"` alone — it is a
|
||
fixed, non-unique, trivially spoofable string. Usable only as belt-and-braces *after*
|
||
the exact match, or as foreign-instance hazard detection.
|
||
|
||
### 5.4 Foreign or second AEC instances — a product decision
|
||
|
||
If a node matching `node.link-group` prefix `echo-cancel-` exists that is **not** ours,
|
||
peerspeak is not the only echo canceller on the box. Options: (a) fail closed — refuse
|
||
the mode; (b) warn and exclude all `echo-cancel-*` groups, accepting that a legitimate
|
||
unrelated AEC's output silently won't be shared. **Recommendation: (b) with a visible
|
||
warning** — the failure it prevents (echo) is worse than the failure it causes (one
|
||
app's audio missing), and it matches §6's overall posture. **Open decision D3 (§13).**
|
||
|
||
## 6. Eligibility — a broad guarded selector
|
||
|
||
Under a *move* design, mis-selection rewired the user's desktop, which forced a narrow
|
||
allowlist. Under copy, mis-selection costs at most one stream's capture — so the narrow
|
||
allowlist is out. But "fan out everything" is still wrong: it recreates self-echo, it
|
||
recreates call echo, it can build cycles, and a second link participates in format/buffer
|
||
negotiation.
|
||
|
||
### 6.1 ⚠️ Eligibility is a GRAPH property, not a node property
|
||
|
||
**This is the single biggest correction in round 4, and it is reachable on the target
|
||
machine, not hypothetical.**
|
||
|
||
A node-local property check is unsound, because **exclusion does not propagate downstream.**
|
||
If any node re-emits audio it received, the re-emitting node is a fresh, *untagged*
|
||
`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
|
||
`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
|
||
share at full level — the exact bug this feature exists to prevent, arriving through the
|
||
front door. ⚠️ **`easyeffects` is installed on this machine** (`pacman -Qq easyeffects`),
|
||
and `libpipewire-module-filter-chain.so` is present. It happened not to be running during
|
||
the spike, which is *why* the spike did not catch this. Same shape: combine-sinks,
|
||
`module-loopback`, virtual-sink forwarders, tunnel/RTP sinks, DSP bridges, and any
|
||
monitor-sourced stream.
|
||
|
||
**So the predicate is transitive TAINT over the live SIGNAL graph** — and "signal graph"
|
||
is doing real work in that sentence, because it is **not** the graph of `Link` objects.
|
||
|
||
> A node is eligible only if **no** signal path reaches it from a peerspeak-owned node,
|
||
> the live AEC identity, or any pixelpass-owned object (ours *or* another host's
|
||
> `pixelpass_capture_*`). Unresolvable ancestry is **not** eligible.
|
||
|
||
⚠️ **A Link-only walk does NOT catch the leak.** Measured on the live graph 2026-07-21
|
||
by reproducing the forwarder topology with `module-null-sink` + `module-loopback`
|
||
(same shape as EasyEffects, no EasyEffects required):
|
||
|
||
```
|
||
fabletest_sink:monitor_FL ──Link 87──► input.loopback-2541-13 (Stream/Input/Audio, id 109)
|
||
⋮ ← NO LINK OBJECT EXISTS HERE
|
||
output.loopback-2541-13 ──Link 105─► alsa_output…analog-stereo (id 56)
|
||
(Stream/Output/Audio, id 86)
|
||
```
|
||
|
||
**There is no `Link` between the forwarder's input leg (109) and its output leg (86).**
|
||
Walking upstream from the leaking node 86 over Links alone finds *no inbound links at
|
||
all* — a dead end that reads as "clean," and the node gets fanned out. The two legs are
|
||
related only by shared properties:
|
||
|
||
| key | input leg (109) | output leg (86) |
|
||
| --- | --- | --- |
|
||
| `node.link-group` | `loopback-2541-13` | `loopback-2541-13` |
|
||
| `client.id` | `107` | `107` |
|
||
| `pulse.module.id` | `536870917` | `536870917` |
|
||
|
||
So the signal graph needs **three** edge types:
|
||
|
||
1. **Link edges** — `link.output.node → link.input.node`. ✅ Measured: registry Link
|
||
objects carry all four endpoint props (`link.output.node`, `link.output.port`,
|
||
`link.input.node`, `link.input.port`).
|
||
2. **Sink-monitor edges** — ✅ **free at node granularity.** Measured: the monitor
|
||
connection *is* a real `Link` whose output node is the sink node itself
|
||
(`out-node=104 (fabletest_sink) → in-node=109`). So a node-level walk crosses
|
||
`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.**
|
||
⚠️ **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
|
||
|
||
The owner bridge must propagate taint **only when the input leg is itself tainted.**
|
||
The naive rule "this client has both an input and an output leg ⇒ exclude the output" is
|
||
catastrophic: it excludes any app using a microphone. Firefox in a video call has both
|
||
legs, and its playback is perfectly shareable.
|
||
|
||
The correct rule:
|
||
|
||
1. Taint roots: peerspeak-owned nodes, live AEC identity, pixelpass-owned objects.
|
||
2. Taint flows downstream along Link edges (crossing sinks and monitors for free, §6.1
|
||
edge type 2). A sink carrying tainted audio has a **tainted monitor**.
|
||
3. A `Stream/Input/Audio` reading a tainted monitor becomes tainted.
|
||
4. **Only then** does the owner bridge carry taint to that client's `Stream/Output/Audio`
|
||
legs.
|
||
|
||
Result, both cases correct:
|
||
|
||
| scenario | outcome |
|
||
| --- | --- |
|
||
| Firefox playing music (output leg only) | **eligible** — shared |
|
||
| Firefox in a Meet call (mic input leg, untainted source) | **eligible** — shared |
|
||
| Firefox screen-sharing *with desktop audio* (input leg on a tainted monitor) | **excluded** |
|
||
| EasyEffects / loopback / combine-sink forwarding a mix containing peerspeak | **excluded** |
|
||
|
||
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`
|
||
subscribes to global-add/global-remove and never binds links). That is a materially
|
||
larger component than v3.0 implied.
|
||
|
||
Ancestry that cannot be resolved (unknown forwarder shape, unresolvable monitor source)
|
||
is **not** eligible. That is a deliberate reversal of v3.0's "unrecognized ⇒ fan out":
|
||
E2 makes mis-selection cheap *for the user's audio graph*, but it is not cheap for
|
||
**echo**, and echo is the whole feature.
|
||
|
||
### 6.2 The predicate
|
||
|
||
**Start from all `Stream/Output/Audio` nodes, then exclude:**
|
||
|
||
| Exclusion | Basis |
|
||
| --- | --- |
|
||
| `peerspeak.owned` present | §5.1 env tag |
|
||
| `pulse.module.id` == the live AEC index | §5.2, exact equality only |
|
||
| **tainted via the signal graph** (Link + monitor + conditional owner bridge) | §6.1 — the load-bearing one |
|
||
| pixelpass-owned objects, incl. **other hosts'** `pixelpass_capture_*` ancestry | cycle prevention |
|
||
| tainted input leg + output leg with no resolvable bridge | §6.1.1, fail closed |
|
||
| `port.exclusive` ports, encoded/passthrough streams | fan-out will refuse or corrupt |
|
||
| links we already own for that node | idempotence |
|
||
|
||
- `node.dont-move` **drops out entirely** — fan-out is not a metadata move.
|
||
- Links are created **per port**; channel-count and layout mismatches are per-port.
|
||
|
||
### 6.3 Dynamic graph handling — revalidate, don't fire-and-forget
|
||
|
||
A node can appear with incomplete ancestry, pass the predicate, get fanned out, and only
|
||
*then* receive an inbound link from a filter input or `pixelpass_capture_<pid>.monitor`.
|
||
An add-only listener never sees it. Required:
|
||
|
||
1. An **initial registry sync barrier** — register the listener, call `Core::sync()`, wait
|
||
for `done`, and only then consider the initial pass complete.
|
||
⚠️ **Know what this barrier is and is not.** PipeWire's `core.h:261` defines sync as a
|
||
*previous-work roundtrip*: because methods and events are ordered, `done` means
|
||
"everything emitted so far has been handled." It is **not** graph quiescence and does
|
||
not promise the graph has stopped changing. Readiness therefore = `done` **plus** "no
|
||
unresolved required Link/Port observations outstanding," with a fail-closed timeout.
|
||
2. Candidates stay **pending** until their ports *and* current inbound links are known.
|
||
3. **Revalidate immediately before creating each Link**, not just at selection time.
|
||
4. On any later link add/remove that makes ancestry unsafe, **drop the owned link
|
||
proxies** for the affected node — retention (§4.2) is what makes revocation possible.
|
||
|
||
### 6.4 Implementation shape and cost
|
||
|
||
`pipewire` crate **0.9.2** (`pixelpass/Cargo.toml:48`, `Cargo.lock:4121`).
|
||
|
||
- **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
|
||
logs show churn.
|
||
|
||
### 6.5 Rejected: the "hardware-sink-only" shortcut
|
||
|
||
Considered and **rejected**: "fan out only nodes whose output links to a real hardware
|
||
sink, excluding anything feeding a virtual/null sink." It is cheaper and it is **unsound
|
||
for exactly the case that matters** — measured: `output.loopback-2541-13` links *directly*
|
||
to `alsa_output.pci-0000_10_00.6.analog-stereo`. The forwarder's output leg **is** a
|
||
hardware-linked node, so the shortcut passes it and leaks the mix. The original app stream
|
||
would meanwhile be excluded for feeding the virtual sink — i.e. the shortcut gets both
|
||
halves backwards.
|
||
|
||
It survives only as an optional degraded fallback mode whose stated behaviour is "no
|
||
EasyEffects/filter/loopback-routed audio is captured at all."
|
||
|
||
### 6.6 Foreign/hazard nodes
|
||
|
||
Treat an unvalidated `echo-cancel-*` output node as an excluded hazard rather than an
|
||
eligible stream (§5.4).
|
||
|
||
## 7. Lifecycle and teardown invariants
|
||
|
||
### 7.1 ⚠️ The invariant
|
||
|
||
> **The AEC module must not unload while pixelpass is alive and fanning out.**
|
||
|
||
If it does, the index pixelpass holds becomes stale, and — because indices are reused
|
||
(§5.2 correction 3) — it can alias onto an unrelated future module, silently un-excluding
|
||
the real hazard or excluding innocent audio.
|
||
|
||
### 7.2 ⚠️ VERIFIED DEFECT — implicit-drop order is inverted (not yet fixed)
|
||
|
||
`ActiveSession` (`peerspeak/src/core/mod.rs:668-691`) declares:
|
||
|
||
```rust
|
||
echo_cancel: Option<EchoCancelGuard>, // :682
|
||
screenshare_host: Option<tokio::process::Child>, // :685 (kill_on_drop)
|
||
```
|
||
|
||
Rust drops fields in **declaration order**. On the **implicit-drop** path — the command
|
||
channels close at `core/mod.rs:1512` (`reliable_rx.recv()` returns `None` → `break`), or
|
||
the core loop unwinds — `ActiveSession` is dropped **without** `shutdown()` running. So
|
||
`echo_cancel` unloads (a blocking `pactl unload`) *before* `screenshare_host`'s
|
||
`kill_on_drop` even fires: **the AEC unloads while pixelpass is still alive.** Exactly the
|
||
ordering the invariant forbids. Verified in source.
|
||
|
||
`shutdown()` (`:694-730`) gets it right — it kills `screenshare_host` at `:699` and drops
|
||
`echo_cancel` at `:730` — but only as an emergent property of statement order, which any
|
||
refactor can silently invert.
|
||
|
||
**⚠️ Reordering the fields is NOT sufficient** (round-4 correction). `screenshare_host`
|
||
is spawned with `kill_on_drop(true)` (`peerspeak/src/screenshare/mod.rs:402`). Tokio's
|
||
`Drop` for such a `Child` *sends* the kill and hands the process to a best-effort orphan
|
||
reaper — it does **not** wait, and gives no promptness guarantee, especially if the
|
||
runtime is itself shutting down. Explicit `child.kill().await` is SIGKILL **plus wait**;
|
||
implicit drop is SIGKILL and move on. So even with `echo_cancel` declared last, the
|
||
`pactl unload` can still run while pixelpass is briefly alive.
|
||
|
||
**Fix, in order of importance:**
|
||
|
||
1. **Eliminate the implicit path.** At both channel-close `break` sites
|
||
(`core/mod.rs:1516` and `:1532`), explicitly `take()` the session and
|
||
`shutdown().await` it rather than letting it drop. The ordered teardown should be the
|
||
*only* teardown.
|
||
2. **Move `echo_cancel` to the last declared field** anyway, with a comment naming the
|
||
invariant — defence in depth for paths 1 does not cover (panics, unwinds).
|
||
3. **Last-ditch ordering in the drop path**: a wrapper whose `Drop` does `start_kill` +
|
||
a bounded `try_wait` loop on the host before the AEC guard unloads.
|
||
|
||
**D4 regression guard** (concrete, replacing v3.0's vague "field-order assertion"): a
|
||
test using a **fake host** and **fake echo-cancel guard** that each record a timestamped
|
||
event into a shared slot on teardown, asserting `host kill+wait completed` strictly
|
||
precedes `echo unload` — plus a core-loop test that closes the command channel and proves
|
||
`shutdown()` actually ran. A field-order assertion alone does not test the behaviour that
|
||
matters.
|
||
|
||
### 7.3 Construction paths
|
||
|
||
There is exactly **one** `echo_cancel::enable` call site (`core/mod.rs:1850`, at session
|
||
join), the guard moves into `ActiveSession`, and there is exactly **one** explicit drop
|
||
(`:730`). **No mid-call AEC reload path exists.** This is what downgraded Codex's demanded
|
||
QUIESCE/SET_AEC/RESUME two-process epoch protocol to:
|
||
|
||
> **P1-impact latent hazard, currently unreachable in normal operation.** Required now:
|
||
> encode teardown ordering for explicit *and* implicit destruction, prevent or review
|
||
> additional AEC construction paths, fail closed if identity is missing or revoked.
|
||
> **An epoch protocol becomes required if and only if hot AEC reload is added.**
|
||
|
||
Anyone adding a second `enable` site, or any hot-reload, re-opens that requirement.
|
||
|
||
### 7.4 Graceful stop is still owed
|
||
|
||
**Stop Share is currently SIGKILL** (`peerspeak/src/core/mod.rs:699` + `:3480`, Tokio
|
||
`Child::kill()`). Under Option C this no longer strands the user's desktop audio — the
|
||
links die with the connection, which is the point. But it still leaks:
|
||
|
||
- The capture null sink is **pactl-loaded and pipewire-pulse-owned**
|
||
(`pixelpass/src/host/audio.rs:69`), so **Stop Share leaks one null-sink module every
|
||
time.** This is true today, independent of this feature.
|
||
|
||
Required:
|
||
1. A graceful control path in peerspeak: **SIGINT** (not SIGTERM — pixelpass installs
|
||
only `tokio::signal::ctrl_c()`, `pixelpass/src/common/signal.rs:6`), bounded wait,
|
||
SIGKILL fallback.
|
||
2. The capture sink should become **connection-owned** rather than pactl-owned, so it
|
||
shares the links' death-with-the-process property.
|
||
3. `--repair` (`pixelpass/src/repair.rs:15-63`) extended to this mode, and safe with a
|
||
second live host.
|
||
|
||
## 8. IPC: getting the index to pixelpass
|
||
|
||
pixelpass is a **separate process** and cannot learn the module index on its own.
|
||
peerspeak spawns it, so:
|
||
|
||
- New pixelpass flag: `--exclude-pulse-module <idx>` (naming, §11).
|
||
- peerspeak passes `EchoCancelGuard`'s index at spawn. The field is currently private
|
||
with only `source_name()` / `sink_name()` accessors — add `module_index()`.
|
||
- If the AEC is **off**, the flag is absent and pixelpass must not invent an exclusion.
|
||
- If the AEC is **on** but the flag is absent, that is a bug in peerspeak; pixelpass
|
||
cannot detect it. ⇒ peerspeak should pass an explicit `--aec=off|<idx>` rather than
|
||
"flag present or not," so pixelpass can distinguish "no AEC" from "someone forgot."
|
||
**Open decision D5 (§13).**
|
||
- Because no hot-reload path exists (§7.3), spawn-time argv is sufficient today. A
|
||
runtime channel is required only alongside hot reload.
|
||
|
||
## 9. What is proven, and what is not
|
||
|
||
### 9.1 Proven by measurement on this machine (PipeWire 1.6.8 / WirePlumber 0.5.15)
|
||
|
||
- Fan-out carries full-level audio (−24.1 dB, matching the speaker monitor) for **paplay,
|
||
mpv, VLC**; the app keeps its speaker link.
|
||
- WirePlumber does **not** reap foreign links across default-sink switch, switch-back,
|
||
suspend, resume, or 100 s steady state.
|
||
- Non-lingering links die on **SIGKILL** of the owner.
|
||
- **E2:** destroying the capture sink mid-share left the app playing to speakers
|
||
undisturbed.
|
||
- `peerspeak.owned=1` lands on paplay/mpv/VLC nodes via `PULSE_PROP`.
|
||
- `pulse.module.id` on all four AEC nodes == the index pactl returned.
|
||
- Naive fan-out of the AEC leg leaks remote audio at ≈desktop level; exact exclusion sits
|
||
at the control floor.
|
||
|
||
### 9.2 ⚠️ Wording discipline — three overclaims already made, do not make a fourth
|
||
|
||
The correct statement of the exclusion result is:
|
||
|
||
> *"No incremental 1500 Hz energy was detectable above the control floor at the analysis
|
||
> resolution in this steady-state run."*
|
||
|
||
**Not** "absent," **not** "conclusive." What survives is the gross-leak distinction: the
|
||
naive arm copies the probe at ≈desktop level, exact exclusion does not. Likewise, equal
|
||
`mean_volume` on a sine proves **signal presence, not fidelity** (blind to xruns, drift,
|
||
dropouts, channel swap, quantum change), and "copy semantics" is a **topology** result —
|
||
it shows the tested link operation did not move the tested Pulse stream, and says nothing
|
||
about gain, latency, continuity, or native clients.
|
||
|
||
The rig is additionally blind to: startup/teardown/cork/relink transients (hidden by
|
||
whole-file averaging), broadband and out-of-band leakage, level-dependent and nonlinear
|
||
products, and nodes created after enumeration.
|
||
|
||
**Minimum rig upgrade before shipping:** two orthogonal PN/MLS probes + windowed
|
||
per-channel normalized cross-correlation, reporting max per-window correlation, plus xrun
|
||
telemetry.
|
||
|
||
### 9.3 Untested — carried forward, each one a real risk
|
||
|
||
AEC reload mid-share · two concurrent AEC instances · two concurrent capture sinks · the
|
||
**native** PipeWire AEC module · a native-PipeWire (non-Pulse) app · pixelpass's own
|
||
null-sink + loopback present simultaneously · the full GStreamer/AAC/network path ·
|
||
fidelity (needs broadband source + correlation + xrun telemetry) · PipeWire daemon restart
|
||
· quantum/latency perturbation from the second link · sample-rate mismatch, surround,
|
||
IEC958 passthrough · browser and game stream shapes.
|
||
|
||
## 10. Prerequisite fixes (split out, land before the feature)
|
||
|
||
Both reviews agree these are their own tasks, not part of this feature.
|
||
|
||
1. **`object.serial` u32 truncation** — 64-bit in PipeWire, parsed as `u32` at
|
||
`pixelpass/src/host/audio.rs:534-540`. Global IDs are reused; serial is the recommended
|
||
stable identifier. Must be fixed before the router grows.
|
||
2. **`@DEFAULT_SINK@` resolved once at module load** (`audio.rs:139-151`), no
|
||
default-metadata listener — the user's `Ctrl+Meta+F` / `Ctrl+Meta+S` output-switch
|
||
hotkeys strand the local monitor mid-share. **This already hurts per-app mode today.**
|
||
Option C does not need the local monitor, so this is decoupled from the feature — but
|
||
it is a live bug.
|
||
3. **`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`.
|
||
Any status surfaced to the user inherits that dishonesty. Option C's link-state
|
||
`ACTIVE` check (§4.2) is the honest replacement for the new mode; the old path should
|
||
be fixed or deleted.
|
||
4. **Graceful stop + connection-owned capture sink** (§7.4).
|
||
5. **Drop-order fix + regression guard** (§7.2).
|
||
|
||
## 11. Naming — the user's call
|
||
|
||
Placeholders throughout. pixelpass mode: `--audio-mode=desktop-shared` /
|
||
`--audio-mode=desktop-excluding`. Exclusion flag: `--exclude-pulse-module <idx>` (or
|
||
`--aec <idx|off>`, §8). Do **not** surface `--exclude-pid` — it names an unreliable
|
||
mechanism and reads like process control.
|
||
|
||
peerspeak picker wording: something like "System audio except peerspeak," noting 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, unit-testable seams with PipeWire at the edges, per house style:
|
||
|
||
- `fn eligibility(node_props: &Props, ctx: &ExclusionCtx) -> Eligibility` with
|
||
`NotEligible { reason }` so status can explain itself. Cases: `peerspeak.owned` present;
|
||
`pulse.module.id` == live index; `pulse.module.id` present but **different** (must be
|
||
ELIGIBLE — correction 2); `pulse.module.id` absent; a pixelpass-owned node; capture-sink
|
||
ancestry; `port.exclusive`; passthrough; non-`Stream/Output/Audio`; missing props
|
||
entirely; the capture sink itself; a foreign `echo-cancel-*` group.
|
||
- `pulse.module.id` parsing: JSON number **and** string forms, values > `u32::MAX`,
|
||
absent, malformed.
|
||
- AEC-identity validation state machine: `NotConfigured` / `Validated` / `Revoked`, and
|
||
the assertion that `Revoked` stops fan-out.
|
||
- Pure capability-probe parsing (§13 D2).
|
||
- Link bookkeeping: per-port link set, "captured" only at all-`ACTIVE`, idempotent
|
||
re-enumeration, proxy retention/drop.
|
||
|
||
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**.
|
||
2. Sharer simultaneously *viewing* another share while sharing.
|
||
3. Lifecycle, each separately: Stop button, room leave, UI crash, pixelpass panic, SIGINT,
|
||
SIGTERM, SIGKILL, last-viewer disconnect, pipewire-pulse restart, PipeWire daemon
|
||
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. An app that **starts playing after** the share began (dynamic node).
|
||
8. Sample-rate / channel / passthrough behaviour on real sinks; CPU cost.
|
||
|
||
⚠️ **Test-rig discipline** (each of these already produced a wrong result once):
|
||
filter on `media.class` **first** when selecting nodes with `jq` — `media.name` also
|
||
matches the Client object; **never** filter stderr out of a measurement run; **verify the
|
||
link exists in the graph** before measuring; reproduce over the transport production
|
||
actually uses (`parec -d <sink>.monitor`, not `pw-record --target <sink-node-id>`, which
|
||
reads −91 dB silence).
|
||
|
||
## 13. Decisions — resolved in round 4
|
||
|
||
Both reviewers agree on all seven. Recorded as decided; reopen only with new evidence.
|
||
|
||
- **D1 — fail closed, but only after a bounded validation wait** (§5.3). Never "share
|
||
anyway, warn" as the default. ✅
|
||
- **D2 — a versioned machine-readable capability response or bitset.** The
|
||
`pixelpass --help` substring probe (`src/screenshare/mod.rs:245-278`, invoked at
|
||
`src/core/mod.rs:3368`) stays only as a compatibility fallback, and this mode must
|
||
**not** overload `app_audio_supported: bool` — strict per-app and desktop-excluding are
|
||
independent capabilities. ✅
|
||
- **D3 — warn and exclude foreign `echo-cancel-*` nodes** for the first implementation;
|
||
never fan them out. ✅
|
||
- **D4 — explicit async shutdown + a fake-resource ordering test** (§7.2). A field-order
|
||
assertion alone is inadequate. ✅
|
||
- **D5 — `--aec=off|pulse-module:<idx>`, always passed.** Absence of the flag is not a
|
||
protocol state (§8). ✅
|
||
- **D6 — land §10 items 1, 4 and 5 first**, plus the new graph-ancestry predicate (§6.1).
|
||
Items 2 and 3 are real per-app debt but do not block Option C unless the new mode reuses
|
||
those code paths. ✅
|
||
- **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
|
||
|
||
v3.0 was reviewed and blocked; v3.1 (this revision) applies the three required rewrites.
|
||
The minimum unblock set was:
|
||
|
||
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.
|
||
|
||
**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.
|