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 <noreply@anthropic.com>
This commit is contained in:
alehelandClaude Opus 5.5 authored and Dylan M. Taylor committed 2026-11-13 21:09:46 -05:00
1 parent bdf8cb2a19
commit 7c652beec4
4 files changed
+23 -9

No files matched your search

+7 -5
View File
@@ -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
+5 -1
View File
@@ -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<Self, Error> {
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)
}
+5 -3
View File
@@ -17,10 +17,12 @@ fn ffmpeg() -> Option<std::path::PathBuf> {
/// 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<Vec<f32>> {
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();
@@ -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]