From ac8c5aae5d508021053978283a2ec51217ffb8fb Mon Sep 17 00:00:00 2001 From: alehel Date: Sat, 7 Nov 2026 22:26:05 +0100 Subject: [PATCH] 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 --- crates/audio/README.md | 5 +- crates/audio/src/aac/config.rs | 101 ++++++++++++++++++-- crates/audio/src/aac/decoder.rs | 3 + crates/audio/src/aac/mod.rs | 13 ++- crates/audio/src/read/native.rs | 23 +++-- crates/audio/src/track.rs | 21 ++-- crates/audio/tests/codecs/routing_oracle.rs | 34 +++++++ 7 files changed, 170 insertions(+), 30 deletions(-) diff --git a/crates/audio/README.md b/crates/audio/README.md index 667ae2a7..923b898f 100644 --- a/crates/audio/README.md +++ b/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 diff --git a/crates/audio/src/aac/config.rs b/crates/audio/src/aac/config.rs index 01cc8254..9856edd2 100644 --- a/crates/audio/src/aac/config.rs +++ b/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, 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 { 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> { + 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= 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")); diff --git a/crates/audio/src/aac/mod.rs b/crates/audio/src/aac/mod.rs index 13419bcc..23aef936 100644 --- a/crates/audio/src/aac/mod.rs +++ b/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> { - 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)); diff --git a/crates/audio/src/read/native.rs b/crates/audio/src/read/native.rs index ad0c8335..616aa71c 100644 --- a/crates/audio/src/read/native.rs +++ b/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) -> 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) -> 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 { diff --git a/crates/audio/src/track.rs b/crates/audio/src/track.rs index 3afc18fe..39418357 100644 --- a/crates/audio/src/track.rs +++ b/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::()); - 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::().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`). diff --git a/crates/audio/tests/codecs/routing_oracle.rs b/crates/audio/tests/codecs/routing_oracle.rs index 484a9c48..bcb60c1e 100644 --- a/crates/audio/tests/codecs/routing_oracle.rs +++ b/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"); +}