From 3427ec56fcdb5576df2f676c5541892fc626fa5b Mon Sep 17 00:00:00 2001 From: atagen Date: Thu, 21 May 2026 18:24:01 +1000 Subject: [PATCH] F3: force-bypass surround streams; generalise N-channel pairing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex flagged that routing was channel-blind and the explicit-link pairer hardcoded `take(2)`. For a 5.1 stream the consequences depended on the route decision: Route::Processed silently dropped the centre, LFE, and both surround channels (only FL/FR linked to the stereo processed sink); Route::Bypass to a 5.1-capable real sink had the destruction pass kill 4 of 6 links because they weren't in the 2-pair `want_set`. Either way the user lost channels. PLAN §12 already documented the intent ("anything >2ch is routed directly to the real sink, bypass behaviour, regardless of profile rule") but the code didn't honour it. This commit makes the contract load-bearing. Changes - `PwNodeInfo` gains `audio_channels: Option`, populated in `build_node_info` from the stream's `audio.channels` property. `None` for clients that don't advertise (older PW, odd toolkits) — those fall through to normal rule evaluation on the assumption they're stereo or mono. - `routing::evaluate` short-circuits to `Route(Bypass)` when `audio_channels > 2`, ahead of rule matching. The bus filter is F32 stereo by construction, so this is the only honest answer: forcing surround into the processed path either drops channels or invents an unrequested downmix. - `apply_pending_routes`' link pairing generalised from `take(2)` to `take(min(src_outs.len(), target_ins.len()))`. Stereo → stereo is unchanged (`min(2, 2) = 2`); 5.1 → 5.1 real sink now pairs all six channels; 5.1 → stereo real sink pairs two (PipeWire's source-side adapter does the downmix, which is its job, not ours). The destruction pass already only nukes links to *known sinks*, so taps + non-sink consumers stay untouched as before. - PLAN §12 updated: the surround bullet now describes enforced behaviour rather than aspirational documentation. Tests - `routing::tests::surround_streams_force_bypass_regardless_of_rule_match` — a 6-channel stream matching the default profile's "browser is processed" rule must still bypass. - `routing::tests::stereo_and_mono_streams_follow_normal_rules` — confirms the forcer only triggers for `>2ch` (None, Some(1), Some(2) all flow through to the rule). 188 tests pass; clippy clean at -D warnings --all-targets. Live regression check (stereo 1 kHz sine into processed): 51 non-floor meter ticks over 3 s, bus DSP path still flowing, integrated LUFS around -28. Stereo path unaffected by the generalised pairing. --- PLAN.md | 13 ++++++- crates/headroom-core/src/pw/registry.rs | 20 ++++++++-- crates/headroom-core/src/routing.rs | 51 +++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 6 deletions(-) diff --git a/PLAN.md b/PLAN.md index c2b5374..4cc64a1 100644 --- a/PLAN.md +++ b/PLAN.md @@ -1114,8 +1114,17 @@ for current status per risk. filter should source its rate from the real sink and convert on the capture side only. - **Surround content downmix vs. passthrough.** v0 punts: anything - >2ch is routed directly to the real sink (bypass behaviour) - regardless of profile rule. Documented behaviour. + `>2ch` is force-bypassed regardless of profile rule. The bus + filter is F32 stereo by construction and pulling a 5.1+ stream + into it would either drop the centre/LFE/surround channels (with + explicit links pairing only the first two ports) or run our DSP + on a downmix that wasn't asked for. The check fires in + `routing::evaluate` based on `PwNodeInfo.audio_channels` (parsed + from the stream's `audio.channels` property). The explicit-link + pairing in `apply_pending_routes` was generalised from `take(2)` + to `take(min(src, dst))` so wide bypass to a wide real sink links + all channels; narrower sinks let PipeWire's source-side adapter + handle downmix as usual. --- diff --git a/crates/headroom-core/src/pw/registry.rs b/crates/headroom-core/src/pw/registry.rs index 13c0790..6a0bdd4 100644 --- a/crates/headroom-core/src/pw/registry.rs +++ b/crates/headroom-core/src/pw/registry.rs @@ -969,8 +969,19 @@ impl RoutingState { tracing::debug!(node_id, target_node, "pending route: target has no input ports yet"); continue; }; - // Stereo v0 — pair by ordinal. - if src_outs.len() < 2 || target_ins.len() < 2 { + // Pair by ordinal up to whichever side has fewer + // channels. For stereo→stereo this is the original + // `take(2)`. For wider streams (surround) routed + // Bypass to a wide sink we pair all N channels — fixes + // the F3 bug where the old `take(2)` silently dropped + // the centre, LFE, and surround channels of a 5.1 + // stream. If the wider side has more channels than the + // narrower side, the extra ports are left unlinked + // here; the destruction pass below also leaves them + // alone (they don't land on the target sink at all). + // PipeWire's adapter is responsible for any downmix. + let pair_count = src_outs.len().min(target_ins.len()); + if pair_count < 2 { tracing::debug!( node_id, src_outs = src_outs.len(), @@ -981,8 +992,8 @@ impl RoutingState { } let want: Vec<(u32, u32)> = src_outs .iter() - .take(2) - .zip(target_ins.iter().take(2)) + .take(pair_count) + .zip(target_ins.iter().take(pair_count)) .map(|(o, i)| (o.port_id, i.port_id)) .collect(); let want_set: std::collections::HashSet<(u32, u32)> = want.iter().copied().collect(); @@ -1515,6 +1526,7 @@ fn build_node_info(node_id: u32, dict: &DictRef) -> PwNodeInfo { .map(str::to_owned), media_role: dict.get("media.role").map(str::to_owned), dont_move: dict.get("node.dont-move") == Some("true"), + audio_channels: dict.get("audio.channels").and_then(|s| s.parse::().ok()), } } diff --git a/crates/headroom-core/src/routing.rs b/crates/headroom-core/src/routing.rs index 7f412b9..a2c194b 100644 --- a/crates/headroom-core/src/routing.rs +++ b/crates/headroom-core/src/routing.rs @@ -34,6 +34,15 @@ pub struct PwNodeInfo { /// `node.dont-move` — if set true, the stream opted out of being /// rerouted. Honoured by skipping routing entirely. pub dont_move: bool, + /// `audio.channels` — the stream's declared channel count. + /// `None` if the property is absent (older PipeWire / odd + /// clients). Used to force `>2ch` streams onto the bypass + /// path: the bus filter is stereo-only by construction, so + /// pulling a 5.1 stream into the processed sink would either + /// drop four channels (if we used explicit links naively) or + /// produce a downmix that wasn't asked for. Either way the + /// safer default is "leave surround alone." + pub audio_channels: Option, } impl PwNodeInfo { @@ -64,6 +73,17 @@ pub fn evaluate(info: &PwNodeInfo, profile: &Profile) -> RoutingDecision { if !info.is_routable_playback() { return RoutingDecision::Skip; } + // Force-bypass anything wider than stereo. PLAN §3's surround + // contract: the bus filter is F32 stereo by construction, so + // pulling a 5.1+ stream into `headroom-processed` either drops + // channels (with explicit links) or produces an unrequested + // downmix (if WP's adapter gets involved). Routing it straight + // to the real sink preserves the user's intended layout. If + // the real sink isn't 5.1-capable PipeWire's source-side + // adapter handles the downmix — that's its job, not ours. + if matches!(info.audio_channels, Some(ch) if ch > 2) { + return RoutingDecision::Route(Route::Bypass); + } for rule in &profile.rules { if matches(info, &rule.match_) { return RoutingDecision::Route(rule.route); @@ -124,6 +144,37 @@ mod tests { assert_eq!(evaluate(&info, &profile), RoutingDecision::Skip); } + #[test] + fn surround_streams_force_bypass_regardless_of_rule_match() { + // The default profile routes `firefox` to processed. A 5.1 + // firefox stream (rare but valid — some browser content + // declares surround) must still bypass: the bus filter is + // stereo-only and the explicit-link path would otherwise + // drop FC/LFE/SL/SR. PLAN §3 surround contract. + let mut info = playback("firefox"); + info.audio_channels = Some(6); + let profile = Profile::default_v0(); + assert_eq!( + evaluate(&info, &profile), + RoutingDecision::Route(Route::Bypass) + ); + } + + #[test] + fn stereo_and_mono_streams_follow_normal_rules() { + // Sanity: the surround forcer only kicks in for >2ch. + let profile = Profile::default_v0(); + for ch in [None, Some(1), Some(2)] { + let mut info = playback("firefox"); + info.audio_channels = ch; + assert_eq!( + evaluate(&info, &profile), + RoutingDecision::Route(Route::Processed), + "channels={ch:?}" + ); + } + } + #[test] fn matches_bypass_rule_for_known_music_player() { let info = playback("mpv");