audio: only FEC-recover a gap from its immediate successor packet
CI / check (push) Failing after 10m54s
CI / check (push) Failing after 10m54s
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 <noreply@anthropic.com>
This commit is contained in:
+88
-3
@@ -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");
|
||||
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<Vec<u8>> = (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();
|
||||
|
||||
Reference in New Issue
Block a user