From fa9ca366973c0883c9c5750e2204903534fc04ef Mon Sep 17 00:00:00 2001 From: alehel Date: Thu, 12 Nov 2026 20:28:37 +0100 Subject: [PATCH] craft-audio: SBR review fixes - A VARFIX pointer of 1 marks no transient envelope, like 0. - A start at QMF channel 32 is valid; kx is checked once the crossover is known. - The noise and harmonic gains take q / (1 + q) first, so two clamped levels no longer overflow to infinity and NaN. - A header that runs past its fill element is refused before it replaces the one in use. - A new program config layout drops the SBR decoders, whose history was the old order's. - The ffmpeg oracle requires a high band to compare on every channel but the 5.1 LFE. Refs #1121 Co-Authored-By: Claude Opus 5.5 --- crates/audio/src/aac/decoder.rs | 11 +++- crates/audio/src/aac/sbr/bitstream.rs | 6 ++- crates/audio/src/aac/sbr/freq.rs | 13 ++++- crates/audio/src/aac/sbr/hf.rs | 7 +-- crates/audio/src/aac/sbr/mod.rs | 51 +++++++++++++++---- .../audio/tests/codecs/aac/he_aac_oracle.rs | 10 ++-- 6 files changed, 78 insertions(+), 20 deletions(-) diff --git a/crates/audio/src/aac/decoder.rs b/crates/audio/src/aac/decoder.rs index 1d8713f54..ec5dae9fc 100644 --- a/crates/audio/src/aac/decoder.rs +++ b/crates/audio/src/aac/decoder.rs @@ -135,6 +135,10 @@ impl Decoder { } self.layout = layout; self.offsets = offsets; + // the SBR decoders' history belongs to the old element order: `apply_sbr` rebuilds them + if let Some(e) = self.sbr_out.as_mut() { + e.clear(); + } self.channels = vec![ChannelState { overlap: vec![0.0; 1024], prev_shape: WindowShape::Sine }; n]; } @@ -700,7 +704,7 @@ mod tests { /// its SBR fill element describes is there; with SBR off, the core alone (silence). #[test] fn sbr_fill_elements_rebuild_the_high_band() { - let payload = sbr::test_payload(44_100, |_| 40, 0).unwrap(); + let payload = sbr::test_payload(44_100, |_| 40, |_| 0).unwrap(); let mut bw = BitWriter::new(); bw.write_bits(ID_SCE, 3); bw.write_bits(0, 4); @@ -736,6 +740,11 @@ mod tests { assert_eq!(out.len(), 4 * 2048); let rms = (out[2048..].iter().map(|s| s * s).sum::() / (out.len() - 2048) as f32).sqrt(); assert!(rms > 1e-4, "{rms}"); + // a new layout drops the SBR decoders (their history is the old element order's) and + // keeps SBR on + dec.set_layout(vec![(ElementType::Sce, 0)]); + assert_eq!(dec.sbr_out.as_ref().map(Vec::len), Some(0)); + assert_eq!(dec.decode(&au).unwrap()[0].len(), 2048); } /// An access unit whose elements are all empty: `elements` are (id, tag), then ID_END. diff --git a/crates/audio/src/aac/sbr/bitstream.rs b/crates/audio/src/aac/sbr/bitstream.rs index 6233504fa..1d343bf7b 100644 --- a/crates/audio/src/aac/sbr/bitstream.rs +++ b/crates/audio/src/aac/sbr/bitstream.rs @@ -204,8 +204,9 @@ impl Grid { if t[0] < 0 || t[..=n].windows(2).any(|w| w[0] >= w[1]) { return Err(bad()); } + // l_A (ISO/IEC 14496-3 §4.6.18.3.3): VARFIX pointer 1, like 0, marks no envelope let la = match (pointer, class) { - (0, _) => None, + (0, _) | (1, VARFIX) => None, (p, FIXVAR | VARVAR) => Some((n + 1).checked_sub(p).ok_or_else(bad)?), (p, _) => Some(p - 1), }; @@ -468,6 +469,9 @@ mod tests { // VARFIX: leading border 1, relative 2, pointer 2: l_A = 1 let g = grid(&[(VARFIX as u32, 2), (1, 2), (1, 2), (0, 2), (2, 2), (1, 1), (0, 1)]).unwrap(); assert_eq!((g.num_env, &g.t_env[..3], g.la, g.t_noise), (2, &[1, 3, 16][..], Some(1), [1, 3, 16])); + // the same with pointer 1: no transient envelope, the noise floors still split at 3 + let g = grid(&[(VARFIX as u32, 2), (1, 2), (1, 2), (0, 2), (1, 2), (1, 1), (0, 1)]).unwrap(); + assert_eq!((g.la, g.t_noise), (None, [1, 3, 16])); // VARVAR: 1 .. 17 with one border from each end let g = grid(&[(VARVAR as u32, 2), (1, 2), (1, 2), (1, 2), (1, 2), (0, 2), (0, 2), (0, 2), (0, 1), (0, 1), (0, 1)]).unwrap(); assert_eq!((g.num_env, &g.t_env[..4], g.la), (3, &[1, 3, 15, 17][..], None)); diff --git a/crates/audio/src/aac/sbr/freq.rs b/crates/audio/src/aac/sbr/freq.rs index dc2809c10..d12fbdc02 100644 --- a/crates/audio/src/aac/sbr/freq.rs +++ b/crates/audio/src/aac/sbr/freq.rs @@ -75,8 +75,9 @@ fn start_stop(h: &Header, fs: u32) -> Option<(usize, usize)> { } } .min(64); - // the low band reaches only the analysis bank's 32 channels - (1..32).contains(&k0).then_some(())?; + // the low band reaches only the analysis bank's 32 channels (`kx` is checked against them + // once the crossover is known) + (1..=32).contains(&k0).then_some(())?; (k2 > k0).then_some((k0 as usize, k2 as usize)) } @@ -341,6 +342,14 @@ mod tests { assert!(valid > 10_000, "{valid}"); } + /// The highest start a header can give, 32 at 24 kHz (start 15), is valid: with stop 14 + /// (twice the start) and linear bands it covers channels 32 to 64. + #[test] + fn a_start_at_channel_32_is_accepted() { + let t = FreqTables::new(&header(15, 14, 0, 0, false, 2), 24_000).unwrap(); + assert_eq!((t.k0, t.kx, t.m), (32, 32, 32)); + } + /// A typical 44.1 kHz header: start channel 12 + offset, stop near 64, three noise bands. #[test] fn a_44k_header_has_the_expected_layout() { diff --git a/crates/audio/src/aac/sbr/hf.rs b/crates/audio/src/aac/sbr/hf.rs index 43ae572cf..1c2fd81ac 100644 --- a/crates/audio/src/aac/sbr/hf.rs +++ b/crates/audio/src/aac/sbr/hf.rs @@ -201,13 +201,14 @@ fn envelope_gains(f: &Frame, l: usize, sine_index: &[bool; 64], delta: bool) -> } let mut out = EnvelopeGains { gain: [0.0; 64], noise: [0.0; 64], sine: [0.0; 64] }; for c in 0..m { + // `q / (1 + q)` first: energy and noise ratio are each clamped, their product is not let (eo, ec, q) = (e_orig[c], e_curr[c], q[c]); - out.noise[c] = (eo * q / (1.0 + q)).sqrt(); + let share = q / (1.0 + q); + out.noise[c] = (eo * share).sqrt(); if sine_index[c] { out.sine[c] = (eo / (1.0 + q)).sqrt(); } - out.gain[c] = - if s_band[c] { (eo / (EPS + ec) * q / (1.0 + q)).sqrt() } else { (eo / ((EPS + ec) * (1.0 + if delta { q } else { 0.0 }))).sqrt() }; + out.gain[c] = if s_band[c] { (eo / (EPS + ec) * share).sqrt() } else { (eo / ((EPS + ec) * (1.0 + if delta { q } else { 0.0 }))).sqrt() }; } // limit per limiter band, then boost back the energy the limiter took let lim_gain = LIMITER_GAINS[usize::from(hdr.limiter_gains & 3)]; diff --git a/crates/audio/src/aac/sbr/mod.rs b/crates/audio/src/aac/sbr/mod.rs index 32a6c55b0..752cd03a0 100644 --- a/crates/audio/src/aac/sbr/mod.rs +++ b/crates/audio/src/aac/sbr/mod.rs @@ -285,6 +285,8 @@ impl Element { } if br.read_flag().ok()? { let h = Header::read(&mut br).ok()?; + // a header that runs past its fill element is damaged: keep the one in use + (br.position() <= p.end).then_some(())?; if self.header != Some(h) { if self.header.is_none_or(|o| !o.same_tables(&h)) { for c in &mut self.channels { @@ -377,13 +379,13 @@ pub(crate) const TEST_HEADER: Header = Header { /// An SBR single channel element for tests at output rate `rate`: [`TEST_HEADER`] (3 dB steps, /// start 5, stop 9, the default extra fields), then one FIXFIX envelope at high resolution with -/// band `k` at `env(k)` (1.5 dB steps, as a single FIXFIX envelope has) and every noise floor -/// value `noise`. +/// band `k` at `env(k)` (1.5 dB steps, as a single FIXFIX envelope has) and noise band `k` at +/// `noise(k)`. #[cfg(test)] -pub(crate) fn test_payload(rate: u32, env: impl Fn(usize) -> i32, noise: u32) -> Option> { +pub(crate) fn test_payload(rate: u32, env: impl Fn(usize) -> i32, noise: impl Fn(usize) -> i32) -> Option> { use craft_media::bitstream::BitWriter; let t = FreqTables::new(&TEST_HEADER, rate)?; - let (env, n_q): (Vec, usize) = ((0..t.n_high()).map(env).collect(), t.n_q()); + let (env, noise): (Vec, Vec) = ((0..t.n_high()).map(env).collect(), (0..t.n_q()).map(noise).collect()); let mut bw = BitWriter::new(); bw.write_bits(1, 1); // bs_header_flag for (v, n) in [(1, 1), (5, 4), (9, 4), (0, 3), (0, 2), (0, 1), (0, 1)] { @@ -394,7 +396,7 @@ pub(crate) fn test_payload(rate: u32, env: impl Fn(usize) -> i32, noise: u32) -> bw.write_bits(0, 2); // one envelope bw.write_bits(1, 1); // high resolution bw.write_bits(0, 2); // frequency-direction envelope and noise floor - for _ in 0..n_q { + for _ in &noise { bw.write_bits(0, 2); // no inverse filtering } bw.write_bits(env[0] as u32, 7); @@ -402,9 +404,9 @@ pub(crate) fn test_payload(rate: u32, env: impl Fn(usize) -> i32, noise: u32) -> let (code, len) = tables::F_ENV_1_5DB[(w[1] - w[0] + 60) as usize]; bw.write_bits(code, u32::from(len)); } - bw.write_bits(noise, 5); - for _ in 1..n_q { - let (code, len) = tables::F_ENV_3_0DB[31]; + bw.write_bits(noise[0] as u32, 5); + for w in noise.windows(2) { + let (code, len) = tables::F_ENV_3_0DB[(w[1] - w[0] + 31) as usize]; bw.write_bits(code, u32::from(len)); } bw.write_bits(0, 2); // no sinusoids, no extended data @@ -460,7 +462,7 @@ mod tests { fn the_high_band_follows_the_envelope() { let t = tables(); let mid = t.n_high() / 2; - let data = test_payload(RATE, |k| if k < mid { 30 } else { 42 }, 0).unwrap(); + let data = test_payload(RATE, |k| if k < mid { 30 } else { 42 }, |_| 0).unwrap(); let mut e = Element::new(1, RATE); let out = run(&mut e, &data, 6); let tail = &out[out.len() - 4096..]; @@ -481,7 +483,7 @@ mod tests { /// finite and of full length. #[test] fn damaged_sbr_data_is_survived() { - let good = test_payload(RATE, |k| 20 + k as i32, 3).unwrap(); + let good = test_payload(RATE, |k| 20 + k as i32, |_| 3).unwrap(); let mut seed = 0x1234_5678u32; let mut next = || { seed ^= seed << 13; @@ -510,4 +512,33 @@ mod tests { assert!(out.iter().all(|c| c.len() == 2048 && c.iter().all(|s| s.is_finite()))); } } + + /// The largest envelope (127 steps of 1.5 dB) with the largest noise floor (each band 31 + /// steps below the last) keeps the output finite: the gains are formed so that no + /// intermediate product of two clamped levels overflows. + #[test] + fn extreme_levels_stay_finite() { + let rate = + [22_050, 24_000, 32_000, 44_100, 48_000].into_iter().find(|&r| FreqTables::new(&TEST_HEADER, r).is_some_and(|t| t.n_q() >= 3)).unwrap(); + let data = test_payload(rate, |_| 127, |k| -31 * k as i32).unwrap(); + let mut e = Element::new(1, rate); + run(&mut e, &data, 3); + } + + /// A header that runs past the end of its fill element is not taken: the one in use stays, + /// and the frame is dropped. + #[test] + fn a_header_cut_short_is_not_taken() { + let data = test_payload(RATE, |_| 30, |_| 0).unwrap(); + let mut e = Element::new(1, RATE); + run(&mut e, &data, 1); + assert_eq!(e.header, Some(TEST_HEADER)); + // another start frequency (bits 2..6), in an element that ends 6 bits in + let mut cut = data.clone(); + cut[0] ^= 0x04; + assert!(e.read(Payload { data: &cut, start: 0, end: 6, crc: false }).is_none()); + assert_eq!(e.header, Some(TEST_HEADER)); + assert!(e.read(Payload { data: &cut, start: 0, end: 8 * cut.len(), crc: false }).is_some()); + assert_ne!(e.header, Some(TEST_HEADER)); + } } diff --git a/crates/audio/tests/codecs/aac/he_aac_oracle.rs b/crates/audio/tests/codecs/aac/he_aac_oracle.rs index e41493e16..0eeba69c8 100644 --- a/crates/audio/tests/codecs/aac/he_aac_oracle.rs +++ b/crates/audio/tests/codecs/aac/he_aac_oracle.rs @@ -149,8 +149,12 @@ fn matches_ffmpeg_he_aac_decoder() { } } } - let mean = diffs.iter().sum::() / diffs.len().max(1) as f64; - let mad = diffs.iter().map(|d| d.abs()).sum::() / diffs.len().max(1) as f64; + // the LFE (WAV channel 3 of 5.1) has no SBR, so no high band; every other channel must + // have one to compare + let lfe = case.channels == 6 && c == 3; + assert!(lfe || !diffs.is_empty(), "{} ch{c}: no high band to compare", case.name); + let mean = diffs.iter().sum::() / diffs.len() as f64; + let mad = diffs.iter().map(|d| d.abs()).sum::() / diffs.len() as f64; println!( "{} ch{c}: full SNR {full:.1} dB, low band SNR {low:.1} dB, high band level {mean:+.2} dB (mean |Δ| {mad:.2} dB over {})", case.name, @@ -159,7 +163,7 @@ fn matches_ffmpeg_he_aac_decoder() { // The low band differs by the core decoders' rounding; the high band's noise comes // from the same table, so whole frames match too. assert!(full > 45.0 && low > 55.0, "{} ch{c}: SNR {full:.1} / {low:.1} dB", case.name); - assert!(mad < 0.1, "{} ch{c}: high band off by {mad:.2} dB", case.name); + assert!(lfe || mad < 0.1, "{} ch{c}: high band off by {mad:.2} dB", case.name); } } }