craft-audio: review fixes, round 12 (#442)

- FLAC files read their layout from the channel mask comment, or else
  the one decoders assume for the count, which is what the writer omits
  the comment for
- an unbuilt format is Unsupported before the layout is checked
- FLAC encode no longer reserves half the PCM size up front
- flac-encode's lib tests stream a file and compare it with encode
- the seek test compares samples after the seek

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

crates/audio/README.md

@@ -264,7 +264,7 @@ match craft_audio::decode(&bytes, None) {
   `WAVEFORMATEXTENSIBLE_CHANNEL_MASK` comment. This includes the usual layouts for 5 and 6 channels:
   without the comment, back surrounds are read as side surrounds. It includes the usual 7-channel
   layout too: without the comment, its back left, back right and back centre are read as back
-  centre, side left and side right.
+  centre, side left and side right. craft-audio reads the comment in FLAC files, not in FLAC in Ogg.
 - FLAC at 32 bits codes stereo channels independently, because a side channel would need 33 bits.
 - Outside RFC 9639's streamable subset (§7), which some decoders require: FLAC at 32 bits, at a
   sample rate the frame header cannot name, or with a layout that needs the mask comment.

crates/audio/src/layout.rs

@@ -263,6 +263,19 @@ impl std::fmt::Display for ChannelLayout {
     }
 }
 
+/// The layout decoders assume for 1–8 FLAC channels with no channel mask comment, as a WAV mask.
+/// RFC 9639 §9.1.3 leaves "back/surround" open for 5 and 6 channels; libFLAC and ffmpeg read side
+/// surrounds. 7 channels are 6.1 with sides (FL FR FC LFE BC SL SR).
+#[cfg(any(feature = "symphonia", feature = "flac-encode"))]
+pub(crate) fn flac_assumed_mask(channels: usize) -> u32 {
+    match channels {
+        5 => 0x607,
+        6 => 0x60F,
+        7 => 0x70F,
+        n => ChannelLayout::default_for(n as u16).mask().unwrap_or(0),
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

crates/audio/src/read/flac.rs

@@ -67,16 +67,66 @@ pub(crate) fn empty_stream_info(b: &[u8], at: usize) -> Option<AudioInfo> {
         17..=24 => SampleFormat::Int24,
         _ => SampleFormat::Int32,
     };
-    Some(AudioInfo::new(Container::Flac, CodecId::Flac, sample_format, sample_rate, flac_layout(usize::from(channels)), 0))
+    Some(AudioInfo::new(Container::Flac, CodecId::Flac, sample_format, sample_rate, layout_of(b, at, channels), 0))
 }
 
-/// The FLAC channel assignment for a channel count (RFC 9639 §9.1.3).
-pub(crate) fn flac_layout(channels: usize) -> ChannelLayout {
-    match channels {
-        7 => ChannelLayout::SURROUND_6_1,
-        1..=8 => ChannelLayout::default_for(crate::layout::count(channels)),
-        n => ChannelLayout::unordered(crate::layout::count(n)),
+/// The layout of the FLAC stream at `at`, from its STREAMINFO channel count and its channel mask
+/// comment, or `None` when it does not start with a STREAMINFO block.
+pub(crate) fn layout(b: &[u8], at: usize) -> Option<ChannelLayout> {
+    let head = be_u32(b, at.checked_add(4)?)?;
+    if (head >> 24) & 0x7f != 0 {
+        return None;
     }
+    // In STREAMINFO: 3 bits of channels - 1 after the 20 bits of rate.
+    let channels = u16::from((b.get(at.checked_add(4 + 4 + 10 + 2)?)? >> 1) & 0x7) + 1;
+    Some(layout_of(b, at, channels))
+}
+
+/// `channels` channels described by the channel mask comment of the stream at `at`, or else in
+/// the layout decoders assume for the count (RFC 9639 §8.6.2, §9.1.3).
+fn layout_of(b: &[u8], at: usize, channels: u16) -> ChannelLayout {
+    let mask = channel_mask(b, at).unwrap_or_else(|| crate::layout::flac_assumed_mask(usize::from(channels)));
+    ChannelLayout::from_mask_and_channels(mask, channels)
+}
+
+/// The value of the first `WAVEFORMATEXTENSIBLE_CHANNEL_MASK` comment that holds a `0x` hexadecimal
+/// number, in the stream at `at`.
+fn channel_mask(b: &[u8], at: usize) -> Option<u32> {
+    let mut pos = at.checked_add(4)?;
+    loop {
+        let head = be_u32(b, pos)?;
+        let (body, len) = (pos.checked_add(4)?, (head & 0x00ff_ffff) as usize);
+        let end = body.checked_add(len)?.min(b.len());
+        if (head >> 24) & 0x7f == 4 {
+            return comments_mask(b.get(body..end)?);
+        }
+        if head >> 31 == 1 {
+            return None;
+        }
+        pos = body.checked_add(len)?;
+    }
+}
+
+/// The channel mask in a Vorbis comment block's body.
+fn comments_mask(block: &[u8]) -> Option<u32> {
+    let vendor = le_u32(block, 0)? as usize;
+    let mut at = 4usize.checked_add(vendor)?;
+    let count = le_u32(block, at)?;
+    at += 4;
+    for _ in 0..count {
+        let len = le_u32(block, at)? as usize;
+        let comment = block.get(at + 4..(at + 4).checked_add(len)?)?;
+        at += 4 + len;
+        let Some(eq) = comment.iter().position(|&c| c == b'=') else { continue };
+        if !comment[..eq].eq_ignore_ascii_case(b"WAVEFORMATEXTENSIBLE_CHANNEL_MASK") {
+            continue;
+        }
+        let hex = comment[eq + 1..].strip_prefix(b"0x").or_else(|| comment[eq + 1..].strip_prefix(b"0X"));
+        if let Some(mask) = hex.and_then(|h| std::str::from_utf8(h).ok()).and_then(|h| u32::from_str_radix(h, 16).ok()) {
+            return Some(mask);
+        }
+    }
+    None
 }
 
 /// Whether a comment in a (little-endian) Vorbis comment block claims more than the block holds.
@@ -102,3 +152,42 @@ fn overruns(b: &[u8], mut at: usize, end: usize, read: fn(&[u8], usize) -> Optio
     }
     false
 }
+
+#[cfg(test)]
+mod tests {
+    use super::comments_mask;
+
+    fn block(comments: &[&str]) -> Vec<u8> {
+        let mut b = 3u32.to_le_bytes().to_vec();
+        b.extend_from_slice(b"abc");
+        b.extend_from_slice(&(comments.len() as u32).to_le_bytes());
+        for c in comments {
+            b.extend_from_slice(&(c.len() as u32).to_le_bytes());
+            b.extend_from_slice(c.as_bytes());
+        }
+        b
+    }
+
+    #[test]
+    fn the_channel_mask_comment_is_found_in_any_case() {
+        assert_eq!(comments_mask(&block(&["TITLE=x", "WAVEFORMATEXTENSIBLE_CHANNEL_MASK=0x0107"])), Some(0x107));
+        assert_eq!(comments_mask(&block(&["waveformatextensible_channel_mask=0X3f"])), Some(0x3F));
+        assert_eq!(comments_mask(&block(&["TITLE=x"])), None);
+    }
+
+    #[test]
+    fn a_bad_channel_mask_comment_is_ignored() {
+        for value in ["63", "0x", "0xZZ", "0x1FFFFFFFF"] {
+            assert_eq!(comments_mask(&block(&[&format!("WAVEFORMATEXTENSIBLE_CHANNEL_MASK={value}")])), None, "{value}");
+        }
+        assert_eq!(comments_mask(&block(&["WAVEFORMATEXTENSIBLE_CHANNEL_MASK=bad", "WAVEFORMATEXTENSIBLE_CHANNEL_MASK=0x3"])), Some(3));
+    }
+
+    #[test]
+    fn a_truncated_comment_block_has_no_mask() {
+        let whole = block(&["WAVEFORMATEXTENSIBLE_CHANNEL_MASK=0x3"]);
+        for cut in 0..whole.len() {
+            assert_eq!(comments_mask(&whole[..cut]), None, "{cut}");
+        }
+    }
+}

crates/audio/src/read/symph.rs

@@ -81,9 +81,10 @@ fn guarded<T>(f: impl FnOnce() -> Result<T>) -> Result<T> {
     catch_unwind(AssertUnwindSafe(f)).unwrap_or_else(|_| Err(AudioError::Malformed("decoder failed on malformed input".into())))
 }
 
-/// What [`open`] found: a reader, or a stream that holds no audio frames and has no reader.
+/// What [`open`] found: a reader (with the layout of a FLAC stream), or a stream that holds no
+/// audio frames and has no reader.
 enum Opened {
-    Reader(Box<dyn FormatReader>),
+    Reader(Box<dyn FormatReader>, Option<ChannelLayout>),
     Empty(AudioInfo),
 }
 
@@ -124,6 +125,8 @@ enum Precheck {
     Empty(AudioInfo),
     /// The size checks ran for the container the probe reports under this short name.
     Checked(&'static str),
+    /// As `Checked`, for a FLAC stream with this layout, which symphonia's reader does not parse.
+    Flac(&'static str, Option<ChannelLayout>),
     /// No container size check was needed.
     Unchecked,
 }
@@ -330,7 +333,7 @@ fn precheck(b: &[u8], start: usize, detected: Option<Container>) -> Result<Prech
             if let Some(info) = super::flac::empty_stream_info(b, at) {
                 return Ok(Precheck::Empty(info));
             }
-            Ok(Precheck::Checked(expected))
+            Ok(Precheck::Flac(expected, super::flac::layout(b, at)))
         }
         FirstMarker::Caf(at) => {
             super::caf::check(b, at)?;
@@ -356,10 +359,11 @@ fn open(bytes: Cow<'_, [u8]>, detected: Option<Container>) -> Result<Opened> {
     if start > 0 && start >= bytes.len() {
         return Err(AudioError::Malformed("ID3v2 tag extends past the end of the file".into()));
     }
-    let checked = precheck(&bytes, start, detected)?;
-    if let Precheck::Empty(info) = checked {
-        return Ok(Opened::Empty(info));
-    }
+    let (checked, flac_layout) = match precheck(&bytes, start, detected)? {
+        Precheck::Empty(info) => return Ok(Opened::Empty(info)),
+        Precheck::Flac(expected, layout) => (Precheck::Checked(expected), layout),
+        checked => (checked, None),
+    };
     // 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());
@@ -381,7 +385,7 @@ fn open(bytes: Cow<'_, [u8]>, detected: Option<Container>) -> Result<Opened> {
         }
         _ => {}
     }
-    Ok(Opened::Reader(reader))
+    Ok(Opened::Reader(reader, flac_layout.filter(|_| probed == "flac")))
 }
 
 /// The codec symphonia reports, as a [`CodecId`].
@@ -543,8 +547,8 @@ impl Symph {
     /// Open audio track `track` (the first when `None`) of the file in `bytes`.
     pub(crate) fn open(bytes: Cow<'_, [u8]>, detected: Option<Container>, track: Option<usize>) -> Result<Symph> {
         guarded(move || {
-            let reader = match open(bytes, detected)? {
-                Opened::Reader(reader) => reader,
+            let (reader, flac_layout) = match open(bytes, detected)? {
+                Opened::Reader(reader, layout) => (reader, layout),
                 Opened::Empty(info) => {
                     // a stream with no audio still has one track, as WAV and AIFF do
                     if let Some(n) = track.filter(|&n| n > 0) {
@@ -566,7 +570,10 @@ impl Symph {
                     });
                 }
             };
-            let (track_id, info) = info_of(reader.as_ref(), detected, track)?;
+            let (track_id, mut info) = info_of(reader.as_ref(), detected, track)?;
+            if let Some(layout) = flac_layout.filter(|l| l.channels() == info.channels) {
+                info.layout = layout;
+            }
             let t = reader.tracks().iter().find(|t| t.id == track_id).ok_or_else(|| AudioError::Malformed("track vanished".into()))?;
             let params = audio_params(t).cloned().ok_or_else(|| AudioError::Malformed("track vanished".into()))?;
             let alac = params.extra_data.as_deref().filter(|_| params.codec == CODEC_ID_ALAC).and_then(|d| be_u32(d, 0));
@@ -759,7 +766,7 @@ impl Symph {
 pub(crate) fn tracks(bytes: Cow<'_, [u8]>, detected: Option<Container>) -> Result<Vec<(CodecId, u32, u16)>> {
     guarded(move || {
         let reader = match open(bytes, detected)? {
-            Opened::Reader(reader) => reader,
+            Opened::Reader(reader, _) => reader,
             Opened::Empty(info) => return Ok(vec![(info.codec, info.sample_rate, info.channels)]),
         };
         Ok(audio_tracks(reader.as_ref())

crates/audio/src/write/flac/mod.rs

@@ -39,23 +39,11 @@ fn check(channels: usize, sample_rate: u32, depth: BitDepth) -> Result<u8> {
 /// Frames quantized and handed to the encoder at a time: its block size.
 const CHUNK: usize = 4096;
 
-/// The layout decoders assume for 1–8 channels with no channel mask comment, as a WAV mask. RFC
-/// 9639 §9.1.3 leaves "back/surround" open for 5 and 6 channels; libFLAC and ffmpeg read side
-/// surrounds. 7 channels are 6.1 with sides (FL FR FC LFE BC SL SR).
-fn assumed_mask(channels: usize) -> u32 {
-    match channels {
-        5 => 0x607,
-        6 => 0x60F,
-        7 => 0x70F,
-        n => ChannelLayout::default_for(n as u16).mask().unwrap_or(0),
-    }
-}
-
 /// The `WAVEFORMATEXTENSIBLE_CHANNEL_MASK` comment (RFC 9639 §8.6.2) for a layout decoders would
 /// not assume: `mask` (the usual layout for the count when `None`).
 fn mask_comment(channels: usize, mask: Option<u32>) -> Option<String> {
     let mask = mask.unwrap_or_else(|| ChannelLayout::default_for(channels as u16).mask().unwrap_or(0));
-    (mask != assumed_mask(channels)).then(|| format!("WAVEFORMATEXTENSIBLE_CHANNEL_MASK=0x{mask:04X}"))
+    (mask != crate::layout::flac_assumed_mask(channels)).then(|| format!("WAVEFORMATEXTENSIBLE_CHANNEL_MASK=0x{mask:04X}"))
 }
 
 fn encoder_error(e: encoder::Error) -> AudioError {
@@ -150,12 +138,9 @@ impl<W: Write + Seek> Writer<W> {
 
 pub(crate) fn encode(buf: &AudioBuffer, depth: BitDepth, dither: bool, mask: Option<u32>) -> Result<Vec<u8>> {
     validate_buffer(buf)?;
-    let bps = check(buf.channels.len(), buf.sample_rate, depth)?;
+    check(buf.channels.len(), buf.sample_rate, depth)?;
     let frames = buf.frames();
-    let mut out = Vec::new();
-    out.try_reserve(frames.saturating_mul(buf.channels.len()).saturating_mul(usize::from(bps) / 8) / 2)
-        .map_err(|_| AudioError::TooLarge("cannot allocate FLAC output".into()))?;
-    let mut w = Writer::new(Cursor::new(out), buf.channels.len(), buf.sample_rate, depth, dither, mask)?;
+    let mut w = Writer::new(Cursor::new(Vec::new()), buf.channels.len(), buf.sample_rate, depth, dither, mask)?;
     w.write(&buf.channels, frames)?;
     Ok(w.finish()?.into_inner())
 }

crates/audio/src/write/mod.rs

@@ -26,6 +26,7 @@ use std::io::{Seek, Write};
 /// # }
 /// ```
 pub fn encode(buf: &AudioBuffer, opts: &EncodeOptions) -> Result<Vec<u8>> {
+    check_built(opts.format)?;
     // Checked for every format; WAV and FLAC record it.
     let mask = opts.layout.map(|l| wav_mask(&l, buf.channels.len())).transpose()?;
     match opts.format {
@@ -36,15 +37,15 @@ pub fn encode(buf: &AudioBuffer, opts: &EncodeOptions) -> Result<Vec<u8>> {
         #[cfg(feature = "flac-encode")]
         EncodeFormat::Flac => flac::encode(buf, opts.bit_depth, opts.dither, mask),
         #[allow(unreachable_patterns)]
-        f => Err(not_built(f)),
+        f => Err(AudioError::Unsupported(format!("writing {f:?}"))),
     }
 }
 
-/// The error for a format whose writer is not built.
-#[allow(dead_code)]
-fn not_built(format: EncodeFormat) -> AudioError {
-    let feature = if format == EncodeFormat::Flac { "flac-encode" } else { "pcm-encode" };
-    AudioError::Unsupported(format!("writing {format:?} needs the `{feature}` feature"))
+/// Refuses a format whose writer is not built, before anything else about the request is checked.
+fn check_built(format: EncodeFormat) -> Result<()> {
+    let (built, feature) =
+        if format == EncodeFormat::Flac { (cfg!(feature = "flac-encode"), "flac-encode") } else { (cfg!(feature = "pcm-encode"), "pcm-encode") };
+    if built { Ok(()) } else { Err(AudioError::Unsupported(format!("writing {format:?} needs the `{feature}` feature"))) }
 }
 
 /// The file extensions of the formats [`encode`] can write (those whose features are on).
@@ -150,6 +151,7 @@ impl<W: Write + Seek> FileEncoder<W> {
     /// Starts a file of `channels` channels at `sample_rate` at the current position of `out`.
     /// `opts.layout`, when set, must have `channels` channels.
     pub fn new(out: W, opts: &EncodeOptions, sample_rate: u32, channels: u16) -> Result<Self> {
+        check_built(opts.format)?;
         let (depth, dither, channels) = (opts.bit_depth, opts.dither, usize::from(channels));
         let mask = opts.layout.map(|l| wav_mask(&l, channels)).transpose()?;
         let writer = match opts.format {
@@ -167,7 +169,7 @@ impl<W: Write + Seek> FileEncoder<W> {
             #[cfg(feature = "flac-encode")]
             EncodeFormat::Flac => Writer::Flac(flac::Writer::new(out, channels, sample_rate, depth, dither, mask)?),
             #[allow(unreachable_patterns)]
-            f => return Err(not_built(f)),
+            f => return Err(AudioError::Unsupported(format!("writing {f:?}"))),
         };
         Ok(FileEncoder { writer, channels })
     }
@@ -259,5 +261,29 @@ 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);
+        // Also with a layout it could not use.
+        let opts = opts.with_layout(ChannelLayout::STEREO);
+        assert!(matches!(encode(&buf, &opts), Err(AudioError::Unsupported(_))));
+        let enc = FileEncoder::new(std::io::Cursor::new(Vec::new()), &opts, 48_000, 1);
+        assert!(matches!(enc, Err(AudioError::Unsupported(_))));
+    }
+
+    /// Streaming in any block sizes writes the bytes `encode` writes.
+    #[cfg(feature = "flac-encode")]
+    #[test]
+    fn streamed_flac_matches_encode() {
+        let channels: Vec<Vec<f32>> = (0..2).map(|c| (0..20_000).map(|i| ((i * (c + 3)) as f32 * 0.01).sin() * 0.5).collect()).collect();
+        let buf = AudioBuffer { sample_rate: 48_000, channels };
+        let opts = EncodeOptions::new(EncodeFormat::Flac);
+        let whole = encode(&buf, &opts).unwrap();
+        let mut enc = FileEncoder::new(std::io::Cursor::new(Vec::new()), &opts, 48_000, 2).unwrap();
+        let mut at = 0;
+        for n in [1000, 4096, 1, 9000, 5903] {
+            let block: Vec<&[f32]> = buf.channels.iter().map(|c| &c[at..at + n]).collect();
+            enc.write(&block, n).unwrap();
+            at += n;
+        }
+        assert_eq!(at, buf.frames());
+        assert_eq!(enc.finish().unwrap().into_inner(), whole);
     }
 }

crates/audio/tests/audio/roundtrip.rs

@@ -386,3 +386,29 @@ fn a_byte_rate_past_32_bits_is_an_error() {
     assert!(matches!(wav_float_header(4_000_000_000, 2, 0), Err(craft_audio::AudioError::Encode(_))));
     assert!(wav_float_header(4_000_000_000 / 8, 2, 0).is_ok());
 }
+
+/// The channel mask comment of a FLAC file is its layout, for `probe`, `decode` and `Decoder`, and
+/// for a file with no audio frames.
+#[test]
+fn flac_layouts_read_back_from_the_mask_comment() {
+    use craft_audio::{DecodeOptions, Decoder};
+    let cases = [
+        (6, Some(ChannelLayout::SURROUND_5_1_SIDE), ChannelLayout::SURROUND_5_1_SIDE),
+        (6, None, ChannelLayout::SURROUND_5_1),
+        (7, Some(ChannelLayout::SURROUND_6_1), ChannelLayout::SURROUND_6_1),
+        (7, None, ChannelLayout::default_for(7)),
+        (4, Some(ChannelLayout::from_mask(0x107)), ChannelLayout::from_mask(0x107)),
+        (2, Some(ChannelLayout::unordered(2)), ChannelLayout::unordered(2)),
+    ];
+    for (channels, layout, expected) in cases {
+        for frames in [4096, 0] {
+            let mut opts = EncodeOptions::new(EncodeFormat::Flac);
+            opts.layout = layout;
+            let file = craft_audio::encode(&AudioBuffer::new(48_000, channels, frames), &opts).unwrap();
+            assert_eq!(probe(&file, None).unwrap().layout, expected, "probe {channels} {layout:?} {frames}");
+            assert_eq!(decode(&file, None).unwrap().0.layout, expected, "decode {channels} {layout:?} {frames}");
+            let dec = Decoder::from_bytes(&file, None, &DecodeOptions::default()).unwrap();
+            assert_eq!(dec.info().layout, expected, "Decoder {channels} {layout:?} {frames}");
+        }
+    }
+}

crates/audio/tests/audio/stream.rs

@@ -182,8 +182,16 @@ fn seeking_to_or_past_the_end_ends_the_stream() {
             assert!(dec.next_block().unwrap().is_none(), "{format:?} seek({target})");
             // Seeking back still works.
             assert_eq!(dec.seek(4000).unwrap(), 4000, "{format:?}");
-            let left: usize = std::iter::from_fn(|| dec.next_block().unwrap()).map(|b| b.frames()).sum();
-            assert_eq!(left, 800, "{format:?}");
+            let mut left = vec![Vec::new(); buf.channels.len()];
+            while let Some(b) = dec.next_block().unwrap() {
+                for (l, c) in left.iter_mut().zip(b.channels) {
+                    l.extend(c);
+                }
+            }
+            for (l, source) in left.iter().zip(&buf.channels) {
+                assert_eq!(l.len(), 800, "{format:?}");
+                assert!(l.iter().zip(&source[4000..]).all(|(a, b)| (a - b.clamp(-1.0, 1.0)).abs() <= 2.0 / 32768.0), "{format:?}");
+            }
         }
     }
 }