From f318d0157f4620a45f23a7cde32b5e58a6056fc2 Mon Sep 17 00:00:00 2001 From: alehel Date: Wed, 4 Nov 2026 15:10:19 +0100 Subject: [PATCH] craft-audio: review fixes, round 4 (#443) - TrackDecoder takes an ADTS stream's format from the one most of the lookahead shares, as the file path did, so one damaged first header does not fail every packet of an MPEG-TS stream. open_adts now just asks it. - MP4 samples of several ADTS frames keep the sample table's timing; a packet count of one frame each truncated them silently. - Sample durations past what the packets decode to no longer over-report the length. - split_adts_upto is not behind aac-decode, as split_adts is not: an aac-encode build in the next PR did not compile. Removes an unreachable ID_END check and open_adts's unused core rate. Co-Authored-By: Claude Opus 5.5 --- crates/audio/src/aac/config.rs | 1 - crates/audio/src/aac/decoder.rs | 3 -- crates/audio/src/aac/mod.rs | 1 - crates/audio/src/read/native.rs | 29 +++++-------- crates/audio/src/track.rs | 45 ++++++++++++++++++--- crates/audio/tests/codecs/routing_oracle.rs | 1 + 6 files changed, 51 insertions(+), 29 deletions(-) diff --git a/crates/audio/src/aac/config.rs b/crates/audio/src/aac/config.rs index 538604da..e4599b58 100644 --- a/crates/audio/src/aac/config.rs +++ b/crates/audio/src/aac/config.rs @@ -385,7 +385,6 @@ pub fn split_adts(data: &[u8]) -> Result> { } /// [`split_adts`], stopping after `max` frames. -#[cfg(feature = "aac-decode")] pub(crate) fn split_adts_upto(mut data: &[u8], max: usize) -> Result> { let mut out = Vec::new(); while data.len() >= 7 && out.len() < max { diff --git a/crates/audio/src/aac/decoder.rs b/crates/audio/src/aac/decoder.rs index c0549499..a8a865d5 100644 --- a/crates/audio/src/aac/decoder.rs +++ b/crates/audio/src/aac/decoder.rs @@ -211,9 +211,6 @@ impl Decoder { } _ => break, // ID_END } - if id == ID_END { - break; - } } if nch == 0 && !saw_element { return Err(Error::InvalidConfig("channel configuration 0 without a program config element")); diff --git a/crates/audio/src/aac/mod.rs b/crates/audio/src/aac/mod.rs index 13419bcc..a9e56f89 100644 --- a/crates/audio/src/aac/mod.rs +++ b/crates/audio/src/aac/mod.rs @@ -26,7 +26,6 @@ mod mdct; mod tables; mod tns; -#[cfg(feature = "aac-decode")] pub(crate) use config::split_adts_upto; pub use config::{AdtsHeader, AudioSpecificConfig, ElementType, ProgramConfig, config_layout, split_adts}; pub use decoder::Decoder; diff --git a/crates/audio/src/read/native.rs b/crates/audio/src/read/native.rs index 78fd9031..35822b1e 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; +use crate::track::{TrackDecoder, adts_format_of, adts_stream_header, several_adts_frames}; use crate::{AudioBuffer, AudioError, AudioInfo, CodecId, CodecParams, Container, PcmFormat, Result, aac}; use craft_media::{isobmff, matroska, ogg}; @@ -573,27 +573,19 @@ fn open_adts<'a>(src: &Src<'a>, at: u64) -> Option>> { Ok(frames) => frames?, Err(e) => return Some(Err(e)), }; - 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 = |h: &aac::AdtsHeader| format(h) == format(&stream) && h.raw_data_blocks == 1; - if first.iter().filter(|(_, _, h)| usable(h)).count() * 2 <= first.len() { + let headers: Vec = first.iter().map(|&(_, _, h)| h).collect(); + let stream = adts_format_of(&adts_stream_header(&headers)?); + if headers.iter().filter(|h| adts_format_of(h) == stream && h.raw_data_blocks == 1).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, &ordered, crate::track::LOOKAHEAD_PACKETS); + // Without extradata the configuration comes from the frames themselves. + let params = CodecParams::new(CodecId::Aac); + let lookahead = raw_lookahead(src, &frames, 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 }, ¶ms, &lookahead, 0, None, None)?; - // HE-AAC decodes twice the core frame length. - let per_block = rescale(aac::FRAME_LEN as u64, u64::from(core), u64::from(n.info.sample_rate)) as u32; + let per_block = n.dec.frames_per_packet()? as u32; if let Packets::Raw { frames, .. } = &mut n.packets { for f in frames.iter_mut() { f.2 *= per_block; @@ -660,10 +652,11 @@ 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. - let per_packet = probe.frames_per_packet().map(|n| n as u64); + // Samples of several ADTS frames (not standard, but they decode) have no one length. + let per_packet = probe.frames_per_packet().map(|n| n as u64).filter(|_| !lookahead.iter().any(|p| several_adts_frames(p))); 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 > decoded || decoded - stored >= n { stored = decoded; } } diff --git a/crates/audio/src/track.rs b/crates/audio/src/track.rs index 7e1d6d69..800effe4 100644 --- a/crates/audio/src/track.rs +++ b/crates/audio/src/track.rs @@ -78,8 +78,8 @@ impl TrackDecoder { /// inside the stream is found in them. /// /// Streams without codec configuration are accepted where the packets carry it: AAC with - /// empty `extradata` takes its configuration from the first packet's ADTS header (MPEG-TS, - /// raw `.aac`), and ADTS headers are stripped from every packet. Other missing configuration is + /// empty `extradata` takes its configuration from the ADTS headers of the lookahead (MPEG-TS, + /// raw `.aac`), the one most of them share, and ADTS headers are stripped from every packet. Other missing configuration is /// [`AudioError::Malformed`]. /// /// Codecs without a native packet decoder yet are [`AudioError::Unsupported`]; @@ -97,7 +97,8 @@ impl TrackDecoder { 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(|| { + let headers: Vec = lookahead.iter().filter_map(|p| aac::AdtsHeader::parse(p).ok()).collect(); + let h = adts_stream_header(&headers).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)); @@ -293,7 +294,20 @@ fn append(out: &mut Vec>, more: Vec>) { #[cfg(feature = "aac-decode")] const MAX_ADTS_FRAMES_PER_PACKET: usize = 1024; -/// The configuration of an AAC stream from its first ADTS header. Channel configuration 0 leaves +/// The header of the format most of `headers` share, the earliest on a tie, so one damaged header +/// does not decide a stream's format. +#[cfg(feature = "aac-decode")] +pub(crate) fn adts_stream_header(headers: &[aac::AdtsHeader]) -> Option { + headers.iter().rev().max_by_key(|h| headers.iter().filter(|o| adts_format_of(o) == adts_format_of(h)).count()).copied() +} + +/// 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")] fn adts_config(header: &aac::AdtsHeader, first_units: &[&[u8]]) -> Result> { @@ -326,10 +340,10 @@ fn adts_frames(packet: &[u8]) -> Option> { /// What an ADTS frame says the stream's format is: profile, sampling frequency index and channel /// configuration. #[cfg(feature = "aac-decode")] -type AdtsFormat = (u8, u8, u8); +pub(crate) type AdtsFormat = (u8, u8, u8); #[cfg(feature = "aac-decode")] -fn adts_format_of(h: &aac::AdtsHeader) -> AdtsFormat { +pub(crate) fn adts_format_of(h: &aac::AdtsHeader) -> AdtsFormat { (h.object_type, h.sf_index, h.channel_config) } @@ -535,6 +549,25 @@ mod tests { assert!(dec.decode(&frame).is_err(), "elements the configuration does not have"); } + /// One damaged header at the start of a stream does not decide its format. + #[cfg(feature = "aac-decode")] + #[test] + fn the_adts_format_is_the_one_most_of_the_lookahead_shares() { + let good = adts_frame(2); + let mut damaged = good.clone(); + damaged[2] = (damaged[2] & 0xC3) | (4 << 2); // sampling frequency index 4 where the rest have 3 + let lookahead: [&[u8]; 4] = [&damaged, &good, &good, &good]; + let mut dec = TrackDecoder::new(&aac_params(), &lookahead).unwrap(); + assert_eq!(dec.sample_rate(), 48_000); + assert!(dec.decode(&damaged).is_err()); + assert_eq!(dec.decode(&good).unwrap().frames(), 1024); + // on a tie, the earlier one + 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])); + } + #[cfg(feature = "aac-decode")] #[test] fn a_packet_of_too_many_adts_frames_is_refused() { diff --git a/crates/audio/tests/codecs/routing_oracle.rs b/crates/audio/tests/codecs/routing_oracle.rs index 2e7f050d..014a18fe 100644 --- a/crates/audio/tests/codecs/routing_oracle.rs +++ b/crates/audio/tests/codecs/routing_oracle.rs @@ -531,6 +531,7 @@ fn mp4_with_wrong_durations_or_edits_presents_its_audio() { ("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)), + ("last delta longer than a packet", patched(&bytes, b"stts", &[stts_delta + 8], |_| 1500)), ] { assert_ne!(file, bytes, "{name}"); let (info, buf) = decode(&file, None).unwrap();