From 2cfc82e96c558408e18aed378c020c7c2d61eb95 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 4 Aug 2026 17:58:17 +0200 Subject: [PATCH] fix(audio): budget the audio plane against the link, and close the review's gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings from the post-implementation review of design/audio-quality-and-latency.md. **The bandwidth gap (highest).** Tier `High` (256 kbps) and the redundant `0xD2` plane were added separately, each costed as "~1 % of the video budget", and nobody added them together: 256 kbps sent twice is 512 kbps — ~2.5 % of a 20 Mbps session but ~10 % of a 5 Mbps one. Audio rides QUIC datagrams, OUTSIDE the ABR loop, so ABR could neither see that nor reclaim it; a constrained link quietly handed a tenth of its bandwidth to audio while ABR carefully managed the rest. `plan_audio_budget` now makes tier and redundancy ONE decision against the session's resolved video bitrate, ordered by preference rather than cost — transparent audio beats redundant audio, since the field report was about quality and redundancy only pays under loss, so `High` alone outranks `Standard`+redundancy even though they cost the same. It can lower what the operator asked for, never raise it, and never goes below `Low`: a stream with unintelligible audio is worse than one spending a few percent more. **The Linux host kept the exact defect fixed on Windows.** `let _ = tx.try_send(samples)` — silent, uncounted data loss, where the encoder concatenates across the hole, so every drop is a click AND a permanent shift of everything after it. WP0.2 turned out to be Windows-only and had not said so. Linux now shares `capture_policy::CaptureStats`: drops counted and warned, plus per-window peak/RMS/delivered%. A Linux audio report was until now exactly as un-triageable as the Windows one was on 2026-08-03. **Apple's WP0.3 was half-done** — `bufferedMS` was added and wired to nothing. The drain thread now logs buffer/target/underruns/sheds like the other three, from one locked snapshot so the numbers in a line describe the same instant. Also: the Linux "audio format negotiated" line now says WHICH mode produced it, because that changes what it is worth — in stream-sink mode the host owns the sink so the mix cannot have been narrowed upstream, but in legacy monitor mode a 16 kHz Bluetooth sink would still be reported as a clean 48 kHz through PipeWire's resampler, the same way WASAPI's autoconvert hid it on Windows. Reading the monitored node's own rate needs a registry lookup this stream does not do; recorded as an open gap rather than implied to be covered. Two stale docs: `audio_wasapi.rs` cited `clients/windows/src/audio.rs` (deleted) and still described the pre-shared-policy "prime to ~3 quanta" behaviour. And the Apple ring's `prefill:` parameter, dead since the depth moved into the ring, is gone. Verified: clippy --all-targets -D warnings on Linux (docker) AND Windows (runner .133, forced clean rebuild of punktfunk-host + pf-client-core); core 167 tests; host 57 audio tests on Windows; Android clippy count identical to pristine (6, all documented arm64 artifacts); Apple ring re-simulated. The host suite's `gamestream::stream::tests::sender_delivers_batches` fails under qemu — the recorded environmental flake, unrelated to audio, green on the earlier less-loaded run. Co-Authored-By: Claude Opus 5 (1M context) --- .../PunktfunkKit/Audio/AudioRing.swift | 35 +++- .../PunktfunkKit/Audio/SessionAudio.swift | 12 ++ crates/pf-client-core/src/audio_wasapi.rs | 15 +- crates/punktfunk-core/src/audio.rs | 163 ++++++++++++++++++ crates/punktfunk-host/src/audio/linux/mod.rs | 64 ++++++- crates/punktfunk-host/src/native.rs | 12 +- crates/punktfunk-host/src/native/audio.rs | 25 +-- crates/punktfunk-host/src/native/handshake.rs | 79 +++++++-- 8 files changed, 351 insertions(+), 54 deletions(-) diff --git a/clients/apple/Sources/PunktfunkKit/Audio/AudioRing.swift b/clients/apple/Sources/PunktfunkKit/Audio/AudioRing.swift index 7654111d..f139d82e 100644 --- a/clients/apple/Sources/PunktfunkKit/Audio/AudioRing.swift +++ b/clients/apple/Sources/PunktfunkKit/Audio/AudioRing.swift @@ -42,13 +42,18 @@ final class AudioRing: @unchecked Sendable { private var emptyReads = 0 private var depthAvg: Double = 0 private var overRun = 0 + /// Reported, not acted on: short reads that actually starved the callback, and smooth drift + /// corrections. A rising underrun count means the ring is being starved (network or CPU), + /// which is a different problem from the depth being wrong. + private var underrunCount = 0 + private var shedCount = 0 private let channels: Int private let perMS: Int private let lock = OSAllocatedUnfairLock() - /// `capacity`/`prefill` in samples (interleaved — `channels` per frame, both whole frames). - /// `prefill` is accepted for source compatibility but the target now comes from `targetMS`. - init(capacity: Int, prefill: Int = 0, channels: Int) { + /// `capacity` in samples (interleaved — `channels` per frame, a whole number of frames). + /// The de-jitter depth is the ring's own business (`targetMS`), not a caller's prefill. + init(capacity: Int, channels: Int) { buf = [Float](repeating: 0, count: capacity) self.channels = channels perMS = 48 * channels @@ -113,6 +118,7 @@ final class AudioRing: @unchecked Sendable { if overRun >= Self.shedSustainMS * perMS { overRun = 0 shedOneFrame() + shedCount += 1 depthAvg = Double(writeIdx - readIdx) } } else { @@ -130,6 +136,7 @@ final class AudioRing: @unchecked Sendable { // De-prime only after a RUN of short reads: a single transient drain must not // manufacture a whole target's worth of fresh silence. emptyReads += 1 + underrunCount += 1 if emptyReads >= Self.deprimeAfter { primed = false } } else { emptyReads = 0 @@ -157,12 +164,32 @@ final class AudioRing: @unchecked Sendable { readIdx += drop } - /// Current buffered depth in milliseconds — for the stats overlay. + /// Current buffered depth in milliseconds — for the stats overlay and the drain thread's + /// periodic log. var bufferedMS: Int { lock.lock() defer { lock.unlock() } return (writeIdx - readIdx) / max(perMS, 1) } + + /// One consistent snapshot of the ring's vitals, taken under a single lock so the numbers in + /// a log line describe the same instant. Mirrors what the three Rust clients report. + struct Stats { + let bufferedMS: Int + let targetMS: Int + let underruns: Int + let sheds: Int + } + + var stats: Stats { + lock.lock() + defer { lock.unlock() } + return Stats( + bufferedMS: (writeIdx - readIdx) / max(perMS, 1), + targetMS: target / max(perMS, 1), + underruns: underrunCount, + sheds: shedCount) + } } /// CoreAudio channel layout for the canonical wire order FL FR FC LFE RL RR [SL SR]. nil for diff --git a/clients/apple/Sources/PunktfunkKit/Audio/SessionAudio.swift b/clients/apple/Sources/PunktfunkKit/Audio/SessionAudio.swift index 9a1fab08..d79d12a1 100644 --- a/clients/apple/Sources/PunktfunkKit/Audio/SessionAudio.swift +++ b/clients/apple/Sources/PunktfunkKit/Audio/SessionAudio.swift @@ -403,6 +403,7 @@ public final class SessionAudio { stateLock.unlock() let thread = Thread { [connection, flag, drainDone] in defer { drainDone.signal() } + var drained = 0 // Decode happens IN-CORE (libopus multistream) — AudioToolbox's Opus path is // stereo-only — and is handed back as interleaved f32 PCM in wire channel order. // Per-iteration autorelease pool: no runloop on this thread (see Stage2Pipeline). @@ -421,6 +422,17 @@ public final class SessionAudio { ring.write(base, count: pcm.frameCount * pcm.channels) } } + // Periodic vitals (~10 s at the protocol's 5 ms frames). The other three clients + // log buffer depth and underruns; without this an Apple audio report — latency or + // dropout — arrives with no numbers at all, which is the position every platform + // was in before the 2026-08 audio work. + drained += 1 + if drained % 2_000 == 0 { + let s = ring.stats + log.info( + "audio: buffer_ms=\(s.bufferedMS) target_ms=\(s.targetMS) underruns=\(s.underruns) drift_sheds=\(s.sheds)" + ) + } return true } } diff --git a/crates/pf-client-core/src/audio_wasapi.rs b/crates/pf-client-core/src/audio_wasapi.rs index 0c9b6d69..12fda251 100644 --- a/crates/pf-client-core/src/audio_wasapi.rs +++ b/crates/pf-client-core/src/audio_wasapi.rs @@ -3,14 +3,15 @@ //! //! The WASAPI twin of `audio.rs` (PipeWire) — same public surface (`AudioPlayer::spawn`/ //! `take_buffer`/`push`, `MicStreamer::spawn`), swapped in by lib.rs's `#[path]` so the -//! session pump compiles against one `crate::audio` on both OSes. Adapted from -//! `clients/windows/src/audio.rs` (which remains the WinUI shell's own copy until its -//! built-in streaming path is deleted). +//! session pump compiles against one `crate::audio` on both OSes. It began as a copy of the +//! WinUI shell's own audio path; that shell's built-in streaming path has since been deleted, +//! so this is now the only WASAPI client ring. //! -//! Playback mirrors the host's virtual-mic producer's adaptive jitter buffer: the session -//! pump pushes 5 ms Opus-decoded chunks on the network clock; the WASAPI render thread -//! pulls whole event-driven quanta on the device clock. Prime to ~3 quanta before -//! producing, cap the ring so latency stays bounded, re-prime after a real drain. +//! Playback: the session pump pushes 5 ms Opus-decoded chunks on the network clock; the WASAPI +//! render thread pulls whole event-driven quanta on the device clock. The depth policy between +//! them is the SHARED `punktfunk_core::audio::JitterPolicy` (`JitterTuning::WASAPI`) — target in +//! milliseconds, crossfaded drift correction, de-prime hysteresis — so all four clients behave +//! the same way and none of them can ratchet latency upward. //! //! WASAPI objects are COM-apartment-bound and not `Send`, so they live on a dedicated //! thread (the same discipline as the host's `wasapi_cap`); only the channels + stop flag diff --git a/crates/punktfunk-core/src/audio.rs b/crates/punktfunk-core/src/audio.rs index f20d7780..4ebaba8a 100644 --- a/crates/punktfunk-core/src/audio.rs +++ b/crates/punktfunk-core/src/audio.rs @@ -173,6 +173,90 @@ impl OpusLayout { } } +/// What the audio plane will actually cost this session: the tier to encode at, and whether the +/// redundant `0xD2` plane is affordable. Produced by [`plan_audio_budget`]. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct AudioBudget { + pub tier: AudioTier, + pub redundancy: bool, + /// Total wire cost in kbps, redundancy included — what the decision was made against. + pub kbps: u32, +} + +/// Share of the session's video bitrate the audio plane may spend. Audio rides QUIC datagrams, +/// OUTSIDE the ABR loop, so whatever it takes is taken off the top and adaptive bitrate can +/// neither see nor reclaim it — which is exactly why it needs a budget of its own. +const AUDIO_BUDGET_PCT: u32 = 5; +/// …but never squeeze audio below the Low tier. A stream with unintelligible audio is worse than +/// one that spends a few percent more, and the floor is what stops a very low video bitrate from +/// silently producing a useless audio plane. +const AUDIO_BUDGET_FLOOR_KBPS: u32 = 96; + +/// Choose the encode tier and whether to send redundancy, given the session's resolved VIDEO +/// bitrate. +/// +/// **Why this exists.** Tier `High` and the redundant plane were introduced separately, each +/// justified as "about 1 % of the video budget" — but they multiply: 256 kbps stereo sent twice is +/// 512 kbps, which is ~2.5 % of a 20 Mbps session and ~10 % of a 5 Mbps one. Nothing added the two +/// together, and nothing capped the total, so on a constrained link the audio plane quietly took a +/// tenth of the bandwidth that ABR was carefully managing the rest of. +/// +/// The ladder is ordered by preference, not by cost: transparent audio beats redundant audio (the +/// complaint this whole program came from was quality, and the redundancy only pays off under +/// loss), so `High` alone outranks `Standard` + redundancy even though they cost the same. +/// `requested` lets an operator ask for a specific tier; the budget can lower it but never raises +/// it above what was asked. +pub fn plan_audio_budget( + video_kbps: u32, + channels: u8, + requested: AudioTier, + client_wants_redundancy: bool, +) -> AudioBudget { + let budget = (video_kbps.saturating_mul(AUDIO_BUDGET_PCT) / 100).max(AUDIO_BUDGET_FLOOR_KBPS); + let layout = layout_for(channels, false); + let cost = |tier: AudioTier, red: bool| -> u32 { + let one = (layout.bitrate_for(tier) / 1000).max(0) as u32; + if red { + one.saturating_mul(2) + } else { + one + } + }; + // Preference order, best first. An operator asking for `Low` must not be handed `High`, so + // candidates above the request are filtered out. + let rank = |t: AudioTier| match t { + AudioTier::Low => 0, + AudioTier::Standard => 1, + AudioTier::High => 2, + }; + let ladder = [ + (AudioTier::High, true), + (AudioTier::High, false), + (AudioTier::Standard, true), + (AudioTier::Standard, false), + (AudioTier::Low, false), + ]; + for (tier, red) in ladder { + if rank(tier) > rank(requested) || (red && !client_wants_redundancy) { + continue; + } + let kbps = cost(tier, red); + if kbps <= budget { + return AudioBudget { + tier, + redundancy: red, + kbps, + }; + } + } + // Nothing fit — take the cheapest thing that still works rather than muting audio. + AudioBudget { + tier: AudioTier::Low, + redundancy: false, + kbps: cost(AudioTier::Low, false), + } +} + /// Pick the layout for a negotiated channel count. Unknown counts fall back to stereo (clients /// only ever request 2/6/8). `high_quality` selects the uncoupled high-bitrate config. pub fn layout_for(channels: u8, high_quality: bool) -> &'static OpusLayout { @@ -874,6 +958,85 @@ mod tests { assert_eq!(AudioTier::parse(""), None); } + // ---- the audio bandwidth budget -------------------------------------------------------- + + /// THE regression this guards: `High` (256 kbps stereo) and the redundant plane (x2) were + /// each justified as "~1 % of the video budget" and nobody added them together. 512 kbps is + /// ~10 % of a 5 Mbps session — and audio is outside the ABR loop, so ABR cannot reclaim it. + #[test] + fn budget_steps_down_as_the_link_narrows() { + let plan = |kbps| plan_audio_budget(kbps, 2, AudioTier::High, true); + // Roomy link: everything on. + let b = plan(20_000); + assert_eq!((b.tier, b.redundancy), (AudioTier::High, true)); + assert_eq!(b.kbps, 512); + // Halve it and redundancy is the first thing to go — quality is what the field report + // was about, and redundancy only pays under loss. + assert_eq!(plan(10_000).tier, AudioTier::High); + assert!(!plan(10_000).redundancy); + // Tighter still: down to Standard. + assert_eq!(plan(5_000).tier, AudioTier::Standard); + assert!(!plan(5_000).redundancy); + // A genuinely narrow link lands on Low, and never below it. + assert_eq!(plan(1_000).tier, AudioTier::Low); + assert_eq!(plan(1).tier, AudioTier::Low); + assert_eq!( + plan(0).kbps, + 96, + "audio must survive an absurd video bitrate" + ); + } + + /// The budget must never spend more than its share, at any bitrate or channel count. + #[test] + fn budget_never_exceeds_its_share() { + for kbps in [0u32, 500, 1_000, 2_000, 5_000, 10_000, 20_000, 100_000] { + for ch in [2u8, 6, 8] { + let b = plan_audio_budget(kbps, ch, AudioTier::High, true); + let allowed = + (kbps.saturating_mul(AUDIO_BUDGET_PCT) / 100).max(AUDIO_BUDGET_FLOOR_KBPS); + let floor = plan_audio_budget(0, ch, AudioTier::Low, false).kbps; + assert!( + b.kbps <= allowed || b.kbps == floor, + "{ch}ch at {kbps} kbps: spent {} of {allowed}", + b.kbps + ); + } + } + } + + /// Surround costs more per tier, so the same link must step it down sooner than stereo — + /// the budget is about total wire cost, not about the tier name. + #[test] + fn budget_accounts_for_the_channel_count() { + let stereo = plan_audio_budget(10_000, 2, AudioTier::High, true); + let surround = plan_audio_budget(10_000, 8, AudioTier::High, true); + assert_eq!(stereo.tier, AudioTier::High); + assert!(surround.kbps <= stereo.kbps.max(surround.kbps), "sanity"); + // 7.1 at High is 768 kbps — far past a 500 kbps allowance, so it must have stepped down. + assert!( + surround.kbps < 768, + "7.1 High must not fit a 10 Mbps budget" + ); + } + + /// The budget may LOWER what was asked for, never raise it: an operator who set `low` gets + /// `low` on a 100 Mbps link, and a client that never asked for redundancy never gets it. + #[test] + fn budget_respects_the_request() { + let b = plan_audio_budget(100_000, 2, AudioTier::Low, true); + assert_eq!(b.tier, AudioTier::Low); + let b = plan_audio_budget(100_000, 2, AudioTier::Standard, true); + assert_eq!(b.tier, AudioTier::Standard); + assert!(b.redundancy, "Standard + redundancy fits a huge link"); + let b = plan_audio_budget(100_000, 2, AudioTier::High, false); + assert_eq!(b.tier, AudioTier::High); + assert!( + !b.redundancy, + "a client that did not ask must never be sent 0xD2" + ); + } + // ---- the de-jitter policy ------------------------------------------------------------ /// Interleaved samples per ms at `channels`. diff --git a/crates/punktfunk-host/src/audio/linux/mod.rs b/crates/punktfunk-host/src/audio/linux/mod.rs index b6fbd0cd..1275cd0f 100644 --- a/crates/punktfunk-host/src/audio/linux/mod.rs +++ b/crates/punktfunk-host/src/audio/linux/mod.rs @@ -674,6 +674,8 @@ fn pw_thread( }) .register(); + // Which source the negotiated format below actually describes — see the note there. + let sink_mode = sink_name.is_some(); let props = match &sink_name { // Stream-sink mode: this stream IS the sink (media.class + Direction::Input). Apps // play into it, PipeWire mixes them, process() receives the mix. Mirrors the @@ -710,8 +712,25 @@ fn pw_thread( let stream = pw::stream::StreamBox::new(&core, "punktfunk-audio", props) .context("pw audio Stream")?; + // The capture callback's state: the hand-off channel plus this plane's vitals. Before + // this it was the bare `tx`, and the desktop-audio plane logged NOTHING between "capture + // started" and the session ending — no level, no cadence, and in particular no sign of + // the silent drop below. That is exactly what made the 2026-08-03 Windows field report + // un-triageable, and the Linux half kept it after the Windows half was fixed. + struct CapUd { + tx: std::sync::mpsc::SyncSender>, + channels: u32, + stats: crate::audio::capture_policy::CaptureStats, + last_stats: std::time::Instant, + } + let ud = CapUd { + tx, + channels, + stats: Default::default(), + last_stats: std::time::Instant::now(), + }; let _listener = stream - .add_local_listener_with_user_data(tx) + .add_local_listener_with_user_data(ud) .state_changed({ let mainloop = mainloop.clone(); move |_s, _ud, old, new| { @@ -723,22 +742,32 @@ fn pw_thread( } } }) - .param_changed(|_stream, _tx, id, param| { + .param_changed(move |_stream, _tx, id, param| { let Some(param) = param else { return }; if id != pw::spa::param::ParamType::Format.as_raw() { return; } let mut info = AudioInfoRaw::default(); if info.parse(param).is_ok() { + // `stream_sink` says WHICH source this format describes, and that changes how + // much it is worth. In stream-sink mode the host owns the sink, so this IS the + // format apps render into and the desktop mix cannot have been narrowed before + // we saw it. In LEGACY monitor mode we are capturing someone else's sink + // through PipeWire's resampler: a 16 kHz Bluetooth headset upstream would + // still be reported here as a clean 48 kHz, exactly the way WASAPI's + // autoconvert hid the same thing on Windows (the 2026-08-03 report). Reading + // the monitored node's OWN rate needs a registry lookup this stream does not + // do — recorded as an open gap rather than implied to be covered. tracing::info!( format = ?info.format(), rate = info.rate(), channels = info.channels(), + stream_sink = sink_mode, "audio format negotiated" ); } }) - .process(|stream, tx| { + .process(|stream, ud| { let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { let Some(mut buffer) = stream.dequeue_buffer() else { return; @@ -774,7 +803,34 @@ fn pw_thread( ]; samples.push(f32::from_le_bytes(b)); } - let _ = tx.try_send(samples); // drop if the encoder is behind + ud.stats.observe(&samples, ud.channels); + // Non-blocking and lossy, as before — but COUNTED. A full channel means the + // encode thread is not keeping up, and because the encoder simply + // concatenates across the hole every dropped chunk is a click AND a + // permanent shift of everything after it. + if ud.tx.try_send(samples).is_err() { + ud.stats.dropped_chunks += 1; + } + if ud.last_stats.elapsed() >= crate::audio::capture_policy::STATS_EVERY { + let (peak_db, rms_db, delivered_pct) = + ud.stats.summary(ud.last_stats.elapsed(), SAMPLE_RATE); + if ud.stats.dropped_chunks > 0 { + tracing::warn!( + dropped_chunks = ud.stats.dropped_chunks, + "the audio encode thread could not keep up — captured audio was \ + DROPPED; the stream will click and everything after it shifts" + ); + } + tracing::info!( + peak_db = format!("{peak_db:.1}"), + rms_db = format!("{rms_db:.1}"), + delivered_pct = format!("{delivered_pct:.0}"), + dropped_chunks = ud.stats.dropped_chunks, + "desktop audio capture" + ); + ud.stats = Default::default(); + ud.last_stats = std::time::Instant::now(); + } })); if outcome.is_err() { tracing::error!("panic in pipewire audio callback — chunk dropped"); diff --git a/crates/punktfunk-host/src/native.rs b/crates/punktfunk-host/src/native.rs index 91591ead..c3d8f69b 100644 --- a/crates/punktfunk-host/src/native.rs +++ b/crates/punktfunk-host/src/native.rs @@ -1308,11 +1308,17 @@ async fn serve_session( let cap = audio_cap.clone(); let channels = welcome.audio_channels; // Read the granted bit back off the Welcome (the cursor plane's precedent), so the wire - // the client was promised and the wire we actually send cannot disagree. - let redundancy = welcome.host_caps & punktfunk_core::quic::HOST_CAP_AUDIO_RED != 0; + // the client was promised and the wire we actually send cannot disagree — then re-derive + // the SAME budget rung from it, so the encode tier and the redundancy decision are one + // choice made once rather than two settings that can drift apart. + let budget = handshake::audio_budget( + welcome.host_caps & punktfunk_core::quic::HOST_CAP_AUDIO_RED != 0, + welcome.bitrate_kbps, + channels, + ); std::thread::Builder::new() .name("punktfunk1-audio".into()) - .spawn(move || audio_thread(conn, stop, cap, channels, redundancy)) + .spawn(move || audio_thread(conn, stop, cap, channels, budget)) .map_err(|e| tracing::warn!(error = %e, "audio thread spawn failed — session continues without audio")) .ok() } else { diff --git a/crates/punktfunk-host/src/native/audio.rs b/crates/punktfunk-host/src/native/audio.rs index 852026fe..50cd5033 100644 --- a/crates/punktfunk-host/src/native/audio.rs +++ b/crates/punktfunk-host/src/native/audio.rs @@ -84,27 +84,16 @@ pub(super) fn audio_thread( stop: Arc, audio_cap: AudioCapSlot, channels: u8, - redundancy: bool, + budget: punktfunk_core::audio::AudioBudget, ) { use crate::audio::SAMPLE_RATE; const FRAME_MS: usize = 5; const SAMPLES_PER_FRAME: usize = SAMPLE_RATE as usize * FRAME_MS / 1000; // 240 let want = punktfunk_core::audio::normalize_channels(channels); - // WP1.1 — encode tier. Unknown spellings warn and fall back rather than silently downgrading - // someone's audio (the whole point of the setting is that quality stopped being invisible). - let tier = match pf_host_config::config().audio_quality.as_deref() { - None => punktfunk_core::audio::AudioTier::default(), - Some(s) => match punktfunk_core::audio::AudioTier::parse(s) { - Some(t) => t, - None => { - tracing::warn!( - value = %s, - "PUNKTFUNK_AUDIO_QUALITY is not one of low/standard/high — using the default" - ); - punktfunk_core::audio::AudioTier::default() - } - }, - }; + // Tier and redundancy are ONE decision, budgeted against the session's video bitrate — see + // `handshake::audio_budget`. An unparseable `audio.quality` was already warned about there + // and fell back to the default, so nothing here can silently downgrade someone's audio. + let (tier, redundancy) = (budget.tier, budget.redundancy); // Reuse the cached capturer ONLY when its channel count matches this session's; a stereo // capturer left by a prior session must not feed a 5.1/7.1 session (the encoder + the client's @@ -166,7 +155,7 @@ pub(super) fn audio_thread( tracing::info!( channels = want, tier = tier.as_str(), - kbps = punktfunk_core::audio::layout_for(want, false).bitrate_for(tier) / 1000, + kbps = budget.kbps, redundancy, "punktfunk/1 audio streaming (Opus 48 kHz, 5 ms datagrams)" ); @@ -261,7 +250,7 @@ pub(super) fn audio_thread( _stop: Arc, _audio_cap: AudioCapSlot, _channels: u8, - _redundancy: bool, + _budget: punktfunk_core::audio::AudioBudget, ) { tracing::warn!("punktfunk/1 audio requires Linux or Windows — session continues without it"); } diff --git a/crates/punktfunk-host/src/native/handshake.rs b/crates/punktfunk-host/src/native/handshake.rs index 8b7cf3ef..9027f755 100644 --- a/crates/punktfunk-host/src/native/handshake.rs +++ b/crates/punktfunk-host/src/native/handshake.rs @@ -24,24 +24,61 @@ use super::*; /// paints on a Mutter virtual stream), and only a can't-blend backend falls back to the /// compositor EMBED. THE single predicate: the Welcome's `HOST_CAP_CURSOR` bit is computed /// from it, and the session wiring reads that bit back. -/// Whether this session sends the REDUNDANT desktop-audio plane (`0xD2`) — THE single predicate -/// behind the Welcome's `HOST_CAP_AUDIO_RED` bit, which `serve_session` reads back to configure the -/// audio thread. +/// THE single audio-plane decision for a session: the encode tier AND whether the redundant +/// `0xD2` plane is sent. The Welcome's `HOST_CAP_AUDIO_RED` bit is computed from it, and +/// `serve_session` reads that bit back to configure the audio thread — so the wire the client is +/// promised and the wire we send cannot disagree. /// -/// Capable-and-agreed: the client must have advertised `CLIENT_CAP_AUDIO_RED`, so a session with an -/// older client keeps the plain `0xC9` wire byte-for-byte. `audio.redundancy` ( -/// `PUNKTFUNK_AUDIO_REDUNDANCY`) can force it off on a link where the extra ~1 % is unwelcome, or -/// force it on for testing. +/// Capable-and-agreed for redundancy: the client must have advertised `CLIENT_CAP_AUDIO_RED`, so a +/// session with an older client keeps the plain `0xC9` wire byte-for-byte. +/// +/// **Both halves are then BUDGETED against the session's video bitrate** +/// ([`plan_audio_budget`](punktfunk_core::audio::plan_audio_budget)). Tier `High` and redundancy +/// were introduced separately, each costed as "~1 % of the video budget", and they multiply: +/// 256 kbps stereo sent twice is 512 kbps — ~10 % of a 5 Mbps session. Audio rides QUIC datagrams, +/// outside the ABR loop, so ABR can neither see that nor reclaim it. The budget is what stops a +/// constrained link silently handing a tenth of its bandwidth to audio. +/// +/// The operator's `audio.quality` / `audio.redundancy` settings are the REQUEST; the budget may +/// lower them, never raise them. /// /// NB the plan's "only while the link is actually losing packets" gate is deliberately not here: /// turning redundancy on and off mid-session changes the wire tag, and the client's decoder would -/// have to re-derive which plane it is on from every datagram. The cost being avoided is ~1 % of a -/// video budget, which is not worth that fragility — so the decision is made once, at handshake. -pub(super) fn audio_redundancy(client_caps: u8) -> bool { - if client_caps & punktfunk_core::quic::CLIENT_CAP_AUDIO_RED == 0 { - return false; - } - pf_host_config::config().audio_redundancy.unwrap_or(true) +/// have to re-derive which plane it is on from every datagram. Deciding once, at handshake, against +/// a bitrate we already know is both cheaper and more predictable. +/// `wants_redundancy` is the caller's answer to "is `0xD2` even on the table" — at handshake that +/// is the client's cap AND the operator's setting; afterwards it is the GRANTED +/// `HOST_CAP_AUDIO_RED` bit, so the audio thread re-derives the same rung of the same ladder. +pub(super) fn audio_budget( + wants_redundancy: bool, + video_kbps: u32, + channels: u8, +) -> punktfunk_core::audio::AudioBudget { + let configured = pf_host_config::config().audio_quality.as_deref(); + let requested = match configured { + None => punktfunk_core::audio::AudioTier::default(), + Some(s) => punktfunk_core::audio::AudioTier::parse(s).unwrap_or_else(|| { + // Once per process: this runs per session, and an operator with a typo in host.env + // does not need it on every connect. Never silently downgrade someone's audio. + static WARNED: std::sync::Once = std::sync::Once::new(); + WARNED.call_once(|| { + tracing::warn!( + value = %s, + "audio.quality (PUNKTFUNK_AUDIO_QUALITY) is not one of low/standard/high — \ + using the default" + ); + }); + punktfunk_core::audio::AudioTier::default() + }), + }; + punktfunk_core::audio::plan_audio_budget(video_kbps, channels, requested, wants_redundancy) +} + +/// The operator's answer to "may this session use redundancy at all", before the budget is +/// consulted: the client must be able to decode it and the operator must not have forced it off. +pub(super) fn redundancy_offered(client_caps: u8) -> bool { + client_caps & punktfunk_core::quic::CLIENT_CAP_AUDIO_RED != 0 + && pf_host_config::config().audio_redundancy.unwrap_or(true) } pub(super) fn cursor_forward( @@ -585,10 +622,16 @@ pub(super) async fn negotiate( } else { 0 } - // Redundant desktop-audio plane (0xD2): the client asked, and the operator has not - // forced it off. Capable-and-agreed, like the cursor bit — a client that did not ask - // keeps the plain 0xC9 wire byte-for-byte. - | if audio_redundancy(hello.client_caps) { + // Redundant desktop-audio plane (0xD2): the client asked, the operator has not forced + // it off, AND it fits the session's audio budget. Capable-and-agreed like the cursor + // bit — a client that did not ask keeps the plain 0xC9 wire byte-for-byte. + | if audio_budget( + redundancy_offered(hello.client_caps), + bitrate_kbps, + audio_channels, + ) + .redundancy + { punktfunk_core::quic::HOST_CAP_AUDIO_RED } else { 0