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

From CodeRabbit's review of round 8:
- AudioSpecificConfig::parse consumes extensionFlag3 after the config when
  extensionFlag is set, so the 0x2b7 sync extension after it is found.
- AudioSpecificConfig::to_bytes refuses a sampling rate of 0, which would not
  parse back.
- ADTS-framed HE-AAC in MP4 whose sample durations count core frames is not cut
  to the durations' half length: durations short of one frame per sample no
  longer end an ADTS-framed track, and its length is unknown until decoded.

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

crates/audio/src/aac/config.rs

@@ -280,8 +280,13 @@ impl AudioSpecificConfig {
         if br.read_bits(1)? == 1 {
             br.skip(14)?; // coreCoderDelay
         }
-        let _extension_flag = br.read_bits(1)?;
+        let extension_flag = br.read_bits(1)? == 1;
         let pce = if channel_config == 0 { Some(ProgramConfig::parse(&mut br)?) } else { None };
+        // GASpecificConfig ends with extensionFlag3 when extensionFlag is set (AAC-LC has no other
+        // extension fields); what follows it is the sync extension.
+        if extension_flag && br.bits_left() > 0 {
+            br.skip(1)?;
+        }
         if frame_length_flag {
             return Err(Error::Unsupported("960-sample frames"));
         }
@@ -297,11 +302,14 @@ impl AudioSpecificConfig {
     }
 
     /// Serialise (AAC-LC, no SBR signalling). Channel configuration 0 needs its program config
-    /// element: without one the bytes would not parse back.
+    /// element, and the sampling rate must not be 0: otherwise 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"));
         }
+        if self.sample_rate == 0 {
+            return Err(Error::InvalidConfig("a sampling rate of 0"));
+        }
         let mut bw = BitWriter::new();
         bw.write_bits(self.object_type as u32, 5);
         match crate::aac::tables::sample_rate_index(self.sample_rate) {
@@ -478,6 +486,13 @@ mod tests {
         }
         let asc = AudioSpecificConfig::parse(&bw.finish()).unwrap();
         assert_eq!((asc.sbr, asc.extension_sample_rate, asc.ps), (true, Some(44_100), true));
+        // with extensionFlag set, extensionFlag3 comes first
+        let mut bw = BitWriter::new();
+        for (value, bits) in [(2, 5), (7, 4), (1, 4), (0b001, 3), (0, 1), (0x2b7, 11), (5, 5), (1, 1), (4, 4)] {
+            bw.write_bits(value, bits);
+        }
+        let asc = AudioSpecificConfig::parse(&bw.finish()).unwrap();
+        assert_eq!((asc.sbr, asc.extension_sample_rate), (true, Some(44_100)));
         // 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();
@@ -497,6 +512,7 @@ mod tests {
     #[test]
     fn configs_that_would_not_parse_back_are_not_written() {
         assert!(AudioSpecificConfig::lc(4, 0).to_bytes().is_err());
+        assert!(AudioSpecificConfig::lc(13, 2).to_bytes().is_err(), "a sampling rate of 0");
         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);

crates/audio/src/read/native.rs

@@ -664,19 +664,25 @@ fn open_mp4<'a>(src: &Src<'a>, container: Container, track: Option<usize>) -> Op
     // 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.
     // 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.
+    // reading every sample: their sample table's timing is kept instead, unless it falls short of
+    // one frame per sample (durations counting HE-AAC's core frames). Then it can't end the track,
+    // and its length is unknown until decoded.
     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 {
-            stored = decoded;
+    let mut length_known = true;
+    if let Some(n) = probe.frames_per_packet().map(|n| n as u64) {
+        let one_each = (t.samples.len() as u64).saturating_mul(n).saturating_sub(skip);
+        if !adts && (stored > one_each || one_each - stored >= n) {
+            stored = one_each;
+        } else if adts && one_each.saturating_sub(stored) >= n {
+            stored = 0;
+            length_known = limit.is_some();
         }
     }
     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, per_packet };
-    native(container, packets, &params, &lookahead, skip, limit, Some(frames)).map(Ok)
+    native(container, packets, &params, &lookahead, skip, limit, length_known.then_some(frames)).map(Ok)
 }
 
 /// A Matroska / WebM file's audio track, if its codec is native.
@@ -1009,6 +1015,31 @@ mod adts_tests {
         assert!(matches!(opened, Err(AudioError::Io(_))), "{:?}", opened.err());
     }
 
+    /// ADTS-framed HE-AAC whose sample durations count core frames is not cut to the durations'
+    /// half length.
+    #[test]
+    fn adts_framed_he_aac_in_mp4_with_core_durations_keeps_its_length() {
+        use craft_media::bitstream::BitWriter;
+        use craft_media::isobmff::{Brand, Mp4Writer, SampleEntry, TrackConfig, WriteSample, WriterOptions};
+        // mono silence with SBR data in a fill element (implicit HE-AAC), at 22.05 kHz
+        let mut bw = BitWriter::new();
+        for (value, bits) in [(0, 7), (0, 22), (6, 3), (1, 4), (13, 4), (0, 4), (7, 3)] {
+            bw.write_bits(value, bits);
+        }
+        let au = bw.finish();
+        let frame = [&crate::aac::AdtsHeader::write(2, 7, 1, au.len(), 0x7FF).unwrap()[..], &au].concat();
+        let asc = crate::aac::AudioSpecificConfig::lc(7, 1).to_bytes().unwrap();
+        let mut w = Mp4Writer::new(std::io::Cursor::new(Vec::new()), WriterOptions::new(Brand::Mp4)).unwrap();
+        let track = w.add_track(TrackConfig::new(SampleEntry::aac(asc, 1, 44_100), 44_100)).unwrap();
+        for _ in 0..12 {
+            w.write_sample(track, WriteSample { data: &frame, duration: 1024, composition_offset: 0, is_sync: true }).unwrap();
+        }
+        let file = w.finish().unwrap().into_inner();
+        let (info, buf) = decode(&file, None).unwrap();
+        assert_eq!((info.codec, info.sample_rate, buf.frames()), (crate::CodecId::HeAac, 44_100, 12 * 2048));
+        assert_eq!(crate::probe(&file, None).unwrap().frames, 12 * 2048);
+    }
+
     /// A sample of several ADTS frames past the lookahead still counts them all.
     #[test]
     fn a_later_mp4_sample_of_several_adts_frames_keeps_its_length() {