From cab6bafce58decc40cae51796a672ff03b0513d4 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Tue, 21 Jul 2026 03:16:30 -0400 Subject: [PATCH] =?UTF-8?q?docs:=20v3=20audio-exclusion=20design=20?= =?UTF-8?q?=E2=80=94=20rewrite=20around=20Option=20C=20+=20AEC=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v1/v2 described a move-based design that Option C superseded on 2026-07-20, and the AEC playback-leg identity gate has since passed. Roughly two thirds of v2 documented problems Option C does not have, so this is a rewrite rather than a patch (v1/v2 remain at 88ad5a0 / 10203e1). Folds in: the four AEC gate results, the five corrections that constrain them (observed correlation not a contract; exact-equality only; index/link-group reuse and node-id recycling; group prefix = hazard detection not ownership; application.process.id == pipewire-pulse for module-created streams), the verified implicit-drop ordering defect in ActiveSession, fail-closed validation/revocation, the IPC shape, and the split-out prerequisites. Co-Authored-By: Claude Opus 4.8 --- docs/screenshare-audio-exclusion-plan.md | 768 +++++++++++------------ 1 file changed, 372 insertions(+), 396 deletions(-) diff --git a/docs/screenshare-audio-exclusion-plan.md b/docs/screenshare-audio-exclusion-plan.md index efb8b44..044a0f5 100644 --- a/docs/screenshare-audio-exclusion-plan.md +++ b/docs/screenshare-audio-exclusion-plan.md @@ -1,517 +1,493 @@ -# Design v2: whole-desktop screen-share audio without self-echo +# Design v3: whole-desktop screen-share audio without self-echo -**Status:** proposal, not implemented. v2 supersedes v1 after adversarial review. -**Date:** 2026-07-20 (v1: 2026-07-19) +**Status:** design complete, **implementation-planning phase — not approved for merge.** +**Date:** 2026-07-21 (v1: 2026-07-19 · v2: 2026-07-20 · Option C adopted 2026-07-20) **Origin:** Joe's suggestion — "whitelist all audio except audio coming from peerspeak." -**Review history:** v1 reviewed by Codex (gpt-5.6-sol, high) — -`~/Documents/handoff-docs/Codex/peerspeak/review-2026-07-19-audio-exclusion-design.md`. -Verdict: "not sound enough to implement as written," 4 release blockers, all -independently verified against source and all correct. v2 is a redesign, not a patch. -**Scope:** new capture mode in pixelpass `src/host/pipeline.rs` + `src/host/audio.rs`; -playback tagging and a graceful-stop protocol in peerspeak. +**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. -> **⚠️ This document is still gated on empirical data.** Section 10 lists the -> observations that must exist before any of this is implemented. Every claim below -> marked **OPEN-Q** is a guess with a named experiment attached, not a decision. -> No `pw-dump` of this machine's graph has been captured yet. +**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. +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. +`--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.** -Joe's ask is the missing third option: **share the desktop, minus peerspeak.** - -## 2. Feasibility, and the correct framing +## 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 **routing**: every playback stream is a node whose sink target can be -reassigned at runtime. +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. Same observable behaviour, entirely different mechanism. +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 mixed at all. +path, where coupling is acoustic and needs adaptive cancellation. Here the signals never +need to be summed at all. -**Correction from v1:** v1 called the separation "exact and lossless." That was an -overclaim. The *membership* of the constructed mix is exact — peerspeak's audio is -never summed in. The *delivered audio* is not lossless: pixelpass downmixes to 48 kHz -stereo and AAC-encodes at 128 kbps (`pixelpass/src/host/pipeline.rs:303-324`), and the -null-sink/loopback stages may resample streams whose layout or clock differs. +**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 actually exists today (v1 got this wrong) +## 3. What exists today -**v1's premise was false.** v1 claimed whole-desktop capture already loads -`@DEFAULT_SINK@.monitor → pixelpass_capture_` and that the feature is "the -existing per-app path with an inverted predicate." It is not. +`setup_audio` (`pixelpass/src/host/pipeline.rs: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. -`setup_audio` (`pixelpass/src/host/pipeline.rs:121-140`) activates `Routing` **only** -when `--app` is set or `PIXELPASS_AUDIO_VIA_NULL_SINK` is set (a dogfood path, per its -own comment). peerspeak's no-app argv is exactly `--host --output json` -(`peerspeak/src/screenshare/mod.rs:135-165`). Normal whole-desktop capture therefore -**bypasses `Routing` entirely** and hands the real default monitor to `pulsesrc`. - -What `Routing::start` actually does (`pixelpass/src/host/audio.rs:65-212`): +`Routing::start` (`pixelpass/src/host/audio.rs:65-212`) today provides: | Piece | Condition | | --- | --- | | `module-null-sink pixelpass_capture_` | always, when `Routing` runs at all | | `module-loopback @DEFAULT_SINK@.monitor → capture` | skipped only when `--app` **and** `--strict-audio` | | `StreamRouter` (libpipewire thread) | only when `opts.app` is `Some` | -| local-monitor `capture.monitor → @DEFAULT_SINK@` | loaded on `FirstRoutedStream`, unloaded on `LastRoutedStreamGone` | +| local-monitor `capture.monitor → @DEFAULT_SINK@` | on `FirstRoutedStream`, unloaded on `LastRoutedStreamGone` | -So this feature needs a **new capture mode in `pipeline.rs`**, plus a substantially -new router. It is not a predicate inversion. v1's "every primitive already exists" -claim is withdrawn. +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). -Two further corrections to v1's model of the existing router: +## 4. Architecture — Option C: copy, don't move -- `StreamRouter` subscribes to registry global-add/global-remove only - (`audio.rs:523-560`). It does not bind nodes or watch properties. v1's risk 4 - ("pavucontrol overrides get re-fought") is **backwards** — a manual move generates no - new global-add, so the override currently wins until the node is destroyed. If the - new mode must enforce routing continuously, that is new behaviour with new loop- - prevention requirements. -- `try_flush` records *intent*, not success (`audio.rs:642-655`): it ignores - `Metadata::set_property`'s return and appends every pending ID to `routed_node_ids` - regardless. There is no link-state confirmation. Any status this feature surfaces to - the user inherits that dishonesty unless it is fixed. +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. -## 4. The core redesign: ownership by inherited tag, not by PID +``` + app output ──────────────────────► existing hardware / filter path (UNTOUCHED) + └── pixelpass-owned link ────► pixelpass_capture_ ──► gst pulsesrc ──► viewers -v1 identified peerspeak's audio by `application.process.id`. Codex showed that fails in -three separate ways, and it fails for a common reason: **PID is a property of a process, -but what we need to identify is a *stream's owner*, and peerspeak's audio is emitted by -processes and modules it does not run inside.** + peerspeak-owned playback ────────► speakers only (never linked to capture) + AEC playback leg ────────────────► speakers only (never linked to capture) +``` -Concretely, PID-matching misses: +### 4.1 Why this beats the move-based design -1. **The AEC playback leg.** `module-echo-cancel` is loaded via pactl - (`peerspeak/src/audio/echo_cancel.rs:83-94`) and its virtual-sink→speaker playback - stream lives in `pipewire-pulse`, not peerspeak. An inverted predicate would route - *remote call audio* into the capture — the exact thing the feature removes. -2. **The media player.** `spawn_player` launches mpv/VLC as a detached child with - `kill_on_drop(false)` (`peerspeak/src/screenshare/mod.rs:761`). Excluding the - pixelpass *viewer* PID excludes a tunnel process that plays nothing. -3. **Notification sounds.** `pw-play`/`paplay`/`aplay` children (`src/notify.rs:265`). +| 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 | -Plus PID is not authoritative in general: PipeWire reports `application.process.id` as -the **pipewire-pulse PID** for Pulse-emulated clients, and the property can be absent or -overridden. +**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.1 The proposal +### 4.2 Ownership of links is the safety mechanism -**peerspeak tags every stream it owns, at creation, and the router excludes by tag.** +In the Rust `pipewire` crate, **dropping a `Link` proxy destroys the object.** So: -The lever that makes this cheap: every peerspeak-owned playback path is a -`Command::new(…)` spawn, so a tag can be *inherited through the environment*. +- 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 ` 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 +`. 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 — ✅ MEASURED WORKING | Owner | Mechanism | | --- | --- | | native call playback (`src/audio/pipewire_impl.rs:374-388`) | set the tag directly in the stream's property dict | -| mpv / VLC (`src/screenshare/mod.rs:761`) | `.env("PIPEWIRE_PROPS", …)` / `PULSE_PROP` on the `Command` | +| mpv / VLC (`src/screenshare/mod.rs:761`) | `.env("PULSE_PROP", …)` / `PIPEWIRE_PROPS` on the `Command` | | `pw-play` / `paplay` / `aplay` (`src/notify.rs:265`) | same | -| `module-echo-cancel` legs | pass properties at `pactl load-module`, **OPEN-Q 3** | -This dissolves blockers 2 and 3 instead of patching them. No live PID channel, no -PID-reuse hazard, no re-evaluation of already-existing nodes when a PID is retired. +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`.) -**Proposed tag:** a single stable key, e.g. `peerspeak.owned=1`. Naming is the user's -call (§11). Verified working on this machine for all three spawned-child cases — -see §10.1. +**The tag is a correctness mechanism, not a security boundary.** Any same-user client can +set `peerspeak.owned=1` and opt itself out of capture, 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. -**The tag is a correctness mechanism, not a security boundary.** Any same-user client -can set `peerspeak.owned=1` and opt itself out of capture; a child can also sanitize its -environment. PipeWire is explicit that only `pipewire.*` properties are suitable for -security decisions. That is acceptable here — the threat model is "don't echo the -user's own call back at them," not "defend against a hostile local process." It must be -stated rather than assumed. A related hazard worth handling: a stable boolean **leaks -into grandchildren**, so anything mpv or VLC itself spawns inherits the exemption. +⚠️ **Known leak: a stable boolean is inherited by grandchildren.** Anything mpv or VLC +spawns is exempted too. Accepted for now; revisit if it bites. -### 4.2 Fail closed: eligibility, not exclusion +### 5.2 The AEC playback leg — 🟢 gate passed, with five load-bearing corrections -Even with perfect tagging, "route everything that isn't ours" sweeps in third-party -infrastructure: other apps' loopbacks, filter-chains, virtual-sink forwarders, tunnel -sinks, combine-sinks, DSP bridges. Moving those can create cycles, bypass DSP, or break -an unrelated application's graph. +`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: -**So the predicate is an allowlist, not a denylist.** Route a node only if it is -positively identified as ordinary end-user application playback. Anything unrecognized -is **left alone and reported as not-captured**, never optimistically moved. +> **Measured (3 arms, links verified in-graph before measuring, recorded via +> `parec -d .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.** -That inverts the failure mode from "we broke your audio graph" to "that app's audio -didn't make it into the share," which is recoverable and visible. +**What `module-echo-cancel` actually creates (RESULT 1):** four nodes, not two — +`Audio/Sink peerspeak_ec_sink.`, `Audio/Source peerspeak_ec_source.`, +`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 exact property set defining "ordinary application playback" **cannot be written -today** — it depends on what real nodes look like on this machine. Candidates to -evaluate from a `pw-dump`: `node.passive`, `node.virtual`, `media.class`, `media.role`, -`client.id` → Client object ownership, presence of a `pipewire.sec.pid`, -`node.dont-move`. **OPEN-Q 1.** - -### 4.3 The self-match, corrected - -pixelpass's own local-monitor loopback (`capture.monitor → @DEFAULT_SINK@`) has a -playback half that appears as a `Stream/Output/Audio` node. An unqualified predicate -selects it and points it back at the capture sink — an immediate feedback topology. - -v1 proposed tagging it via `source_output_properties=`. **That is the wrong half.** -`source_output_properties` applies to the capture/source-output leg; the self-matching -node is the sink-input/playback leg. Correct form: +**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. ``` -sink_input_properties=node.name=pixelpass_local_monitor_ sink_dont_move=true +exclude every node where pulse.module.id == ``` -`sink_dont_move=true` is defense in depth: session policy refuses the move even if the -predicate regresses. Do **not** fall back to matching a generic `loopback.*` shape — -that either catches unrelated loopbacks or misses a second pixelpass host's. +**⚠️ The five corrections. These are the design, not footnotes:** -Whether `sink_input_properties=node.name=…` actually surfaces on the registry node on -the deployed PipeWire build is **OPEN-Q 2**. Whether WirePlumber rejects the cycle or -permits audible runaway feedback if the predicate does regress is **OPEN-Q 5**. +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. **`application.process.id` on the AEC client is `1974` = pipewire-pulse** (verified). + This reconciles two contradictory prior claims: *real* Pulse clients (paplay) report + their own PID; *module-created* streams report pipewire-pulse's, because + pipewire-pulse is the client. Both were right about different cases. **PID-based AEC + exclusion stays unusable**; PID remains a fallback hint, never identity. -## 5. Topology (Option A, still preferred — for a corrected reason) +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`. -New host mode. Name TBD (§11); referred to here as *routed-desktop*. +### 5.3 Fail closed — validation and revocation -``` - eligible app streams ────► pixelpass_capture_ ──► gst capture ──► viewers - (routed by StreamRouter) │ - └─ local-monitor loopback ──► @DEFAULT_SINK@ ──► speakers - ▲ - peerspeak-tagged playback ──────────────────────────────────────────────┘ - (never routed; AEC, call, mpv, notifications) -``` +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**: -- Capture null-sink as today. -- **No** `@DEFAULT_SINK@.monitor → capture` loopback, ever, in this mode — that - loopback *is* the echo. Unlike best-effort per-app mode there is no - fallback-to-desktop path to oscillate. -- Local monitor created **synchronously, before the first target move**, tagged and - `sink_dont_move` per §4.3. +- **At start:** given `--exclude-pulse-module `, enumerate the graph and confirm at + least one node carries that `pulse.module.id`. If peerspeak said the AEC is on and no + such node exists, **do not start fan-out** — report a capability failure and fall back + to a mode with no echo risk (no audio, or an explicit user override). +- **On revocation:** if the validated AEC leg **disappears** while sharing, **stop + fan-out immediately.** Do not keep the numeric index and hope. Disappearance means + either the AEC unloaded (index may be recycled onto something else) or our model of the + graph is wrong; both are fail-closed conditions. +- **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. -**Option B (make a null-sink the system default) is still rejected — but v1's reason -was wrong.** v1 argued a pixelpass crash leaves the desktop silent. Codex is right that -this is unproven: pactl modules are owned by `pipewire-pulse` and *persist* past a -pixelpass crash rather than vanishing, and WirePlumber selects a best-available sink -when a configured default is absent. +### 5.4 Foreign or second AEC instances — a product decision -The real reason to reject B on this machine: it hijacks the global default sink, which -directly conflicts with the user's `Ctrl+Meta+F` / `Ctrl+Meta+S` output-switch scripts. -B would have to either fight the user's device choice or let new streams silently bypass -the share. It also mutates system-wide state for unrelated clients and needs -saved-default restoration plus idempotent crash repair. +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).** -B's genuine advantages, recorded honestly: new apps land in the mix automatically, no -registry-add race, and no sweeping up of arbitrary module playback legs. +## 6. Eligibility — a broad guarded selector -## 5.1 Option C — copy instead of move — ✅ **DECIDED: this supersedes Option A** +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. -> **STATUS 2026-07-20: feasibility spike RUN, Option C ADOPTED by both reviewers.** -> Measurements: `~/Documents/handoff-docs/Claude/peerspeak/fanout-spike-results-2026-07-20.md`. -> Codex ratification: `…/Codex/peerspeak/review-2026-07-20-fanout-spike-round4-ratification.md`. -> Everything below in §5 (Option A/B), §6, §7 and §8.2–8.3 is **retained for the record -> but is no longer the plan** — most of it describes problems Option C does not have. -> **A v3 rewrite is owed once the AEC gate (§10, OPEN-Q 3) is settled.** -> -> Measured 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 and 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; and non-lingering links are destroyed automatically when their owning -> connection is **SIGKILLed**. -> **Strongest result — 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." -> -> **Not yet proven** (do not overclaim these): fidelity beyond signal presence — equal -> `mean_volume` on a sine cannot see xruns, drift or dropouts; daemon restart; -> quantum/latency perturbation; sample-rate mismatch, surround, `port.exclusive`, -> IEC958 passthrough; two concurrent capture sinks. -> **Also still true:** the capture null sink is `pactl`-loaded and pipewire-pulse-owned -> (`audio.rs:69`), so **Stop Share still leaks one null-sink module every time**. -> Option C fixes stranded *desktop audio*, not the sink leak — §6's graceful stop is -> still owed, and the capture sink should become connection-owned too. +Agreed predicate: -Codex's round-2 review made the strongest architectural point either review has -produced: **the design is overbuilt because it *moves* audio that pixelpass only needs -to *copy*.** +**Start from all `Stream/Output/Audio` nodes, then exclude:** -PipeWire permits a node's output ports to link to more than one sink. So instead of -reassigning `target.object` and dragging every desktop stream off the user's speakers, -pixelpass creates a *second*, pixelpass-owned link from each eligible playback stream to -the capture sink, leaving the existing speaker link untouched: - -``` - app output ──────────────────► existing hardware / filter path (untouched) - └── pixelpass-owned link ──► capture sink ──► viewers -``` - -If those links are created with `object.linger=false`, they are owned by the pixelpass -connection and **vanish when the process dies** — SIGKILL stops being catastrophic. - -This deletes, rather than solves, most of §6–§8: - -| Problem in the move-based design | Under copy | +| Exclusion | Basis | | --- | --- | -| local-monitor loopback | not needed — audio never leaves the speakers | -| added playback latency for the sharer | gone | -| output-device switch mid-share (§8.2) | gone | -| prior-target capture/restore (§7) | gone — nothing is retargeted | -| pavucontrol conflicts | gone | -| two hosts fighting over `target.object` (§8.3) | gone — links are independent | -| stranded desktop audio after SIGKILL (§6) | gone with `object.linger=false` | +| `peerspeak.owned` present | §5.1 env tag | +| `pulse.module.id` == the live AEC index | §5.2, exact equality only | +| pixelpass-owned objects, and any node with capture-sink ancestry | cycle prevention | +| `port.exclusive` ports, encoded/passthrough streams | fan-out will refuse or corrupt | +| links we already own for that node | idempotence | -It does **not** remove the eligibility/ownership problem (§4.2) — pixelpass still has to -decide which streams to fan out. But the failure mode of a wrong decision drops from -"we rewired or stranded your desktop" to "that stream wasn't captured," which is exactly -the posture §4.2 is reaching for. +Notes: -**Known risks:** `port.exclusive` and passthrough streams may refuse fan-out; and the -open question is whether **WirePlumber tears down foreign links** it did not create as -part of its own policy management. That is the one thing that would kill Option C, and -it is cheap to test. +- `node.dont-move` **drops out of the predicate entirely** — fan-out is not a metadata + move, so a node's move policy is irrelevant. +- Links are created **per port**, not per node; channel-count and layout mismatches are + a per-port concern. +- Anything unrecognized is **fanned out** (copy is cheap) *unless* it hits an exclusion — + the inverse of v2's posture, and a direct consequence of E2. +- Dynamic nodes: the selector must run on registry global-add for the life of the share, + not once at start. Nodes created *after* enumeration are exactly the case the spike rig + was blind to. -### 5.2 Eligibility under Option C — a broad guarded selector +## 7. Lifecycle and teardown invariants -Because copying is cheap and non-destructive, the narrow allowlist of §4.2 is no longer -justified — but "fan out everything and let mis-selection be harmless" is **also wrong**: -selecting peerspeak-owned playback recreates self-echo, selecting the AEC leg recreates -call echo, and a second link still participates in format/buffer negotiation, so it can -perturb the source even though the original link survives. +### 7.1 ⚠️ The invariant -Agreed replacement: +> **The AEC module must not unload while pixelpass is alive and fanning out.** -- Start from **all** `Stream/Output/Audio` nodes. -- Exclude `peerspeak.owned` (§4.1). -- Exclude the proven AEC identity (**OPEN-Q 3 — the remaining gate**). -- Exclude pixelpass-owned objects and any graph path with capture ancestry (cycles). -- Skip `port.exclusive`, encoded/passthrough, and already-owned links. -- Create **per-port** links, **retain the Link proxies** for the session, and count a - stream captured only once every required link reaches `ACTIVE`. In the Rust - `pipewire` crate, dropping a proxy destroys the object — that is the ownership - mechanism, and it is also what makes SIGKILL safe. -- On failure: leave the original route untouched, report the stream as unsupported. +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. -`node.dont-move` drops out of the predicate entirely — fan-out is not a metadata move. +### 7.2 ⚠️ VERIFIED DEFECT — implicit-drop order is inverted (not yet fixed) -### 5.3 Next gate +`ActiveSession` (`peerspeak/src/core/mod.rs:668-691`) declares: -**Load `module-echo-cancel` on the live graph and establish a stable, pixelpass- -observable identity for its actual playback `Stream/Output/Audio` leg** — not merely the -virtual sink — that survives unload/reload without PID guesswork. Then fan out an -ordinary tone and demonstrate that remote-call audio is absent from the capture monitor. +```rust +echo_cancel: Option, // :682 +screenshare_host: Option, // :685 (kill_on_drop) +``` -If no stable tag or module/group relationship exists, the AEC path must be redesigned -before this feature proceeds. Both reviewers agree nothing belongs ahead of this. +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. -## 6. Graceful stop is a prerequisite, not a test case +`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. -**Stop Share is currently `SIGKILL`.** `peerspeak/src/core/mod.rs:698` and `:3479` call -Tokio `Child::kill()`, which on Unix is SIGKILL-plus-wait. So `Routing::cleanup`, -`Cmd::Shutdown`, and `Drop` **never run on the normal stop path** — v1 treated this as -an exotic crash scenario when it is the ordinary button. +**Fix (implementation phase):** move `echo_cancel` to be the **last** declared field, add +a comment naming the invariant, and add a regression guard. A drop-order unit test is +awkward in safe Rust; the cheap version is a compile-time/`#[test]` assertion over field +order via a doc-tested constructor, or a `Drop` impl on a wrapper that records ordering +into a test-visible slot. **Open decision D4 (§13).** -Today that strands one app. Under routed-desktop it would strand *every desktop stream* -pointing at an orphan capture sink, on every single stop. This alone makes the feature -unshippable without a lifecycle change. +### 7.3 Construction paths -Required, in order: +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: -1. **A graceful control path in peerspeak** — SIGTERM (or a stdin command) with a - bounded timeout, escalating to SIGKILL only on failure to exit. -2. **pixelpass handling that signal** to run `Routing::cleanup` — currently there is no - signal handler on this path. **OPEN-Q 7.** -3. **Cleanup that confirms rather than assumes** — the current path writes - `target.object` clears without inspecting `set_property`'s result or waiting for a - core sync/done before quitting (`audio.rs:494-512`). -4. **`--repair` extended** to understand this mode. Its current scope is dead-PID - modules only (`pixelpass/src/repair.rs:15-63`); it must also clear or restore - routing state and stay safe with a second live host. +> **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.** -## 7. Restore, don't clear +Anyone adding a second `enable` site, or any hot-reload, re-opens that requirement. -The router sets `target.object` to `None` on shutdown, which returns a stream to -following the default. Any stream the user had *deliberately pinned* to another device -loses that pin. In per-app mode that was one app; here it is the whole desktop. +### 7.4 Graceful stop is still owed -The prior value must be captured before the move and restored on cleanup. This is state -the current router does not keep. +**Stop Share is currently SIGKILL** (`peerspeak/src/core/mod.rs:698` + `:3479`, 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: -Related: `node.dont-move=true` streams will not move at all, and `node.dont-reconnect` / -`node.dont-fallback` streams may error or die rather than fall back when the capture -sink vanishes. The predicate must detect these and report per-node outcome instead of -counting requested moves as routed. **OPEN-Q 4.** +- 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. -## 8. Remaining risks +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. -1. **Latency now applies to everything.** All desktop audio traverses - capture-sink → loopback → hardware at `latency_msec=20`. That is a real regression - for rhythm games and monitoring, paid by the sharer. Whether 20 ms is right for this - mode, or whether `node.latency` should be driven lower, is open. -2. **Output-device switching mid-share — release-blocking.** The local monitor resolves - `@DEFAULT_SINK@` once at module load (`audio.rs:139-151`) with no default-metadata - listener. Switching output mid-share strands the sharer's *entire routed desktop* on - the old device. **This is a genuine pre-existing defect that already hurts per-app - mode**; this feature raises its blast radius from one app to everything. Must ship - with a tested retarget/reload — or an enforced "restart the share after switching - output" limitation. -3. **Two hosts cannot coexist.** Both routers write the same `target.object` key on - every eligible stream; last writer wins, and stopping one can clear the other's - target. Either serialize routed-desktop hosts behind a per-user lock with a clear - error, or build ownership/generation tokens. The exclusion namespace must cover - *all* pixelpass-owned loopbacks, not just this host's. -4. **"What the sharer hears" is not what viewers get.** The capture sink sees stream - volume but not the hardware sink's mute/volume, per-device DSP, or spatial - processing. A sharer can mute their speakers and viewers still receive full-level - audio. UX wording must be "eligible desktop application audio," not "everything you - hear." -5. **Startup race.** `try_flush` acts only once both sink serial and `default` metadata - are bound; before that, streams are unrouted — viewers get silence, which is the safe - direction, but the front-end should say so. Note `Routing::start` returns before any - readiness acknowledgement and router-thread failure is only logged - (`audio.rs:439-446`), so the host can report success while viewers get permanent - silence. -6. **Idle nodes are not gone nodes.** `handle_global_remove` fires only on global - destruction (`audio.rs:601-620`), so a paused app stays counted as routed. State - events based on it are approximate. -7. **Stale prerequisites.** The router cannot rebind `sink_serial` / `default_metadata` - if either disappears, so a pipewire-pulse or WirePlumber restart leaves it alive and - inert. -8. **Pre-existing type bug.** `object.serial` is 64-bit in PipeWire but parsed as `u32` - (`audio.rs:534-540`). Global IDs are reused; serial is the recommended stable - identifier. Worth fixing before the router grows. +## 8. IPC: getting the index to pixelpass -## 9. Capability negotiation +pixelpass is a **separate process** and cannot learn the module index on its own. +peerspeak spawns it, so: -v1 cited `src/core/messages.rs:503` as capability negotiation. It is not — that file -only defines the `AudioAppsListed` UI event. Actual detection is a `pixelpass --help` -substring probe (`src/screenshare/mod.rs:245-278`, invoked at `src/core/mod.rs:3368`). +- New pixelpass flag: `--exclude-pulse-module ` (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|` 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. -A help-token probe is an acceptable continuation of the existing pattern, but this -feature must **not** overload `app_audio_supported: bool`. Strict per-app and -routed-desktop are independent capabilities; a pixelpass build may have one and not the -other. Model them separately (enum or bitset), name the exact new probe token, and -define the fallback UX when it is absent. +## 9. What is proven, and what is not -## 10. What must be measured before implementing +### 9.1 Proven by measurement on this machine (PipeWire 1.6.8 / WirePlumber 0.5.15) -**Nothing in §4.2's predicate can be finalized without this.** A `pw-dump` of this -machine with peerspeak in a call (AEC on), mpv playing a viewed share, a notification -sound firing, and representative browser/game streams: +- 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. -| # | Question | Resolves | -| --- | --- | --- | -| OPEN-Q 1 | What properties distinguish ordinary app playback from infrastructure legs? | §4.2 predicate | -| OPEN-Q 2 | Does `sink_input_properties=node.name=…` surface on the registry node? | §4.3 tag | -| OPEN-Q 3 | Can `module-echo-cancel` legs carry a tag from `pactl load-module`? If not, what identifies them? (node names are PID-stamped: `echo_cancel.rs:80-82`) | §4.1 row 4 | -| OPEN-Q 4 | Real fallback behaviour per stream class when the capture sink vanishes — graceful vs SIGKILL | §7 | -| OPEN-Q 5 | Does WirePlumber reject the capture-monitor cycle, or is it audible feedback? | §4.3 | -| OPEN-Q 6 | Does `PIPEWIRE_PROPS` / `PULSE_PROP` env tagging actually land on mpv/VLC/pw-play nodes? | **✅ RESOLVED POSITIVELY — measured, see below** | -| OPEN-Q 7 | Does pixelpass currently handle SIGTERM at all on the host path? | **✅ RESOLVED: no** — only `tokio::signal::ctrl_c()` (SIGINT), `pixelpass/src/common/signal.rs:6`. Use SIGINT, not SIGTERM, for the graceful path in §6 | -| OPEN-Q 8 | `@DEFAULT_SINK@` behaviour with the user's real hotkey scripts, old sink still present | §8.2 | +### 9.2 ⚠️ Wording discipline — three overclaims already made, do not make a fourth -### 10.1 OPEN-Q 6 — MEASURED 2026-07-20 on this machine +The correct statement of the exclusion result is: -Codex's round-2 review asserted this was "resolved negatively — environment inheritance -is not a backend-independent ownership contract; the redesign collapses as written," -on the grounds that `PULSE_PROP` is undocumented and "absent from the installed libpulse -binaries." +> *"No incremental 1500 Hz energy was detectable above the control floor at the analysis +> resolution in this steady-state run."* -**That is factually wrong and the experiment disproves it.** `PULSE_PROP` and -`PULSE_PROP_OVERRIDE` are both present in `/usr/lib/pulseaudio/libpulsecommon-17.0.so` -(where `pa_proplist_update_from_environment` lives; `libpulse.so.0` links it), and the -tag reaches the graph for every player peerspeak actually spawns: +**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. -| Client | Backend | Env used | `peerspeak.owned` on the node | -| --- | --- | --- | --- | -| `paplay` | Pulse-emulated | `PULSE_PROP` | **`1`** ✅ | -| `mpv` | default ao | `PULSE_PROP` + `PIPEWIRE_PROPS` | **`1`** ✅ | -| `vlc` (LibVLC 3.0.23) | `client.api=pipewire-pulse` | `PULSE_PROP` | **`1`** ✅ | +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. -Method: play a 12 s 48 kHz tone under each client with the env set, then -`pw-dump | jq` the `Stream/Output/Audio` nodes. VLC on this box has no native PipeWire -aout (`/usr/lib/vlc/plugins/audio_output/` = dummy, file, alsa, amem, pulse) — Codex is -right about that — but it is irrelevant, because its Pulse output goes through libpulse -and `PULSE_PROP` is honoured there. +**Minimum rig upgrade before shipping:** two orthogonal PN/MLS probes + windowed +per-channel normalized cross-correlation, reporting max per-window correlation, plus xrun +telemetry. -**So §4.1's ownership model stands for the three spawned-child cases.** Codex's *other* -objection to it — that the tag is client-controlled and spoofable, so it is a -correctness mechanism and explicitly **not** a security boundary — is correct and is -now stated as such in §4.1. +### 9.3 Untested — carried forward, each one a real risk -**Also measured, and it corrects both prior reviews:** `application.process.id` on the -`paplay` node was **paplay's own PID (192571)**, not pipewire-pulse's. Codex asserted -the pipewire-pulse claim in round 1, then retracted it in round 2; the retraction is -right. The server-PID warning belongs to `pipewire.sec.pid` (which was *absent* on -these nodes). PID remains untrusted and may be missing or overridden, so it stays a -fallback signal — but it was dismissed too aggressively in v1 and v2. +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. -**Still genuinely open: OPEN-Q 3, the AEC playback leg**, which is the one owned-audio -path that cannot inherit an environment tag because pactl loads it inside -`pipewire-pulse`. Codex reports the Pulse-compat `module-echo-cancel` exposes only -`sink_properties` (populating the virtual sink, not the playback stream), so the options -are: load the *native* module instead, correlate the four legs by module/group identity, -or redesign the AEC target. **Not yet verified on this machine** — verifying it requires -loading the module on the live graph. +## 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 -Everything here is a placeholder. Candidates for the pixelpass mode: -`--audio-mode=desktop-routed`, `--audio-mode=desktop-excluding`. Internal selector: -`--exclude-audio-tag=`. Do **not** surface `--exclude-pid` — it names an -unreliable mechanism and reads like process control. +Placeholders throughout. pixelpass mode: `--audio-mode=desktop-shared` / +`--audio-mode=desktop-excluding`. Exclusion flag: `--exclude-pulse-module ` (or +`--aec `, §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," with a note -that call and watched-share playback are excluded. Whether "All system audio" stays -alongside it (with a stated echo risk) or is replaced when the capability is present is -a product decision. +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 and unit-testable, PipeWire at the edges, per house style: +Pure, unit-testable seams with PipeWire at the edges, per house style: -- `fn eligibility(node_props, &Ownership) -> Eligibility` — allowlist semantics, with - `NotEligible { reason }` so status can explain itself. Cases: peerspeak tag present; - AEC leg; own local-monitor node name; another host's local monitor; `dont-move`; - non-`Stream/Output/Audio`; missing props; the capture sink itself. -- Pure module-argument construction for both loopbacks, asserting the §4.3 tag and - `sink_dont_move`. -- Pure prior-target capture/restore logic (§7). -- Pure capability-probe parsing (§9). +- `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** off. +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, - SIGTERM, SIGINT, SIGKILL, last-viewer disconnect, pipewire-pulse restart, - `pixelpass --repair`. +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. Sample-rate/channel/passthrough behaviour on real sinks; CPU and latency cost of - routing the whole desktop. +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 .monitor`, not `pw-record --target `, which +reads −91 dB silence). ## 13. Open decisions for the reviewer -1. Is tag-inheritance (§4.1) sound as the ownership contract, and is OPEN-Q 6 the right - thing to gate the whole design on? -2. Is allowlist-eligibility (§4.2) the right risk posture, given it means some app audio - silently won't be shared until its shape is recognized? -3. Is the graceful-stop work (§6) properly a prerequisite commit, or does it land inside - this feature? -4. Should the two pre-existing defects (§8.2 output switching, §8.8 `object.serial`) - be split into their own tasks ahead of this? -5. Is there a materially simpler design that meets Joe's ask that both reviews have - missed? +- **D1** — Is the fail-closed posture of §5.3 (refuse the mode when AEC identity cannot be + validated; stop fan-out on revocation) the right trade against "share anyway, warn"? +- **D2** — Capability negotiation: detection today is a `pixelpass --help` substring probe + (`src/screenshare/mod.rs:245-278`, invoked at `src/core/mod.rs:3368`). This mode must + **not** overload `app_audio_supported: bool` — strict per-app and desktop-excluding are + independent capabilities. Enum, bitset, or a version handshake? +- **D3** — Foreign/second AEC policy (§5.4): fail closed, or warn + exclude all + `echo-cancel-*`? +- **D4** — What is the actual regression guard for drop order (§7.2)? +- **D5** — Explicit `--aec off` vs flag-absent (§8)? +- **D6** — Do the §10 prerequisites all land first, or only 1, 4 and 5? +- **D7** — Is there a materially simpler design meeting Joe's ask that three rounds have + missed?