From baaec8b0a591d062198e1eb9d5da38670d78d5ea Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 15:28:57 +0000 Subject: [PATCH] address review: shared counted selector budget, 30 s preroll, cache hardening (shared temp roots, matte discard, exact LZ4 length, render version 7), orient nesting guard --- crates/render/src/audio.rs | 2 +- crates/render/src/disk_cache.rs | 45 +++++++++++++++++++++------ crates/render/src/text.rs | 47 +++++++++++++++++++++++------ crates/render/src/text/tests.rs | 9 ++++++ crates/render/src/three_d/camera.rs | 11 ++++++- 5 files changed, 93 insertions(+), 21 deletions(-) diff --git a/crates/render/src/audio.rs b/crates/render/src/audio.rs index 7c6656f63..2c1538133 100644 --- a/crates/render/src/audio.rs +++ b/crates/render/src/audio.rs @@ -28,7 +28,7 @@ const MAX_DEPTH: usize = 16; const MAX_SPEED: f64 = 100.0; /// The most effect preroll (history before a block) mixed, in seconds, across a layer's effects. -const MAX_PREROLL_SECONDS: f64 = 10.0; +const MAX_PREROLL_SECONDS: f64 = 30.0; /// The longest footage a waveform summarises (a day). const MAX_PEAK_SECONDS: i64 = 24 * 3600; diff --git a/crates/render/src/disk_cache.rs b/crates/render/src/disk_cache.rs index ffe236a99..3674b2667 100644 --- a/crates/render/src/disk_cache.rs +++ b/crates/render/src/disk_cache.rs @@ -496,17 +496,23 @@ fn make_private(folder: &Path) -> std::io::Result<()> { use std::os::unix::fs::PermissionsExt; // Judge the folder as written and as it resolves (a relative path, a link or an alias such as // /tmp -> /private/tmp can all land in the shared temp folder). - let tmp = std::env::temp_dir(); - let tmp_real = tmp.canonicalize().unwrap_or_else(|_| tmp.clone()); + // The shared roots: the process's temp folder (`TMPDIR`) and the system's own, which a cache + // configured by path lands in whatever `TMPDIR` says. + let roots: Vec = [std::env::temp_dir(), PathBuf::from("/tmp"), PathBuf::from("/var/tmp")] + .into_iter() + .flat_map(|t| { + let real = t.canonicalize().unwrap_or_else(|_| t.clone()); + [t, real] + }) + .collect(); let given = std::path::absolute(folder).unwrap_or_else(|_| folder.to_path_buf()); let real = folder.canonicalize().unwrap_or_else(|_| given.clone()); // The shared folder itself is not ours to change or to keep a cache in: refused. - if [&given, &real].iter().any(|p| **p == tmp || **p == tmp_real) { + if [&given, &real].iter().any(|p| roots.contains(p)) { return Err(std::io::Error::new(std::io::ErrorKind::PermissionDenied, "the disk cache folder is the shared temp folder")); } // Only folders inside it are made private. - let inside = |p: &PathBuf, t: &PathBuf| p.starts_with(t) && p != t; - if ![&given, &real].iter().any(|p| inside(p, &tmp) || inside(p, &tmp_real)) { + if ![&given, &real].iter().any(|p| roots.iter().any(|t| p.starts_with(t) && **p != *t)) { return Ok(()); } if std::fs::symlink_metadata(folder)?.file_type().is_symlink() { @@ -1203,8 +1209,12 @@ impl DiskCache { /// A segmentation (chain key, scale) written by [`DiskCache::put_seg`]. pub fn get_seg(&self, key: u64, scale: f64) -> Option { let p = self.read(Kind::Matte, seg_key(key, scale))?; - let raw = decompress_checked(&p, None)?; - effectcraft_effects::roto::FrameSeg::from_bytes(&raw) + let seg = decompress_checked(&p, None).and_then(|raw| effectcraft_effects::roto::FrameSeg::from_bytes(&raw)); + if seg.is_none() { + // A rejected entry would otherwise stay indexed and keep `put_seg` from replacing it. + self.discard(Kind::Matte, seg_key(key, scale)); + } + seg } /// Queue a segmentation for writing. @@ -1455,7 +1465,8 @@ fn decompress_checked(p: &[u8], expected: Option) -> Option> { if size > MAX_ENTRY_BYTES || expected.is_some_and(|e| e != size) { return None; } - lz4_flex::decompress(p.get(4..)?, size).ok() + // A block that ends short of the size it states is damaged: callers index by that size. + lz4_flex::decompress(p.get(4..)?, size).ok().filter(|raw| raw.len() == size) } fn decode_frame(p: &[u8]) -> Option { @@ -1565,7 +1576,7 @@ fn decode_layer(p: &[u8]) -> Option { // ---------------------------------------------------------------- content keys /// Bump when rendering changes in a way that makes old disk entries wrong. -const RENDER_VERSION: &str = "effectcraft-render-6"; +const RENDER_VERSION: &str = "effectcraft-render-7"; /// Feeds the `Debug` form of `v` into the hash, followed by a separator byte. fn hash_debug(h: &mut Hash128, v: &impl std::fmt::Debug) { @@ -1685,7 +1696,7 @@ mod tests { let dc = DiskCache::open(&d, 1 << 20).unwrap(); assert_eq!(std::fs::metadata(&d).unwrap().permissions().mode() & 0o777, 0o700); drop(dc); - let link = tmpdir("link"); + let link = tmpdir("link-private-temp"); std::os::unix::fs::symlink(&d, &link).unwrap(); assert!(DiskCache::open(&link, 1 << 20).is_err()); let _ = std::fs::remove_file(&link); @@ -1739,6 +1750,20 @@ mod tests { assert_eq!(decode_frame(&ok).map(|f| f.rgba), Some(vec![7; 16])); } + #[test] + fn a_block_shorter_than_the_size_it_states_is_rejected() { + // The payload states 100 bytes but holds a valid block of 10: `lz4_flex` returns the 10. + let mut p = 100u32.to_le_bytes().to_vec(); + p.extend_from_slice(&lz4_flex::block::compress(&[5u8; 10])); + assert!(decompress_checked(&p, None).is_none()); + // A half-float frame whose planes come up short is refused, not indexed past. + let mut f16 = 2u32.to_le_bytes().to_vec(); + f16.extend_from_slice(&2u32.to_le_bytes()); + f16.extend_from_slice(&32u32.to_le_bytes()); + f16.extend_from_slice(&lz4_flex::block::compress(&[1u8; 8])); + assert!(decode_frame_f16(&f16).is_none()); + } + fn tmpdir(name: &str) -> PathBuf { let d = std::env::temp_dir().join(format!("effectcraft-disk-cache-{name}-{}", std::process::id())); let _ = std::fs::remove_dir_all(&d); diff --git a/crates/render/src/text.rs b/crates/render/src/text.rs index 5257597a9..eef5b4067 100644 --- a/crates/render/src/text.rs +++ b/crates/render/src/text.rs @@ -198,6 +198,35 @@ pub fn in_index_units(layer: &Layer, uid: effectcraft_project::Uid) -> bool { /// Selection of every glyph by an animator's selectors, per dimension. pub fn animator_selection(ctx: &EvalCtx, layer: &Layer, anim: &PropGroup, lay: &TextLayout) -> Vec<[f64; 3]> { + animator_selection_within(ctx, layer, anim, lay, &SelectorBudget::new()) +} + +/// The Expression Selector evaluations one layer's animators may run in all: counted, not timed, +/// so the same frame renders the same on every machine. +pub struct SelectorBudget(std::cell::Cell); + +impl SelectorBudget { + pub fn new() -> SelectorBudget { + SelectorBudget(std::cell::Cell::new(MAX_SELECTOR_EVALS)) + } + + /// Take one evaluation; `false` when none are left. + fn take(&self) -> bool { + let left = self.0.get(); + self.0.set(left.saturating_sub(1)); + left > 0 + } +} + +impl Default for SelectorBudget { + fn default() -> Self { + Self::new() + } +} + +/// [`animator_selection`] drawing its Expression Selector evaluations from `budget`, which the +/// caller shares across the layer's animators. +pub fn animator_selection_within(ctx: &EvalCtx, layer: &Layer, anim: &PropGroup, lay: &TextLayout, budget: &SelectorBudget) -> Vec<[f64; 3]> { let sels: Vec<&PropGroup> = anim.sub("selectors").map(|s| s.groups().filter(|g| g.enabled).collect()).unwrap_or_default(); if sels.is_empty() { // No selectors: every character is fully affected. @@ -262,17 +291,15 @@ pub fn animator_selection(ctx: &EvalCtx, layer: &Layer, anim: &PropGroup, lay: & ctx.value(layer, prop) } .as_vec3(); - // One allowance for the whole layer: the expression runs per character, each run - // with its own time limit, so a slow expression on a long text is stopped once it - // has failed or the layer's evaluations pass MAX_SELECTOR_TIME. - let start = web_time::Instant::now(); + // One allowance for the whole layer (`budget`, shared by its animators): the + // expression runs per character, so a slow expression on a long text is stopped + // once it has failed or the layer's evaluations pass MAX_SELECTOR_EVALS. let stopped = std::cell::Cell::new(false); Box::new(move |i, n, prev| { let pv = prev.map(|x| x * 100.0); // Without an expression engine: the static amount scales the selection. let fallback = [0, 1, 2].map(|d| stat[d] * prev[d]); - let live = !stopped.get() && start.elapsed() < MAX_SELECTOR_TIME; - let r = match ctx.expr.filter(|_| live && prop.has_expression()) { + let r = match ctx.expr.filter(|_| prop.has_expression() && !stopped.get() && budget.take()) { Some(h) => h.eval_text_selector(ctx, layer, prop, i, n, pv).unwrap_or_else(|_| { stopped.set(true); fallback @@ -302,15 +329,17 @@ pub fn animator_selection(ctx: &EvalCtx, layer: &Layer, anim: &PropGroup, lay: & acc } -/// The most time one layer's Expression Selectors may take across all its characters. -const MAX_SELECTOR_TIME: std::time::Duration = std::time::Duration::from_secs(2); +/// The most Expression Selector evaluations one layer's animators run across all its characters +/// (a text of 32,768 characters under one expression selector, or fewer under several). +const MAX_SELECTOR_EVALS: usize = 32_768; /// Evaluate animators for every glyph. pub fn char_transforms(ctx: &EvalCtx, layer: &Layer, text: &PropGroup, lay: &TextLayout) -> Vec { let mut out = vec![CharXf::default(); lay.glyphs.len()]; let Some(anims) = text.sub("animators") else { return out }; + let budget = SelectorBudget::new(); for anim in anims.groups().filter(|g| g.enabled) { let Some(props) = anim.sub("properties") else { continue }; - let sel = animator_selection(ctx, layer, anim, lay); + let sel = animator_selection_within(ctx, layer, anim, lay, &budget); let get = |m: &str| props.get(m).map(|p| ctx.value(layer, p).components()); let get1 = |m: &str| props.get(m).map(|p| ctx.value(layer, p).as_f64()); let c3 = |v: Vec, d: f64| [v.first().copied().unwrap_or(d), v.get(1).copied().unwrap_or(d), v.get(2).copied().unwrap_or(d)]; diff --git a/crates/render/src/text/tests.rs b/crates/render/src/text/tests.rs index 9b52d3098..bbc1cd28e 100644 --- a/crates/render/src/text/tests.rs +++ b/crates/render/src/text/tests.rs @@ -802,3 +802,12 @@ fn text_notes_the_font_files_it_draws_with() { assert!(files.is_complete(), "{files:?}"); assert_eq!(files.get(&font.to_string_lossy()), Some(stamp), "{files:?}"); } + +#[test] +fn selector_evaluations_are_counted_across_the_layer_not_timed() { + let b = SelectorBudget(std::cell::Cell::new(3)); + // Taken by any animator's selectors in turn; the fourth finds none, and stays none. + assert!([b.take(), b.take(), b.take()].iter().all(|t| *t)); + assert!(!b.take() && !b.take()); + assert_eq!(SelectorBudget::new().0.get(), MAX_SELECTOR_EVALS); +} diff --git a/crates/render/src/three_d/camera.rs b/crates/render/src/three_d/camera.rs index 360e232c4..18f6dfe8a 100644 --- a/crates/render/src/three_d/camera.rs +++ b/crates/render/src/three_d/camera.rs @@ -350,9 +350,18 @@ pub fn auto_orient_3d(ctx: &EvalCtx, layer: &Layer, pos: Vec3) -> Option { if nesting >= MAX_ORIENT_NESTING { return None; } + // Restored when this scope ends, a panic unwinding through it included (the CPU retry + // after one must not start with a raised count). + struct Restore(usize); + impl Drop for Restore { + fn drop(&mut self) { + ORIENT_NESTING.with(|n| n.set(self.0)); + } + } ORIENT_NESTING.with(|n| n.set(nesting + 1)); + let restore = Restore(nesting); let parent = parent_world(ctx, layer); - ORIENT_NESTING.with(|n| n.set(nesting)); + drop(restore); let eye = active_camera(ctx).eye; let e = parent.inverse().unwrap_or(Mat4::IDENTITY).apply(eye); let d = pos - e;