From 7c88c201a530f8e461f718cd1dc1d6d80f3d1b2f Mon Sep 17 00:00:00 2001 From: alehel Date: Tue, 3 Nov 2026 21:06:54 +0100 Subject: [PATCH] craft-audio: review fixes, round 1 (#442) - FileEncoder/encode for FLAC hand the encoder one 4096-frame block at a time: a whole-buffer encode drained the pending samples quadratically (480 s of stereo took 19 s, now 5 s) and copied the whole input twice. - Frame headers name sample rates outside the table (codes 12-14), so streams at rates like 11025 or 37800 Hz stay in the streamable subset. - The decode doctest needs symphonia to read FLAC at this point. - README: FLAC output is verified against ffmpeg, not the flac tool. - Tests: keep the AIFF force-RF64 check; exact 2^36 frame boundary. Co-Authored-By: Claude Opus 5.5 --- crates/audio/README.md | 2 +- crates/audio/src/read/mod.rs | 2 +- crates/audio/src/write/flac/frame.rs | 31 +++++++++++++++++---- crates/audio/src/write/flac/mod.rs | 28 ++++++++++++------- crates/audio/src/write/flac/oracle_tests.rs | 6 +++- crates/audio/tests/audio/roundtrip.rs | 2 ++ crates/audio/tests/audio/stream.rs | 5 ++++ 7 files changed, 57 insertions(+), 19 deletions(-) diff --git a/crates/audio/README.md b/crates/audio/README.md index b6b9a9a5..3ba2fcde 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; up to 655,350 Hz; LPC, stereo decorrelation, STREAMINFO MD5 | bit-exact through ffmpeg and the reference `flac` decoder | +| FLAC encoder | RFC 9639 | 16, 24, 32 bits; 1–8 channels; 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/read/mod.rs b/crates/audio/src/read/mod.rs index 2a62ef1e..f205e83b 100644 --- a/crates/audio/src/read/mod.rs +++ b/crates/audio/src/read/mod.rs @@ -364,7 +364,7 @@ pub fn probe_with(bytes: &[u8], ext_hint: Option<&str>, opts: &DecodeOptions) -> /// file instead, and [`decode_with`] for another track or size limit. /// /// ``` -/// # #[cfg(feature = "flac-encode")] { +/// # #[cfg(all(feature = "flac-encode", feature = "symphonia"))] { /// use craft_audio::{AudioBuffer, EncodeFormat, EncodeOptions, decode, encode}; /// /// let tone: Vec = (0..4800).map(|i| (i as f32 * 0.05).sin() * 0.5).collect(); diff --git a/crates/audio/src/write/flac/frame.rs b/crates/audio/src/write/flac/frame.rs index 6b3d28e8..aa4bafaf 100644 --- a/crates/audio/src/write/flac/frame.rs +++ b/crates/audio/src/write/flac/frame.rs @@ -27,9 +27,10 @@ fn block_size_code(n: u16) -> (u8, Option<(u32, u32)>) { } } -/// Frame header sample rate code; 0 refers to STREAMINFO. -fn sample_rate_code(rate: u32) -> u8 { - match rate { +/// Frame header sample rate code, with the value stored after the header for codes 12–14 (kHz, +/// Hz, tens of Hz); 0 refers to STREAMINFO and is left for rates none of those can name. +fn sample_rate_code(rate: u32) -> (u8, Option<(u32, u32)>) { + let code = match rate { 88_200 => 1, 176_400 => 2, 192_000 => 3, @@ -41,8 +42,12 @@ fn sample_rate_code(rate: u32) -> u8 { 44_100 => 9, 48_000 => 10, 96_000 => 11, + r if r % 1000 == 0 && r / 1000 <= 0xFF => return (12, Some((r / 1000, 8))), + r if r <= 0xFFFF => return (13, Some((r, 16))), + r if r % 10 == 0 && r / 10 <= 0xFFFF => return (14, Some((r / 10, 16))), _ => 0, - } + }; + (code, None) } /// Frame header sample size code; 0 refers to STREAMINFO. @@ -160,9 +165,11 @@ pub fn write(spec: &FrameSpec) -> Vec { Some(Stereo::MidSide) => 10, _ => spec.channels.len().saturating_sub(1) as u8, }; - let mut h = vec![0xFF, 0xF8, bs_code << 4 | sample_rate_code(spec.sample_rate), channel_code << 4 | sample_size_code(spec.bps) << 1]; + let (sr_code, sr_extra) = sample_rate_code(spec.sample_rate); + let mut h = vec![0xFF, 0xF8, bs_code << 4 | sr_code, channel_code << 4 | sample_size_code(spec.bps) << 1]; coded_number(spec.number, &mut h); - if let Some((v, n)) = bs_extra { + // the uncommon block size, then the uncommon sample rate (§9.1.6, §9.1.7) + for (v, n) in [bs_extra, sr_extra].into_iter().flatten() { if n == 8 { h.push(v as u8); } else { @@ -202,4 +209,16 @@ mod tests { assert_eq!(code(0x7FFF_FFFF), [0xFD, 0xBF, 0xBF, 0xBF, 0xBF, 0xBF]); assert_eq!(code(0xF_FFFF_FFFF), [0xFE, 0xBF, 0xBF, 0xBF, 0xBF, 0xBF, 0xBF]); } + + #[test] + fn sample_rates_outside_the_table_are_named_in_the_header() { + assert_eq!(sample_rate_code(48_000), (10, None)); + assert_eq!(sample_rate_code(64_000), (12, Some((64, 8)))); + assert_eq!(sample_rate_code(11_025), (13, Some((11_025, 16)))); + assert_eq!(sample_rate_code(37_800), (13, Some((37_800, 16)))); + assert_eq!(sample_rate_code(352_800), (14, Some((35_280, 16)))); + assert_eq!(sample_rate_code(655_350), (14, Some((65_535, 16)))); + // neither whole kHz, at most 65535 Hz, nor whole tens of Hz + assert_eq!(sample_rate_code(100_001), (0, None)); + } } diff --git a/crates/audio/src/write/flac/mod.rs b/crates/audio/src/write/flac/mod.rs index b4069474..bbe46072 100644 --- a/crates/audio/src/write/flac/mod.rs +++ b/crates/audio/src/write/flac/mod.rs @@ -36,6 +36,9 @@ 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; + fn encoder_error(e: encoder::Error) -> AudioError { AudioError::Encode(format!("FLAC: {e}")) } @@ -88,16 +91,21 @@ impl Writer { return Err(super::failed_write_error()); } let total = self.check_room(frames as u64)?; - for (c, (p, q)) in self.planar.iter_mut().zip(self.quantizers.iter_mut()).enumerate() { - let src = block.get(c).map(AsRef::as_ref).unwrap_or_default(); - p.clear(); - p.extend((0..frames).map(|i| q.quantize(src.get(i).copied().unwrap_or(0.0)))); - } - let refs: Vec<&[i32]> = self.planar.iter().map(Vec::as_slice).collect(); - let written = self.encoder.encode(&refs).map_err(encoder_error).and_then(|bytes| Ok(self.out.write_all(&bytes)?)); - if let Err(e) = written { - self.failed = true; - return Err(e); + // One FLAC block at a time: the encoder then holds less than two blocks, and the copies + // stay small however long `block` is. + for at in (0..frames).step_by(CHUNK) { + let end = frames.min(at + CHUNK); + for (c, (p, q)) in self.planar.iter_mut().zip(self.quantizers.iter_mut()).enumerate() { + let src = block.get(c).map(AsRef::as_ref).unwrap_or_default(); + p.clear(); + p.extend((at..end).map(|i| q.quantize(src.get(i).copied().unwrap_or(0.0)))); + } + let refs: Vec<&[i32]> = self.planar.iter().map(Vec::as_slice).collect(); + let written = self.encoder.encode(&refs).map_err(encoder_error).and_then(|bytes| Ok(self.out.write_all(&bytes)?)); + if let Err(e) = written { + self.failed = true; + return Err(e); + } } self.frames = total; Ok(()) diff --git a/crates/audio/src/write/flac/oracle_tests.rs b/crates/audio/src/write/flac/oracle_tests.rs index 500365df..8fa63edd 100644 --- a/crates/audio/src/write/flac/oracle_tests.rs +++ b/crates/audio/src/write/flac/oracle_tests.rs @@ -133,7 +133,7 @@ fn every_signal_round_trips_exactly() { #[test] fn channel_layouts_rates_and_sizes() { - let cases: [(&str, u8, u8, u32, usize); 8] = [ + let cases: [(&str, u8, u8, u32, usize); 12] = [ ("mono-8bit", 1, 8, 8_000, 3_001), ("mono-24-44k1", 1, 24, 44_100, 9_999), ("stereo-20bit", 2, 20, 96_000, 10_000), @@ -141,6 +141,10 @@ fn channel_layouts_rates_and_sizes() { ("mono-32", 1, 32, 48_000, 7_777), ("5.1-32", 6, 32, 96_000, 12_345), ("odd-rate", 2, 16, 37_800, 5_000), + ("rate-khz", 2, 16, 64_000, 5_000), + ("rate-tens", 2, 24, 352_800, 5_000), + ("rate-max", 1, 16, 655_350, 5_000), + ("rate-from-streaminfo", 1, 16, 100_001, 5_000), ("tiny", 2, 16, 48_000, 1), ]; for (name, ch, bps, rate, n) in cases { diff --git a/crates/audio/tests/audio/roundtrip.rs b/crates/audio/tests/audio/roundtrip.rs index 67db9880..a043a631 100644 --- a/crates/audio/tests/audio/roundtrip.rs +++ b/crates/audio/tests/audio/roundtrip.rs @@ -140,6 +140,8 @@ fn bwf_roundtrip() { .with_time_reference(0x1_2345_6789); // > 32 bits let src = sine(96_000, 2, 1000); // RF64 is a WAV layout: other formats ignore it. + let aiff = encode(&src, &EncodeOptions::new(EncodeFormat::Aiff).with_force_rf64(true)).unwrap(); + assert_eq!(&aiff[..4], b"FORM"); let flac = encode(&src, &EncodeOptions::new(EncodeFormat::Flac).with_force_rf64(true)).unwrap(); assert_eq!(&flac[..4], b"fLaC"); for depth in DEPTHS { diff --git a/crates/audio/tests/audio/stream.rs b/crates/audio/tests/audio/stream.rs index b3271b40..bfab2fbd 100644 --- a/crates/audio/tests/audio/stream.rs +++ b/crates/audio/tests/audio/stream.rs @@ -108,6 +108,11 @@ fn blocks_past_a_format_limit_are_refused_before_anything_is_written() { enc.write(&buf.channels, 100).unwrap(); assert!(enc.check_room(limit / 2).is_ok(), "{format:?}"); assert!(matches!(enc.check_room(limit), Err(AudioError::TooLarge(_))), "{format:?}"); + if format == EncodeFormat::Flac { + // 100 frames are in: STREAMINFO counts up to 2^36 - 1 in all. + assert!(enc.check_room(limit - 101).is_ok()); + assert!(matches!(enc.check_room(limit - 100), Err(AudioError::TooLarge(_)))); + } // A block that does not hold the frames it claims is refused before the limit is checked. if let Ok(frames) = usize::try_from(limit) { assert!(matches!(enc.write::>(&[], frames), Err(AudioError::Encode(_))), "{format:?}");