From 8291279c8b7a4fb91aaa6115910e6d4e27bf06e6 Mon Sep 17 00:00:00 2001 From: alehel Date: Sat, 7 Nov 2026 22:41:48 +0100 Subject: [PATCH] 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 --- crates/audio/src/aac/config.rs | 20 +++++++++++++-- crates/audio/src/read/native.rs | 43 ++++++++++++++++++++++++++++----- 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/crates/audio/src/aac/config.rs b/crates/audio/src/aac/config.rs index 9856edd2..eb7ed7e2 100644 --- a/crates/audio/src/aac/config.rs +++ b/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> { 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); diff --git a/crates/audio/src/read/native.rs b/crates/audio/src/read/native.rs index 616aa71c..0441486b 100644 --- a/crates/audio/src/read/native.rs +++ b/crates/audio/src/read/native.rs @@ -664,19 +664,25 @@ fn open_mp4<'a>(src: &Src<'a>, container: Container, track: Option) -> 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, ¶ms, &lookahead, skip, limit, Some(frames)).map(Ok) + native(container, packets, ¶ms, &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() {