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

Corrects round 2:
- ADTS headers are compared with the stream's format only when the first header
  gave it. With extradata the format comes from there, so ADTS in MP4 whose
  headers say something else decodes again; every packet had been concealed.
- A stream's only program config element is found in its first frames even when
  that frame's header is damaged.
- The test of wrong sample durations patched the second run's count, not the
  delta, so "durations of one" changed nothing. It now patches the delta, and
  checks a file was changed and that seeks land on the same audio.

MP4 seeks place a packet by its index when every packet decodes to the same
length, as decoding does; they used the durations round 2 had stopped trusting
for the length, and returned the wrong audio. A last delta of 0 is overridden too.

Docs: what TrackInfo says for raw ADTS, that opening one reads the whole file, and
which codecs symphonia reads.

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

crates/audio/README.md

@@ -149,8 +149,8 @@ known stops at the end, and the next `next_block` returns `None`. When `length_k
 of the file to count them. `Decoder::probe` is also how to probe a file on disk.
 
 Native formats read their samples from the source packet by packet. The container's packet index
-stays in memory (tens of bytes per packet), and opening a Matroska file reads through the whole
-file to build it. Formats symphonia decodes are read whole first.
+stays in memory (tens of bytes per packet), and opening a Matroska file or a raw ADTS stream reads
+through the whole file to build it. Formats symphonia decodes are read whole first.
 
 A damaged packet is concealed and counted in `info().damaged`. A source that fails a read (a
 network drop) returns `AudioError::Io` from `next_block` instead, and that packet is skipped. To

crates/audio/src/aac/config.rs

@@ -307,7 +307,7 @@ pub struct AdtsHeader {
     pub object_type: u8,
     /// `sampling_frequency_index` (4 bits).
     pub sf_index: u8,
-    /// `channel_configuration` (3 bits; 0 = program config element in the first frame).
+    /// `channel_configuration` (3 bits; 0 = program config element in the stream).
     pub channel_config: u8,
     /// Whole frame length including the header.
     pub frame_length: usize,

crates/audio/src/aac/decoder.rs

@@ -133,8 +133,8 @@ impl Decoder {
     }
 
     /// Decode one access unit (one `raw_data_block`) into 1024 samples per channel. An access unit
-    /// that fails leaves the decoder as it was: its damage, including a damaged program config
-    /// element, does not reach the access units after it.
+    /// that fails leaves the decoder as it was, but for [`Self::sbr`]: its damage, including a damaged
+    /// program config element, does not reach the access units after it.
     pub fn decode(&mut self, au: &[u8]) -> Result<Vec<Vec<f32>>> {
         let before = (self.asc.pce.clone(), self.layout.clone(), self.offsets.clone(), self.channels.clone(), self.noise_state);
         let decoded = self.decode_block(au, true);

crates/audio/src/aac/mod.rs

@@ -202,8 +202,9 @@ mod tests {
             out.extend(up.process(vec![block.to_vec()]).remove(0));
         }
         assert_eq!(out.len(), 4000);
-        assert_eq!(out[(100 + UPSAMPLE_DELAY) * 2], 1.0);
-        assert!(out[..(100 + UPSAMPLE_DELAY - UPSAMPLE_HALF) * 2].iter().all(|&s| s == 0.0));
+        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 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/mod.rs

@@ -164,9 +164,10 @@ impl DecodeOptions {
 pub struct TrackInfo {
     /// The value for [`DecodeOptions::track`].
     pub index: usize,
-    /// The codec the container names (an AAC track that turns out to be HE-AAC is `Aac` here).
+    /// The codec the container names (an AAC track that turns out to be HE-AAC is `Aac` here). A
+    /// raw ADTS stream names none, so this is what decoding finds.
     pub codec: CodecId,
-    /// The sample rate the container declares, if it does.
+    /// The sample rate the container declares, if it does; for raw ADTS, the decoded rate.
     pub sample_rate: Option<u32>,
     /// The channel count the container declares, if it does.
     pub channels: Option<u16>,
@@ -184,8 +185,8 @@ enum Inner<'a> {
 ///
 /// Native formats (WAV, AIFF, AAC and PCM in MP4/MOV and Matroska/WebM, and raw ADTS streams)
 /// read their sample data from the source packet by packet. The container's packet index is held
-/// in memory (tens of bytes per packet), and opening a Matroska file reads through the whole file
-/// to build it; WAV and AIFF need no index. Formats symphonia decodes (MP1/MP2/MP3, FLAC, ALAC,
+/// in memory (tens of bytes per packet), and opening a Matroska file or a raw ADTS stream reads
+/// through the whole file to build it; WAV and AIFF need no index. Formats symphonia decodes (MP1/MP2/MP3, FLAC, ALAC,
 /// Vorbis, CAF, ADPCM, A-law, µ-law) read the whole source into memory first; their decoded audio
 /// still streams.
 ///

crates/audio/src/read/native.rs

@@ -44,6 +44,9 @@ enum Packets<'a> {
         /// Media timescale and output rate, for packet start times.
         timescale: u32,
         rate: u32,
+        /// Frames every packet decodes to, when the codec says: then a packet's place on the
+        /// decoded timeline is its index, whatever the sample table's durations claim.
+        per_packet: Option<u64>,
     },
     /// Matroska is read packet by packet, which carries each block's `DiscardPadding`.
     Mkv {
@@ -114,8 +117,13 @@ impl Packets<'_> {
             found
         }
         match self {
-            Packets::Mp4 { file, track, next, timescale, rate, .. } => {
+            Packets::Mp4 { file, track, next, timescale, rate, per_packet, .. } => {
                 let samples = file.tracks.get(*track).map(|t| t.samples.as_slice()).unwrap_or_default();
+                if let Some(n) = per_packet.filter(|&n| n > 0) {
+                    let i = (frame / n).min((samples.len() as u64).saturating_sub(1));
+                    *next = i as usize;
+                    return Ok(i * n);
+                }
                 let first = samples.first().map_or(0, |s| s.dts);
                 let starts =
                     samples.iter().map(|s| rescale(u64::try_from(s.dts.saturating_sub(first)).unwrap_or(0), u64::from(*timescale), u64::from(*rate)));
@@ -568,15 +576,19 @@ fn open_adts<'a>(src: &Src<'a>, at: u64) -> Option<Result<Native<'a>>> {
     let format = |h: &aac::AdtsHeader| (h.object_type, h.sf_index, h.channel_config);
     let first = &frames[..frames.len().min(crate::track::LOOKAHEAD_PACKETS)];
     let &(_, _, stream) = first.iter().max_by_key(|(_, _, h)| first.iter().filter(|(_, _, o)| format(o) == format(h)).count())?;
-    let usable: Vec<_> = first.iter().filter(|(_, _, h)| format(h) == format(&stream) && h.raw_data_blocks == 1).copied().collect();
-    if usable.len() * 2 <= first.len() {
+    let usable = |h: &aac::AdtsHeader| format(h) == format(&stream) && h.raw_data_blocks == 1;
+    if first.iter().filter(|(_, _, h)| usable(h)).count() * 2 <= first.len() {
         return None;
     }
+    // The stream's own frames first: the first of them gives the configuration. The others can
+    // still hold the stream's only program config element.
+    let (mut ordered, others): (Vec<_>, Vec<_>) = first.iter().copied().partition(|(_, _, h)| usable(h));
+    ordered.extend(others);
     let core = stream.to_asc().sample_rate;
     // Without extradata the configuration comes from the first header (and, for channel
     // configuration 0, a program config element in the stream).
     let params = CodecParams::new(CodecId::Aac).with_sample_rate(core);
-    let lookahead = raw_lookahead(src, &usable, crate::track::LOOKAHEAD_PACKETS);
+    let lookahead = raw_lookahead(src, &ordered, crate::track::LOOKAHEAD_PACKETS);
     // Decoded frames are set below, when the decoder has said what a frame decodes to.
     let packets: Vec<(u64, u32, u32)> = frames.iter().map(|&(p, n, h)| (p, n, u32::from(h.raw_data_blocks))).collect();
     let mut n = native(Container::Adts, Packets::Raw { frames: packets, src: src.clone(), next: 0 }, &params, &lookahead, 0, None, None)?;
@@ -648,15 +660,16 @@ 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.
-    if let Some(n) = probe.frames_per_packet().map(|n| n as u64) {
+    let per_packet = probe.frames_per_packet().map(|n| n as u64);
+    if let Some(n) = per_packet {
         let decoded = (t.samples.len() as u64).saturating_mul(n).saturating_sub(skip);
-        if stored.abs_diff(decoded) > n {
+        if stored.abs_diff(decoded) >= n {
             stored = decoded;
         }
     }
     let frames = if stored > 0 { limit.map_or(stored, |l| l.min(stored)) } else { limit.unwrap_or(0) };
     let limit = (stored > 0).then_some(frames).or(limit);
-    let packets = Packets::Mp4 { file: Box::new(file), src: src.clone(), track, next: 0, timescale, rate };
+    let packets = Packets::Mp4 { file: Box::new(file), src: src.clone(), track, next: 0, timescale, rate, per_packet };
     native(container, packets, &params, &lookahead, skip, limit, Some(frames)).map(Ok)
 }
 
@@ -908,9 +921,7 @@ mod adts_tests {
         one[6] |= 1; // two raw data blocks
         let file = one.repeat(8);
         // The file is refused, not decoded at half its length.
-        assert!(
-            !matches!(decode(&file, Some("aac")), Ok((info, _)) if info.codec == crate::CodecId::Aac && info.container == Container::Adts && info.damaged == 0)
-        );
+        assert!(decode(&file, Some("aac")).is_err());
     }
 
     /// Frames of several blocks after the stream has started are concealed at their whole length,
@@ -922,6 +933,7 @@ mod adts_tests {
         let file = [frame(3).repeat(8), two, frame(3).repeat(2)].concat();
         let (info, buf) = decode(&file, None).unwrap();
         assert_eq!((info.damaged, info.frames, buf.frames()), (1, 12 * 1024, 12 * 1024));
+        assert_eq!(crate::probe(&file, None).unwrap().frames, 12 * 1024);
     }
 
     /// One flipped bit in the first header does not silence the stream.

crates/audio/src/track.rs

@@ -17,6 +17,8 @@ enum Inner {
         dec: Box<aac::Decoder>,
         up: Option<aac::Upsample2x>,
         order: Option<&'static [usize]>,
+        /// The format ADTS packets must keep, when the first ADTS header gave the configuration.
+        adts_format: Option<AdtsFormat>,
     },
     Pcm {
         kind: PcmKind,
@@ -80,8 +82,8 @@ impl TrackDecoder {
     /// raw `.aac`), and ADTS headers are stripped from every packet. Other missing configuration is
     /// [`AudioError::Malformed`].
     ///
-    /// Codecs without a native packet decoder yet (MP3, FLAC, ALAC, Vorbis, Opus) are
-    /// [`AudioError::Unsupported`]; `craft_audio::decode` decodes them through symphonia when the
+    /// Codecs without a native packet decoder yet are [`AudioError::Unsupported`];
+    /// `craft_audio::decode` decodes MP3, FLAC, ALAC and Vorbis through symphonia when the
     /// `symphonia` feature is on.
     pub fn new(params: &CodecParams, lookahead: &[&[u8]]) -> Result<TrackDecoder> {
         let rate = params.sample_rate.unwrap_or(0);
@@ -93,10 +95,12 @@ impl TrackDecoder {
                 // header: without extradata, the first header gives it.
                 let payloads: Vec<&[u8]> =
                     lookahead.iter().map(|p| adts_frames(p).and_then(|f| f.first().map(|(_, unit)| *unit)).unwrap_or(p)).collect();
+                let mut adts_format = None;
                 let asc = if params.extradata.is_empty() {
                     let h = lookahead.first().and_then(|p| aac::AdtsHeader::parse(p).ok()).ok_or_else(|| {
                         AudioError::Malformed("AAC track without an AudioSpecificConfig (extradata), and its packets are not ADTS".into())
                     })?;
+                    adts_format = Some(adts_format_of(&h));
                     adts_config(&h, &payloads)?
                 } else {
                     params.extradata.clone()
@@ -111,7 +115,7 @@ impl TrackDecoder {
                 let cfg = dec.config().channel_config;
                 let layout = aac_layout(cfg, dec.channels());
                 let order = layout.is_ordered().then(|| aac::wav_order(cfg)).flatten();
-                (Inner::Aac { dec: Box::new(dec), up, order }, codec, out, layout, SampleFormat::Compressed)
+                (Inner::Aac { dec: Box::new(dec), up, order, adts_format }, codec, out, layout, SampleFormat::Compressed)
             }
             CodecId::Pcm => {
                 let pcm = params.pcm.ok_or_else(|| AudioError::Malformed("PCM track without a sample format".into()))?;
@@ -158,10 +162,10 @@ impl TrackDecoder {
     pub fn decode(&mut self, packet: &[u8]) -> Result<AudioBuffer> {
         let channels = match &mut self.inner {
             #[cfg(feature = "aac-decode")]
-            Inner::Aac { dec, up, order } => {
+            Inner::Aac { dec, up, order, adts_format } => {
                 let mut out: Vec<Vec<f32>> = Vec::new();
                 let units = match adts_frames(packet) {
-                    Some(frames) => adts_units(frames, dec.config())?,
+                    Some(frames) => adts_units(frames, *adts_format)?,
                     None => vec![packet],
                 };
                 for unit in units {
@@ -255,7 +259,7 @@ fn unsupported(codec: CodecId) -> AudioError {
     match feature {
         // A codec this crate decodes, in a build without its feature.
         Some(f) => AudioError::Unsupported(format!("decoding {} needs the `{f}` feature", codec.name())),
-        None if cfg!(feature = "symphonia") => {
+        None if cfg!(feature = "symphonia") && matches!(codec, CodecId::Mp3 | CodecId::Flac | CodecId::Alac | CodecId::Vorbis) => {
             AudioError::Unsupported(format!("no native {} packet decoder yet (craft_audio::decode reads it through symphonia)", codec.name()))
         }
         None => AudioError::Unsupported(format!("no native {} packet decoder yet", codec.name())),
@@ -319,18 +323,29 @@ fn adts_frames(packet: &[u8]) -> Option<Vec<(aac::AdtsHeader, &[u8])>> {
     (whole == packet.len() || frames.len() > MAX_ADTS_FRAMES_PER_PACKET).then_some(frames)
 }
 
-/// The raw access units of an ADTS-framed packet of a stream configured as `config`. A frame that
-/// changes the sample rate, profile or channel configuration would play wrongly, so it is
-/// malformed; frames with several raw data blocks are unsupported.
+/// What an ADTS frame says the stream's format is: profile, sampling frequency index and channel
+/// configuration.
 #[cfg(feature = "aac-decode")]
-fn adts_units<'a>(frames: Vec<(aac::AdtsHeader, &'a [u8])>, config: &aac::AudioSpecificConfig) -> Result<Vec<&'a [u8]>> {
+type AdtsFormat = (u8, u8, u8);
+
+#[cfg(feature = "aac-decode")]
+fn adts_format_of(h: &aac::AdtsHeader) -> AdtsFormat {
+    (h.object_type, h.sf_index, h.channel_config)
+}
+
+/// The raw access units of an ADTS-framed packet. In a stream whose first header gave its `format`,
+/// a frame that changes it would play wrongly, so it is malformed; with extradata, that decides the
+/// format and headers are not compared with it. Frames with several raw data blocks are
+/// unsupported.
+#[cfg(feature = "aac-decode")]
+fn adts_units(frames: Vec<(aac::AdtsHeader, &[u8])>, format: Option<AdtsFormat>) -> Result<Vec<&[u8]>> {
     if frames.len() > MAX_ADTS_FRAMES_PER_PACKET {
         return Err(AudioError::Malformed(format!("a packet of more than {MAX_ADTS_FRAMES_PER_PACKET} ADTS frames")));
     }
     if frames.iter().any(|(h, _)| h.raw_data_blocks > 1) {
         return Err(AudioError::Unsupported("ADTS frames with several raw data blocks".into()));
     }
-    if frames.iter().any(|(h, _)| (h.object_type, h.sf_index, h.channel_config) != (config.object_type, config.sf_index, config.channel_config)) {
+    if format.is_some_and(|f| frames.iter().any(|(h, _)| adts_format_of(h) != f)) {
         return Err(AudioError::Malformed("an ADTS frame changes the stream's format".into()));
     }
     Ok(frames.into_iter().map(|(_, unit)| unit).collect())
@@ -495,6 +510,29 @@ mod tests {
         blocks[6] |= 2; // three raw data blocks
         assert!(matches!(dec.decode(&blocks), Err(AudioError::Unsupported(_))));
         assert_eq!(dec.conceal(Some(&blocks)).unwrap().frames(), 3072);
+        // the profile and the channel configuration are part of the format too
+        let mut main = frame.clone();
+        main[2] &= 0x3F; // profile 0: Main
+        assert!(matches!(dec.decode(&main), Err(AudioError::Malformed(_))));
+        let mut mono = frame.clone();
+        mono[3] = (mono[3] & 0x3F) | (1 << 6);
+        assert!(matches!(dec.decode(&mono), Err(AudioError::Malformed(_))));
+    }
+
+    /// With extradata, a header's differing fields are not a format change (ADTS in MP4 with a
+    /// program config element in its AudioSpecificConfig, or channel configuration 0 in its headers).
+    #[cfg(feature = "aac-decode")]
+    #[test]
+    fn extradata_decides_the_format_of_adts_packets() {
+        let frame = adts_frame(0);
+        let mut stereo = adts_frame(2);
+        let params = aac_params().with_extradata(aac::AudioSpecificConfig::lc(3, 2).to_bytes());
+        let mut dec = TrackDecoder::new(&params, &[]).unwrap();
+        assert_eq!(dec.decode(&stereo).unwrap().channels.len(), 2);
+        stereo[3] &= 0x3F; // channel configuration 0 in the header
+        stereo[2] &= 0xFE;
+        assert_eq!(dec.decode(&stereo).unwrap().channels.len(), 2);
+        assert!(dec.decode(&frame).is_err(), "elements the configuration does not have");
     }
 
     #[cfg(feature = "aac-decode")]

crates/audio/tests/codecs/routing_oracle.rs

@@ -468,6 +468,7 @@ fn adts_with_a_program_config_element_decodes() {
     let (info, buf) = decode(&bytes, None).unwrap();
     assert_eq!((info.container, info.codec, buf.channels.len()), (c.container, c.codec, c.channels));
     assert_eq!(info.layout, c.layout);
+    assert_eq!(buf.frames(), reference(&ff, &path).len() / c.channels);
     // Decoded order (C, L, R, surrounds, LFE) against ffmpeg's (L, R, C, LFE, surrounds).
     let wav: Vec<Vec<f32>> = [1, 2, 0, 5, 3, 4].iter().map(|&i| buf.channels[i].clone()).collect();
     let s = snr(&wav, &reference(&ff, &path));
@@ -521,18 +522,27 @@ fn mp4_with_wrong_durations_or_edits_presents_its_audio() {
     let Some(path) = fixture(&ff, c) else { return };
     let bytes = std::fs::read(path).unwrap();
     let (_, whole) = decode(&bytes, None).unwrap();
-    // After the name: version and flags (4), entries (4); stts: count (4), delta (4) of the first
-    // run; elst v0: duration (4).
-    let stts_delta = 4 + 4 + 4 + 4 + 4;
+    // From the name's start: the name (4), version and flags (4), entries (4), then stts's first
+    // run's count (4) and delta, or elst v0's first duration.
+    let stts_delta = 4 + 4 + 4 + 4;
     let elst_duration = 4 + 4 + 4;
     for (name, file) in [
         ("durations halved", patched(&bytes, b"stts", &[stts_delta], |v| v / 2)),
         ("durations of one", patched(&bytes, b"stts", &[stts_delta], |_| 1)),
         ("durations doubled", patched(&bytes, b"stts", &[stts_delta], |v| v * 2)),
+        ("durations of zero", patched(&bytes, b"stts", &[stts_delta], |_| 0)),
     ] {
+        assert_ne!(file, bytes, "{name}");
         let (info, buf) = decode(&file, None).unwrap();
         assert!(buf.frames().abs_diff(whole.frames()) <= 1024, "{name}: {} frames", buf.frames());
         assert_eq!((probe(&file, None).unwrap().frames, info.frames), (buf.frames() as u64, buf.frames() as u64), "{name}");
+        // A seek lands on the same audio: a packet's place comes from its index, not the durations.
+        let mut d = craft_audio::Decoder::from_bytes(&file, None, &craft_audio::DecodeOptions::default()).unwrap();
+        assert_eq!(d.seek(50_000).unwrap(), 50_000, "{name}");
+        let block = d.next_block().unwrap().unwrap();
+        for (a, b) in block.channels[0].iter().zip(&buf.channels[0][50_000..]) {
+            assert!((a - b).abs() < 1e-4, "{name}: seek");
+        }
     }
     let file = patched(&bytes, b"elst", &[elst_duration], |v| v * 2);
     let (info, buf) = decode(&file, None).unwrap();