From 82424e59c3dfa31cbabf334591778293dbb0ac3e Mon Sep 17 00:00:00 2001 From: alehel Date: Tue, 3 Nov 2026 21:44:20 +0100 Subject: [PATCH] craft-audio: review fixes, round 5 (#442) - libFLAC and ffmpeg read a FLAC with 5 or 6 channels and no mask comment as having side surrounds, so the comment is now written for anything else, including the crate's usual back-surround layouts. The round 3 check treated back surrounds as FLAC's assignment, which lost them. - Tests: the mask comment rules, opposite-polarity stereo, STREAMINFO block and frame sizes. - A stale comment said only WAV records the layout. Co-Authored-By: Claude Opus 5.5 --- crates/audio/README.md | 2 +- crates/audio/src/lib.rs | 7 +-- crates/audio/src/write/flac/mod.rs | 23 +++++---- crates/audio/src/write/flac/oracle_tests.rs | 52 +++++++++++++++++++-- crates/audio/src/write/mod.rs | 2 +- 5 files changed, 69 insertions(+), 17 deletions(-) diff --git a/crates/audio/README.md b/crates/audio/README.md index 2000fe96..5cf6b9f9 100644 --- a/crates/audio/README.md +++ b/crates/audio/README.md @@ -271,7 +271,7 @@ match craft_audio::decode(&bytes, None) { | Codec | Spec | Scope | Verified against | |---|---|---|---| | WAV, AIFF | Microsoft RIFF, Apple AIFF/AIFF-C | read: integer 8–32-bit, float 32/64-bit, `WAVE_FORMAT_EXTENSIBLE` masks, RF64/BW64, BWF `bext`, truncated files; write: 16/24/32-bit integer and 32-bit float, RF64 above 4 GiB, any layout, TPDF dither | round trips; hostile-input tests | -| FLAC encoder | RFC 9639 | 16, 24, 32 bits; 1–8 channels, layouts outside FLAC's channel assignment as a `WAVEFORMATEXTENSIBLE_CHANNEL_MASK` comment; up to 655,350 Hz; LPC, stereo decorrelation, STREAMINFO MD5 | bit-exact through ffmpeg, STREAMINFO MD5 checked against ffmpeg | +| FLAC encoder | RFC 9639 | 16, 24, 32 bits; 1–8 channels, layouts decoders would not assume for the count as a `WAVEFORMATEXTENSIBLE_CHANNEL_MASK` comment; up to 655,350 Hz; LPC, stereo decorrelation, STREAMINFO MD5 | bit-exact through ffmpeg, STREAMINFO MD5 checked against ffmpeg | ## symphonia diff --git a/crates/audio/src/lib.rs b/crates/audio/src/lib.rs index 70bf9a3f..f582c74f 100644 --- a/crates/audio/src/lib.rs +++ b/crates/audio/src/lib.rs @@ -269,9 +269,10 @@ pub struct EncodeOptions { /// The speaker layout to record, for layouts a channel count alone cannot identify (5.1 with /// side vs back surrounds, 7.0 vs 6.1, Ambisonics as [`ChannelLayout::unordered`]). `None` is /// the usual layout for the channel count. WAV stores it as `dwChannelMask`, FLAC as a - /// `WAVEFORMATEXTENSIBLE_CHANNEL_MASK` comment when it is not FLAC's fixed assignment for the - /// count (which for 7 channels is 6.1 with side surrounds, not the usual layout); AIFF has a - /// fixed channel order and ignores it. It must have as many channels as the audio. + /// `WAVEFORMATEXTENSIBLE_CHANNEL_MASK` comment when decoders would assume another layout for + /// the count (they read 5 and 6 channels with side surrounds and 7 as 6.1 with sides, so the + /// usual layouts for those counts get one); AIFF has a fixed channel order and ignores it. It + /// must have as many channels as the audio. pub layout: Option, /// Write WAV in the RF64 layout even below 4 GiB (WAV only; ignored for other formats). pub force_rf64: bool, diff --git a/crates/audio/src/write/flac/mod.rs b/crates/audio/src/write/flac/mod.rs index f8accca4..5ada9ad0 100644 --- a/crates/audio/src/write/flac/mod.rs +++ b/crates/audio/src/write/flac/mod.rs @@ -39,18 +39,23 @@ fn check(channels: usize, sample_rate: u32, depth: BitDepth) -> Result { /// Frames quantized and handed to the encoder at a time: its block size. const CHUNK: usize = 4096; -/// FLAC's channel assignment for 1–8 channels (RFC 9639 §9.1.3) as a WAV mask: the usual layout -/// for the count, except that 7 channels are 6.1 with side surrounds (FL FR FC LFE BC SL SR). -fn flac_mask(channels: usize) -> u32 { - let layout = if channels == 7 { ChannelLayout::SURROUND_6_1 } else { ChannelLayout::default_for(channels as u16) }; - layout.mask().unwrap_or(0) +/// 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 FLAC's channel -/// assignment does not already say: `mask` (the usual layout for the count when `None`). +/// 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) -> Option { let mask = mask.unwrap_or_else(|| ChannelLayout::default_for(channels as u16).mask().unwrap_or(0)); - (mask != flac_mask(channels)).then(|| format!("WAVEFORMATEXTENSIBLE_CHANNEL_MASK=0x{mask:04X}")) + (mask != assumed_mask(channels)).then(|| format!("WAVEFORMATEXTENSIBLE_CHANNEL_MASK=0x{mask:04X}")) } fn encoder_error(e: encoder::Error) -> AudioError { @@ -73,7 +78,7 @@ pub(crate) struct Writer { impl Writer { /// `mask` is the speaker layout as a WAV channel mask (the usual layout for the count when - /// `None`), recorded as a comment when FLAC's channel assignment says otherwise. + /// `None`), recorded as a comment when decoders would assume another one. pub(crate) fn new(mut out: W, channels: usize, sample_rate: u32, depth: BitDepth, dither: bool, mask: Option) -> Result { let bps = check(channels, sample_rate, depth)?; // `check` bounds channels to 1..=8. diff --git a/crates/audio/src/write/flac/oracle_tests.rs b/crates/audio/src/write/flac/oracle_tests.rs index 772ea9ae..997f75cc 100644 --- a/crates/audio/src/write/flac/oracle_tests.rs +++ b/crates/audio/src/write/flac/oracle_tests.rs @@ -209,10 +209,10 @@ fn full_scale_32_bit_stereo_round_trips() { assert!(enc.encode(&[&[i32::MIN, i32::MAX], &[0, -1]]).is_ok()); } -/// A layout FLAC's channel assignment does not name is recorded as a channel mask comment, which -/// ffmpeg reads: the usual 7-channel layout (backs and a back centre) differs from FLAC's 6.1. +/// A layout decoders would not assume for the channel count is recorded as a channel mask comment, +/// which ffmpeg reads: they read 6 channels with side surrounds, and 7 as 6.1 with sides. #[test] -fn layouts_other_than_flacs_are_recorded() { +fn layouts_decoders_would_not_assume_are_recorded() { use crate::{AudioBuffer, ChannelLayout, EncodeFormat, EncodeOptions}; let Some(ffprobe) = common::ffprobe() else { return }; let cases = [ @@ -220,6 +220,8 @@ fn layouts_other_than_flacs_are_recorded() { ("layout-7-default", 7, None, "6.1(back)"), ("layout-6.1-side", 7, Some(ChannelLayout::SURROUND_6_1), "6.1"), ("layout-5.1-side", 6, Some(ChannelLayout::SURROUND_5_1_SIDE), "5.1(side)"), + ("layout-5.1-back", 6, Some(ChannelLayout::SURROUND_5_1), "5.1"), + ("layout-6-default", 6, None, "5.1"), ]; for (name, channels, layout, expected) in cases { let buf = AudioBuffer::new(48_000, channels, 100); @@ -233,3 +235,47 @@ fn layouts_other_than_flacs_are_recorded() { assert_eq!(String::from_utf8_lossy(&out.stdout).trim(), expected, "{name}"); } } + +#[test] +fn the_mask_comment_is_written_only_when_decoders_would_assume_otherwise() { + use super::mask_comment; + let comment = |m: &str| Some(format!("WAVEFORMATEXTENSIBLE_CHANNEL_MASK={m}")); + for channels in [1, 2, 3, 4, 8] { + assert_eq!(mask_comment(channels, None), None, "{channels}"); + } + assert_eq!(mask_comment(5, None), comment("0x0037")); + assert_eq!(mask_comment(5, Some(0x607)), None); + assert_eq!(mask_comment(6, None), comment("0x003F")); + assert_eq!(mask_comment(6, Some(0x60F)), None); + assert_eq!(mask_comment(7, None), comment("0x013F")); + assert_eq!(mask_comment(7, Some(0x70F)), None); + // no speaker assignment + assert_eq!(mask_comment(2, Some(0)), comment("0x0000")); +} + +/// Left and right at opposite full scale: the side channel needs its extra bit at every depth. +#[test] +fn opposite_polarity_stereo_round_trips() { + for bps in [16u8, 24] { + let left = signal("square", 9_000, bps, 1); + let right: Vec = left.iter().map(|&v| !v).collect(); + let planar = vec![left, right]; + if round_trip(&format!("opposite-{bps}"), &planar, EncoderConfig::new(48_000, 2, bps), 4096).is_none() { + return; + } + } +} + +#[test] +fn streaminfo_states_the_block_and_frame_sizes() { + let planar = [signal("music", 10_000, 16, 21), signal("noise", 10_000, 16, 22)]; + let mut enc = Encoder::new(EncoderConfig::new(48_000, 2, 16)).unwrap(); + let parts: Vec<&[i32]> = planar.iter().map(Vec::as_slice).collect(); + let mut sizes: Vec = enc.encode_frames(&parts).unwrap().iter().map(Vec::len).collect(); + sizes.push(enc.finish().len()); + let si = enc.streaminfo(); + let u24 = |b: &[u8]| usize::from(b[0]) << 16 | usize::from(b[1]) << 8 | usize::from(b[2]); + assert_eq!(&si[..4], &[0x10, 0x00, 0x10, 0x00]); + assert_eq!(u24(&si[4..7]), sizes.iter().copied().min().unwrap()); + assert_eq!(u24(&si[7..10]), sizes.iter().copied().max().unwrap()); +} diff --git a/crates/audio/src/write/mod.rs b/crates/audio/src/write/mod.rs index 852d9283..9492d5e1 100644 --- a/crates/audio/src/write/mod.rs +++ b/crates/audio/src/write/mod.rs @@ -26,7 +26,7 @@ use std::io::{Seek, Write}; /// # } /// ``` pub fn encode(buf: &AudioBuffer, opts: &EncodeOptions) -> Result> { - // Checked for every format; only a WAV records it. + // 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 { #[cfg(feature = "pcm-encode")]