craft-audio: review round 2 for the coded formats

- craft-media: Mp4Writer counts chunk offsets from the first byte of the
  file, not from position 0 of the writer, so a file begun mid-writer is
  valid. This replaces the seek adapter the M4A writer wrapped its sink in.
- Tests pin the bytes of the roll sample group and each format's feature.
- Docs: ALAC's length comes from the sample durations, not an edit list;
  which readers take an empty AAC file; speaker names as the errors print
  them; craft-media's README lists roll_distance.
- Error messages for a layout a format has not no longer suggest setting
  a layout that was set.

Refs #1230

Cargo.toml

@@ -85,10 +85,10 @@ craft-keymap = { path = "crates/keymap" }
 craft-assuan = { path = "crates/assuan" }
 craft-kra = { path = "crates/kra" }
 craft-audio = { path = "crates/audio" }
-# No default features: craft-audio turns on the craft-media modules each of its features needs.
 craft-latex = { path = "crates/latex" }
 craft-markdown = { path = "crates/markdown" }
 craft-math = { path = "crates/math" }
+# No default features: craft-audio turns on the craft-media modules each of its features needs.
 craft-media = { path = "crates/media", default-features = false }
 craft-materialx = { path = "crates/materialx" }
 craft-media-testkit = { path = "crates/media-testkit" }

crates/audio/README.md

