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 at88ad5a0/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 <noreply@anthropic.com>
494 lines
28 KiB
Markdown
494 lines
28 KiB
Markdown
# Design v3: whole-desktop screen-share audio without self-echo
|
||
|
||
**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."
|
||
**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 — ✅ 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("PULSE_PROP", …)` / `PIPEWIRE_PROPS` on the `Command` |
|
||
| `pw-play` / `paplay` / `aplay` (`src/notify.rs:265`) | same |
|
||
|
||
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. **`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.
|
||
|
||
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**:
|
||
|
||
- **At start:** given `--exclude-pulse-module <idx>`, 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.
|
||
|
||
### 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.
|
||
|
||
Agreed 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 |
|
||
| 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 |
|
||
|
||
Notes:
|
||
|
||
- `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.
|
||
|
||
## 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.
|
||
|
||
**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).**
|
||
|
||
### 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: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:
|
||
|
||
- 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. Open decisions for the reviewer
|
||
|
||
- **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?
|