pauljd/craft-libs
1
0
Fork 0
mirror of https://github.com/storytold/craft-libs.git synced 2026-11-07 12:13:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
A track index in an Ogg file means the same stream everywhere: the
file's order, as `tracks` lists them. Streams the native path does not
read fell back to symphonia with the same index, but symphonia orders
tracks by serial, which ffmpeg picks at random. A file could decode the
wrong track, or fail on a FLAC stream listed before an Opus one.

The tests write each file eight times, so both serial orders occur, and
cover a track with no mask comment next to one with a mask.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
alehel
2026-11-04 15:32:10 +01:00
co-authored by Claude Opus 5.5
parent 2f4fb8bfe0
commit a4f02a3854
3 changed files with 90 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/audio/src/read/flac.rs
+5 -4
View File
@@ -124,15 +124,16 @@ fn comment_mask(key: &[u8], value: &[u8]) -> Option<u32> {
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)> {
/// The audio streams of the Ogg file `b` in file order, as `tracks` lists them: each one's serial
/// and, for FLAC, its channel mask comment. Empty when the demuxer cannot read the file.
pub(crate) fn ogg_audio_streams(b: &[u8]) -> Vec<(u32, Option<u32>)> {
let Ok(file) = ogg::open(b) else { return Vec::new() };
let comments = |s: &ogg::Stream| {
let mask = |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()
file.streams.iter().filter(|s| s.codec.codec_id().is_audio()).map(|s| (s.serial, mask(s).filter(|_| s.codec == ogg::Codec::Flac))).collect()
}
/// The channel mask in a Vorbis comment block's body.
crates/audio/src/read/symph.rs
+38 -24
View File
@@ -81,20 +81,21 @@ 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 symphonia's FLAC and Ogg readers leave out about a file's layout.
enum FlacLayouts {
/// What [`open`] read from the file for what symphonia's FLAC and Ogg readers leave out.
enum Preread {
/// Not a FLAC file or an Ogg file.
Unknown,
Nothing,
/// 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)>),
Flac(ChannelLayout),
/// An Ogg file's audio streams in file order: each one's serial, which is symphonia's track ID,
/// and its FLAC channel mask. Symphonia orders the tracks by serial, and the tags its reader
/// collects do not say which track they are from.
Ogg(Vec<(u32, Option<u32>)>),
}
/// What [`open`] found: a reader, or a stream that holds no audio frames and has no reader.
enum Opened {
Reader(Box<dyn FormatReader>, FlacLayouts),
Reader(Box<dyn FormatReader>, Preread),
Empty(AudioInfo),
}
@@ -375,7 +376,7 @@ fn open(bytes: Cow<'_, [u8]>, detected: Option<Container>) -> Result<Opened> {
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));
let ogg_streams = craft_media::ogg::sniff(body).then(|| super::flac::ogg_audio_streams(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());
@@ -397,12 +398,12 @@ fn open(bytes: Cow<'_, [u8]>, detected: Option<Container>) -> Result<Opened> {
}
_ => {}
}
let layouts = match (flac_layout.filter(|_| probed == "flac"), ogg_masks) {
(Some(layout), _) => FlacLayouts::File(layout),
(None, Some(masks)) => FlacLayouts::Ogg(masks),
_ => FlacLayouts::Unknown,
let preread = match (flac_layout.filter(|_| probed == "flac"), ogg_streams.filter(|_| probed == "ogg")) {
(Some(layout), _) => Preread::Flac(layout),
(None, Some(streams)) => Preread::Ogg(streams),
_ => Preread::Nothing,
};
Ok(Opened::Reader(reader, layouts))
Ok(Opened::Reader(reader, preread))
}
/// The codec symphonia reports, as a [`CodecId`].
@@ -509,10 +510,22 @@ fn audio_tracks(reader: &dyn FormatReader) -> impl Iterator<Item = &Track> {
reader.tracks().iter().filter(|t| audio_params(t).is_some_and(|p| p.codec != CODEC_ID_NULL_AUDIO))
}
/// The `n`-th audio track (the first when `None`), with the stream information the UI shows.
fn info_of(reader: &dyn FormatReader, detected: Option<Container>, n: Option<usize>) -> Result<(u32, AudioInfo)> {
/// The `n`-th audio track (the first when `None`), counting an Ogg file's `streams` in file order
/// when they are known. With no `n`, the first stream symphonia has a track for: it has none for
/// some of the codecs `tracks` lists.
fn pick_track<'a>(reader: &'a dyn FormatReader, streams: &[(u32, Option<u32>)], n: Option<usize>) -> Option<&'a Track> {
let with_serial = |serial: u32| audio_tracks(reader).find(|t| t.id == serial);
match n {
_ if streams.is_empty() => audio_tracks(reader).nth(n.unwrap_or(0)),
Some(n) => with_serial(streams.get(n)?.0),
None => streams.iter().find_map(|s| with_serial(s.0)),
}
}
/// The `n`-th audio track (see [`pick_track`]), with the stream information the UI shows.
fn info_of(reader: &dyn FormatReader, detected: Option<Container>, ogg_streams: &[(u32, Option<u32>)], n: Option<usize>) -> Result<(u32, AudioInfo)> {
let container = container_of(reader.format_info().short_name, detected);
let track = audio_tracks(reader).nth(n.unwrap_or(0)).ok_or_else(|| AudioError::Unsupported("no decodable audio track".into()))?;
let track = pick_track(reader, ogg_streams, n).ok_or_else(|| AudioError::Unsupported("no decodable audio track".into()))?;
let p = audio_params(track).ok_or_else(|| AudioError::Unsupported("no decodable audio track".into()))?;
let sample_rate = p.sample_rate.filter(|&r| r > 0).ok_or_else(|| AudioError::Malformed("unknown sample rate".into()))?;
let layout = layout_of(p.channels.as_ref());
@@ -564,8 +577,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, flac_layouts) = match open(bytes, detected)? {
Opened::Reader(reader, layouts) => (reader, layouts),
let (reader, preread) = match open(bytes, detected)? {
Opened::Reader(reader, preread) => (reader, preread),
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) {
@@ -587,11 +600,12 @@ impl Symph {
});
}
};
let (track_id, mut info) = info_of(reader.as_ref(), detected, track)?;
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);
let ogg_streams = if let Preread::Ogg(streams) = &preread { streams.as_slice() } else { &[] };
let (track_id, mut info) = info_of(reader.as_ref(), detected, ogg_streams, track)?;
match &preread {
Preread::Flac(layout) if layout.channels() == info.channels => info.layout = *layout,
Preread::Ogg(streams) if info.codec == CodecId::Flac => {
let mask = streams.iter().find(|(serial, _)| *serial == track_id).and_then(|&(_, mask)| mask);
info.layout = super::flac::layout_from(mask, info.channels);
}
_ => {}
crates/audio/src/write/flac/oracle_tests.rs
+47 -21
View File
@@ -266,31 +266,57 @@ 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.
/// Two streams in an Ogg file written by ffmpeg, which picks their serial numbers at random. The
/// tests below write each file a few times, so that both serial orders occur.
#[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"])
fn two_ogg_streams(ff: &Path, name: &str, args: &[&str]) -> Option<Vec<u8>> {
let path = common::fixture_subdir("flac").join(format!("{name}.ogg"));
let status = Command::new(ff)
.args(["-v", "error", "-y", "-f", "lavfi", "-i", "aevalsrc=0|0|0|0:c=4c:d=0.1:s=48000"])
.args(["-f", "lavfi", "-i", "aevalsrc=0|0|0|0:c=4c:d=0.1:s=44100"])
.args(["-map", "0", "-map", "1"])
.args(args)
.args(["-f", "ogg"])
.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}");
status.success().then(|| std::fs::read(&path).unwrap())
}
/// A track index means the same stream in `tracks`, `probe` and `decode`: the file's order. Each
/// track has its own mask comment, or else the layout decoders assume for the count.
#[cfg(feature = "symphonia")]
#[test]
fn ogg_flac_tracks_are_in_file_order_with_their_own_mask_comments() {
use crate::{ChannelLayout, DecodeOptions};
let Some(ff) = ffmpeg() else { return };
let args = ["-c:a", "flac", "-metadata:s:a:0", "WAVEFORMATEXTENSIBLE_CHANNEL_MASK=0x0107"];
for _ in 0..8 {
let bytes = two_ogg_streams(&ff, "ogg-one-mask", &args).unwrap();
let rates: Vec<_> = crate::tracks(&bytes, None).unwrap().iter().map(|t| t.sample_rate).collect();
assert_eq!(rates, [Some(48_000), Some(44_100)]);
for (track, rate, layout) in [(0, 48_000, ChannelLayout::from_mask(0x107)), (1, 44_100, ChannelLayout::default_for(4))] {
let opts = DecodeOptions::default().with_track(track);
let info = crate::probe_with(&bytes, None, &opts).unwrap();
assert_eq!((info.sample_rate, info.layout), (rate, layout), "probe track {track}");
let info = crate::decode_with(&bytes, None, &opts).unwrap().0;
assert_eq!((info.sample_rate, info.layout), (rate, layout), "decode track {track}");
}
assert_eq!(crate::decode(&bytes, None).unwrap().0.sample_rate, 48_000, "the first track");
}
}
/// A FLAC stream before a stream symphonia cannot decode is still the first track.
#[cfg(feature = "symphonia")]
#[test]
fn ogg_flac_before_opus_is_the_first_track() {
use crate::CodecId;
let Some(ff) = ffmpeg() else { return };
let args = ["-c:a:0", "flac", "-c:a:1", "libopus", "-ac:a:1", "2"];
for _ in 0..8 {
let Some(bytes) = two_ogg_streams(&ff, "ogg-flac-opus", &args) else { return };
assert_eq!(crate::tracks(&bytes, None).unwrap()[0].codec, CodecId::Flac);
assert_eq!(crate::decode(&bytes, None).unwrap().0.codec, CodecId::Flac);
}
}