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

Each FLAC stream in an Ogg file reads its own channel mask comment. The
tags symphonia's reader collects do not say which track they are from,
so a track could get another track's mask. The masks now come from
craft-media's Ogg demuxer, by serial, which is symphonia's track ID.

The Ogg layout test also covers 7 channels with no comment and a zero
mask.

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

crates/audio/src/read/flac.rs

@@ -1,5 +1,7 @@
 //! FLAC metadata checks run before symphonia decodes a stream (RFC 9639 metadata blocks).
 
+use craft_media::ogg;
+
 use crate::pcm::{be_u32, le_u32};
 use crate::{AudioError, AudioInfo, ChannelLayout, CodecId, Container, Result, SampleFormat};
 
@@ -95,15 +97,6 @@ pub(crate) fn layout_from(mask: Option<u32>, channels: u16) -> ChannelLayout {
     ChannelLayout::from_mask_and_channels(mask, channels)
 }
 
-/// The mask in a comment, when its key is the channel mask's and its value a `0x` hexadecimal number.
-pub(crate) fn comment_mask(key: &[u8], value: &[u8]) -> Option<u32> {
-    if !key.eq_ignore_ascii_case(b"WAVEFORMATEXTENSIBLE_CHANNEL_MASK") {
-        return None;
-    }
-    let hex = value.strip_prefix(b"0x").or_else(|| value.strip_prefix(b"0X"))?;
-    u32::from_str_radix(std::str::from_utf8(hex).ok()?, 16).ok()
-}
-
 /// 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> {
@@ -122,6 +115,26 @@ fn channel_mask(b: &[u8], at: usize) -> Option<u32> {
     }
 }
 
+/// The mask in a comment, when its key is the channel mask's and its value a `0x` hexadecimal number.
+fn comment_mask(key: &[u8], value: &[u8]) -> Option<u32> {
+    if !key.eq_ignore_ascii_case(b"WAVEFORMATEXTENSIBLE_CHANNEL_MASK") {
+        return None;
+    }
+    let hex = value.strip_prefix(b"0x").or_else(|| value.strip_prefix(b"0X"))?;
+    u32::from_str_radix(std::str::from_utf8(hex).ok()?, 16).ok()
+}
+
+/// The channel mask comment of each FLAC stream that has one in the Ogg file `b`, by serial.
+pub(crate) fn ogg_channel_masks(b: &[u8]) -> Vec<(u32, u32)> {
+    let Ok(file) = ogg::open(b) else { return Vec::new() };
+    let comments = |s: &ogg::Stream| {
+        // After the first packet, each header packet is a metadata block with its 4-byte header.
+        let block = s.headers.iter().skip(1).find(|h| h.first().is_some_and(|t| t & 0x7f == 4))?;
+        comments_mask(block.get(4..)?)
+    };
+    file.streams.iter().filter(|s| s.codec == ogg::Codec::Flac).filter_map(|s| Some((s.serial, comments(s)?))).collect()
+}
+
 /// 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;

crates/audio/src/read/symph.rs

@@ -16,7 +16,7 @@ use symphonia::core::errors::Error as SymError;
 use symphonia::core::formats::probe::{Hint, ProbeOptions, ProbeableFormat, Score, Scoreable};
 use symphonia::core::formats::{FormatOptions, FormatReader, SeekMode, SeekTo, Track};
 use symphonia::core::io::{MediaSourceStream, MediaSourceStreamOptions, ScopedStream};
-use symphonia::core::meta::{MetadataOptions, RawValue};
+use symphonia::core::meta::MetadataOptions;
 use symphonia::core::units::{TimeBase, Timestamp};
 
 use super::id3v2_len;
@@ -81,10 +81,20 @@ 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 (with the layout of a FLAC stream), or a stream that holds no
-/// audio frames and has no reader.
+/// What symphonia's FLAC and Ogg readers leave out about a file's layout.
+enum FlacLayouts {
+    /// Not a FLAC file or an Ogg file.
+    Unknown,
+    /// A FLAC file's layout.
+    File(ChannelLayout),
+    /// The channel masks of an Ogg file's FLAC streams, by serial (symphonia's track ID). The tags
+    /// its reader collects do not say which track they are from.
+    Ogg(Vec<(u32, u32)>),
+}
+
+/// What [`open`] found: a reader, or a stream that holds no audio frames and has no reader.
 enum Opened {
-    Reader(Box<dyn FormatReader>, Option<ChannelLayout>),
+    Reader(Box<dyn FormatReader>, FlacLayouts),
     Empty(AudioInfo),
 }
 
@@ -364,6 +374,8 @@ fn open(bytes: Cow<'_, [u8]>, detected: Option<Container>) -> Result<Opened> {
         Precheck::Flac(expected, layout) => (Precheck::Checked(expected), layout),
         checked => (checked, None),
     };
+    let body = bytes.get(start..).unwrap_or_default();
+    let ogg_masks = craft_media::ogg::sniff(body).then(|| super::flac::ogg_channel_masks(body));
     // 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());
@@ -385,7 +397,12 @@ fn open(bytes: Cow<'_, [u8]>, detected: Option<Container>) -> Result<Opened> {
         }
         _ => {}
     }
-    Ok(Opened::Reader(reader, flac_layout.filter(|_| probed == "flac")))
+    let layouts = match (flac_layout.filter(|_| probed == "flac"), ogg_masks) {
+        (Some(layout), _) => FlacLayouts::File(layout),
+        (None, Some(masks)) => FlacLayouts::Ogg(masks),
+        _ => FlacLayouts::Unknown,
+    };
+    Ok(Opened::Reader(reader, layouts))
 }
 
 /// The codec symphonia reports, as a [`CodecId`].
