collective
Browse / github.com/storytold/filmcraft / patch
#2798: contain Symphonia audio decoder panics
#2798: contain Symphonia audio decoder panics Symphonia panics during audio decoding now return `CodecError::Decode` instead of unwinding into callers. A packet decoder that panics stays disabled across reset; reopening the media creates a fresh decoder. Standalone imports fail even if earlier packets decoded, so a panic cannot produce a successful partial source. Ordinary corrupt-packet handling is preserved. Refs #2798. This covers the Symphonia audio portion only; H.264/HEVC worker containment remains separate, as requested in the issue discussion. No dependencies, codec implementations, assets or public API signatures change. ### Va
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- thecyberbutler (imported from pull request #3338, open)
- Base
320cae243d4ae85e5357eee5144e131c3ada3371- Size
- +306 −29 in 3 files
- Review
- pending
- Shared
- 2026-10-28
- Id
5a9e306cb0d42ecadc9657e3
How it went
No other operator has reported using it yet.
Diff
raw · crates/codecs/README.md crates/codecs/src/audio.rs crates/codecs/src/audio_panic_tests.rs
new file mode 100644@@ -0,0 +1,48 @@+# filmcraft-codecs++The L2 container and codec hub connects media sources to FilmCraft's software and registered+hardware decoders. It provides container openers, GOP-aware video seeking, packet audio decoding+and standalone audio imports. See the crate-level API documentation in `src/lib.rs` and+[`docs/architecture.md`](../../docs/architecture.md) for the source and decoder interfaces.++## Symphonia audio failure handling (#2798)++Symphonia remains an unmodified MPL-2.0 dependency. Its decoder construction, packet decoding,+sample conversion and reset calls are guarded against unwinding. On targets built with+`panic = "unwind"`, a panic becomes a `CodecError::Decode` asking the caller to reopen the media.+A packet decoder that panicked is quarantined: later decode calls fail, and reset never calls+into its potentially damaged state.+Because `PacketDecoder::reset` has no error return, a panic during reset is logged and reported+by the next decode. A newly constructed decoder can be used to retry.++Standalone imports guard the whole attempt, including format probing, packet reading and+decoder construction. A decoder panic aborts the import even if earlier packets decoded;+partially decoded samples are not returned as a successful source. Ordinary returned errors+retain the existing behavior: packet decoding returns an error without quarantining the+decoder, and standalone imports skip corrupt packets. This change does not change that+pre-existing damaged-file policy or harden the separate H.264/HEVC worker paths in #2798.++The default `wasm32-unknown-unknown` target uses `panic = "abort"`, so decoder panics cannot be+caught or converted to `CodecError::Decode` there. The guard handles Rust unwinding; it cannot+recover from a process abort or an allocation failure that aborts. It does not replace input+validation or repair bugs inside a dependency.++## Tests++`src/audio_panic_tests.rs` wraps a real PCM decoder through Symphonia's public `Decoder` API+with per-instance synthetic decode/reset failures. No dependency implementation is copied.+The tests check errors after a successful packet, no reuse after panic (including reset),+fresh-decoder recovery, ordinary-error recovery, constructor failure, and rejection of partial+standalone imports. WAV truncations and header mutations run under `catch_unwind`; all fixtures+are generated in memory. Existing MP3, AIFF, FLAC and Vorbis tests cover the normal+audio paths with FFmpeg used only as an external fixture generator/oracle.++```sh+cargo test -p filmcraft-codecs --lib audio::panic_tests+FILMCRAFT_REQUIRE_ORACLES=1 cargo test -p filmcraft-codecs+cargo xtask ci+```++Validation on macOS: all seven focused tests pass; disabling the guards makes the four+panic regressions fail. `FILMCRAFT_REQUIRE_ORACLES=1 cargo test -p filmcraft-codecs` passes+204 tests (three existing ignored tests), including the existing audio oracle coverage.@@ -14,6 +14,69 @@ use symphonia::core::probe::Hint; use crate::{CodecError, Result}; +#[cfg(test)]+#[path = "audio_panic_tests.rs"]+mod panic_tests;++/// A decoder that unwound may have partially changed its state. Keep it quarantined, including+/// across seeks, until its owner creates a fresh decoder. Ordinary decode errors remain retryable.+struct SymphoniaDecoder {+ decoder: Box<dyn Decoder>,+ failed: bool,+}++fn audio_panic(operation: &str) -> CodecError {+ CodecError::Decode(format!("audio decoder panicked while {operation}; reopen the media to retry"))+}++/// Converts decoder panics to `CodecError::Decode` only when built with `panic = "unwind"`.+/// The default `wasm32-unknown-unknown` target uses `panic = "abort"`, so decoder panics cannot+/// be caught or converted. Process aborts and allocation failures that abort remain unrecoverable.+fn guard_audio<T>(operation: &str, f: impl FnOnce() -> Result<T>) -> Result<T> {+ std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)).map_err(|_| audio_panic(operation))?+}++impl SymphoniaDecoder {+ fn new(params: &CodecParameters) -> Result<Self> {+ guard_audio("initializing Symphonia", || {+ let decoder = symphonia::default::get_codecs().make(params, &DecoderOptions::default()).map_err(|e| CodecError::Unsupported(e.to_string()))?;+ Ok(Self { decoder, failed: false })+ })+ }++ fn call<T>(&mut self, operation: &str, f: impl FnOnce(&mut dyn Decoder) -> T) -> Result<T> {+ if self.failed {+ return Err(CodecError::Decode("audio decoder is unavailable after a panic; reopen the media to retry".into()));+ }+ match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(self.decoder.as_mut()))) {+ Ok(result) => Ok(result),+ Err(_) => {+ self.failed = true;+ Err(audio_panic(operation))+ }+ }+ }++ // Keep the two errors distinct: standalone imports skip ordinary corrupt packets, but must+ // stop on a panic rather than report partially decoded audio as a successful import.+ fn decode(&mut self, packet: &Packet) -> Result<symphonia::core::errors::Result<Vec<Vec<f32>>>> {+ self.call("decoding a Symphonia packet", |decoder| {+ let buf = decoder.decode(packet)?;+ let spec = *buf.spec();+ let ch = spec.channels.count().max(1);+ let mut sb = SampleBuffer::<f32>::new(buf.capacity() as u64, spec);+ sb.copy_interleaved_ref(buf);+ let mut out = vec![Vec::with_capacity(sb.samples().len() / ch); ch];+ for frame in sb.samples().chunks_exact(ch) {+ for (channel, sample) in out.iter_mut().zip(frame) {+ channel.push(*sample);+ }+ }+ Ok(out)+ })+ }+}+ enum Inner { /// Our own AAC-LC decoder. `up` interpolates HE-AAC's core to the output rate (see [`Upsample2x`]). /// Surround output is reordered to WAV/SMPTE order (see [`aac_to_wav_order`]).@@ -27,7 +90,7 @@ enum Inner { /// `order` maps output channel → decoded channel (Vorbis → WAV/SMPTE order for surround). Opus { dec: Box<filmcraft_opus::Decoder>, order: Option<&'static [usize]> }, /// Bootstrap decoders (MP3, ALAC, FLAC, HE-AAC…) via symphonia.- Symphonia(Box<dyn Decoder>),+ Symphonia(SymphoniaDecoder), /// Our AC-3 and E-AC-3 decoder (ATSC A/52, Annex E). Ac3(Box<filmcraft_ac3::Decoder>), /// Our MPEG-1/2 audio Layer I and II decoder (ISO/IEC 11172-3, 13818-3).@@ -111,7 +174,7 @@ impl PacketDecoder { if let Some(x) = extra { p.with_extra_data(x.into_boxed_slice()); }- let dec = symphonia::default::get_codecs().make(&p, &DecoderOptions::default()).map_err(|e| CodecError::Unsupported(e.to_string()))?;+ let dec = SymphoniaDecoder::new(&p)?; Ok(Self { inner: Inner::Symphonia(dec), channels: 0 }) } /// AAC: our decoder for AAC-LC and the AAC-LC core of HE-AAC (v1/v2); symphonia for other@@ -262,20 +325,8 @@ impl PacketDecoder { } }; let pkt = Packet::new_from_slice(0, ts, 0, data);- let buf = dec.decode(&pkt).map_err(|e| CodecError::Decode(e.to_string()))?;- let spec = *buf.spec();- let ch = spec.channels.count().max(1);- self.channels = ch;- let mut sb = SampleBuffer::<f32>::new(buf.capacity() as u64, spec);- sb.copy_interleaved_ref(buf);- let s = sb.samples();- let n = s.len() / ch;- let mut out = vec![Vec::with_capacity(n); ch];- for i in 0..n {- for (c, o) in out.iter_mut().enumerate() {- o.push(s[i * ch + c]);- }- }+ let out = dec.decode(&pkt)?.map_err(|e| CodecError::Decode(e.to_string()))?;+ self.channels = out.len(); Ok(out) } pub fn reset(&mut self) {@@ -290,7 +341,15 @@ impl PacketDecoder { } Inner::Mpa(d) => d.reset(), Inner::Opus { dec, .. } => dec.reset(),- Inner::Symphonia(d) => d.reset(),+ Inner::Symphonia(d) => {+ // The legacy reset API cannot return an error. Log it now and keep the failure+ // latched so the next decode reports it to the caller as well.+ if !d.failed+ && let Err(error) = d.call("resetting Symphonia", |decoder| decoder.reset())+ {+ log::error!("{error}");+ }+ } Inner::Ac3(d) => d.reset(), Inner::AacPending | Inner::MpegPending(_) => {} }@@ -797,6 +856,16 @@ pub struct AudioFileSource { impl AudioFileSource { pub fn decode(name: &str, bytes: Arc<[u8]>) -> Result<Self> {+ Self::decode_with(name, bytes, SymphoniaDecoder::new)+ }++ fn decode_with(name: &str, bytes: Arc<[u8]>, make_decoder: impl FnOnce(&CodecParameters) -> Result<SymphoniaDecoder>) -> Result<Self> {+ // Probing, demuxing, decoder construction and sample conversion can all see hostile input.+ // Everything owned by this attempt is discarded if it unwinds; no partial source escapes.+ guard_audio(&format!("opening {name}"), || Self::decode_inner(name, bytes, make_decoder))+ }++ fn decode_inner(name: &str, bytes: Arc<[u8]>, make_decoder: impl FnOnce(&CodecParameters) -> Result<SymphoniaDecoder>) -> Result<Self> { let ext = name.rsplit('.').next().unwrap_or("").to_ascii_lowercase(); if matches!(ext.as_str(), "mp2" | "mp3") && let Some((layer, rate, samples)) = decode_mpeg_layer12(&bytes)@@ -840,25 +909,18 @@ impl AudioFileSource { let mut format = probed.format; let track = format.default_track().ok_or_else(|| CodecError::Unsupported("no audio track".into()))?.clone(); let rate = track.codec_params.sample_rate.unwrap_or(48_000);- let mut dec =- symphonia::default::get_codecs().make(&track.codec_params, &DecoderOptions::default()).map_err(|e| CodecError::Unsupported(e.to_string()))?;+ let mut dec = make_decoder(&track.codec_params)?; let mut samples: Vec<Vec<f32>> = Vec::new(); while let Ok(pkt) = format.next_packet() { if pkt.track_id() != track.id { continue; }- let Ok(buf) = dec.decode(&pkt) else { continue };- let spec = *buf.spec();- let ch = spec.channels.count().max(1);+ let Ok(channels) = dec.decode(&pkt)? else { continue }; if samples.is_empty() {- samples = vec![Vec::new(); ch];+ samples = vec![Vec::new(); channels.len()]; }- let mut sb = SampleBuffer::<f32>::new(buf.capacity() as u64, spec);- sb.copy_interleaved_ref(buf);- for (i, v) in sb.samples().iter().enumerate() {- if let Some(c) = samples.get_mut(i % ch) {- c.push(*v);- }+ for (out, channel) in samples.iter_mut().zip(channels) {+ out.extend(channel); } } if samples.is_empty() {new file mode 100644@@ -0,0 +1,167 @@+//! Synthetic failures at the third-party API boundary; no codec implementation is copied.+use super::*;+use std::sync::atomic::{AtomicUsize, Ordering};+use symphonia::core::audio::{AudioBufferRef, Channels};+use symphonia::core::codecs::{CODEC_TYPE_PCM_S16LE, CodecDescriptor, FinalizeResult};+use symphonia::core::errors::{Error as SymphoniaError, Result as SymphoniaResult};++struct FaultDecoder {+ inner: Box<dyn Decoder>,+ calls: Arc<AtomicUsize>,+ resets: Arc<AtomicUsize>,+ panic_at: usize,+ error_at: usize,+ panic_reset: bool,+}++impl Decoder for FaultDecoder {+ fn try_new(_: &CodecParameters, _: &DecoderOptions) -> SymphoniaResult<Self> {+ Err(SymphoniaError::Unsupported("test decoder is constructed explicitly"))+ }++ fn supported_codecs() -> &'static [CodecDescriptor] {+ &[]+ }++ fn codec_params(&self) -> &CodecParameters {+ self.inner.codec_params()+ }++ fn reset(&mut self) {+ self.resets.fetch_add(1, Ordering::SeqCst);+ assert!(!self.panic_reset, "synthetic decoder reset panic");+ self.inner.reset();+ }++ fn decode(&mut self, packet: &Packet) -> SymphoniaResult<AudioBufferRef<'_>> {+ let call = self.calls.fetch_add(1, Ordering::SeqCst) + 1;+ assert_ne!(call, self.panic_at, "synthetic decoder panic after state mutation");+ if call == self.error_at {+ return Err(SymphoniaError::DecodeError("synthetic corrupt packet"));+ }+ self.inner.decode(packet)+ }++ fn finalize(&mut self) -> FinalizeResult {+ self.inner.finalize()+ }++ fn last_decoded(&self) -> AudioBufferRef<'_> {+ self.inner.last_decoded()+ }+}++fn injected(params: &CodecParameters, panic_at: usize, error_at: usize, panic_reset: bool) -> (SymphoniaDecoder, Arc<AtomicUsize>, Arc<AtomicUsize>) {+ let calls = Arc::new(AtomicUsize::new(0));+ let resets = Arc::new(AtomicUsize::new(0));+ let inner = symphonia::default::get_codecs().make(params, &DecoderOptions::default()).unwrap();+ let decoder = Box::new(FaultDecoder { inner, calls: calls.clone(), resets: resets.clone(), panic_at, error_at, panic_reset });+ (SymphoniaDecoder { decoder, failed: false }, calls, resets)+}++fn pcm_params() -> CodecParameters {+ let mut params = CodecParameters::new();+ params+ .for_codec(CODEC_TYPE_PCM_S16LE)+ .with_sample_rate(48_000)+ .with_channels(Channels::FRONT_LEFT | Channels::FRONT_RIGHT)+ .with_bits_per_sample(16)+ .with_max_frames_per_packet(1024);+ params+}++fn assert_panic_error<T>(result: Result<T>) {+ match result {+ Err(CodecError::Decode(message)) => {+ assert!(message.contains("panic"), "{message}");+ assert!(message.contains("reopen"), "{message}");+ }+ Err(other) => panic!("unexpected error: {other}"),+ Ok(_) => panic!("a decoder panic must not produce successful audio"),+ }+}++#[test]+fn packet_panic_is_an_error_and_decoder_is_not_reused() {+ let (inner, calls, resets) = injected(&pcm_params(), 2, 0, false);+ let mut decoder = PacketDecoder { inner: Inner::Symphonia(inner), channels: 0 };+ let pcm = [0x00, 0x20, 0x00, 0xc0]; // left +0.25, right -0.5+ assert_eq!(decoder.decode(&pcm, 0).unwrap(), vec![vec![0.25], vec![-0.5]]);+ let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| decoder.decode(&pcm, 1)));+ assert_panic_error(outcome.expect("packet decoder panic escaped"));+ decoder.reset();+ assert_panic_error(decoder.decode(&pcm, 2));+ assert_eq!(calls.load(Ordering::SeqCst), 2, "damaged decoder must never be called again");+ assert_eq!(resets.load(Ordering::SeqCst), 0, "reset must not touch damaged state");+ assert_eq!(decoder.channels, 2);+ let mut fresh = PacketDecoder { inner: Inner::Symphonia(SymphoniaDecoder::new(&pcm_params()).unwrap()), channels: 0 };+ assert_eq!(fresh.decode(&pcm, 0).unwrap(), vec![vec![0.25], vec![-0.5]]);+}++#[test]+fn reset_panic_is_contained_and_reported_on_next_decode() {+ let (inner, calls, resets) = injected(&pcm_params(), 0, 0, true);+ let mut decoder = PacketDecoder { inner: Inner::Symphonia(inner), channels: 0 };+ decoder.reset();+ decoder.reset();+ assert_panic_error(decoder.decode(&[0; 4], 0));+ assert_eq!(calls.load(Ordering::SeqCst), 0);+ assert_eq!(resets.load(Ordering::SeqCst), 1);+}++#[test]+fn ordinary_packet_error_does_not_quarantine_decoder() {+ let (inner, calls, resets) = injected(&pcm_params(), 0, 1, false);+ let mut decoder = PacketDecoder { inner: Inner::Symphonia(inner), channels: 0 };+ let expected = SymphoniaError::DecodeError("synthetic corrupt packet").to_string();+ assert!(matches!(decoder.decode(&[0; 4], 0), Err(CodecError::Decode(message)) if message == expected));+ decoder.reset();+ assert_eq!(decoder.decode(&[0; 4], 1).unwrap(), vec![vec![0.0], vec![0.0]]);+ assert_eq!(calls.load(Ordering::SeqCst), 2);+ assert_eq!(resets.load(Ordering::SeqCst), 1);+}++fn wav() -> Arc<[u8]> {+ filmcraft_media::wav::write_wav16(&[0.25; 32_768], 2, 48_000).into()+}++#[test]+fn standalone_panic_after_good_audio_rejects_partial_source() {+ let result = AudioFileSource::decode_with("test.wav", wav(), |params| Ok(injected(params, 2, 0, false).0));+ assert_panic_error(result);+ let source = AudioFileSource::decode("test.wav", wav()).unwrap();+ assert_eq!(source.samples.len(), 2);+ assert_eq!(source.samples[0].len(), 16_384);+ assert!(source.samples.iter().flatten().all(|sample| (*sample - 0.25).abs() < 1e-4));+}++#[test]+fn standalone_constructor_panic_is_an_error() {+ assert_panic_error(AudioFileSource::decode_with("test.wav", wav(), |_| panic!("synthetic constructor panic")));+}++#[test]+fn standalone_ordinary_corrupt_packet_keeps_existing_recovery() {+ let source = AudioFileSource::decode_with("test.wav", wav(), |params| Ok(injected(params, 0, 2, false).0)).unwrap();+ assert!(!source.samples[0].is_empty());+ assert!(source.samples[0].len() < 16_384);+ assert_eq!(source.samples[0].len(), source.samples[1].len());+}++#[test]+fn standalone_mutated_headers_never_unwind() {+ let seed = filmcraft_media::wav::write_wav16(&[0.25; 64], 2, 48_000);+ let check = |bytes: Vec<u8>| {+ assert!(std::panic::catch_unwind(|| AudioFileSource::decode("hostile.wav", bytes.into())).is_ok());+ };+ for end in 0..seed.len() {+ check(seed[..end].to_vec());+ }+ for offset in 0..44 {+ for mask in [1, 0x80, 0xff] {+ let mut bytes = seed.clone();+ bytes[offset] ^= mask;+ check(bytes);+ }+ }+}