From ed54a8cd15c51c546ca4db0209c9f9526aeba0d6 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 4 Oct 2026 12:10:09 -0400 Subject: [PATCH] Never crash: interchange export/import without expect() Removes the six expect() calls from timeline export/import and denies the panic lints in the crate. A precomp layer whose source item is not a composition (a damaged project, or an item that was deleted) made export panic in Exporter::sequence; it now leaves that layer out. Exporter::sequence and track_item return Option, the Motion/Opacity/Volume effect defaults come from effect_or_new (None if FilmCraft's registry lacks one), and import's motion() skips a layer without a transform group. Co-Authored-By: Claude Opus 5.5 --- clippy.toml | 4 +++ crates/interchange/src/export.rs | 57 ++++++++++++++++++-------------- crates/interchange/src/import.rs | 2 +- crates/interchange/src/lib.rs | 2 ++ crates/interchange/src/tests.rs | 21 ++++++++++++ 5 files changed, 60 insertions(+), 26 deletions(-) create mode 100644 clippy.toml diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 00000000..4b6c79f5 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,4 @@ +allow-unwrap-in-tests = true +allow-expect-in-tests = true +allow-panic-in-tests = true +allow-indexing-slicing-in-tests = true diff --git a/crates/interchange/src/export.rs b/crates/interchange/src/export.rs index c7e032ff..7da18913 100644 --- a/crates/interchange/src/export.rs +++ b/crates/interchange/src/export.rs @@ -254,7 +254,7 @@ pub fn export(project: &Project, comp: ItemId, opts: &ExportOptions, prerendered bins: HashMap::new(), stack: vec![], }; - let seq = ex.sequence(comp); + let seq = ex.sequence(comp).ok_or(Error::NoComp(comp.0))?; let fc_opts = filmcraft_interchange::ExportOptions { relative_to: opts.relative_to.clone(), name: Some(name), ..Default::default() }; let (bytes, report) = filmcraft_interchange::export(&ex.fp, seq, opts.format.fc(), &fc_opts)?; for e in report.entries { @@ -390,12 +390,12 @@ impl Exporter<'_> { id } - /// The sequence for a comp (created once). - fn sequence(&mut self, cid: ItemId) -> fp::ItemId { + /// The sequence for a comp (created once); None if `cid` is not a composition. + fn sequence(&mut self, cid: ItemId) -> Option { if let Some(s) = self.seqs.get(&cid) { - return *s; + return Some(*s); } - let comp = self.p.comp_arc(cid).expect("comp"); + let comp = self.p.comp_arc(cid)?; let it = self.p.item(cid); let name = it.map(|i| i.name.clone()).unwrap_or_default(); let settings = fp::SequenceSettings { @@ -430,12 +430,13 @@ impl Exporter<'_> { Plan::Render(why) => match self.pre.get(&(cid, l.id)).cloned() { Some(r) => { let item = self.prerendered_item(&format!("{} (pre-rendered)", l.name), &r); - let mut ti = self.track_item(item, &l.name, fp::TrackKind::Video, &tm, rate); - ti.source_in = filmcraft_time::Tick::ZERO; - ti.speed = 1.0; - ti.reverse = false; - self.out.prerendered += 1; - Some(ti) + self.track_item(item, &l.name, fp::TrackKind::Video, &tm, rate).map(|mut ti| { + ti.source_in = filmcraft_time::Tick::ZERO; + ti.speed = 1.0; + ti.reverse = false; + self.out.prerendered += 1; + ti + }) } None => { self.warn(format!("layer \"{}\" was left out ({why}; pre-rendering is off or unavailable)", l.name)); @@ -457,16 +458,16 @@ impl Exporter<'_> { Some(ItemKind::Solid(s)) => Some(self.solid_item(*item, s.color, &comp)), _ => None, }, - LayerSource::Comp { item } if !self.stack.contains(item) => Some(self.sequence(*item)), + LayerSource::Comp { item } if !self.stack.contains(item) => self.sequence(*item), _ => None, }; - item.map(|item| { - let mut ti = self.track_item(item, &l.name, fp::TrackKind::Video, &tm, rate); + item.and_then(|item| { + let mut ti = self.track_item(item, &l.name, fp::TrackKind::Video, &tm, rate)?; self.motion(&mut ti, l, &comp, &tm); if let Some(h) = l.props.get("timeRemap").filter(|p| !p.is_animated()) { ti.frame_hold = Some(tick_out(Tick::from_seconds_f64(h.value.as_f64()))); } - ti + Some(ti) }) } }; @@ -486,14 +487,14 @@ impl Exporter<'_> { _ => None, }, LayerSource::Comp { item } if self.p.comp(*item).is_some_and(|c| comp_has_audio(self.p, c)) && !self.stack.contains(item) => { - Some(self.sequence(*item)) + self.sequence(*item) } _ => None, }; if let Some(item) = sound && !matches!(pl, Plan::Skip(_)) + && let Some(mut ti) = self.track_item(item, &l.name, fp::TrackKind::Audio, &tm, rate) { - let mut ti = self.track_item(item, &l.name, fp::TrackKind::Audio, &tm, rate); ti.enabled = l.switches.audio; ti.link = has_video_clip.then_some(link); self.levels(&mut ti, l, &tm); @@ -529,18 +530,18 @@ impl Exporter<'_> { seq.work_area = None; } self.stack.pop(); - sid + Some(sid) } - fn track_item(&mut self, item: fp::ItemId, name: &str, kind: fp::TrackKind, tm: &Timing, rate: FrameRate) -> fp::TrackItem { + fn track_item(&mut self, item: fp::ItemId, name: &str, kind: fp::TrackKind, tm: &Timing, rate: FrameRate) -> Option { let range = filmcraft_time::TimeRange::new(tick_out(tm.source_in), tick_out(Tick((tm.duration.0 as f64 * tm.speed).round() as i64))); - let mut ti = self.fp.make_track_item(item, kind, tick_out(tm.start), range, rate_out(rate)).expect("item exists"); + let mut ti = self.fp.make_track_item(item, kind, tick_out(tm.start), range, rate_out(rate))?; ti.name = name.to_string(); ti.duration = tick_out(tm.duration); ti.source_in = tick_out(tm.source_in); ti.speed = tm.speed; ti.reverse = tm.reverse; - ti + Some(ti) } /// Transform → Motion + Opacity (clip-time keyframes). @@ -554,7 +555,7 @@ impl Exporter<'_> { _ => (comp.width, comp.height), }; let center = |x: f64, y: f64| -> fp::ParamValue { fp::ParamValue::Vec2(Vec2 { x, y }) }; - let mut m = ti.effect("motion").cloned().unwrap_or_else(|| fp::find_effect("motion").expect("motion").instance()); + let Some(mut m) = effect_or_new(ti, "motion") else { return }; if let Some(p) = tr.get("position") { let def = [comp.width as f64 / 2.0, comp.height as f64 / 2.0]; if p.is_animated() || differs(&p.value, def) { @@ -594,8 +595,9 @@ impl Exporter<'_> { m.params.insert("rotation".into(), param_out(p, &to_clip, |v| fp::ParamValue::Float(v.as_f64()))); } set_effect(ti, m); - if let Some(p) = tr.get("opacity") { - let mut o = ti.effect("opacity").cloned().unwrap_or_else(|| fp::find_effect("opacity").expect("opacity").instance()); + if let Some(p) = tr.get("opacity") + && let Some(mut o) = effect_or_new(ti, "opacity") + { o.params.insert("opacity".into(), param_out(p, &to_clip, |v| fp::ParamValue::Float(v.as_f64()))); if l.blend_mode != effectcraft_color::BlendMode::Normal && let Some(i) = fp::effect::BLEND_MODES.iter().position(|m| m.eq_ignore_ascii_case(l.blend_mode.label())) @@ -615,7 +617,7 @@ impl Exporter<'_> { if (p.value.as_vec2()[0] - p.value.as_vec2()[1]).abs() > 1e-9 { self.warn("separate left/right audio levels were exported as the left level"); } - let mut v = ti.effect("volume").cloned().unwrap_or_else(|| fp::find_effect("volume").expect("volume").instance()); + let Some(mut v) = effect_or_new(ti, "volume") else { return }; v.params.insert("level".into(), param_out(p, &to_clip, |v| fp::ParamValue::Float(v.as_vec2()[0]))); set_effect(ti, v); } @@ -634,6 +636,11 @@ fn differs(v: &Value, def: [f64; 2]) -> bool { (a[0] - def[0]).abs() > 1e-6 || (a[1] - def[1]).abs() > 1e-6 } +/// The clip's `id` effect, or a new instance of it (None if the registry has no such effect). +fn effect_or_new(ti: &fp::TrackItem, id: &str) -> Option { + ti.effect(id).cloned().or_else(|| fp::find_effect(id).map(|e| e.instance())) +} + fn set_effect(ti: &mut fp::TrackItem, e: fp::EffectInstance) { match ti.effects.iter_mut().find(|x| x.effect == e.effect) { Some(x) => *x = e, diff --git a/crates/interchange/src/import.rs b/crates/interchange/src/import.rs index 0df79f1c..bce99004 100644 --- a/crates/interchange/src/import.rs +++ b/crates/interchange/src/import.rs @@ -434,7 +434,7 @@ impl Importer<'_> { } let fit = if ti.scale_to_frame && size.0 > 0 && size.1 > 0 { (comp.width as f64 / size.0 as f64).min(comp.height as f64 / size.1 as f64) } else { 1.0 }; let lc = l.clone(); - let tr = l.transform_mut().expect("transform"); + let Some(tr) = l.transform_mut() else { return }; // Position / anchor (NaN = centred). for (src, dst) in [("position", "position"), ("anchor", "anchor")] { let Some(p) = m.param(src) else { continue }; diff --git a/crates/interchange/src/lib.rs b/crates/interchange/src/lib.rs index b9e678cf..cc57d22d 100644 --- a/crates/interchange/src/lib.rs +++ b/crates/interchange/src/lib.rs @@ -29,6 +29,8 @@ //! This crate is L3 and does no file I/O: media files are probed through a callback, pre-rendered //! media come in as paths, and documents are bytes. +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] + mod export; mod import; #[cfg(test)] diff --git a/crates/interchange/src/tests.rs b/crates/interchange/src/tests.rs index 474a4a38..375243d8 100644 --- a/crates/interchange/src/tests.rs +++ b/crates/interchange/src/tests.rs @@ -498,3 +498,24 @@ fn formats_parse() { assert_eq!(par_fraction(0.9), (9, 10)); assert_eq!(par_fraction(1.0), (1, 1)); } + +#[test] +fn export_survives_a_precomp_layer_pointing_at_a_non_comp() { + // A damaged project: the precomp layer's source is a footage item (or a deleted item). + let (mut p, cid) = ae_project(); + let footage = p.items.iter().find(|(_, i)| matches!(i.kind, ItemKind::Footage(_))).map(|(id, _)| *id).unwrap(); + for bad in [footage, ItemId(9_999)] { + let mut c = (*p.comp_arc(cid).unwrap()).clone(); + for l in &mut c.layers { + if matches!(l.source, LayerSource::Comp { .. }) { + l.source = LayerSource::Comp { item: bad }; + } + } + p.item_mut(cid).unwrap().kind = ItemKind::Comp(Arc::new(c)); + for format in [TimelineFormat::Fcp7Xml, TimelineFormat::Fcpxml, TimelineFormat::Otio] { + let out = export(&p, cid, &ExportOptions { format, prerender: PrerenderMode::None, ..Default::default() }, &HashMap::new()).unwrap(); + assert_eq!(out.sequences, 1); + } + } + assert!(matches!(export(&p, footage, &ExportOptions::default(), &HashMap::new()), Err(Error::NoComp(_)))); +}