@@ -446,21 +463,6 @@ fn layout_of(channels: Option<&Channels>) -> ChannelLayout {
     }
 }
 
-/// The channel mask comment among the tags the reader has read.
-fn metadata_channel_mask(reader: &mut dyn FormatReader) -> Option<u32> {
-    let mut metadata = reader.metadata();
-    loop {
-        let mask = metadata.current()?.media.tags.iter().find_map(|t| match &t.raw.value {
-            RawValue::String(value) => super::flac::comment_mask(t.raw.key.as_bytes(), value.as_bytes()),
-            _ => None,
-        });
-        if mask.is_some() {
-            return mask;
-        }
-        metadata.pop()?;
-    }
-}
-
 /// The stored sample format, using the codec and its declared bit depth. Lossy and unusual
 /// codecs decode to `Compressed`.
 fn sample_format_of(p: &AudioCodecParameters) -> SampleFormat {
@@ -562,8 +564,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 (mut reader, flac_layout) = match open(bytes, detected)? {
-                Opened::Reader(reader, layout) => (reader, layout),
+            let (reader, flac_layouts) = match open(bytes, detected)? {
+                Opened::Reader(reader, layouts) => (reader, layouts),
                 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) {
@@ -586,10 +588,13 @@ impl Symph {
                 }
             };
             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;
-            } else if info.codec == CodecId::Flac && info.container == Container::Ogg {
-                info.layout = super::flac::layout_from(metadata_channel_mask(reader.as_mut()), info.channels);
+            match flac_layouts {
+                FlacLayouts::File(layout) if layout.channels() == info.channels => info.layout = layout,
+                FlacLayouts::Ogg(masks) if info.codec == CodecId::Flac => {
+                    let mask = masks.iter().find(|(serial, _)| *serial == track_id).map(|&(_, mask)| mask);
+                    info.layout = super::flac::layout_from(mask, info.channels);
+                }
+                _ => {}
             }
             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()))?;

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

@@ -243,10 +243,12 @@ fn layouts_decoders_would_not_assume_are_recorded() {
 fn ogg_flac_reads_the_mask_comment() {
     use crate::ChannelLayout;
     let Some(ff) = ffmpeg() else { return };
-    let cases: [(&str, &str, &[&str], u16, u32); 3] = [
+    let cases: [(&str, &str, &[&str], u16, u32); 5] = [
         ("ogg-mask", "aevalsrc=0|0|0|0:c=4c:d=0.1:s=48000", &["-metadata", "WAVEFORMATEXTENSIBLE_CHANNEL_MASK=0x0107"], 4, 0x107),
         ("ogg-5.1", "aevalsrc=0|0|0|0|0|0:c=5.1:d=0.1:s=48000", &[], 6, 0x60F),
         ("ogg-stereo", "aevalsrc=0|0:c=stereo:d=0.1:s=48000", &["-metadata", "TITLE=x"], 2, 0x3),
+        ("ogg-6.1", "aevalsrc=0|0|0|0|0|0|0:c=6.1:d=0.1:s=48000", &[], 7, 0x70F),
+        ("ogg-zero-mask", "aevalsrc=0|0|0|0|0|0:c=5.1:d=0.1:s=48000", &["-metadata", "WAVEFORMATEXTENSIBLE_CHANNEL_MASK=0x0"], 6, 0),
     ];
     for (name, source, extra, channels, mask) in cases {
         let path = common::fixture_subdir("flac").join(format!("{name}.oga"));
@@ -264,6 +266,34 @@ fn ogg_flac_reads_the_mask_comment() {
     }
 }
 
+/// Each track of an Ogg file has its own mask comment. The tracks are told apart by their audio,
+/// because the track order follows the serial numbers ffmpeg picks at random.
+#[cfg(feature = "symphonia")]
+#[test]
+fn ogg_flac_tracks_read_their_own_mask_comments() {
+    use crate::{ChannelLayout, DecodeOptions};
+    let Some(ff) = ffmpeg() else { return };
+    let path = common::fixture_subdir("flac").join("ogg-two-masks.oga");
+    let status = Command::new(&ff)
+        .args(["-v", "error", "-y", "-f", "lavfi", "-i", "aevalsrc=0.5|0.5|0.5|0.5:c=4c:d=0.1:s=48000"])
+        .args(["-f", "lavfi", "-i", "aevalsrc=0.25|0.25|0.25|0.25:c=4c:d=0.1:s=48000"])
+        .args(["-map", "0", "-map", "1", "-c:a", "flac", "-f", "ogg"])
+        .args(["-metadata:s:a:0", "WAVEFORMATEXTENSIBLE_CHANNEL_MASK=0x0107", "-metadata:s:a:1", "WAVEFORMATEXTENSIBLE_CHANNEL_MASK=0x000F"])
+        .arg(&path)
+        .status()
+        .unwrap();
+    assert!(status.success());
+    let bytes = std::fs::read(&path).unwrap();
+    for track in [0, 1] {
+        let opts = DecodeOptions::default().with_track(track);
+        let (info, buf) = crate::decode_with(&bytes, None, &opts).unwrap();
+        let mask = if buf.channels[0][0] > 0.4 { 0x107 } else { 0xF };
+        let expected = ChannelLayout::from_mask_and_channels(mask, 4);
+        assert_eq!(info.layout, expected, "decode track {track}");
+        assert_eq!(crate::probe_with(&bytes, None, &opts).unwrap().layout, expected, "probe track {track}");
+    }
+}
+
 #[test]
 fn the_mask_comment_is_written_only_when_decoders_would_assume_otherwise() {
     use super::mask_comment;