From 7c652beec4291d73db82c3a9163c2dd21b986cf5 Mon Sep 17 00:00:00 2001 From: alehel Date: Wed, 11 Nov 2026 23:41:20 +0100 Subject: [PATCH] craft-audio: review fixes, round 1 (#446) - `Encoder::with_application` checks the channel count before clamping the bitrate, so 86 channels or more return `Error::Channels` instead of panicking in `clamp`. Tested with 86 and 255. - The ffmpeg oracle test skips a missing `opus`/`libopus` decoder through the testkit's `skip`, so `CRAFT_MEDIA_REQUIRE_ORACLES=1` fails it. - README: the encode example pads at least the pre-skip after the audio before rounding up to a frame, so the encoder delay is flushed; the MP4 note says an explicit edit replaces the `media_start` edit and gives its fields. Co-Authored-By: Claude Opus 5.5 --- crates/audio/README.md | 12 +++++++----- crates/audio/src/opus/encoder/mod.rs | 6 +++++- crates/audio/tests/codecs/opusenc/ffmpeg.rs | 8 +++++--- crates/audio/tests/codecs/opusenc/modes.rs | 6 ++++++ 4 files changed, 23 insertions(+), 9 deletions(-) diff --git a/crates/audio/README.md b/crates/audio/README.md index 57f17054a..518dbeb4c 100644 --- a/crates/audio/README.md +++ b/crates/audio/README.md @@ -273,9 +273,9 @@ use craft_audio::opus::Encoder; let mut enc = Encoder::new(2, 128_000)?; // channels, bits per second let frame_len = Encoder::FRAME_SIZE * 2; // 20 ms, interleaved stereo let mut samples = buf.interleaved(); // 48 kHz audio -// The last frame is padded with silence. -let padding = (frame_len - samples.len() % frame_len) % frame_len; -samples.resize(samples.len() + padding, 0.0); +// Silence after the audio flushes the encoder's delay (the pre-skip) and fills the last frame. +let frames = samples.len() / 2 + enc.pre_skip() as usize; +samples.resize(frames.div_ceil(Encoder::FRAME_SIZE) * frame_len, 0.0); let mut packets = Vec::new(); for frame in samples.chunks_exact(frame_len) { packets.push(enc.encode_float(frame)?); @@ -294,8 +294,10 @@ Matroska/WebM or MP4: `DiscardPadding`, so the padding at the end stays in the file. - **MP4:** the sample entry takes an `isobmff::OpusConfig` (`dOps`) with the same fields as the `OpusHead`: channels, pre-skip, input rate, gain and mapping family. `TrackConfig::media_start` - set to the pre-skip skips it; an explicit `TrackConfig::edits` with the audio's length also - drops the padding. + set to the pre-skip skips it. To drop the padding as well, give `TrackConfig::edits` one + `Edit` instead, which replaces the edit `media_start` makes: `media_time` the pre-skip (track + timescale), `segment_duration` the audio's length (movie timescale), `media_rate` + `0x0001_0000`. ### Waveform overviews diff --git a/crates/audio/src/opus/encoder/mod.rs b/crates/audio/src/opus/encoder/mod.rs index da29b3f62..cbb0d7984 100644 --- a/crates/audio/src/opus/encoder/mod.rs +++ b/crates/audio/src/opus/encoder/mod.rs @@ -178,7 +178,11 @@ impl Encoder { /// (total for all channels; clamped to 6 kb/s per channel – 510 kb/s), choosing the mode /// and bandwidth for `application`. pub fn with_application(channels: u8, bitrate_bps: u32, application: Application) -> Result { - let c = channels.max(1) as u32; + // Checked before the clamp below, whose lower bound passes its upper one from 86 channels. + if channels != 1 && channels != 2 { + return Err(Error::Channels(channels)); + } + let c = channels as u32; let (mode, bandwidth) = choose_mode(application, bitrate_bps.clamp(6_000 * c, 510_000) / c); Self::build(channels, bitrate_bps, application, mode, bandwidth) } diff --git a/crates/audio/tests/codecs/opusenc/ffmpeg.rs b/crates/audio/tests/codecs/opusenc/ffmpeg.rs index b428582c1..41965be20 100644 --- a/crates/audio/tests/codecs/opusenc/ffmpeg.rs +++ b/crates/audio/tests/codecs/opusenc/ffmpeg.rs @@ -17,10 +17,12 @@ fn ffmpeg() -> Option { /// Decodes an Ogg Opus file with ffmpeg's decoder `codec`; returns interleaved f32 output. fn ffmpeg_decode(exe: &std::path::Path, codec: &str, file: &[u8], channels: usize, tag: &str) -> Option> { - let has = Command::new(exe).args(["-hide_banner", "-decoders"]).output().ok()?; + let has = match Command::new(exe).args(["-hide_banner", "-decoders"]).output() { + Ok(out) => out, + Err(e) => return crate::common::skip(&format!("ffmpeg -decoders failed: {e}")), + }; if !String::from_utf8_lossy(&has.stdout).lines().any(|l| l.split_whitespace().nth(1) == Some(codec)) { - eprintln!("ffmpeg has no {codec} decoder; skipping it"); - return None; + return crate::common::skip(&format!("ffmpeg has no {codec} decoder")); } let path = std::env::temp_dir().join(format!("effectcraft-opusenc-{}-{tag}.opus", std::process::id())); std::fs::write(&path, file).unwrap(); diff --git a/crates/audio/tests/codecs/opusenc/modes.rs b/crates/audio/tests/codecs/opusenc/modes.rs index 7060173d4..263858560 100644 --- a/crates/audio/tests/codecs/opusenc/modes.rs +++ b/crates/audio/tests/codecs/opusenc/modes.rs @@ -235,6 +235,12 @@ fn invalid_mode_bandwidth_is_an_error() { assert_eq!(OpusEncoder::with_mode(1, 16_000, Mode::Silk, Bandwidth::Fullband).err(), Some(Error::ModeBandwidth(Mode::Silk, Bandwidth::Fullband))); assert_eq!(OpusEncoder::new(3, 128_000).err(), Some(Error::Channels(3))); assert_eq!(OpusEncoder::new(0, 128_000).err(), Some(Error::Channels(0))); + // From 86 channels the per-channel minimum bitrate passes the 510 kb/s cap. + for channels in [86, u8::MAX] { + assert_eq!(OpusEncoder::new(channels, 128_000).err(), Some(Error::Channels(channels))); + let app = OpusEncoder::with_application(channels, 128_000, Application::Voip).err(); + assert_eq!(app, Some(Error::Channels(channels))); + } } #[test]