From 098eb2e43a0e8c1fb14b7b668bf5fe88da8641c5 Mon Sep 17 00:00:00 2001 From: alehel Date: Sat, 14 Nov 2026 03:02:37 +0100 Subject: [PATCH] craft-audio: review round 3 for the coded formats - craft-media: Mp4Writer::new refuses a writer positioned where the header would end past u64::MAX instead of overflowing the sum (correction to round 2, which let the M4A writer reach that addition). - AAC is written at 6 kbit/s or a quarter of the sample rate per channel at least: the encoder accepts lower rates but codes only silence at them, so with_bitrate(128) gave a silent file. - README: what to do with a layout a coded format refuses, and the names encode_format_for takes for M4A. Refs #1230 --- crates/audio/README.md | 12 ++++++++++-- crates/audio/src/lib.rs | 3 ++- crates/audio/src/write/coded.rs | 9 +++++++++ crates/audio/tests/codecs/file_formats.rs | 10 +++++++++- crates/media/src/isobmff/mux.rs | 20 +++++++++++++++++--- 5 files changed, 47 insertions(+), 7 deletions(-) diff --git a/crates/audio/README.md b/crates/audio/README.md index 3acb44de7..29cc4816f 100644 --- a/crates/audio/README.md +++ b/crates/audio/README.md @@ -239,7 +239,7 @@ let alac = encode(&buf, &EncodeOptions::new(EncodeFormat::Alac).with_bit_depth(B `EncodeOptions::default()` is 24-bit WAV. `with_bwf` adds a BWF `bext` chunk and `with_force_rf64` writes RF64 below 4 GiB; both apply to WAV only. `encode_format_for("aiff")` -maps an extension to an `EncodeFormat`, and `ENCODE_EXTENSIONS` lists the extensions the enabled +maps an extension to an `EncodeFormat` (`"m4a"` to AAC; `"alac"` names ALAC), and `ENCODE_EXTENSIONS` lists the extensions the enabled features can write. `EncodeFormat::ALL` lists the formats and `is_built()` says whether this build writes one; `extension()` is the extension a new file gets and `mime_type()` its media type. `write` refuses a block without one slice per channel, or with a slice shorter than `frames`, and @@ -256,7 +256,8 @@ writes nothing. After a write fails for another reason, every later `with_bitrate(bits_per_second)` sets a constant bit rate for all channels together. A rate the stream cannot carry is moved to the nearest one it can: MP3 and AC-3 use the highest rate of their -tables not above it, AAC and E-AC-3 keep it inside their range. `with_quality(0.0..=1.0)` is AAC's +tables not above it, AAC and E-AC-3 keep it inside their range (AAC from 6 kbit/s or a quarter of +the sample rate per channel, whichever is more: below that the encoder has bits for silence only). `with_quality(0.0..=1.0)` is AAC's constant quality (VBR); the other lossy formats return `Unsupported` for it. Without either, AAC is written at 64 kbit/s per channel, MP3 at 96 kbit/s per channel, and AC-3 and E-AC-3 at 96 kbit/s per full-bandwidth channel (the LFE not counted). @@ -275,6 +276,13 @@ rather than written to the wrong ones: - A surround pair is a surround pair: side and back surrounds are interchangeable where the format has one pair. +The layout `decode` reports (`AudioInfo::layout`) can be passed to `with_layout` as it is. When a +coded format refuses it (2.1 or quad into AAC, a single channel tagged as left, channels with no +speaker assignment), the choices are another format, or audio rearranged into one of the layouts +above with that layout set; leaving `layout` unset declares the usual layout for the count. This +crate's decoder returns 7- and 8-channel AAC without a speaker assignment, in AAC order, so such a +file needs that step before it is written again. + An M4A's edit list skips the AAC encoder's priming (1024 frames) and ends at the last frame written, so the file decodes to the length that went in. An MP3, AC-3 or E-AC-3 stream has no place for that (see [Limits](#limits)). For a container of your own, use the codec modules below. diff --git a/crates/audio/src/lib.rs b/crates/audio/src/lib.rs index 326426b2a..52d630a84 100644 --- a/crates/audio/src/lib.rs +++ b/crates/audio/src/lib.rs @@ -366,7 +366,8 @@ impl EncodeFormat { pub enum RateControl { /// A constant bit rate in bits per second, for all channels together. A format with a fixed /// set of rates (MP3, AC-3) uses the highest one not above it; a rate outside what the - /// stream can carry is moved to the nearest one inside. + /// stream can carry is moved to the nearest one inside. AAC is written at 6 kbit/s or a + /// quarter of the sample rate per channel at least, where its encoder starts to code audio. Bitrate(u32), /// A constant quality from 0.0 (smallest) to 1.0 (best), at whatever bit rate each frame /// needs. AAC only; values outside the range are clamped, and NaN is an error. diff --git a/crates/audio/src/write/coded.rs b/crates/audio/src/write/coded.rs index 8999171a2..c173b502e 100644 --- a/crates/audio/src/write/coded.rs +++ b/crates/audio/src/write/coded.rs @@ -104,12 +104,21 @@ fn aac_order(channels: usize) -> Option<&'static [usize]> { if channels == 8 { Some(&[2, 6, 7, 0, 1, 4, 5, 3]) } else { crate::aac::aac_order(channels) } } +/// The lowest AAC bit rate the writer uses, per channel: 6 kbit/s, and a quarter of the sample +/// rate (256 bits a frame). The encoder accepts less, but then has too few bits for anything but +/// silence, and a file of silence is no answer to a low bit rate (or to kbit/s given as bit/s). +#[cfg(feature = "aac-encode")] +fn aac_floor_per_channel(sample_rate: u32) -> u32 { + (sample_rate / 4).max(6000) +} + /// The AAC rate control for `rate`: a bit rate is kept within what the stream can carry, a /// quality of 0–1 spans the encoder's VBR range. #[cfg(feature = "aac-encode")] fn aac_mode(rate: Option, sample_rate: u32, channels: usize) -> crate::aac::BitrateMode { use crate::aac::BitrateMode; let (lo, hi) = crate::aac::cbr_range(sample_rate, channels); + let lo = lo.max(aac_floor_per_channel(sample_rate).saturating_mul(channels as u32)).min(hi.max(lo)); // An empty range (a rate or channel count the encoder refuses anyway) leaves the rate alone. let fit = |bps: u32| if lo <= hi { bps.clamp(lo, hi) } else { bps }; match rate { diff --git a/crates/audio/tests/codecs/file_formats.rs b/crates/audio/tests/codecs/file_formats.rs index 83679cbd5..46ed901ff 100644 --- a/crates/audio/tests/codecs/file_formats.rs +++ b/crates/audio/tests/codecs/file_formats.rs @@ -268,8 +268,16 @@ fn the_bit_rate_is_followed() { ); assert!((7000..10_000).contains(&low) && (15_000..18_500).contains(&default) && (30_000..35_000).contains(&high), "{low} {default} {high}"); // A rate no stream can carry is moved into range, not refused. - encode(&buf, &EncodeOptions::new(EncodeFormat::Aac).with_bitrate(1)).unwrap(); encode(&buf, &EncodeOptions::new(EncodeFormat::Aac).with_bitrate(u32::MAX)).unwrap(); + // The lowest rate still codes the audio: the encoder accepts rates that leave only silence + // (128 for 128 kbit/s is one), and the writer does not go that low. + for (sample_rate, channels) in [(8000, 1), (16_000, 2), (44_100, 2), (48_000, 6), (96_000, 2)] { + let buf = tones(sample_rate, channels, sample_rate as usize); + let (_, out) = decode(&encode(&buf, &EncodeOptions::new(EncodeFormat::Aac).with_bitrate(128)).unwrap(), Some("m4a")).unwrap(); + let rms = |x: &[f32]| (x.iter().map(|&s| f64::from(s).powi(2)).sum::() / x.len() as f64).sqrt(); + let level = rms(&out.channels[0]) / rms(&buf.channels[0]); + assert!(level > 0.5, "{sample_rate} Hz, {channels} channels: {level} of the level"); + } // Quality is AAC's; ALAC has no rate to control. let (small, large) = (size(&EncodeOptions::new(EncodeFormat::Aac).with_quality(0.0)), size(&EncodeOptions::new(EncodeFormat::Aac).with_quality(1.0))); diff --git a/crates/media/src/isobmff/mux.rs b/crates/media/src/isobmff/mux.rs index b37fbf724..159a9b5ba 100644 --- a/crates/media/src/isobmff/mux.rs +++ b/crates/media/src/isobmff/mux.rs @@ -640,12 +640,15 @@ impl Mp4Writer { let start = w.stream_position()?; let mut b = BoxBuf::new(); write_ftyp(&mut b, opts.brand); - let pad_pos = start + b.len() as u64; + let pad = b.len() as u64; b.leaf(if opts.brand == Brand::Mov { b"wide" } else { b"free" }, &[]); - let mdat_pos = start + b.len() as u64; + let mdat = b.len() as u64; b.leaf(b"mdat", &[]); + // A writer positioned at the very end of what a position can count has no room for the + // header; the positions below are sums that would overflow. + let pos = start.checked_add(b.len() as u64).ok_or_else(|| Error::Mux("the writer's position leaves no room for a file".into()))?; + let (pad_pos, mdat_pos) = (start + pad, start + mdat); w.write_all(&b.buf)?; - let pos = start + b.len() as u64; Ok(Mp4Writer { w, opts, tracks: Vec::new(), start, pad_pos, mdat_pos, pos, last_track: None, chapters: Vec::new() }) } @@ -1401,6 +1404,17 @@ mod tests { assert!(out.windows(both.len()).any(|w| w == both)); } + /// A writer positioned where the header's end would pass `u64::MAX` is refused, not overflowed. + #[test] + fn a_writer_at_the_end_of_the_position_range_is_refused() { + for back in [0, 1, 10, 27] { + let mut bytes = [0u8; 8]; + let mut sink = Cursor::new(&mut bytes[..]); + sink.set_position(u64::MAX - back); + assert!(matches!(Mp4Writer::new(sink, WriterOptions::new(Brand::Mp4)), Err(Error::Mux(_))), "{back} before the end"); + } + } + /// A file begun in the middle of a writer is a file from there: its chunk offsets count from /// its own first byte, with `moov` at the end or in front, a timecode track and chapters. #[test]