docs: screenshare audio exclusion design v2
Rewrite after Codex's adversarial review of v1 found four release blockers, all independently verified against source. v1's premise was wrong: whole-desktop capture bypasses Routing::start entirely (pipeline.rs:121), so this needs a new capture mode rather than an inverted predicate. v2 replaces PID-based identity with ownership by inherited tag, and makes the router an allowlist so unrecognized infrastructure is left alone rather than optimistically moved. Graceful stop becomes a prerequisite: Stop Share is currently SIGKILL, so cleanup never runs on the normal path. Records live measurements taken 2026-07-20: PULSE_PROP tagging reaches the graph for paplay, mpv and VLC, and application.process.id is the client's own PID, not pipewire-pulse's — correcting a claim both the review and v1 relied on. Adds Option C (fan out a second owned link instead of moving streams), which deletes most of the cleanup, latency and multi-host problems the move-based design has to solve. Not yet implemented; gated on a feasibility spike. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.8
@@ -0,0 +1,461 @@
|
||||
# Design v2: 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)
|
||||
**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.
|
||||
|
||||
> **⚠️ 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.
|
||||
|
||||
---
|
||||
|
||||
## 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. Feasibility, and the correct 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.
|
||||
|
||||
So the feature is built by constructing a sink that only eligible streams feed, and
|
||||
capturing that. Same observable behaviour, 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.
|
||||
|
||||
**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.
|
||||
|
||||
## 3. What actually exists today (v1 got this wrong)
|
||||
|
||||
**v1's premise was false.** v1 claimed whole-desktop capture already loads
|
||||
`@DEFAULT_SINK@.monitor → pixelpass_capture_<pid>` and that the feature is "the
|
||||
existing per-app path with an inverted predicate." It is not.
|
||||
|
||||
`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`):
|
||||
|
||||
| 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@` | loaded 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.
|
||||
|
||||
Two further corrections to v1's model of the existing router:
|
||||
|
||||
- `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.
|
||||
|
||||
## 4. The core redesign: ownership by inherited tag, not by PID
|
||||
|
||||
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.**
|
||||
|
||||
Concretely, PID-matching misses:
|
||||
|
||||
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`).
|
||||
|
||||
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.
|
||||
|
||||
### 4.1 The proposal
|
||||
|
||||
**peerspeak tags every stream it owns, at creation, and the router excludes by tag.**
|
||||
|
||||
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*.
|
||||
|
||||
| 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` |
|
||||
| `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.
|
||||
|
||||
**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; 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.
|
||||
|
||||
### 4.2 Fail closed: eligibility, not exclusion
|
||||
|
||||
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.
|
||||
|
||||
**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.
|
||||
|
||||
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.
|
||||
|
||||
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:
|
||||
|
||||
```
|
||||
sink_input_properties=node.name=pixelpass_local_monitor_<host-id> sink_dont_move=true
|
||||
```
|
||||
|
||||
`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.
|
||||
|
||||
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**.
|
||||
|
||||
## 5. Topology (Option A, still preferred — for a corrected reason)
|
||||
|
||||
New host mode. Name TBD (§11); referred to here as *routed-desktop*.
|
||||
|
||||
```
|
||||
eligible app streams ────► pixelpass_capture_<pid> ──► gst capture ──► viewers
|
||||
(routed by StreamRouter) │
|
||||
└─ local-monitor loopback ──► @DEFAULT_SINK@ ──► speakers
|
||||
▲
|
||||
peerspeak-tagged playback ──────────────────────────────────────────────┘
|
||||
(never routed; AEC, call, mpv, notifications)
|
||||
```
|
||||
|
||||
- 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.
|
||||
|
||||
**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.
|
||||
|
||||
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.
|
||||
|
||||
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.
|
||||
|
||||
## 5.1 Option C — copy instead of move (Codex's proposal; now the leading candidate)
|
||||
|
||||
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*.**
|
||||
|
||||
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 |
|
||||
| --- | --- |
|
||||
| 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` |
|
||||
|
||||
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.
|
||||
|
||||
**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.
|
||||
|
||||
**Recommendation: a direct-link feasibility spike before any further work on Option A.**
|
||||
If fan-out holds on this graph, Option C supersedes Option A outright.
|
||||
|
||||
## 6. Graceful stop is a prerequisite, not a test case
|
||||
|
||||
**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.
|
||||
|
||||
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.
|
||||
|
||||
Required, in order:
|
||||
|
||||
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.
|
||||
|
||||
## 7. Restore, don't clear
|
||||
|
||||
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.
|
||||
|
||||
The prior value must be captured before the move and restored on cleanup. This is state
|
||||
the current router does not keep.
|
||||
|
||||
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.**
|
||||
|
||||
## 8. Remaining risks
|
||||
|
||||
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.
|
||||
|
||||
## 9. Capability negotiation
|
||||
|
||||
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`).
|
||||
|
||||
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.
|
||||
|
||||
## 10. What must be measured before implementing
|
||||
|
||||
**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:
|
||||
|
||||
| # | 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 |
|
||||
|
||||
### 10.1 OPEN-Q 6 — MEASURED 2026-07-20 on this machine
|
||||
|
||||
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."
|
||||
|
||||
**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:
|
||||
|
||||
| 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`** ✅ |
|
||||
|
||||
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.
|
||||
|
||||
**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.
|
||||
|
||||
**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.
|
||||
|
||||
**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.
|
||||
|
||||
## 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=<tag>`. 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.
|
||||
|
||||
## 12. Testing
|
||||
|
||||
Pure and unit-testable, 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).
|
||||
|
||||
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.
|
||||
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`.
|
||||
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.
|
||||
|
||||
## 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?
|
||||
Reference in New Issue
Block a user