From 5f52aa150639f9f58448c3fa8882fcca30055d7f Mon Sep 17 00:00:00 2001 From: Mollusk Date: Tue, 30 Jun 2026 16:56:19 -0400 Subject: [PATCH] W12 follow-up: consume in-band FEC, drop redundant DTX MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the two P2 efficacy findings from the Codex audit of the W12 profiles feature. FEC was enabled on the encoder but never used: the jitter buffer's loss path did pure PLC, so the redundancy was wasted bitrate. Now the gap path reconstructs the lost frame from the next buffered packet via Opus in-band FEC (new `AudioDecoder::decode_fec`, libopus decode with fec=true into a one-frame buffer), keeping that packet for its own normal decode and falling back to PLC if FEC decode fails. This is the documented libopus FEC pattern; receiver-side only, no wire change. DTX was enabled on BadNetwork but provided no benefit — the capture noise gate already suppresses silence transmission, and the broadcast DTX silence packets only created seq gaps that grew the jitter cushion. All profiles now set dtx=false (plumbing kept for a future revisit). Adds a jitter-buffer test proving FEC reconstruction beats pure PLC (RMS error < 0.75x) and that the FEC source packet stays buffered. 500 lib tests, clippy + fmt clean, release build clean. Co-Authored-By: Codex (gpt-5.5) Co-Authored-By: Claude Opus 4.8 --- src/app/mod.rs | 2 +- src/codec/mod.rs | 3 ++ src/codec/opus_impl.rs | 21 ++++++--- src/config.rs | 2 +- src/core/jitter.rs | 100 ++++++++++++++++++++++++++++++++++++++--- 5 files changed, 116 insertions(+), 12 deletions(-) diff --git a/src/app/mod.rs b/src/app/mod.rs index 6e0fb8c..5011cf0 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -3139,7 +3139,7 @@ fn audio_profile_hint(profile: AudioProfile) -> &'static str { } AudioProfile::Balanced => "Default: voice quality with light loss recovery.", AudioProfile::BadNetwork => { - "Most resilient on a lossy/congested link: extra loss recovery, lower bitrate." + "Most resilient on a lossy/congested link: heavier loss recovery, lower bitrate." } } } diff --git a/src/codec/mod.rs b/src/codec/mod.rs index a9b9a8f..5ceb8ae 100644 --- a/src/codec/mod.rs +++ b/src/codec/mod.rs @@ -20,6 +20,9 @@ pub trait AudioDecoder: Send { /// If `compressed` is `None` (or `Some(&[])`), it indicates packet loss, /// enabling the decoder to perform packet loss concealment (PLC). fn decode(&mut self, compressed: Option<&[u8]>) -> Result, CodecError>; + + /// Reconstructs the previous lost frame from the next packet's in-band FEC. + fn decode_fec(&mut self, next_payload: &[u8]) -> Result, CodecError>; } pub mod opus_impl; diff --git a/src/codec/opus_impl.rs b/src/codec/opus_impl.rs index fcd46fc..cbf00cd 100644 --- a/src/codec/opus_impl.rs +++ b/src/codec/opus_impl.rs @@ -40,7 +40,7 @@ pub fn opus_params(profile: AudioProfile) -> OpusParams { bitrate: 20_000, inband_fec: true, packet_loss_perc: 25, - dtx: true, + dtx: false, }, } } @@ -162,6 +162,19 @@ impl AudioDecoder for OpusDecoder { pcm.truncate(decoded_per_channel * channels_count); Ok(pcm) } + + fn decode_fec(&mut self, next_payload: &[u8]) -> Result, CodecError> { + let channels_count = self.channels_count(); + let mut pcm = vec![0i16; self.frame_samples * channels_count]; + + let decoded_per_channel = self + .decoder + .decode(next_payload, &mut pcm, true) + .map_err(|e| CodecError::Decode(format!("Opus FEC decoding failed: {}", e)))?; + + pcm.truncate(decoded_per_channel * channels_count); + Ok(pcm) + } } #[cfg(test)] @@ -180,10 +193,8 @@ mod tests { assert!(bal.inband_fec); assert!(bad.inband_fec); - // BadNetwork is the only profile that enables DTX, and it expects the - // heaviest loss. - assert!(bad.dtx); - assert!(!low.dtx && !bal.dtx); + // Capture-side gating suppresses silence; no profile adds Opus DTX. + assert!(!low.dtx && !bal.dtx && !bad.dtx); assert!(bad.packet_loss_perc > bal.packet_loss_perc); // BadNetwork trims base bitrate to make room for FEC redundancy. diff --git a/src/config.rs b/src/config.rs index b78f2bf..c694605 100644 --- a/src/config.rs +++ b/src/config.rs @@ -105,7 +105,7 @@ pub enum AudioProfile { #[default] Balanced, /// Maximum resilience on a lossy/congested link: in-band FEC tuned for heavy - /// loss plus DTX, at a lower bitrate to leave headroom for the redundancy. + /// loss, at a lower bitrate to leave headroom for the redundancy. BadNetwork, } diff --git a/src/core/jitter.rs b/src/core/jitter.rs index de526e0..c935705 100644 --- a/src/core/jitter.rs +++ b/src/core/jitter.rs @@ -202,11 +202,15 @@ impl JitterBuffer { None } else { // Gap with later packets already buffered: a packet was lost - // or reordered out of window. Conceal this frame via Opus PLC - // and grow the cushion — the jitter beat our current delay. + // or reordered out of window. First try Opus in-band FEC from + // the next packet; if unavailable, fall back to plain PLC. self.next_seq = Some(next.wrapping_add(1)); self.note_disruption(); - self.decoder.decode(None).ok() + let next_payload = self.packets.values().next().expect("non-empty"); + self.decoder + .decode_fec(next_payload) + .or_else(|_| self.decoder.decode(None)) + .ok() } } } @@ -221,8 +225,8 @@ impl JitterBuffer { #[cfg(test)] mod tests { use super::*; - use crate::codec::AudioEncoder; - use crate::codec::opus_impl::OpusEncoder; + use crate::codec::opus_impl::{OpusDecoder, OpusEncoder, OpusParams}; + use crate::codec::{AudioDecoder, AudioEncoder}; use opus::{Application, Channels}; /// A real, decodable Opus packet for one 20ms mono frame at amplitude `amp`. @@ -233,6 +237,32 @@ mod tests { enc.encode(&pcm).unwrap() } + fn tone_frame(enc: &mut OpusEncoder, amp: i16, frame_index: usize) -> Vec { + let pcm: Vec = (0..FRAME_SAMPLES) + .map(|i| { + let sample_index = frame_index * FRAME_SAMPLES + i; + let t = sample_index as f32 / 48_000.0; + let fundamental = (t * 220.0 * 2.0 * std::f32::consts::PI).sin(); + let harmonic = (t * 440.0 * 2.0 * std::f32::consts::PI).sin(); + ((fundamental * 0.7 + harmonic * 0.3) * amp as f32) as i16 + }) + .collect(); + enc.encode(&pcm).unwrap() + } + + fn rms_error(a: &[i16], b: &[i16]) -> f64 { + assert_eq!(a.len(), b.len()); + let sum_sq: f64 = a + .iter() + .zip(b) + .map(|(&left, &right)| { + let diff = left as f64 - right as f64; + diff * diff + }) + .sum(); + (sum_sq / a.len() as f64).sqrt() + } + #[test] fn buffers_then_plays_in_order() { let mut enc = OpusEncoder::new(48000, Channels::Mono, Application::Voip).unwrap(); @@ -289,6 +319,66 @@ mod tests { assert!(jb.pop_frame().is_none()); } + #[test] + fn uses_in_band_fec_from_next_packet_for_gap() { + let mut enc = OpusEncoder::new(48000, Channels::Mono, Application::Voip).unwrap(); + enc.apply_params(&OpusParams { + bitrate: 20_000, + inband_fec: true, + packet_loss_perc: 60, + dtx: false, + }) + .unwrap(); + + let dropped_seq = 5usize; + let amps = [1800, 1800, 1800, 1800, 1800, 12_000, 12_000, 12_000]; + let packets: Vec> = amps + .into_iter() + .enumerate() + .map(|(seq, amp)| tone_frame(&mut enc, amp, seq)) + .collect(); + + let mut expected_decoder = OpusDecoder::new(48000, Channels::Mono, FRAME_SAMPLES).unwrap(); + for packet in packets.iter().take(dropped_seq) { + expected_decoder.decode(Some(packet)).unwrap(); + } + let expected_lost = expected_decoder + .decode(Some(&packets[dropped_seq])) + .unwrap(); + + let mut plc_decoder = OpusDecoder::new(48000, Channels::Mono, FRAME_SAMPLES).unwrap(); + for packet in packets.iter().take(dropped_seq) { + plc_decoder.decode(Some(packet)).unwrap(); + } + let pure_plc = plc_decoder.decode(None).unwrap(); + + let mut jb = JitterBuffer::new().unwrap(); + for (seq, packet) in packets.iter().enumerate() { + if seq != dropped_seq { + jb.insert(seq as u32, packet.clone()); + } + } + + for _ in 0..dropped_seq { + assert_eq!(jb.pop_frame().map(|frame| frame.len()), Some(FRAME_SAMPLES)); + } + + let recovered = jb.pop_frame().expect("gap should be reconstructed"); + assert_eq!(recovered.len(), FRAME_SAMPLES); + assert!( + jb.packets.contains_key(&(dropped_seq as u32 + 1)), + "FEC source packet must remain buffered for normal decode" + ); + assert_eq!(jb.pop_frame().map(|frame| frame.len()), Some(FRAME_SAMPLES)); + + let fec_error = rms_error(&recovered, &expected_lost); + let plc_error = rms_error(&pure_plc, &expected_lost); + assert!( + fec_error < plc_error * 0.75, + "FEC reconstruction should be materially closer than PLC (fec_error={fec_error}, plc_error={plc_error})" + ); + } + #[test] fn drops_packets_already_played() { let mut enc = OpusEncoder::new(48000, Channels::Mono, Application::Voip).unwrap();