From e6eb490939c4b3237991a16c57552836eff51229 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Fri, 17 Jul 2026 00:31:47 -0400 Subject: [PATCH] audio: only FEC-recover a gap from its immediate successor packet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The jitter buffer's gap path fed the LOWEST buffered packet to decode_fec regardless of position. Opus in-band FEC in packet N carries a copy of frame N-1 and nothing else, so that reconstruction is only correct when the smallest survivor is exactly next+1 (single loss). On burst loss it spliced a later frame's audio into the wrong slot — worse than concealment. Gate FEC on adjacency (new fec_covers_gap(), wraparound-aware); everything else falls back to plain PLC. Two new tests: the gate itself, and a burst-loss test proven to bite — it compares bit-exact against a twin decoder and fails against the old unconditional-FEC behavior (checked by mutation). Fixes finding 3 of the 2026-07-16 full-codebase review. Co-Authored-By: Claude Fable 5 --- src/core/jitter.rs | 99 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 92 insertions(+), 7 deletions(-) diff --git a/src/core/jitter.rs b/src/core/jitter.rs index c935705..c289a79 100644 --- a/src/core/jitter.rs +++ b/src/core/jitter.rs @@ -202,15 +202,20 @@ impl JitterBuffer { None } else { // Gap with later packets already buffered: a packet was lost - // or reordered out of window. First try Opus in-band FEC from - // the next packet; if unavailable, fall back to plain PLC. + // or reordered out of window. Try Opus in-band FEC from the + // packet right after the gap; if that packet isn't buffered + // (burst loss) or FEC fails, fall back to plain PLC. self.next_seq = Some(next.wrapping_add(1)); self.note_disruption(); - let next_payload = self.packets.values().next().expect("non-empty"); - self.decoder - .decode_fec(next_payload) - .or_else(|_| self.decoder.decode(None)) - .ok() + let (&smallest, next_payload) = self.packets.iter().next().expect("non-empty"); + if fec_covers_gap(next, smallest) { + self.decoder + .decode_fec(next_payload) + .or_else(|_| self.decoder.decode(None)) + .ok() + } else { + self.decoder.decode(None).ok() + } } } } @@ -222,6 +227,15 @@ impl JitterBuffer { } } +/// Opus in-band FEC in packet N carries a low-fidelity copy of frame N-1 and +/// nothing else — a lost frame `next` is FEC-recoverable solely from packet +/// `next+1`. Any later successor's FEC data is a different frame's audio, and +/// splicing it into this gap plays sound from the wrong position; the caller +/// must conceal with plain PLC instead. +fn fec_covers_gap(next: u32, smallest_buffered: u32) -> bool { + smallest_buffered == next.wrapping_add(1) +} + #[cfg(test)] mod tests { use super::*; @@ -379,6 +393,77 @@ mod tests { ); } + #[test] + fn fec_covers_gap_only_for_the_immediate_successor() { + // Packet next+1 is the only one whose in-band FEC describes frame `next`. + assert!(fec_covers_gap(4, 5)); + // A burst gap: the smallest survivor's FEC is some other frame's audio. + assert!(!fec_covers_gap(3, 5)); + assert!(!fec_covers_gap(3, 3_000)); + // Sequence wraparound still counts as adjacent. + assert!(fec_covers_gap(u32::MAX, 0)); + } + + #[test] + fn burst_gap_falls_back_to_plc_not_wrong_position_fec() { + 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(); + + // Frames 0..=6; 3 and 4 are lost as a burst, so when playout reaches + // seq 3 the smallest buffered packet is 5 — whose FEC data is frame 4, + // NOT frame 3. The buffer must conceal 3 with plain PLC rather than + // splice frame 4's audio into the wrong position. + let packets: Vec> = (0..7).map(|seq| tone_frame(&mut enc, 8_000, seq)).collect(); + + // Twin decoder replaying the exact call sequence the jitter buffer + // should make for seq 3: decode 0,1,2 then a plain PLC conceal. + let mut twin = OpusDecoder::new(48000, Channels::Mono, FRAME_SAMPLES).unwrap(); + for packet in packets.iter().take(3) { + twin.decode(Some(packet)).unwrap(); + } + let expected_plc = twin.decode(None).unwrap(); + + let mut jb = JitterBuffer::new().unwrap(); + for (seq, packet) in packets.iter().enumerate() { + if seq != 3 && seq != 4 { + jb.insert(seq as u32, packet.clone()); + } + } + + for _ in 0..3 { + assert_eq!(jb.pop_frame().map(|frame| frame.len()), Some(FRAME_SAMPLES)); + } + + // Seq 3: burst gap — bit-exact PLC (same decoder state, same inputs), + // which decode_fec(packet 5) could never produce. + let concealed = jb.pop_frame().expect("gap should be concealed"); + assert_eq!(concealed, expected_plc, "burst gap must use plain PLC"); + + // Seq 4: packet 5 IS the immediate successor, so its FEC data is + // frame 4's audio — the correctly-positioned recovery still applies. + let recovered = jb + .pop_frame() + .expect("adjacent gap should be reconstructed"); + let mut fec_twin = OpusDecoder::new(48000, Channels::Mono, FRAME_SAMPLES).unwrap(); + for packet in packets.iter().take(3) { + fec_twin.decode(Some(packet)).unwrap(); + } + fec_twin.decode(None).unwrap(); + let expected_fec = fec_twin.decode_fec(&packets[5]).unwrap(); + assert_eq!(recovered, expected_fec, "adjacent gap should still use FEC"); + + // Then 5 and 6 play normally. + assert_eq!(jb.pop_frame().map(|frame| frame.len()), Some(FRAME_SAMPLES)); + assert_eq!(jb.pop_frame().map(|frame| frame.len()), Some(FRAME_SAMPLES)); + assert!(jb.pop_frame().is_none()); + } + #[test] fn drops_packets_already_played() { let mut enc = OpusEncoder::new(48000, Channels::Mono, Application::Voip).unwrap();