From ffcbf1fc6119baf23c6a252d9dc58ff0e68c67df Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 15:13:25 +0000 Subject: [PATCH] address review: a frame too large to analyse is a distinct error for scene detection and auto-trace --- crates/engine/src/analysis/tracking.rs | 64 ++++++++++++++++++++++---- crates/engine/src/cmd/autotrace.rs | 12 +++-- crates/engine/src/cmd/scene_detect.rs | 20 ++++++-- 3 files changed, 79 insertions(+), 17 deletions(-) diff --git a/crates/engine/src/analysis/tracking.rs b/crates/engine/src/analysis/tracking.rs index c23e088be..16c95e382 100644 --- a/crates/engine/src/analysis/tracking.rs +++ b/crates/engine/src/analysis/tracking.rs @@ -199,9 +199,27 @@ pub(crate) struct Work { /// Most pixels a source frame is brought to for analysis (2 GiB as float RGBA). const MAX_SOURCE_PIXELS: u64 = 1 << 27; -/// The layer's source frame at comp time `t` at 100 % (layer pixels + offset = image pixels). -/// Footage at its native size is used straight from the footage source (no copy unless its -/// Cineon Settings convert it). +/// Why a layer's source frame could not be had. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum FrameError { + /// The layer has no pixels at that time. + Missing, + /// The frame at 100 % would be `width` x `height`, more than analysis holds ([`MAX_SOURCE_PIXELS`]). + TooLarge { width: u32, height: u32 }, +} + +impl std::fmt::Display for FrameError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + FrameError::Missing => f.write_str("the layer has no pixels"), + FrameError::TooLarge { width, height } => { + write!(f, "the layer's frame is {width} x {height} pixels at 100 %, more than the {MAX_SOURCE_PIXELS} pixels analysis works on") + } + } + } +} + +/// [`source_frame_checked`] without the reason: `None` when there is no frame to use. pub(crate) fn source_frame( r: &Renderer, project: &Project, @@ -211,6 +229,22 @@ pub(crate) fn source_frame( t: Tick, expr: Option<&dyn ExprHost>, ) -> Option<(Arc, [f64; 2])> { + source_frame_checked(r, project, cid, comp, layer, t, expr).ok() +} + +/// The layer's source frame at comp time `t` at 100 % (layer pixels + offset = image pixels). +/// Footage at its native size is used straight from the footage source (no copy unless its +/// Cineon Settings convert it). A frame too large to bring to 100 % is [`FrameError::TooLarge`], +/// not an absent frame. +pub(crate) fn source_frame_checked( + r: &Renderer, + project: &Project, + cid: ItemId, + comp: &Comp, + layer: &Layer, + t: Tick, + expr: Option<&dyn ExprHost>, +) -> Result<(Arc, [f64; 2]), FrameError> { let ctx = EvalCtx { project, comp_id: cid, comp, time: t, expr, footage: None, property_overrides: &[], media_overrides: &[] }; if let effectcraft_project::LayerSource::Footage { item } = &layer.source && let Some(effectcraft_project::ItemKind::Footage(f)) = project.item(*item).map(|i| &i.kind) @@ -219,9 +253,9 @@ pub(crate) fn source_frame( && img.width == f.width && img.height == f.height { - return Some((effectcraft_render::cineon_frame(img, f), [0.0; 2])); + return Ok((effectcraft_render::cineon_frame(img, f), [0.0; 2])); } - let buf = r.layer_source(&ctx, layer)?; + let buf = r.layer_source(&ctx, layer).ok_or(FrameError::Missing)?; if (buf.scale - 1.0).abs() > 1e-9 && buf.scale > 0.0 { // Footage decoded at another size: bring it to layer pixels. let w = (buf.img.width as f64 / buf.scale).round().max(1.0) as u32; @@ -229,12 +263,12 @@ pub(crate) fn source_frame( // The scale comes from the footage's saved width alone: a decoded frame of another // aspect would give a height far beyond anything the layer shows. if u64::from(w) * u64::from(h) > MAX_SOURCE_PIXELS { - return None; + return Err(FrameError::TooLarge { width: w, height: h }); } let off = [buf.offset[0] / buf.scale, buf.offset[1] / buf.scale]; - return Some((Arc::new(effectcraft_raster::resample(&buf.img, w, h)), off)); + return Ok((Arc::new(effectcraft_raster::resample(&buf.img, w, h)), off)); } - Some((Arc::new(buf.img), buf.offset)) + Ok((Arc::new(buf.img), buf.offset)) } pub(crate) async fn run_work(w: Work, shared: &TrackShared) { @@ -636,3 +670,17 @@ mod tests { } } } + +#[cfg(test)] +mod frame_error_tests { + use super::FrameError; + + #[test] + fn a_frame_too_large_to_analyse_is_told_apart_from_a_missing_one() { + let big = FrameError::TooLarge { width: 160_000, height: 160_000 }; + assert_ne!(big, FrameError::Missing); + let text = big.to_string(); + assert!(text.contains("160000 x 160000") && text.contains("analysis"), "{text}"); + assert_eq!(FrameError::Missing.to_string(), "the layer has no pixels"); + } +} diff --git a/crates/engine/src/cmd/autotrace.rs b/crates/engine/src/cmd/autotrace.rs index 8dabfd013..f7860ad97 100644 --- a/crates/engine/src/cmd/autotrace.rs +++ b/crates/engine/src/cmd/autotrace.rs @@ -112,13 +112,15 @@ fn run(s: &mut Session, p: &Value) -> Result { let r = &r; let (project, expr) = (&s.project, s.expr.as_deref()); use rayon::prelude::*; - times + let traced: std::result::Result>, String> = times .par_iter() - .map(|t| match crate::tracking::source_frame(r, project, cid, &comp, &layer, *t, expr) { - Some((img, off)) => trace_frame(&img, off, ch, invert, blur, &o), - None => vec![], + .map(|t| match crate::tracking::source_frame_checked(r, project, cid, &comp, &layer, *t, expr) { + Ok((img, off)) => Ok(trace_frame(&img, off, ch, invert, blur, &o)), + Err(crate::tracking::FrameError::Missing) => Ok(vec![]), + Err(e) => Err(format!("{e} (at {:.3} s)", t.seconds())), }) - .collect() + .collect(); + traced.map_err(|e| bad(c, e))? }; let n_masks = frames.iter().map(Vec::len).max().unwrap_or(0); if n_masks == 0 { diff --git a/crates/engine/src/cmd/scene_detect.rs b/crates/engine/src/cmd/scene_detect.rs index 07519b65b..3141f640c 100644 --- a/crates/engine/src/cmd/scene_detect.rs +++ b/crates/engine/src/cmd/scene_detect.rs @@ -31,14 +31,17 @@ impl LayerFrames { LayerFrames { project: s.project.clone(), footage: s.footage.clone(), expr: s.expr.clone(), cache: s.layer_cache.clone(), comp, layer } } /// Run `f` with a frame getter: comp time → (layer source pixels, offset in layer space). - pub fn with(&self, f: impl FnOnce(&(dyn Fn(Tick) -> Option<(Arc, [f64; 2])> + Sync)) -> R) -> Option { + pub fn with( + &self, + f: impl FnOnce(&(dyn Fn(Tick) -> std::result::Result<(Arc, [f64; 2]), crate::tracking::FrameError> + Sync)) -> R, + ) -> Option { let comp = self.project.comp(self.comp)?; let layer = comp.layer(self.layer)?; let mut r = Renderer::new(&self.project, self.footage.as_ref(), RenderOpts::default()); r.expr = self.expr.as_deref(); r.cache = Some(&self.cache); let r = &r; - let get = |t: Tick| crate::tracking::source_frame(r, &self.project, self.comp, comp, layer, t, self.expr.as_deref()); + let get = |t: Tick| crate::tracking::source_frame_checked(r, &self.project, self.comp, comp, layer, t, self.expr.as_deref()); Some(f(&get)) } } @@ -62,8 +65,17 @@ pub(crate) fn analyse( // Signatures in batches (parallel), with progress and cancel between batches. for (k, chunk) in times.chunks(8).enumerate() { use rayon::prelude::*; - let batch: Vec = chunk.par_iter().map(|t| get(*t).map(|(img, _)| cuts::signature(&img)).unwrap_or_default()).collect(); - sigs.extend(batch); + // No pixels at a time is an empty signature (as before); a frame too large to analyse + // fails the detection rather than reading as "no cut". + let batch: std::result::Result, String> = chunk + .par_iter() + .map(|t| match get(*t) { + Ok((img, _)) => Ok(cuts::signature(&img)), + Err(crate::tracking::FrameError::Missing) => Ok(cuts::Signature::default()), + Err(e) => Err(format!("{e} (at {:.3} s)", t.seconds())), + }) + .collect(); + sigs.extend(batch?); if !progress(((k + 1) * 8).min(times.len()) as u64, times.len() as u64) { return Err(crate::render_queue::CANCELLED.to_string()); }