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