craft-audio: review fixes, round 8 (#443)

From wanikhawar's review:
- AudioSpecificConfig::parse reads backward-compatible SBR and PS signalling
  (a 0x2b7 sync extension after the config), which craft-media's Matroska
  demuxer writes for legacy HE-AAC IDs. Without lookahead those streams decoded
  as their core.
- An MP4 media edit after one empty edit is applied: its priming skip and its
  end. The empty edit's delay is not added as silence.
- A packet lost whole is concealed as long as the last one decoded, so a stream
  of multi-frame ADTS packets keeps its timing.
- ADTS-framed MP4 samples keep their sample table's timing, instead of every
  sample being read at open to look for several frames in one.
- AdtsHeader::write refuses an object type, sampling frequency index or channel
  configuration its fields cannot hold; it masked them into other values.
- AudioSpecificConfig::lc no longer panics for a sampling frequency index with no
  rate, and the decoder refuses such a config.
- AudioSpecificConfig::to_bytes refuses channel configuration 0 without a program
  config element, which would not parse back.
- Upsample2x starts each channel's history afresh when the channel count
  changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

crates/audio/README.md

@@ -311,8 +311,9 @@ match craft_audio::decode(&bytes, None) {
 - Not decoded at all: AC-3, E-AC-3, Opus and Speex.
 - `TrackDecoder` returns `Unsupported` for MP3, FLAC, ALAC and Vorbis; `decode` and `Decoder` read
   those through symphonia.
-- MP4 edit lists: a single leading media edit (encoder priming) is applied. Empty edits and lists
-  of more than one edit are not; the media plays as it is stored.
+- MP4 edit lists: a single media edit (encoder priming and the end) is applied, alone or after one
+  empty edit; the empty edit's delay is not added as silence. Other edit lists are not applied, and
+  the media plays as it is stored.
 
 ## Codecs
 

crates/audio/src/aac/config.rs

@@ -217,12 +217,29 @@ fn read_rate(br: &mut BitReader) -> Result<(u8, u32)> {
     Ok((i, r))
 }
 
+/// Backward-compatible explicit SBR signalling (ISO/IEC 14496-3 §1.6.6.2): after an AAC-LC
+/// config, a sync extension `0x2b7` with extension object type 5 names SBR and its output rate,
+/// and `0x548` after it names PS. Returns `(sbr, extension rate, ps)`, or `None` when the bits
+/// that follow are not that extension (they are then ignored, as decoders that don't know it do).
+fn sync_extension(br: &mut BitReader) -> Option<(bool, Option<u32>, bool)> {
+    if br.bits_left() < 16 || br.read_bits(11).ok()? != 0x2b7 || read_aot(br).ok()? != 5 {
+        return None;
+    }
+    if br.read_bits(1).ok()? != 1 {
+        return Some((false, None, false));
+    }
+    let (_, rate) = read_rate(br).ok()?;
+    let ps = br.bits_left() >= 12 && br.read_bits(11).ok()? == 0x548 && br.read_bits(1).ok()? == 1;
+    Some((true, Some(rate), ps))
+}
+
 impl AudioSpecificConfig {
-    /// A plain AAC-LC configuration.
+    /// A plain AAC-LC configuration. A sampling frequency index with no rate (13 and above) gives a
+    /// sample rate of 0, which [`crate::aac::Decoder`] refuses.
     pub fn lc(sf_index: u8, channel_config: u8) -> AudioSpecificConfig {
         AudioSpecificConfig {
             object_type: 2,
-            sample_rate: SAMPLE_RATES[sf_index as usize],
+            sample_rate: SAMPLE_RATES.get(usize::from(sf_index)).copied().unwrap_or(0),
             sf_index,
             channel_config,
             frame_length_flag: false,
@@ -234,7 +251,8 @@ impl AudioSpecificConfig {
     }
 
     /// Parse an `AudioSpecificConfig` (§1.6.2.1). Only AAC-LC cores are accepted; explicit SBR/PS
-    /// signalling (audio object types 5 and 29) is recorded.
+    /// signalling is recorded, both hierarchical (audio object types 5 and 29) and backward
+    /// compatible (a `0x2b7` sync extension after the config).
     pub fn parse(data: &[u8]) -> Result<AudioSpecificConfig> {
         let mut br = BitReader::new(data);
         let mut aot = read_aot(&mut br)?;
@@ -267,6 +285,9 @@ impl AudioSpecificConfig {
         if frame_length_flag {
             return Err(Error::Unsupported("960-sample frames"));
         }
+        if !sbr && let Some((s, rate, p)) = sync_extension(&mut br) {
+            (sbr, extension_sample_rate, ps) = (s, rate, p);
+        }
         let cfg =
             AudioSpecificConfig { object_type: aot, sample_rate, sf_index, channel_config, frame_length_flag, sbr, ps, extension_sample_rate, pce };
         if cfg.channels() == 0 {
@@ -275,8 +296,12 @@ impl AudioSpecificConfig {
         Ok(cfg)
     }
 
-    /// Serialise (AAC-LC, no SBR signalling).
+    /// Serialise (AAC-LC, no SBR signalling). Channel configuration 0 needs its program config
+    /// element: without one the bytes would not parse back.
     pub fn to_bytes(&self) -> Result<Vec<u8>> {
+        if self.channel_config == 0 && self.pce.is_none() {
+            return Err(Error::InvalidConfig("channel configuration 0 without a program config element"));
+        }
         let mut bw = BitWriter::new();
         bw.write_bits(self.object_type as u32, 5);
         match crate::aac::tables::sample_rate_index(self.sample_rate) {
@@ -366,22 +391,33 @@ impl AdtsHeader {
         AudioSpecificConfig { object_type: self.object_type, ..AudioSpecificConfig::lc(self.sf_index, self.channel_config) }
     }
 
-    /// Write a 7-byte header (no CRC) for a frame whose raw payload is `payload_len` bytes. A frame
-    /// too long for the header's 13-bit length is refused.
+    /// Write a 7-byte header (no CRC) for a frame whose raw payload is `payload_len` bytes. What the
+    /// header's fields cannot hold is refused: a frame too long for its 13-bit length, an object
+    /// type outside its 2-bit profile (1–4), a sampling frequency index with no rate, and channel
+    /// configurations above 7 (11 and 12 need a program config element in ADTS).
     pub fn write(object_type: u8, sf_index: u8, channel_config: u8, payload_len: usize, buffer_fullness: u16) -> Result<[u8; 7]> {
         if payload_len > 0x1FFF - 7 {
             return Err(Error::InvalidConfig("an ADTS frame longer than its 13-bit length"));
         }
+        if !(1..=4).contains(&object_type) {
+            return Err(Error::InvalidConfig("an ADTS profile holds audio object types 1 to 4"));
+        }
+        if usize::from(sf_index) >= SAMPLE_RATES.len() {
+            return Err(Error::InvalidConfig("a sampling frequency index with no rate"));
+        }
+        if channel_config > 7 {
+            return Err(Error::InvalidConfig("an ADTS channel configuration above 7"));
+        }
         let len = (payload_len + 7) as u32;
         let mut bw = BitWriter::new();
         bw.write_bits(0xFFF, 12);
         bw.write_bits(0, 1); // MPEG-4
         bw.write_bits(0, 2);
         bw.write_bits(1, 1); // protection absent
-        bw.write_bits((object_type.max(1) - 1) as u32 & 3, 2);
+        bw.write_bits(u32::from(object_type - 1), 2);
         bw.write_bits(sf_index as u32, 4);
         bw.write_bits(0, 1);
-        bw.write_bits(channel_config as u32 & 7, 3);
+        bw.write_bits(u32::from(channel_config), 3);
         bw.write_bits(0, 4);
         bw.write_bits(len & 0x1FFF, 13);
         bw.write_bits(buffer_fullness as u32 & 0x7FF, 11);
@@ -428,6 +464,55 @@ pub(crate) fn split_adts_upto(mut data: &[u8], max: usize) -> Result<Vec<(AdtsHe
 mod tests {
     use super::*;
 
+    /// Backward-compatible signalling, as craft-media's Matroska demuxer and some MP4 muxers
+    /// write it: an AAC-LC config, then `0x2b7`, object type 5 and the output rate.
+    #[test]
+    fn a_sync_extension_after_the_config_signals_sbr_and_ps() {
+        // AAC-LC, 22.05 kHz, stereo; 0x2b7, AOT 5, SBR, 44.1 kHz
+        let asc = AudioSpecificConfig::parse(&[0x13, 0x90, 0x56, 0xE5, 0xA0]).unwrap();
+        assert_eq!((asc.object_type, asc.sample_rate, asc.sbr, asc.extension_sample_rate, asc.ps), (2, 22_050, true, Some(44_100), false));
+        // ... then 0x548 and PS
+        let mut bw = BitWriter::new();
+        for (value, bits) in [(2, 5), (7, 4), (1, 4), (0, 3), (0x2b7, 11), (5, 5), (1, 1), (4, 4), (0x548, 11), (1, 1)] {
+            bw.write_bits(value, bits);
+        }
+        let asc = AudioSpecificConfig::parse(&bw.finish()).unwrap();
+        assert_eq!((asc.sbr, asc.extension_sample_rate, asc.ps), (true, Some(44_100), true));
+        // SBR flagged off, a different extension, or too few bits: plain AAC-LC
+        for bytes in [&[0x13, 0x90, 0x56, 0xE5, 0x00][..], &[0x13, 0x90, 0x12, 0x34], &[0x13, 0x90, 0x56]] {
+            let asc = AudioSpecificConfig::parse(bytes).unwrap();
+            assert_eq!((asc.sbr, asc.extension_sample_rate), (false, None), "{bytes:02x?}");
+        }
+    }
+
+    #[test]
+    fn an_index_with_no_rate_does_not_panic_and_is_refused() {
+        for sf_index in 13..=15 {
+            let asc = AudioSpecificConfig::lc(sf_index, 2);
+            assert_eq!(asc.sample_rate, 0);
+            assert!(crate::aac::Decoder::from_config(asc).is_err());
+        }
+    }
+
+    #[test]
+    fn configs_that_would_not_parse_back_are_not_written() {
+        assert!(AudioSpecificConfig::lc(4, 0).to_bytes().is_err());
+        let pce = ProgramConfig { front: vec![(true, 0)], ..Default::default() };
+        let asc = AudioSpecificConfig { pce: Some(pce), ..AudioSpecificConfig::lc(4, 0) };
+        assert_eq!(AudioSpecificConfig::parse(&asc.to_bytes().unwrap()).unwrap(), asc);
+    }
+
+    #[test]
+    fn adts_headers_refuse_fields_they_cannot_hold() {
+        for (object_type, sf_index, channel_config) in [(0, 4, 2), (5, 4, 2), (29, 4, 2), (2, 13, 2), (2, 15, 2), (2, 4, 8), (2, 4, 11), (2, 4, 12)] {
+            assert!(AdtsHeader::write(object_type, sf_index, channel_config, 100, 0x7FF).is_err(), "{object_type} {sf_index} {channel_config}");
+        }
+        for object_type in 1..=4 {
+            let h = AdtsHeader::parse(&AdtsHeader::write(object_type, 12, 7, 100, 0x7FF).unwrap()).unwrap();
+            assert_eq!((h.object_type, h.sf_index, h.channel_config, h.frame_length), (object_type, 12, 7, 107));
+        }
+    }
+
     #[test]
     fn program_configs_refuse_more_elements_than_their_counts_hold() {
         let fits = ProgramConfig { front: vec![(false, 0); 15], lfe: vec![0; 3], assoc_data: vec![0; 7], ..Default::default() };

crates/audio/src/aac/decoder.rs

@@ -76,6 +76,9 @@ impl Decoder {
         if asc.object_type != 2 {
             return Err(Error::Unsupported("audio object type other than AAC LC"));
         }
+        if usize::from(asc.sf_index) >= crate::aac::SAMPLE_RATES.len() || asc.sample_rate == 0 {
+            return Err(Error::InvalidConfig("a sampling frequency index with no rate"));
+        }
         let layout = asc.layout();
         if layout.is_empty() && asc.channel_config != 0 {
             return Err(Error::InvalidConfig("unsupported channel configuration"));

crates/audio/src/aac/mod.rs

@@ -163,8 +163,10 @@ impl Upsample2x {
 
     /// Upsample planar channels by two: each output channel has twice the input samples.
     pub fn process(&mut self, input: Vec<Vec<f32>>) -> Vec<Vec<f32>> {
-        if self.hist.len() < input.len() {
-            self.hist.resize(input.len(), vec![0.0; UPSAMPLE_HISTORY]);
+        // A changed channel count is a new layout: history of channels that were absent meanwhile
+        // (or that held other speakers) would put a step into the output.
+        if self.hist.len() != input.len() {
+            self.hist = vec![vec![0.0; UPSAMPLE_HISTORY]; input.len()];
         }
         let m = UPSAMPLE_HALF;
         input
@@ -205,6 +207,13 @@ mod tests {
         assert_eq!(UPSAMPLE_DELAY, 481);
         assert_eq!(out[(100 + 481) * 2], 1.0);
         assert!(out[..(100 + 481 - UPSAMPLE_HALF) * 2].iter().all(|&s| s == 0.0));
+        // a change of channel count starts each channel's history afresh
+        let mut fresh = Upsample2x::new(2);
+        let block = vec![vec![0.5; 1024], vec![0.25; 1024]];
+        let mut up = Upsample2x::new(2);
+        up.process(block.clone());
+        up.process(vec![vec![0.5; 1024]]);
+        assert_eq!(up.process(block.clone()), fresh.process(block));
         // a reset forgets the history
         up.reset();
         assert!(up.process(vec![vec![0.0; 600]]).remove(0).iter().all(|&s| s == 0.0));

crates/audio/src/read/native.rs

@@ -11,7 +11,7 @@ use super::aiff::{self, AiffParse};
 use super::id3v2_len;
 use super::source::{Src, io_error};
 use super::wav::{self, WavParse};
-use crate::track::{TrackDecoder, adts_format_of, adts_framed, adts_stream_header, several_adts_frames};
+use crate::track::{TrackDecoder, adts_format_of, adts_framed, adts_stream_header};
 use crate::{AudioBuffer, AudioError, AudioInfo, CodecId, CodecParams, Container, PcmFormat, Result, aac};
 use craft_media::{isobmff, matroska, ogg};
 
@@ -642,8 +642,16 @@ fn open_mp4<'a>(src: &Src<'a>, container: Container, track: Option<usize>) -> Op
     let rate = probe.sample_rate();
     // A leading media edit (AAC priming) is skipped, and the edit's duration is the
     // presentation length. Empty edits and multi-segment lists play the media as it is.
-    let (skip, limit) = match t.edits.as_slice() {
-        [e] if e.media_time >= 0 && e.media_rate == 0x0001_0000 => {
+    // A leading empty edit (a delay before the track starts, ffmpeg's `-itsoffset` and muxers
+    // aligning audio to video) does not change which media is presented: the media edit after it
+    // still skips the priming and ends the track. The delay itself is not added as silence.
+    let media_edit = match t.edits.as_slice() {
+        [e] => Some(e),
+        [empty, e] if empty.media_time < 0 => Some(e),
+        _ => None,
+    };
+    let (skip, limit) = match media_edit {
+        Some(e) if e.media_time >= 0 && e.media_rate == 0x0001_0000 => {
             let skip = rescale(e.media_time.unsigned_abs(), u64::from(timescale), u64::from(rate));
             let limit = (e.segment_duration > 0).then(|| rescale(e.segment_duration, u64::from(file.timescale), u64::from(rate)));
             (skip, limit)
@@ -655,11 +663,10 @@ fn open_mp4<'a>(src: &Src<'a>, container: Container, track: Option<usize>) -> Op
     let mut stored = rescale(media, u64::from(timescale), u64::from(rate)).saturating_sub(skip);
     // Durations only trim the last packet: ones that end anywhere else are wrong (some count a
     // HE-AAC packet's core frames) and can't cut audio off.
-    // Samples of several ADTS frames (not standard, but they decode) have no one length. Only a
-    // file of ADTS-framed samples, which is rare, has its samples read to look for them.
-    let several_frames = lookahead.iter().any(|p| adts_framed(p))
-        && (0..t.samples.len()).any(|i| file.read_sample(src, track, i).is_ok_and(|p| several_adts_frames(&p)));
-    let per_packet = probe.frames_per_packet().map(|n| n as u64).filter(|_| !several_frames);
+    // ADTS-framed samples (not standard) may hold several frames each, and finding out would mean
+    // reading every sample: their sample table's timing is kept instead.
+    let adts = lookahead.iter().any(|p| adts_framed(p));
+    let per_packet = probe.frames_per_packet().map(|n| n as u64).filter(|_| !adts);
     if let Some(n) = per_packet {
         let decoded = (t.samples.len() as u64).saturating_mul(n).saturating_sub(skip);
         if stored > decoded || decoded - stored >= n {

crates/audio/src/track.rs

@@ -215,8 +215,14 @@ impl TrackDecoder {
         let frames = match &mut self.inner {
             #[cfg(feature = "aac-decode")]
             Inner::Aac { up, .. } => {
-                let units = adts_frames(data).map_or(1, |f| f.iter().map(|(h, _)| usize::from(h.raw_data_blocks)).sum::<usize>());
-                Some(units.min(MAX_ADTS_FRAMES_PER_PACKET) * aac::FRAME_LEN * if up.is_some() { 2 } else { 1 })
+                let per_unit = aac::FRAME_LEN * if up.is_some() { 2 } else { 1 };
+                match adts_frames(data) {
+                    Some(f) => Some(f.iter().map(|(h, _)| usize::from(h.raw_data_blocks)).sum::<usize>().min(MAX_ADTS_FRAMES_PER_PACKET) * per_unit),
+                    // A packet lost whole: as long as the last one, in a stream of multi-frame
+                    // packets as in any other.
+                    None if data.is_empty() && self.last_frames > 0 => Some(self.last_frames),
+                    None => Some(per_unit),
+                }
             }
             Inner::Pcm { kind, channels } => Some(data.len() / (kind.bytes() * *channels).max(1)),
         };
@@ -323,12 +329,6 @@ pub(crate) fn adts_framed(packet: &[u8]) -> bool {
     adts_frames(packet).is_some()
 }
 
-/// Whether `packet` is several ADTS frames, which decode to more than one packet's length.
-#[cfg(feature = "decode-native")]
-pub(crate) fn several_adts_frames(packet: &[u8]) -> bool {
-    adts_frames(packet).is_some_and(|frames| frames.len() > 1)
-}
-
 /// The configuration of an AAC stream from its ADTS `header`. Channel configuration 0 leaves
 /// the layout to a program config element in the stream, looked for in `first_units`.
 #[cfg(feature = "aac-decode")]
@@ -481,7 +481,8 @@ mod tests {
         assert!(dec.decode(&damaged).is_err());
         assert_eq!(dec.conceal(Some(&damaged)).unwrap().frames(), 3072);
         assert_eq!(dec.conceal(Some(&frame)).unwrap().frames(), 1024);
-        assert_eq!(dec.conceal(None).unwrap().frames(), 1024);
+        // a packet lost whole lasts as long as the last one decoded: here, three frames
+        assert_eq!(dec.conceal(None).unwrap().frames(), 3072);
     }
 
     /// Implicitly signalled HE-AAC: an AAC-LC configuration at 24 kHz or less, with SBR data in a
@@ -586,7 +587,7 @@ mod tests {
         let tied: [&[u8]; 2] = [&good, &damaged];
         assert_eq!(TrackDecoder::new(&aac_params(), &tied).unwrap().sample_rate(), 48_000);
         #[cfg(feature = "decode-native")]
-        assert!(several_adts_frames(&good.repeat(2)) && !several_adts_frames(&good) && !several_adts_frames(&[0; 8]));
+        assert!(adts_framed(&good) && adts_framed(&good.repeat(2)) && !adts_framed(&[0; 8]));
     }
 
     /// `au` as a stereo ADTS frame at 48 kHz (channel configuration `channel_config`).

crates/audio/tests/codecs/routing_oracle.rs

@@ -579,3 +579,37 @@ fn mp4_with_wrong_durations_or_edits_presents_its_audio() {
     assert!(buf.frames() < whole.frames() + 2048, "edit doubled: {} frames", buf.frames());
     assert_eq!((probe(&file, None).unwrap().frames, info.frames), (buf.frames() as u64, buf.frames() as u64), "edit doubled");
 }
+
+/// An empty edit before the media edit (QuickTime's and GPAC's delayed audio, ffmpeg's
+/// `-itsoffset`) still has the media edit applied: its priming skip and its end, as ffmpeg's
+/// decode does. ffmpeg writes such a media edit with no skip, so the test sets one.
+#[test]
+fn mp4_with_an_empty_edit_before_its_media_edit_applies_the_media_edit() {
+    let ff = require_ffmpeg!();
+    let dir = crate::testkit::fixtures_dir("routing");
+    let out = dir.join("aac-itsoffset.m4a");
+    let input = format!("{MONO}:s=44100");
+    let made = crate::testkit::fixtures::generate(&out, |tmp| {
+        let tmp = tmp.to_string_lossy();
+        run(ff.as_path(), &["-itsoffset", "0.5", "-f", "lavfi", "-i", &input, "-t", "5", "-c:a", "aac", "-aac_pns", "0", &tmp])
+    });
+    assert!(made.is_some() || !craft_media_testkit::oracles_required(), "ffmpeg could not write aac-itsoffset.m4a");
+    let Some(path) = made else { return };
+    let bytes = std::fs::read(&path).unwrap();
+    // elst v0 from the name's start: the name (4), version and flags (4), entries (4), then 12
+    // bytes per edit: duration, media time, rate.
+    let elst = bytes.windows(4).position(|w| w == b"elst").unwrap();
+    assert_eq!((bytes[elst + 4], u32::from_be_bytes(bytes[elst + 8..elst + 12].try_into().unwrap())), (0, 2), "an empty edit and a media edit");
+    assert_eq!(i32::from_be_bytes(bytes[elst + 12 + 4..elst + 12 + 8].try_into().unwrap()), -1, "the first edit is empty");
+    let media_time = elst + 12 + 12 + 4;
+    let mut file = bytes.clone();
+    file[media_time..media_time + 4].copy_from_slice(&1024i32.to_be_bytes());
+    let patched = dir.join("aac-itsoffset-priming.m4a");
+    std::fs::write(&patched, &file).unwrap();
+    let reference = reference(&ff, &patched);
+    let (info, buf) = decode(&file, None).unwrap();
+    assert_eq!(buf.frames(), reference.len(), "presented frames");
+    assert_eq!((probe(&file, None).unwrap().frames, info.frames), (buf.frames() as u64, buf.frames() as u64));
+    let s = snr(&buf.channels, &reference);
+    assert!(s >= 100.0, "SNR {s:.1} dB: the priming skip is not applied");
+}