@@ -241,8 +241,9 @@ let alac = encode(&buf, &EncodeOptions::new(EncodeFormat::Alac).with_bit_depth(B
 `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
 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 writes nothing. After a write fails for another reason, every later
+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
+writes nothing. After a write fails for another reason, every later
 `write` and `finish` returns an error.
 
 | `EncodeFormat` | File | Audio | Options used |
@@ -265,7 +266,7 @@ count when unset) names the speakers, and the channels are reordered into the co
 Each coded format has a fixed set of layouts, and audio with other speakers is `Unsupported`
 rather than written to the wrong ones:
 
-- AAC: C; L R; L R C; L R C Cs; L R C Ls Rs; 5.1; 6.1; and for 8 channels L R C LFE Ls Rs Lc Rc
+- AAC: C; L R; L R C; L R C Cb; L R C Ls Rs; 5.1; 6.1; and for 8 channels L R C LFE Lb Rb Lc Rc
   (a front pair left and right of centre, mask `0xFF`). ALAC: the same, with 6.1's back centre last.
   WAV 7.1 (backs and sides) is not that 7.1, and quad (the usual 4-channel layout) has no centre:
   both need another format.
@@ -465,8 +466,9 @@ match craft_audio::decode(&bytes, None) {
 - `EncodeFormat::Aac` with 8 channels is AAC channel configuration 7 (C, Lc Rc, L R, Ls Rs, LFE),
   which Apple's decoders read as written. ffmpeg assumes such a stream is a mislabelled 7.1 with
   backs and sides and moves the channels accordingly, unless run with `-strict 1`.
-- A file with no audio: MP3, AC-3 and E-AC-3 are then zero bytes, which nothing can probe, and an
-  empty AAC M4A (a track with no samples) is read by ffmpeg but not by this crate's decoder.
+- A file with no audio: MP3, AC-3 and E-AC-3 are then zero bytes, which nothing can probe. An
+  empty AAC M4A (a track with no samples) is read by ffmpeg; this crate's native reader passes it
+  on, so `decode-native` alone refuses it and symphonia reads only some as zero frames.
 - ALAC 32-bit streams: ffmpeg does not decode their uncompressed (escaped) stereo frames, which
   Apple's encoder and this one write for audio that does not compress; Apple's decoder and this
   one do.

crates/audio/src/write/coded.rs

@@ -7,13 +7,13 @@
 // other halves of the shared code are unused.
 #![cfg_attr(
     not(all(feature = "aac-encode", feature = "alac-encode", feature = "mp3-encode", feature = "ac3-encode", feature = "eac3-encode")),
-    allow(dead_code, unused_imports, unused_variables, unused_mut)
+    allow(dead_code, unused_imports, unused_variables)
 )]
 
 use super::failed_write_error;
 use super::pcm::validate_layout;
 use crate::{AudioBuffer, AudioError, ChannelLayout, EncodeFormat, EncodeOptions, RateControl, Result, Speaker};
-use std::io::{Cursor, Seek, SeekFrom, Write};
+use std::io::{Cursor, Seek, Write};
 
 /// Frames handed to the encoder at a time, so its buffers and the packets waiting to be written
 /// stay small however long a block is. Also the frames of an ALAC packet.
@@ -70,14 +70,15 @@ fn fixed_map(name: &str, mask: u32, order: Option<&[usize]>, layout: &ChannelLay
         return Err(AudioError::Unsupported(format!("{name} cannot hold {} channels", layout.channels())));
     }
     channel_map(mask, order, layout).ok_or_else(|| {
-        let given = match layout.mask() {
-            Some(m) => labels(m),
-            None => "no speaker assignment".into(),
-        };
-        AudioError::Unsupported(format!("{name} holds {} channels as {}, not {given}; set `EncodeOptions::layout`", layout.channels(), labels(mask)))
+        AudioError::Unsupported(format!("{name} holds {} channels as {}; the audio is {}", layout.channels(), labels(mask), given(layout)))
     })
 }
 
+/// The speakers of the caller's layout, for an error message.
+fn given(layout: &ChannelLayout) -> String {
+    layout.mask().map_or_else(|| "without speaker assignment".into(), labels)
+}
+
 /// The WAV mask of the speakers an AAC stream of `channels` channels has, as this crate's encoder
 /// writes it and its decoder reports it: channel configurations 1–6, the 6.1 program config
 /// element for 7, and configuration 7 (7.1 with a front pair left and right of centre) for 8.
@@ -153,11 +154,7 @@ fn ac3_config(
         Some((config, map))
     });
     let Some((mut config, map)) = found else {
-        let given = match layout.mask() {
-            Some(m) => labels(m),
-            None => "no speaker assignment".into(),
-        };
-        return Err(AudioError::Unsupported(format!("{name} has no layout of {} channels as {given}", layout.channels())));
+        return Err(AudioError::Unsupported(format!("{name} has no coding mode for {} channels that are {}", layout.channels(), given(layout))));
     };
     let full = config.mode.channels() as u32;
     let want = constant_kbps(name, rate, DEFAULT_KBPS_PER_CHANNEL * full)?;
@@ -329,44 +326,6 @@ fn mux_error(e: craft_media::isobmff::Error) -> AudioError {
     }
 }
 
-/// A sink seen from where the file starts: position 0 is the sink's position when the file was
-/// begun. The MP4 muxer writes chunk offsets as stream positions, and they count from the first
-/// byte of the file, which need not be the first byte of the sink.
-#[cfg(any(feature = "aac-encode", feature = "alac-encode"))]
-struct FromStart<W> {
-    out: W,
-    start: u64,
-}
-
-#[cfg(any(feature = "aac-encode", feature = "alac-encode"))]
-impl<W: Write> Write for FromStart<W> {
-    fn write(&mut self, data: &[u8]) -> std::io::Result<usize> {
-        self.out.write(data)
-    }
-
-    fn write_all(&mut self, data: &[u8]) -> std::io::Result<()> {
-        self.out.write_all(data)
-    }
-
-    fn flush(&mut self) -> std::io::Result<()> {
-        self.out.flush()
-    }
-}
-
-#[cfg(any(feature = "aac-encode", feature = "alac-encode"))]
-impl<W: Seek> Seek for FromStart<W> {
-    fn seek(&mut self, to: SeekFrom) -> std::io::Result<u64> {
-        let to = match to {
-            SeekFrom::Start(at) => SeekFrom::Start(
-                self.start.checked_add(at).ok_or_else(|| std::io::Error::new(std::io::ErrorKind::InvalidInput, "seek past the end of the sink"))?,
-            ),
-            relative => relative,
-        };
-        let at = self.out.seek(to)?;
-        at.checked_sub(self.start).ok_or_else(|| std::io::Error::new(std::io::ErrorKind::InvalidInput, "seek to before the start of the file"))
-    }
-}
-
 /// An M4A file being written: one audio track whose samples are the packets.
 ///
 /// The last packet is held back until the length is known: its duration is cut to the audio's
@@ -375,7 +334,7 @@ impl<W: Seek> Seek for FromStart<W> {
 /// keeps that edit exact.
 #[cfg(any(feature = "aac-encode", feature = "alac-encode"))]
 struct M4a<W: Write + Seek> {
-    mux: craft_media::isobmff::Mp4Writer<FromStart<W>>,
+    mux: craft_media::isobmff::Mp4Writer<W>,
     /// The track, added with the first packet: a file with no audio gets a track without an edit.
     track: Option<usize>,
     entry: craft_media::isobmff::SampleEntry,
@@ -390,10 +349,8 @@ struct M4a<W: Write + Seek> {
 
 #[cfg(any(feature = "aac-encode", feature = "alac-encode"))]
 impl<W: Write + Seek> M4a<W> {
-    fn new(mut out: W, codec: &Codec, sample_rate: u32, channels: u32) -> Result<Self> {
+    fn new(out: W, codec: &Codec, sample_rate: u32, channels: u32) -> Result<Self> {
         use craft_media::isobmff::{Brand, Mp4Writer, WriterOptions};
-        let start = out.stream_position()?;
-        let out = FromStart { out, start };
         let entry = codec.sample_entry(sample_rate, channels, 0, 0).ok_or_else(|| AudioError::Unsupported("M4A holds AAC and ALAC".into()))?;
         let mut options = WriterOptions::new(Brand::Mp4);
         options.movie_timescale = sample_rate;
@@ -444,7 +401,7 @@ impl<W: Write + Seek> M4a<W> {
         if let Some(entry) = codec.sample_entry(self.sample_rate, self.channels, self.bytes, self.frames) {
             self.mux.set_sample_entry(track, entry).map_err(mux_error)?;
         }
-        Ok(self.mux.finish().map_err(mux_error)?.out)
+        self.mux.finish().map_err(mux_error)
     }
 }
 
@@ -495,7 +452,7 @@ impl<W: Write + Seek> Writer<W> {
                 };
                 let enc = crate::alac::Encoder::new(crate::alac::EncoderConfig::new(sample_rate, count, bits).with_frame_length(CHUNK as u32))?;
                 let quantizers = super::pcm::Quantizer::per_channel(u32::from(bits), opts.dither, channels);
-                (Codec::Alac { enc: Box::new(enc), quantizers, pending: vec![Vec::with_capacity(CHUNK); channels] }, map)
+                (Codec::Alac { enc: Box::new(enc), quantizers, pending: vec![Vec::new(); channels] }, map)
             }
             #[cfg(feature = "mp3-encode")]
             EncodeFormat::Mp3 => {
@@ -514,7 +471,7 @@ impl<W: Write + Seek> Writer<W> {
             EncodeFormat::Ac3 | EncodeFormat::Eac3 => {
                 let codec = if opts.format == EncodeFormat::Ac3 { crate::ac3::encoder::Codec::Ac3 } else { crate::ac3::encoder::Codec::Eac3 };
                 let (config, map) = ac3_config(codec, sample_rate, &layout, opts.rate_control)?;
-                (Codec::Ac3 { enc: Box::new(crate::ac3::encoder::Encoder::new(config)?), planes: vec![Vec::with_capacity(CHUNK); channels] }, map)
+                (Codec::Ac3 { enc: Box::new(crate::ac3::encoder::Encoder::new(config)?), planes: vec![Vec::new(); channels] }, map)
             }
             #[allow(unreachable_patterns)]
             f => return Err(AudioError::Unsupported(format!("writing {f:?}"))),

crates/audio/src/write/mod.rs

@@ -22,9 +22,10 @@ use std::io::{Seek, Write};
 /// Encode `buf` as the file `opts.format` names. Integer depths clamp samples to [-1, 1]; float
 /// keeps values above full scale. `opts.layout`, when set, must have `buf`'s channel count.
 ///
-/// The lossy formats take `opts.rate_control`. AAC and ALAC are written as M4A, whose edit list
-/// makes the file decode to exactly `buf`'s frames. MP3, AC-3 and E-AC-3 are raw streams, which
-/// have no place for that: they decode with the codec's delay in front and padded to whole frames.
+/// The lossy formats take `opts.rate_control`. AAC and ALAC are written as M4A, which records the
+/// encoder's priming and the exact length, so the file decodes to `buf`'s frames. MP3, AC-3 and
+/// E-AC-3 are raw streams, which have no place for that: they decode with the codec's delay in
+/// front and padded to whole frames.
 ///
 /// ```
 /// # #[cfg(feature = "pcm-encode")] {
@@ -142,9 +143,10 @@ fn failed_write_error() -> AudioError {
 }
 
 /// Writes a file of any [`EncodeFormat`] block by block, producing the same bytes [`encode`]
-/// would for the whole buffer: a file of any length never has to fit in memory. The file starts at the sink's position, and nothing may follow that
-/// position (a new file, or one truncated there): the encoder cannot truncate a `Write + Seek`,
-/// so the bytes of a longer file already there would stay after the new one.
+/// would for the whole buffer: a file of any length never has to fit in memory. The file starts
+/// at the sink's position, and nothing may follow that position (a new file, or one truncated
+/// there): the encoder cannot truncate a `Write + Seek`, so the bytes of a longer file already
+/// there would stay after the new one.
 ///
 /// ```
 /// # #[cfg(feature = "flac-encode")] {
@@ -316,6 +318,7 @@ mod tests {
         let enc = FileEncoder::new(std::io::Cursor::new(Vec::new()), &opts, 48_000, 1);
         assert!(matches!(enc, Err(AudioError::Unsupported(_))));
         assert_eq!(encode_format_for(name), None);
+        assert!(!format.is_built() && EncodeFormat::ALL.iter().any(|f| f.is_built()));
         // Also with a layout it could not use.
         let opts = opts.with_layout(ChannelLayout::STEREO);
         assert!(matches!(encode(&buf, &opts), Err(AudioError::Unsupported(_))));

crates/audio/tests/codecs/file_formats.rs

@@ -391,8 +391,10 @@ fn the_formats_are_named() {
     }
     assert_eq!(ENCODE_EXTENSIONS.len(), 12);
     for format in CODED {
-        assert!(EncodeFormat::ALL.contains(&format) && format.is_built() && format.feature().ends_with("-encode"));
+        assert!(EncodeFormat::ALL.contains(&format) && format.is_built());
     }
+    let features: Vec<_> = EncodeFormat::ALL.iter().map(|f| f.feature()).collect();
+    assert_eq!(features, ["pcm-encode", "pcm-encode", "flac-encode", "alac-encode", "aac-encode", "mp3-encode", "ac3-encode", "eac3-encode"]);
 }
 
 /// ffmpeg's decode of `file` as interleaved `f32`, split into `channels` channels.

crates/media/README.md

@@ -243,6 +243,10 @@ let bytes = w.finish_faststart()?.into_inner(); // or finish() for moov-at-end
   `ipcm`/`fpcm` + `pcmC` in MP4. A PCM `write_sample` call can hold any whole number of frames, and the
   track timescale must equal the sample rate.
 - Edit lists: set `TrackConfig::edits` explicitly, or use `media_start` to get a single edit.
+- Audio pre-roll: `TrackConfig::roll_distance` puts every sample in a `roll` sample group (`sgpd` +
+  `sbgp`). Set -1 for AAC with `media_start` at the priming: Apple's decoders apply that edit only
+  when the track has the group.
+- A file starts at the writer's position: chunk offsets count from there.
 - Timecode: `add_timecode_track(video, TimecodeConfig, start_frame)` adds a `tmcd` track (with `gmhd`)
   and a `tref/tmcd` link from the video track.
 - `WriterOptions::metadata` writes `©xxx` text atoms to `udta`.

crates/media/src/isobmff/mux.rs

@@ -621,6 +621,9 @@ pub struct Mp4Writer<W: Write + Seek> {
     w: W,
     opts: WriterOptions,
     tracks: Vec<WTrack>,
+    /// The writer's position when the file was begun: chunk offsets count from here, the first
+    /// byte of the file, wherever in the writer that is.
+    start: u64,
     /// Offset of the placeholder box (8 bytes) preceding the mdat header.
     pad_pos: u64,
     mdat_pos: u64,
@@ -631,7 +634,8 @@ pub struct Mp4Writer<W: Write + Seek> {
 }
 
 impl<W: Write + Seek> Mp4Writer<W> {
-    /// Start a file: writes `ftyp`, a placeholder box and the `mdat` header.
+    /// Start a file at the writer's current position: writes `ftyp`, a placeholder box and the
+    /// `mdat` header.
     pub fn new(mut w: W, opts: WriterOptions) -> Result<Self> {
         let start = w.stream_position()?;
         let mut b = BoxBuf::new();
@@ -642,7 +646,7 @@ impl<W: Write + Seek> Mp4Writer<W> {
         b.leaf(b"mdat", &[]);
         w.write_all(&b.buf)?;
         let pos = start + b.len() as u64;
-        Ok(Mp4Writer { w, opts, tracks: Vec::new(), pad_pos, mdat_pos, pos, last_track: None, chapters: Vec::new() })
+        Ok(Mp4Writer { w, opts, tracks: Vec::new(), start, pad_pos, mdat_pos, pos, last_track: None, chapters: Vec::new() })
     }
 
     /// Add a track; returns its index (track ids are index + 1).
@@ -731,7 +735,7 @@ impl<W: Write + Seek> Mp4Writer<W> {
             return Err(too_many_samples());
         }
         self.w.write_all(s.data)?;
-        let offset = self.pos;
+        let offset = self.pos - self.start;
         self.pos += s.data.len() as u64;
         let contiguous = self.last_track == Some(track);
         self.last_track = Some(track);
@@ -772,7 +776,7 @@ impl<W: Write + Seek> Mp4Writer<W> {
             };
             let d = rescale(vd, vts, tts).max(tc_dur as u64).min(u32::MAX as u64) as u32;
             self.w.write_all(&start.to_be_bytes())?;
-            let off = self.pos;
+            let off = self.pos - self.start;
             self.pos += 4;
             self.last_track = None;
             let t = &mut self.tracks[i].t;
@@ -830,7 +834,7 @@ impl<W: Write + Seek> Mp4Writer<W> {
         }
         let id = self.tracks.len() as u32 + 1;
         let mut t = Tables::new();
-        let off = self.pos;
+        let off = self.pos - self.start;
         for (i, c) in self.chapters.iter().enumerate() {
             let end = self.chapters.get(i + 1).map_or(duration, |n| n.start);
             let d = u32::try_from(end.saturating_sub(c.start)).map_err(|_| Error::Mux("chapter longer than 2^32 movie ticks".into()))?;
@@ -1389,5 +1393,44 @@ mod tests {
         for out in [file(None, 5), file(Some(-1), 0)] {
             assert!(crate::isobmff::open(out.as_slice()).unwrap().tracks[0].sample_groups.is_empty());
         }
+        // The boxes as ISO/IEC 14496-12 §8.9 has them (and ffmpeg writes them): `sgpd` version 1
+        // with a default description length of 2, one description; `sbgp` version 0, one run.
+        let sgpd = [&26u32.to_be_bytes()[..], b"sgpd", &[1, 0, 0, 0], b"roll", &[0, 0, 0, 2, 0, 0, 0, 1, 0xFF, 0xFF]].concat();
+        let sbgp = [&28u32.to_be_bytes()[..], b"sbgp", &[0, 0, 0, 0], b"roll", &[0, 0, 0, 1, 0, 0, 0, 5, 0, 0, 0, 1]].concat();
+        let both = [sgpd, sbgp].concat();
+        assert!(out.windows(both.len()).any(|w| w == both));
+    }
+
+    /// 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]
+    fn a_file_begun_mid_writer_has_offsets_from_its_start() {
+        let file = |prefix: usize, faststart: bool| {
+            let mut sink = Cursor::new(vec![0xAA; prefix]);
+            sink.set_position(prefix as u64);
+            let mut opts = WriterOptions::new(Brand::Mov);
+            opts.chapters = vec![Chapter { start: 0, title: "one".into() }];
+            let mut w = Mp4Writer::new(sink, opts).unwrap();
+            let v = w.add_track(TrackConfig::new(avc_entry(), 1000)).unwrap();
+            let tc = TimecodeConfig { timescale: 1000, frame_duration: 40, frames_per_second: 25, ..Default::default() };
+            w.add_timecode_track(v, tc, 7).unwrap();
+            for i in 0..4u8 {
+                w.write_sample(v, WriteSample { data: &[i + 1; 10], duration: 40, composition_offset: 0, is_sync: true }).unwrap();
+            }
+            let out = if faststart { w.finish_faststart() } else { w.finish() }.unwrap();
+            assert_eq!(out.position(), out.get_ref().len() as u64);
+            out.into_inner()
+        };
+        for faststart in [false, true] {
+            let (plain, offset) = (file(0, faststart), file(100, faststart));
+            assert!(offset[..100].iter().all(|&b| b == 0xAA));
+            assert!(offset[100..] == plain, "faststart {faststart}");
+            let f = crate::isobmff::open(plain.as_slice()).unwrap();
+            assert_eq!(f.tracks.len(), 3);
+            for i in 0..4 {
+                assert_eq!(f.read_sample(plain.as_slice(), 0, i).unwrap(), vec![i as u8 + 1; 10]);
+            }
+            assert_eq!(f.read_sample(plain.as_slice(), 1, 0).unwrap(), 7u32.to_be_bytes());
+        }
     }
 }