From 542ca365e20605597032d7cc514367b6cb5da210 Mon Sep 17 00:00:00 2001 From: alehel Date: Thu, 5 Nov 2026 06:37:50 +0100 Subject: [PATCH] craft-audio: review fixes, round 7 (#443) Reading: - an element of another type or a tag a program config does not list is refused, not written to another channel; only an LFE coded as an SCE is still allowed, as ffmpeg allows it - an explicit sampling rate of 0 is refused - a packet's later ADTS frames are searched for SBR - a multi-frame ADTS packet that fails leaves the decoder as it was, and one whose frames change the channel count is an error - the whole ADTS frames before a damaged end of a packet decode - a failed read of a raw stream's first frames is an I/O error - an MP4 sample of several ADTS frames past the lookahead keeps its length - an Ogg file with leading bytes keeps its track order and mask comments Writing: - ProgramConfig::write, AudioSpecificConfig::to_bytes and AdtsHeader::write refuse what their fields cannot hold Tests: - the routing test checks one alignment for the ffmpeg version under test - a default decode past an Ogg stream symphonia has no track for Co-Authored-By: Claude Opus 5.5 --- crates/audio/src/aac/config.rs | 80 +++++++-- crates/audio/src/aac/decoder.rs | 50 ++++-- crates/audio/src/read/native.rs | 56 ++++++- crates/audio/src/read/symph.rs | 4 +- crates/audio/src/track.rs | 157 +++++++++++++++--- crates/audio/src/write/flac/oracle_tests.rs | 52 ++++++ .../audio/tests/codecs/aac/decoder_oracle.rs | 2 +- crates/audio/tests/codecs/routing_oracle.rs | 18 +- 8 files changed, 359 insertions(+), 60 deletions(-) diff --git a/crates/audio/src/aac/config.rs b/crates/audio/src/aac/config.rs index e4599b58..01cc8254 100644 --- a/crates/audio/src/aac/config.rs +++ b/crates/audio/src/aac/config.rs @@ -98,8 +98,15 @@ impl ProgramConfig { Ok(p) } - /// Serialise the element (no mixdown signalling; comment truncated to 255 bytes). - pub fn write(&self, bw: &mut BitWriter) { + /// Serialise the element (no mixdown signalling; comment truncated to 255 bytes). More elements + /// than its count fields hold are refused, with nothing written. + pub fn write(&self, bw: &mut BitWriter) -> Result<()> { + if [self.front.len(), self.side.len(), self.back.len(), self.cc.len()].iter().any(|&n| n > 15) + || self.lfe.len() > 3 + || self.assoc_data.len() > 7 + { + return Err(Error::InvalidConfig("more elements than a program config element can list")); + } bw.write_bits(self.element_instance_tag as u32, 4); bw.write_bits(self.object_type as u32, 2); bw.write_bits(self.sf_index as u32, 4); @@ -131,6 +138,7 @@ impl ProgramConfig { for &b in self.comment.iter().take(255) { bw.write_bits(b as u32, 8); } + Ok(()) } /// Elements in output channel order (front, side, back, LFE). @@ -200,6 +208,9 @@ fn read_rate(br: &mut BitReader) -> Result<(u8, u32)> { let i = br.read_bits(4)? as u8; if i == 15 { let r = br.read_bits(24)?; + if r == 0 { + return Err(Error::InvalidConfig("sampling rate of 0")); + } return Ok((table_index_for_rate(r), r)); } let r = *SAMPLE_RATES.get(i as usize).ok_or(Error::InvalidConfig("reserved sampling frequency index"))?; @@ -265,7 +276,7 @@ impl AudioSpecificConfig { } /// Serialise (AAC-LC, no SBR signalling). - pub fn to_bytes(&self) -> Vec { + pub fn to_bytes(&self) -> Result> { let mut bw = BitWriter::new(); bw.write_bits(self.object_type as u32, 5); match crate::aac::tables::sample_rate_index(self.sample_rate) { @@ -280,9 +291,9 @@ impl AudioSpecificConfig { if self.channel_config == 0 && let Some(p) = &self.pce { - p.write(&mut bw); + p.write(&mut bw)?; } - bw.finish() + Ok(bw.finish()) } /// Elements in output channel order. @@ -355,8 +366,12 @@ 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. - pub fn write(object_type: u8, sf_index: u8, channel_config: u8, payload_len: usize, buffer_fullness: u16) -> [u8; 7] { + /// 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. + 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")); + } let len = (payload_len + 7) as u32; let mut bw = BitWriter::new(); bw.write_bits(0xFFF, 12); @@ -374,7 +389,7 @@ impl AdtsHeader { let v = bw.finish(); let mut out = [0u8; 7]; out.copy_from_slice(&v); - out + Ok(out) } } @@ -413,21 +428,60 @@ pub(crate) fn split_adts_upto(mut data: &[u8], max: usize) -> Result, @@ -224,22 +225,16 @@ impl Decoder { } fn find_slot(&self, ty: ElementType, tag: u8, used: &[bool]) -> Result { - let compatible = |t: ElementType| t == ty || (ty != ElementType::Cpe && t != ElementType::Cpe); - // A program config names its elements' tags. An indexed configuration does not: its elements - // are placed by the order they arrive in, whatever their tags. - if self.asc.channel_config == 0 - && let Some(i) = self.layout.iter().enumerate().position(|(i, &(t, g))| t == ty && g == tag && !used[i]) - { - return Ok(i); - } - if let Some(i) = self.layout.iter().enumerate().position(|(i, &(t, _))| t == ty && !used[i]) { - return Ok(i); - } - self.layout - .iter() - .enumerate() - .position(|(i, &(t, _))| compatible(t) && !used[i]) - .ok_or(Error::Bitstream("unexpected channel element for the channel configuration")) + let free = |want: &dyn Fn(ElementType, u8) -> bool| self.layout.iter().enumerate().position(|(i, &(t, g))| want(t, g) && !used[i]); + let slot = if self.asc.channel_config == 0 { + // A program config names its elements' tags. + free(&|t, g| t == ty && g == tag) + } else { + // An indexed configuration does not: its elements are placed by the order they arrive + // in, whatever their tags. Some encoders code an LFE as an SCE, as ffmpeg allows. + free(&|t, _| t == ty).or_else(|| free(&|t, _| ty == ElementType::Sce && t == ElementType::Lfe)) + }; + slot.ok_or(Error::Bitstream("unexpected channel element for the channel configuration")) } fn decode_cpe(&mut self, br: &mut BitReader, ch: usize) -> Result<[Vec; 2]> { @@ -612,7 +607,7 @@ mod tests { let mut bw = BitWriter::new(); if let Some(pce) = pce { bw.write_bits(ID_PCE, 3); - pce.write(&mut bw); + pce.write(&mut bw).unwrap(); } for &(id, tag) in elements { bw.write_bits(id, 3); @@ -647,6 +642,27 @@ mod tests { assert_eq!(dec.find_slot(ElementType::Cpe, 5, &[false, false]), Ok(1)); } + /// A program config lists its elements: one it does not list has no place. + #[test] + fn program_configs_refuse_unlisted_tags() { + let pce = ProgramConfig { front: vec![(true, 3), (true, 5)], ..Default::default() }; + let asc = AudioSpecificConfig { pce: Some(pce), ..AudioSpecificConfig::lc(4, 0) }; + let dec = Decoder::from_config(asc).unwrap(); + assert!(dec.find_slot(ElementType::Cpe, 9, &[false, false]).is_err()); + assert!(dec.find_slot(ElementType::Sce, 3, &[false, false]).is_err()); + } + + /// Some encoders code 5.1's LFE as an SCE, as ffmpeg allows. Nothing else changes type: an LFE + /// is not written to the centre channel. + #[test] + fn indexed_configurations_refuse_elements_of_another_type() { + let dec = Decoder::from_config(AudioSpecificConfig::lc(4, 6)).unwrap(); + // SCE, CPE, CPE, LFE + assert_eq!(dec.find_slot(ElementType::Sce, 1, &[true, false, false, false]), Ok(3)); + assert!(dec.find_slot(ElementType::Lfe, 1, &[false, false, false, true]).is_err()); + assert!(dec.find_slot(ElementType::Cpe, 2, &[false, true, true, false]).is_err()); + } + /// A PCE in an access unit that then fails leaves the layout as it was. #[test] fn a_damaged_in_band_pce_does_not_change_the_layout() { diff --git a/crates/audio/src/read/native.rs b/crates/audio/src/read/native.rs index 35822b1e..ad0c8335 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_stream_header, several_adts_frames}; +use crate::track::{TrackDecoder, adts_format_of, adts_framed, adts_stream_header, several_adts_frames}; use crate::{AudioBuffer, AudioError, AudioInfo, CodecId, CodecParams, Container, PcmFormat, Result, aac}; use craft_media::{isobmff, matroska, ogg}; @@ -558,8 +558,8 @@ fn raw_frames(src: &Src<'_>, at: u64, frame_len: impl Fn(&[u8]) -> Option<(us } /// The first `n` frames of a raw stream. -fn raw_lookahead(src: &Src<'_>, frames: &[(u64, u32, T)], n: usize) -> Vec> { - frames.iter().take(n).filter_map(|&(at, len, _)| src.bytes(at, len as usize)).collect() +fn raw_lookahead(src: &Src<'_>, frames: &[(u64, u32, T)], n: usize) -> Result>> { + frames.iter().take(n).map(|&(at, len, _)| src.read(at, len as usize)).collect() } /// A raw ADTS AAC stream from `at`: its frames, headers included, are the packets. The stream's @@ -581,7 +581,10 @@ fn open_adts<'a>(src: &Src<'a>, at: u64) -> Option>> { } // 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); + let lookahead = match raw_lookahead(src, &frames, crate::track::LOOKAHEAD_PACKETS) { + Ok(lookahead) => lookahead, + Err(e) => return Some(Err(e)), + }; // 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)?; @@ -652,8 +655,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. - // 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))); + // 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); 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 { @@ -897,7 +903,7 @@ mod adts_tests { fn frame(sf_index: u8) -> Vec { // ID_SCE, tag 0, an empty channel stream, ID_END let au = [0x00, 0x00, 0x00, 0x07]; - [&crate::aac::AdtsHeader::write(2, sf_index, 1, au.len(), 0x7FF)[..], &au].concat() + [&crate::aac::AdtsHeader::write(2, sf_index, 1, au.len(), 0x7FF).unwrap()[..], &au].concat() } #[test] @@ -976,6 +982,42 @@ mod adts_tests { } } + /// A frame that reads in the scan and fails when read again. + #[test] + fn a_source_failing_during_the_lookahead_is_an_io_error() { + struct FailsFrames(Vec); + impl ByteSource for FailsFrames { + fn len(&self) -> u64 { + self.0.len() as u64 + } + fn read_at(&self, offset: u64, buf: &mut [u8]) -> std::io::Result<()> { + // A whole frame in the middle: the scan reads windows, to the end of the file. + if buf.len() == 11 && offset + 11 < self.len() { + return Err(std::io::Error::other("the source failed")); + } + self.0.as_slice().read_at(offset, buf) + } + } + let opened = Decoder::open(FailsFrames(frame(3).repeat(64)), Some("aac"), &DecodeOptions::default()); + assert!(matches!(opened, Err(AudioError::Io(_))), "{:?}", opened.err()); + } + + /// 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() { + use craft_media::isobmff::{Brand, Mp4Writer, SampleEntry, TrackConfig, WriteSample, WriterOptions}; + let asc = crate::aac::AudioSpecificConfig::lc(3, 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, 48_000), 48_000)).unwrap(); + for i in 0..12 { + let (data, duration) = if i == 10 { (frame(3).repeat(2), 2048) } else { (frame(3), 1024) }; + w.write_sample(track, WriteSample { data: &data, duration, composition_offset: 0, is_sync: true }).unwrap(); + } + let file = w.finish().unwrap().into_inner(); + let (_, buf) = decode(&file, None).unwrap(); + assert_eq!(buf.frames(), 13 * 1024); + } + #[test] fn a_source_failing_during_the_frame_scan_is_an_io_error() { let bytes = frame(3).repeat(100_000); diff --git a/crates/audio/src/read/symph.rs b/crates/audio/src/read/symph.rs index 126bd5c8..e16c6e31 100644 --- a/crates/audio/src/read/symph.rs +++ b/crates/audio/src/read/symph.rs @@ -376,7 +376,9 @@ fn open(bytes: Cow<'_, [u8]>, detected: Option) -> Result { checked => (checked, None), }; let body = bytes.get(start..).unwrap_or_default(); - let ogg_streams = craft_media::ogg::sniff(body).then(|| super::flac::ogg_audio_streams(body)); + // symphonia finds the first page past leading bytes, so look for it the same way. + let first_page = body.windows(4).take(MAX_PREFIX_SCAN).position(|w| w == b"OggS"); + let ogg_streams = first_page.map(|at| super::flac::ogg_audio_streams(&body[at..])); // symphonia reads from an owned stream: a borrowed buffer is copied once, a buffer read from // a shared source is moved in. let mut mss = MediaSourceStream::new(Box::new(Cursor::new(bytes.into_owned())), MediaSourceStreamOptions::default()); diff --git a/crates/audio/src/track.rs b/crates/audio/src/track.rs index d5eee04f..3afc18fe 100644 --- a/crates/audio/src/track.rs +++ b/crates/audio/src/track.rs @@ -93,8 +93,11 @@ impl TrackDecoder { CodecId::Aac | CodecId::HeAac => { // ADTS-framed packets (MPEG-TS, raw `.aac`) carry their configuration in each // header: without extradata, the headers of the lookahead give 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 payloads: Vec<&[u8]> = lookahead + .iter() + .take(LOOKAHEAD_PACKETS) + .flat_map(|p| adts_frames(p).map_or_else(|| vec![*p], |frames| frames.into_iter().map(|(_, unit)| unit).collect())) + .collect(); let mut adts_format = None; let asc = if params.extradata.is_empty() { let headers: Vec = lookahead.iter().filter_map(|p| aac::AdtsHeader::parse(p).ok()).collect(); @@ -108,7 +111,7 @@ impl TrackDecoder { }; let asc = asc.as_slice(); let dec = aac::Decoder::new(asc)?; - let out = aac::output_rate(asc, payloads.iter().copied().take(LOOKAHEAD_PACKETS)).unwrap_or(dec.sample_rate()); + let out = aac::output_rate(asc, payloads.iter().copied()).unwrap_or(dec.sample_rate()); let up = (dec.sample_rate().checked_mul(2) == Some(out)).then(|| aac::Upsample2x::new(dec.channels())); let codec = if up.is_some() { CodecId::HeAac } else { CodecId::Aac }; // Only twice the core rate is upsampled to; any other signalled rate plays the core. @@ -169,12 +172,21 @@ impl TrackDecoder { Some(frames) => adts_units(frames, *adts_format)?, None => vec![packet], }; - for unit in units { + // A failed frame leaves the decoder as it was, so the frames before it in the + // packet must too: their audio is replaced by the packet's concealment. + let before = (units.len() > 1).then(|| (dec.clone(), up.clone())); + let decoded = units.into_iter().try_for_each(|unit| { let mut block = dec.decode(unit)?; - if let Some(up) = up { + if let Some(up) = up.as_mut() { block = up.process(block); } - append(&mut out, block); + append(&mut out, block) + }); + if let Err(e) = decoded { + if let Some(before) = before { + (*dec, *up) = before; + } + return Err(e); } reorder(out, *order) } @@ -277,16 +289,20 @@ fn reorder(mut channels: Vec>, order: Option<&[usize]>) -> Vec } } -/// Append `more` to `out` channel by channel; a block whose channel count differs is dropped. +/// Append `more` to `out` channel by channel. A packet whose frames differ in channel count is +/// malformed: one buffer cannot hold them. #[cfg(feature = "aac-decode")] -fn append(out: &mut Vec>, more: Vec>) { +fn append(out: &mut Vec>, more: Vec>) -> Result<()> { if out.is_empty() { *out = more; } else if out.len() == more.len() { for (o, c) in out.iter_mut().zip(more) { o.extend(c); } + } else { + return Err(AudioError::Malformed("the frames of a packet change the channel count".into())); } + Ok(()) } /// The most ADTS frames a packet may hold: its audio is decoded and held in memory before any @@ -301,6 +317,12 @@ pub(crate) fn adts_stream_header(headers: &[aac::AdtsHeader]) -> Option 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 { @@ -312,29 +334,28 @@ pub(crate) fn several_adts_frames(packet: &[u8]) -> bool { #[cfg(feature = "aac-decode")] fn adts_config(header: &aac::AdtsHeader, first_units: &[&[u8]]) -> Result> { if header.channel_config != 0 { - return Ok(header.to_asc().to_bytes()); + return Ok(header.to_asc().to_bytes()?); } let mut dec = aac::Decoder::from_adts(header)?; for unit in first_units { let _ = dec.decode(unit); if dec.config().pce.is_some() { - return Ok(dec.config().to_bytes()); + return Ok(dec.config().to_bytes()?); } } Err(AudioError::Malformed("ADTS stream with channel configuration 0, and no program config element in its first packets".into())) } -/// The frames of an ADTS-framed packet (one or more whole frames, nothing else), or `None` when -/// the packet is not ADTS. +/// The whole frames at the start of an ADTS-framed packet, without a damaged or truncated end, or +/// `None` when the packet does not start with one. A raw access unit never looks like one: its first +/// three bits would be 7, ID_END, leaving nothing to decode. #[cfg(feature = "aac-decode")] fn adts_frames(packet: &[u8]) -> Option> { if packet.len() < 7 || packet[0] != 0xFF || packet[1] & 0xF6 != 0xF0 { return None; } // Past the limit, what matters is only that there are too many: `adts_units` refuses them. - let frames = aac::split_adts_upto(packet, MAX_ADTS_FRAMES_PER_PACKET + 1).ok()?; - let whole: usize = frames.iter().map(|(h, _)| h.frame_length).sum(); - (whole == packet.len() || frames.len() > MAX_ADTS_FRAMES_PER_PACKET).then_some(frames) + aac::split_adts_upto(packet, MAX_ADTS_FRAMES_PER_PACKET + 1).ok() } /// What an ADTS frame says the stream's format is: profile, sampling frequency index and channel @@ -429,7 +450,7 @@ mod tests { let mut bw = BitWriter::new(); if channel_config == 0 { bw.write_bits(5, 3); // ID_PCE - aac::ProgramConfig { front: vec![(false, 0), (true, 0)], ..Default::default() }.write(&mut bw); + aac::ProgramConfig { front: vec![(false, 0), (true, 0)], ..Default::default() }.write(&mut bw).unwrap(); bw.write_bits(0, 3); // ID_SCE bw.write_bits(0, 4); bw.write_bits(0, 22); // global_gain, ics_info, no pulse, TNS or gain control data @@ -439,7 +460,7 @@ mod tests { bw.write_bits(0, 44); // two such channel streams bw.write_bits(7, 3); // ID_END let au = bw.finish(); - [&aac::AdtsHeader::write(2, 3, channel_config, au.len(), 0x7FF)[..], &au].concat() + [&aac::AdtsHeader::write(2, 3, channel_config, au.len(), 0x7FF).unwrap()[..], &au].concat() } #[cfg(feature = "aac-decode")] @@ -540,7 +561,7 @@ mod tests { 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 params = aac_params().with_extradata(aac::AudioSpecificConfig::lc(3, 2).to_bytes().unwrap()); let mut dec = TrackDecoder::new(¶ms, &[]).unwrap(); assert_eq!(dec.decode(&stereo).unwrap().channels.len(), 2); stereo[3] &= 0x3F; // channel configuration 0 in the header @@ -568,6 +589,104 @@ mod tests { assert!(several_adts_frames(&good.repeat(2)) && !several_adts_frames(&good) && !several_adts_frames(&[0; 8])); } + /// `au` as a stereo ADTS frame at 48 kHz (channel configuration `channel_config`). + #[cfg(feature = "aac-decode")] + fn adts_of(au: &[u8], channel_config: u8) -> Vec { + [&aac::AdtsHeader::write(2, 3, channel_config, au.len(), 0x7FF).unwrap()[..], au].concat() + } + + /// An access unit of `pce` and `elements` (element id, tag), all empty, with channel streams of + /// 22 bits (a CPE's are preceded by its `common_window` bit). + #[cfg(feature = "aac-decode")] + fn au_of(pce: Option<&aac::ProgramConfig>, elements: &[(u32, u32)]) -> Vec { + use craft_media::bitstream::BitWriter; + let mut bw = BitWriter::new(); + if let Some(pce) = pce { + bw.write_bits(5, 3); // ID_PCE + pce.write(&mut bw).unwrap(); + } + for &(id, tag) in elements { + bw.write_bits(id, 3); + bw.write_bits(tag, 4); + if id == 1 { + bw.write_bits(0, 1); + bw.write_bits(0, 22); + } + bw.write_bits(0, 22); + } + bw.write_bits(7, 3); // ID_END + bw.finish() + } + + #[cfg(feature = "aac-decode")] + fn stereo_pce() -> aac::ProgramConfig { + aac::ProgramConfig { front: vec![(true, 0)], ..Default::default() } + } + + /// HE-AAC whose SBR data first appears in a later frame of a lookahead packet is found. + #[cfg(feature = "aac-decode")] + #[test] + fn sbr_in_a_later_adts_frame_of_a_packet_is_found() { + use craft_media::bitstream::BitWriter; + let unit = |sbr: bool| { + let mut bw = BitWriter::new(); + bw.write_bits(0, 7); // ID_SCE, tag 0 + bw.write_bits(0, 22); + if sbr { + bw.write_bits(6, 3); // ID_FIL + bw.write_bits(1, 4); + bw.write_bits(13, 4); // EXT_SBR_DATA + bw.write_bits(0, 4); + } + bw.write_bits(7, 3); + bw.finish() + }; + let frame = |au: &[u8]| [&aac::AdtsHeader::write(2, 7, 1, au.len(), 0x7FF).unwrap()[..], au].concat(); + let packet = [frame(&unit(false)), frame(&unit(true))].concat(); + let params = aac_params().with_extradata(vec![0x13, 0x88]); + let dec = TrackDecoder::new(¶ms, &[&packet]).unwrap(); + assert_eq!((dec.codec(), dec.sample_rate()), (CodecId::HeAac, 44_100)); + } + + /// A packet whose later frame fails leaves the decoder as a failed single frame does. + #[cfg(feature = "aac-decode")] + #[test] + fn a_failed_frame_undoes_the_earlier_frames_of_its_packet() { + let first = adts_frame(0); + let mut dec = TrackDecoder::new(&aac_params(), &[&first]).unwrap(); + assert_eq!(dec.decode(&first).unwrap().channels.len(), 3); + // A valid frame that changes the layout, then one that fails. + let change = adts_of(&au_of(Some(&stereo_pce()), &[(1, 0)]), 0); + let bad = adts_of(&au_of(None, &[(2, 0)]), 0); // ID_CCE, unsupported + assert!(dec.decode(&[change, bad].concat()).is_err()); + // Still three channels: SCE and CPE with no program config of their own. + let plain = adts_of(&au_of(None, &[(0, 0), (1, 0)]), 0); + assert_eq!(dec.decode(&plain).unwrap().channels.len(), 3); + } + + /// Frames of one packet with different channel counts are an error, not a short packet. + #[cfg(feature = "aac-decode")] + #[test] + fn a_channel_count_change_inside_a_packet_is_an_error() { + let first = adts_frame(0); + let mut dec = TrackDecoder::new(&aac_params(), &[&first]).unwrap(); + let change = adts_of(&au_of(Some(&stereo_pce()), &[(1, 0)]), 0); + assert!(matches!(dec.decode(&[first.clone(), change.clone()].concat()), Err(AudioError::Malformed(_)))); + assert_eq!(dec.conceal(Some(&[first, change].concat())).unwrap().frames(), 2048); + } + + /// Whole frames before a damaged or truncated end of a packet still decode. + #[cfg(feature = "aac-decode")] + #[test] + fn the_whole_frames_before_a_damaged_tail_decode() { + let frame = adts_frame(2); + let mut dec = TrackDecoder::new(&aac_params(), &[&frame]).unwrap(); + let cut = [frame.repeat(2), frame[..frame.len() - 3].to_vec()].concat(); + assert_eq!(dec.decode(&cut).unwrap().frames(), 2048); + let garbage = [frame.repeat(2), vec![0x12; 9]].concat(); + assert_eq!(dec.decode(&garbage).unwrap().frames(), 2048); + } + #[cfg(feature = "aac-decode")] #[test] fn a_packet_of_too_many_adts_frames_is_refused() { @@ -587,7 +706,7 @@ mod tests { assert_eq!(dec.channels(), 3); assert_eq!(dec.decode(&frame).unwrap().channels.len(), 3); // no program config element where one is needed - let bare = aac::AdtsHeader::write(2, 3, 0, 1, 0x7FF); + let bare = aac::AdtsHeader::write(2, 3, 0, 1, 0x7FF).unwrap(); let bare = [&bare[..], &[0xE0]].concat(); assert!(matches!(TrackDecoder::new(&aac_params(), &[&bare]), Err(AudioError::Malformed(_)))); } diff --git a/crates/audio/src/write/flac/oracle_tests.rs b/crates/audio/src/write/flac/oracle_tests.rs index a9f80ece..aaf2322e 100644 --- a/crates/audio/src/write/flac/oracle_tests.rs +++ b/crates/audio/src/write/flac/oracle_tests.rs @@ -357,6 +357,58 @@ fn ogg_flac_before_opus_is_the_first_track() { } } +/// The first page of a Speex stream, which symphonia has no reader for. +#[cfg(feature = "symphonia")] +fn speex_ogg_page(serial: u32) -> Vec { + let mut packet = b"Speex ".to_vec(); + packet.resize(80, 0); + let mut page = b"OggS\0\x02".to_vec(); // the start of a stream + page.extend_from_slice(&[0; 8]); // granule position + page.extend_from_slice(&serial.to_le_bytes()); + page.extend_from_slice(&[0; 8]); // page number and checksum + page.extend_from_slice(&[1, packet.len() as u8]); + page.extend_from_slice(&packet); + let crc = craft_media::ogg::crc32(&page); + page[22..26].copy_from_slice(&crc.to_le_bytes()); + page +} + +/// A stream symphonia has no track for does not stop the default decode from reading the next one, +/// and it keeps its index. +#[cfg(feature = "symphonia")] +#[test] +fn ogg_flac_after_a_stream_with_no_reader_is_the_default_track() { + use crate::{CodecId, DecodeOptions}; + let Some(ff) = ffmpeg() else { return }; + let args = ["-c:a", "flac"]; + for reversed in [false, true] { + let Some(flac) = two_ogg_streams(&ff, "ogg-speex-flac", &args, reversed) else { return }; + for speex_serial in [0, 9] { + let file = [speex_ogg_page(speex_serial), flac.clone()].concat(); + let codecs: Vec<_> = crate::tracks(&file, None).unwrap().iter().map(|t| t.codec).collect(); + assert_eq!(codecs, [CodecId::Speex, CodecId::Flac, CodecId::Flac]); + let info = crate::decode(&file, None).unwrap().0; + assert_eq!((info.codec, info.sample_rate), (CodecId::Flac, 48_000), "reversed {reversed}, speex {speex_serial}"); + assert!(crate::decode_with(&file, None, &DecodeOptions::default().with_track(0)).is_err()); + let second = crate::decode_with(&file, None, &DecodeOptions::default().with_track(2)).unwrap().0; + assert_eq!(second.sample_rate, 44_100, "reversed {reversed}, speex {speex_serial}"); + } + } +} + +/// Bytes before the first page do not lose the file's track order or mask comments. +#[cfg(feature = "symphonia")] +#[test] +fn ogg_flac_after_leading_bytes_keeps_its_track_order() { + use crate::{ChannelLayout, DecodeOptions}; + let Some(ff) = ffmpeg() else { return }; + let args = ["-c:a", "flac", "-metadata:s:a:0", "WAVEFORMATEXTENSIBLE_CHANNEL_MASK=0x0107"]; + let Some(bytes) = two_ogg_streams(&ff, "ogg-leading-bytes", &args, true) else { return }; + let file = [vec![0; 64], bytes].concat(); + let info = crate::decode_with(&file, None, &DecodeOptions::default().with_track(0)).unwrap().0; + assert_eq!((info.sample_rate, info.layout), (48_000, ChannelLayout::from_mask(0x107))); +} + #[test] fn the_mask_comment_is_written_only_when_decoders_would_assume_otherwise() { use super::mask_comment; diff --git a/crates/audio/tests/codecs/aac/decoder_oracle.rs b/crates/audio/tests/codecs/aac/decoder_oracle.rs index e20e2835..3a4eb94b 100644 --- a/crates/audio/tests/codecs/aac/decoder_oracle.rs +++ b/crates/audio/tests/codecs/aac/decoder_oracle.rs @@ -132,7 +132,7 @@ fn pns_matches_ffmpeg() { } bw.write_bits(7, 3); let au = bw.finish(); - stream.extend_from_slice(&AdtsHeader::write(2, 4, channels as u8, au.len(), 0x7FF)); + stream.extend_from_slice(&AdtsHeader::write(2, 4, channels as u8, au.len(), 0x7FF).unwrap()); stream.extend_from_slice(&au); } let path = fixtures().join(format!("handmade_pns_{channels}.aac")); diff --git a/crates/audio/tests/codecs/routing_oracle.rs b/crates/audio/tests/codecs/routing_oracle.rs index 8c4482ac..484a9c48 100644 --- a/crates/audio/tests/codecs/routing_oracle.rs +++ b/crates/audio/tests/codecs/routing_oracle.rs @@ -157,6 +157,14 @@ fn reference(ff: &Path, file: &Path) -> Vec { out.stdout.as_chunks::<4>().0.iter().map(|c| f32::from_le_bytes(*c)).collect() } +/// The major version of this ffmpeg, or `None` for one that does not say (a git build). +fn ffmpeg_major(ff: &Path) -> Option { + let out = Command::new(ff).arg("-version").output().ok()?; + let text = String::from_utf8_lossy(&out.stdout); + let version = text.strip_prefix("ffmpeg version ")?.trim_start_matches('n'); + version.split(|c: char| !c.is_ascii_digit()).next()?.parse().ok() +} + /// How many frames of priming ffmpeg before 9 decodes as audio at the start of `name`, which /// `decode` drops; ffmpeg 9 drops them too. fn reference_lead(name: &str) -> usize { @@ -186,6 +194,7 @@ fn snr(ours: &[Vec], reference: &[f32]) -> f64 { #[test] fn every_container_and_codec_decodes_home_grown() { let ff = require_ffmpeg!(); + let major = ffmpeg_major(&ff); for c in CASES { let Some(path) = fixture(&ff, c) else { eprintln!("SKIPPED {}: this ffmpeg cannot write it", c.name); @@ -198,8 +207,13 @@ fn every_container_and_codec_decodes_home_grown() { assert_eq!(info.layout, c.layout, "{}: layout", c.name); let expected = c.frames.unwrap_or(reference.len() / c.channels); assert_eq!(buf.frames(), expected, "{}: presented frames", c.name); - let mut s = snr(&buf.channels, &reference); - s = s.max(snr(&buf.channels, &reference[reference_lead(c.name) * c.channels..])); + // One alignment, so audio that keeps the priming, or loses more than it, cannot pass. + let lead = reference_lead(c.name) * c.channels; + let s = match major { + Some(9..) => snr(&buf.channels, &reference), + Some(_) => snr(&buf.channels, &reference[lead..]), + None => snr(&buf.channels, &reference).max(snr(&buf.channels, &reference[lead..])), + }; assert!(s >= c.min_snr, "{}: SNR {s:.1} dB < {}", c.name, c.min_snr); let p = probe(&bytes, None).unwrap(); assert_eq!(