Merge pull request #676 from storytold/claude/community-pr-review-lwtrlr

Issue fixes: drag render pile-up, CC Smear/Tiler past text bounds, relative footage paths, GIF playback test

Cargo.lock

@@ -1351,6 +1351,7 @@ dependencies = [
  "effectcraft-svg",
  "effectcraft-time",
  "exr",
+ "gif",
  "image",
  "rayon",
  "serde_json",

crates/effects/src/distort2.rs

@@ -20,7 +20,7 @@ use effectcraft_raster::{Image, Px};
 use rayon::prelude::*;
 
 use crate::generate::value_noise;
-use crate::util::{SRC_NAMES, Src, fit_layer, layer_rect, lerp4, map_xy, pick, premul, remap, smoothstep, src_at, unpremul};
+use crate::util::{SRC_NAMES, Src, fit_layer, layer_cover_pad, layer_rect, lerp4, map_xy, pick, premul, remap, smoothstep, src_at, unpremul};
 use crate::{Buf, EffectCtx, EffectSpec, col, num, p, popup, slider};
 
 fn spec(id: &'static str, name: &'static str, params: Vec<crate::ParamSpec>, render: crate::RenderFn) -> EffectSpec {
@@ -678,6 +678,9 @@ fn page_turn(ctx: &EffectCtx, mut b: Buf) -> Buf {
 
 fn cc_tiler(ctx: &EffectCtx, mut b: Buf) -> Buf {
     let s = (ctx.params.f("scale") / 100.0).max(0.001);
+    if !ctx.adjustment {
+        b.pad(layer_cover_pad(ctx, [b.img.width, b.img.height], b.offset, b.scale));
+    }
     let c = b.to_px(ctx.params.v2("center"));
     let blend = (ctx.params.f("blendWithOriginal") / 100.0) as f32;
     let (x0, y0, w, h) = layer_rect(ctx, &b);

crates/effects/src/distort3.rs

@@ -330,9 +330,12 @@ fn cc_slant(ctx: &EffectCtx, mut b: Buf) -> Buf {
 // ---------------------------------------------------------------- CC Smear
 
 fn cc_smear(ctx: &EffectCtx, mut b: Buf) -> Buf {
+    let reach = ctx.params.f("reach") / 100.0;
+    if !ctx.adjustment {
+        b.pad(cc_smear_pad(ctx, [b.img.width, b.img.height], b.offset, b.scale));
+    }
     let from = b.to_px(ctx.params.v2("from"));
     let to = b.to_px(ctx.params.v2("to"));
-    let reach = ctx.params.f("reach") / 100.0;
     let radius = (ctx.params.f("radius") * b.scale).max(0.5);
     let (dx, dy) = (to.0 - from.0, to.1 - from.1);
     let l2 = dx * dx + dy * dy;
@@ -351,6 +354,21 @@ fn cc_smear(ctx: &EffectCtx, mut b: Buf) -> Buf {
     b
 }
 
+/// The transparent pixels to add on every side of a buffer `size` pixels big (with `offset` and
+/// `scale`) for CC Smear: it moves pixels by up to Reach × From→To, and they may move out to the
+/// layer bounds. A text or shape layer's buffer ends at its content while its bounds are
+/// comp-sized, so the smear was cut off at the content's box (#659).
+pub fn cc_smear_pad(ctx: &EffectCtx, size: [u32; 2], offset: [f64; 2], scale: f64) -> u32 {
+    let reach = ctx.params.f("reach") / 100.0;
+    let (from, to) = (ctx.params.v2("from"), ctx.params.v2("to"));
+    let len = ((to[0] - from[0]).powi(2) + (to[1] - from[1]).powi(2)).sqrt() * scale;
+    let moved = (reach.abs() * len).ceil();
+    if !moved.is_finite() || moved <= 0.0 {
+        return 0;
+    }
+    (moved.min(4096.0) as u32).min(crate::util::layer_cover_pad(ctx, size, offset, scale))
+}
+
 // ---------------------------------------------------------------- CC Split / CC Split 2
 
 fn split_impl(ctx: &EffectCtx, mut b: Buf, s1: f64, s2: f64) -> Buf {
@@ -1109,6 +1127,43 @@ mod tests {
         }
     }
 
+    /// A text layer's buffer: 20 × 10 opaque pixels at (90, 45)–(110, 55) of its comp-sized
+    /// (200 × 100) layer bounds, after `id` with `set`.
+    fn on_text(id: &str, set: &[(&str, Value)]) -> Buf {
+        let s = crate::find(id).unwrap();
+        let mut params = Params { values: s.params.iter().map(|p| (p.id.to_string(), p.default.clone())).collect() };
+        for (k, v) in set {
+            assert!(params.values.contains_key(*k), "{id}: {k}");
+            params.values.insert(k.to_string(), v.clone());
+        }
+        let mut im = Image::new(20, 10);
+        im.data.iter_mut().for_each(|p| *p = [1.0; 4]);
+        let ctx = EffectCtx { params: &params, time: 0.0, layer_size: [200.0, 100.0], seed: 1, adjustment: false, env: Default::default() };
+        crate::apply(s, &ctx, Buf { img: im, offset: [-90.0, -45.0], scale: 1.0 })
+    }
+
+    /// Alpha at layer point `p` (0 outside the buffer).
+    fn alpha_at(b: &Buf, p: [f64; 2]) -> f32 {
+        let (x, y) = b.to_px(p);
+        b.img.get(x.floor() as i64, y.floor() as i64)[3]
+    }
+
+    /// #659: CC Smear and CC Tiler on a text layer reach past the text's box to the layer bounds
+    /// (comp-sized, as in After Effects), not cut off where the glyphs end.
+    #[test]
+    fn effects_on_text_reach_past_its_box() {
+        let smear = on_text("ec.distort.ccsmear", &[("from", pt(100.0, 50.0)), ("to", pt(140.0, 50.0)), ("reach", num(100.0)), ("radius", num(30.0))]);
+        assert!(alpha_at(&smear, [130.0, 50.0]) > 0.9, "the smear is cut off at the text's box");
+        assert!(smear.img.width < 200, "it grows only as far as pixels move");
+        let tiles = on_text("ec.distort.cctiler", &[("scale", num(50.0)), ("center", pt(100.0, 50.0))]);
+        assert!(alpha_at(&tiles, [100.0, 50.0]) > 0.9);
+        assert!(alpha_at(&tiles, [2.0, 50.0]) > 0.9, "tiles fill the layer bounds");
+        assert!(alpha_at(&tiles, [50.0, 50.0]) < 0.1);
+        // A footage layer's buffer is its bounds already: no change.
+        let o = run("ec.distort.ccsmear", &[], img(40, 30), &[]);
+        assert_eq!((o.img.width, o.img.height, o.offset), (40, 30, [0.0, 0.0]));
+    }
+
     #[test]
     fn every_warp_style_runs_and_bends() {
         let im = img(40, 30);

crates/effects/src/lib.rs

@@ -78,7 +78,7 @@ pub use color2::{Curve, auto_correct_settings, color_link_mode, color_link_sampl
 pub use color3::OffsetCurve;
 pub use distort::transform_shutter;
 pub use distort2::{MAGNIFY_MODES, bezier_grid, parse_mesh};
-pub use distort3::{RESHAPE_POINTS, ReshapeSetup, SmearSetup, reshape_setup, smear_setup, warp_inverse_map};
+pub use distort3::{RESHAPE_POINTS, ReshapeSetup, SmearSetup, cc_smear_pad, reshape_setup, smear_setup, warp_inverse_map};
 pub use distort4::liquify_mesh;
 use effectcraft_keyframe::Value;
 use effectcraft_project::build::Ids;

crates/effects/src/util.rs

@@ -433,6 +433,17 @@ pub fn layer_rect_at(ctx: &crate::EffectCtx, offset: [f64; 2], scale: f64) -> (f
     (offset[0], offset[1], ctx.layer_size[0] * scale, ctx.layer_size[1] * scale)
 }
 
+/// Transparent pixels to add on every side ([`crate::Buf::pad`]) of a buffer `size` pixels big,
+/// with `offset` and `scale`, so it covers the whole layer rectangle (at most 4096). A text or
+/// shape layer's buffer only surrounds its content, but its layer bounds are comp-sized (see
+/// the renderer's `effect_bounds`): an effect that fills the layer, such as CC Tiler, fills
+/// those as in After Effects (#659).
+pub fn layer_cover_pad(ctx: &crate::EffectCtx, size: [u32; 2], offset: [f64; 2], scale: f64) -> u32 {
+    let (x0, y0, w, h) = layer_rect_at(ctx, offset, scale);
+    let need = [-x0, -y0, x0 + w - f64::from(size[0]), y0 + h - f64::from(size[1])].into_iter().fold(0.0, f64::max);
+    need.ceil().min(4096.0) as u32
+}
+
 /// Resample another layer (a layer parameter) into `b`'s pixel grid. With `stretch` the other
 /// layer is scaled to this layer's size ("Stretch to Fit"); otherwise it is centred.
 pub fn fit_layer(ctx: &crate::EffectCtx, b: &crate::Buf, other: &crate::LayerPixels, stretch: bool) -> Image {

crates/engine/src/commands/file.rs

@@ -43,7 +43,8 @@ pub(crate) fn open(s: &mut Session, p: &Value) -> Result<Value> {
     let text = String::from_utf8(bytes).map_err(|_| EngineError::Other("not a text project file".into()))?;
     // XML copies (File ▸ Save a Copy As XML…) open like the JSON project.
     let text = if crate::xml_project::is_xml(&text) { crate::xml_project::from_xml(&text).map_err(EngineError::Other)? } else { text };
-    let proj = Project::from_json(&text).map_err(|e| EngineError::Other(format!("{path} is not a project EffectCraft can open: {e}")))?;
+    let mut proj = Project::from_json(&text).map_err(|e| EngineError::Other(format!("{path} is not a project EffectCraft can open: {e}")))?;
+    resolve_relative_footage(&mut proj, path, s.services.as_ref());
     s.replace_project(proj, Some(path.to_string()));
     s.note_project_path(path);
     // Written by a newer EffectCraft: what this version doesn't know would be lost on saving.
@@ -62,6 +63,28 @@ pub(crate) fn open(s: &mut Session, p: &Value) -> Result<Value> {
     Ok(json!({"path": path, "savedBy": saved_by}))
 }
 
+/// Footage saved with a relative path (imported by the CLI or an agent as `clip.mp4`, or written
+/// by hand) is found next to the project file, as After Effects looks for footage relative to
+/// the project. It used to be looked up from the working directory: the CLI started in the
+/// project's folder found it, the desktop app (started from the Finder, Explorer or a launcher,
+/// in another folder) showed and rendered nothing (#652). A path that exists from the working
+/// directory is kept.
+fn resolve_relative_footage(p: &mut Project, project_path: &str, services: &dyn crate::Services) {
+    use std::path::Path;
+    let Some(dir) = Path::new(project_path).parent().filter(|d| !d.as_os_str().is_empty()) else { return };
+    let beside = |f: &str| dir.join(f).to_string_lossy().to_string();
+    for item in p.items.values_mut() {
+        let ItemKind::Footage(f) = &mut item.kind else { continue };
+        if f.path.is_empty() || !Path::new(&f.path).is_relative() || services.exists(&f.path) || !services.exists(&beside(&f.path)) {
+            continue;
+        }
+        f.path = beside(&f.path);
+        for frame in f.sequence.iter_mut().filter(|q| Path::new(q.as_str()).is_relative()) {
+            *frame = beside(frame);
+        }
+    }
+}
+
 /// The project file's text for `path`: `.ecproj` JSON, or XML for `.ecprojx` / `.xml` (a
 /// project opened from an XML copy saves as XML again). Fails, writing nothing, when the project
 /// wouldn't read back.

crates/engine/src/footage_check.rs

@@ -193,6 +193,30 @@ mod tests {
         let _ = std::fs::remove_dir_all(dir);
     }
 
+    /// #652: footage saved with a relative path is found next to the project, wherever the
+    /// app was started (the desktop app doesn't start in the project's folder, the CLI often
+    /// does).
+    #[test]
+    fn relative_footage_is_found_next_to_the_project() {
+        let dir = std::env::temp_dir().join(format!("ec-relative-{}", std::process::id()));
+        std::fs::create_dir_all(dir.join("media")).unwrap();
+        let clip = dir.join("media").join("ec-relative-clip.png");
+        std::fs::write(&clip, b"x").unwrap();
+        let mut s = session_with(&["media/ec-relative-clip.png", "/nonexistent/ec/abs.png", "media/ec-relative-gone.png"]);
+        let path = dir.join("p.ecproj");
+        s.execute("file.saveAs", json!({"path": path.to_string_lossy()})).unwrap();
+        let mut o = Session::default();
+        o.execute("file.open", json!({"path": path.to_string_lossy()})).unwrap();
+        let mut paths: Vec<String> =
+            o.project.items.values().filter_map(|i| if let ItemKind::Footage(f) = &i.kind { Some(f.path.clone()) } else { None }).collect();
+        paths.sort();
+        assert_eq!(paths, ["/nonexistent/ec/abs.png", clip.to_str().unwrap(), "media/ec-relative-gone.png"]);
+        assert!(!o.is_dirty(), "finding the files is no edit");
+        let r = o.execute("footage.check", json!({"wait": true})).unwrap();
+        assert_eq!(r["missing"], 2);
+        let _ = std::fs::remove_dir_all(dir);
+    }
+
     #[test]
     fn open_starts_the_check_when_asked() {
         let mut s = session_with(&["/nonexistent/ec/a.mov"]);

crates/gpu/src/fx_stylize.rs

@@ -299,10 +299,13 @@ fn cc_slant(e: &mut Enc, ctx: &EffectCtx, mut b: GBuf) -> Option<GBuf> {
     )
 }
 
-fn cc_smear(e: &mut Enc, ctx: &EffectCtx, b: GBuf) -> Option<GBuf> {
+fn cc_smear(e: &mut Enc, ctx: &EffectCtx, mut b: GBuf) -> Option<GBuf> {
+    let reach = ctx.params.f("reach") / 100.0;
+    if !ctx.adjustment {
+        b.pad(e, effectcraft_effects::cc_smear_pad(ctx, [b.img.width, b.img.height], b.offset, b.scale))?;
+    }
     let from = b.to_px(ctx.params.v2("from"));
     let to = b.to_px(ctx.params.v2("to"));
-    let reach = ctx.params.f("reach") / 100.0;
     let radius = (ctx.params.f("radius") * b.scale).max(0.5);
     let (dx, dy) = (to.0 - from.0, to.1 - from.1);
     let l2 = dx * dx + dy * dy;
@@ -585,8 +588,11 @@ fn offset(e: &mut Enc, ctx: &EffectCtx, b: GBuf) -> Option<GBuf> {
     table(e, b, &data, false, blend.max(0.0))
 }
 
-fn cc_tiler(e: &mut Enc, ctx: &EffectCtx, b: GBuf) -> Option<GBuf> {
+fn cc_tiler(e: &mut Enc, ctx: &EffectCtx, mut b: GBuf) -> Option<GBuf> {
     let s = (ctx.params.f("scale") / 100.0).max(0.001);
+    if !ctx.adjustment {
+        b.pad(e, effectcraft_effects::util::layer_cover_pad(ctx, [b.img.width, b.img.height], b.offset, b.scale))?;
+    }
     let c = b.to_px(ctx.params.v2("center"));
     let blend = (ctx.params.f("blendWithOriginal") / 100.0) as f32;
     let (x0, y0, w, h) = layer_rect(ctx, &b);

crates/host/Cargo.toml

@@ -31,5 +31,6 @@ effectcraft-raster = { workspace = true }
 effectcraft-time = { workspace = true }
 effectcraft-interchange = { workspace = true }
 image = { workspace = true }
+gif = { workspace = true }
 exr = { workspace = true }
 rayon = { workspace = true }

crates/host/src/lib.rs

@@ -257,6 +257,38 @@ mod tests {
         assert!(red * 20 > img.data.len(), "red cube pixels: {red} of {}", img.data.len());
     }
 
+    /// #651: an animated GIF imported from disk plays: the layer shows each frame at its time,
+    /// in the viewer's renders and in exports alike.
+    #[test]
+    fn an_imported_animated_gif_plays() {
+        let dir = std::env::temp_dir().join(format!("ec-gif-{}", std::process::id()));
+        std::fs::create_dir_all(&dir).unwrap();
+        let path = dir.join("anim.gif");
+        let mut bytes = Vec::new();
+        {
+            let mut enc = gif::Encoder::new(&mut bytes, 4, 4, &[]).unwrap();
+            for rgb in [[255u8, 0, 0], [0, 255, 0], [0, 0, 255]] {
+                let mut f = gif::Frame::from_rgb(4, 4, &rgb.repeat(16));
+                f.delay = 10;
+                enc.write_frame(&f).unwrap();
+            }
+        }
+        std::fs::write(&path, bytes).unwrap();
+        let mut s = super::session();
+        s.execute("comp.new", json!({"name": "G", "width": 4, "height": 4, "frameRate": 30, "duration": 1})).unwrap();
+        s.execute("file.import", json!({"paths": [path.to_string_lossy()], "addToComp": true})).unwrap();
+        let cid = s.active_comp_id().unwrap();
+        let rgb = |t: f64| {
+            let img = s.render(cid, effectcraft_time::Tick::from_seconds_f64(t), effectcraft_engine::render::RenderOpts::default());
+            let p = img.data[5];
+            [p[0].round(), p[1].round(), p[2].round()]
+        };
+        assert_eq!(rgb(0.05), [1.0, 0.0, 0.0]);
+        assert_eq!(rgb(0.15), [0.0, 1.0, 0.0]);
+        assert_eq!(rgb(0.25), [0.0, 0.0, 1.0]);
+        let _ = std::fs::remove_dir_all(dir);
+    }
+
     /// The web app's path: outputs go to a sink (downloads), never to the file system.
     #[test]
     fn render_queue_exports_to_a_sink() {

crates/ui-egui/src/frames.rs

@@ -768,10 +768,8 @@ impl Frames {
         #[cfg(not(target_arch = "wasm32"))]
         if self.remote.is_none() {
             let w = self.worker();
-            // One spawn per job; each spawn renders whichever queued job matters most right now.
-            let job = move || {
-                w.run_next();
-            };
+            // One spawn per job; each spawn renders whichever queued jobs matter most right now.
+            let job = move || w.drain();
             match &self.pool {
                 Some(pool) => pool.spawn(job),
                 None => rayon::spawn(job),
@@ -924,7 +922,42 @@ struct Worker {
     gpu_retired: Arc<AtomicBool>,
 }
 
+#[cfg(not(target_arch = "wasm32"))]
+thread_local! {
+    /// This pool thread is inside [`Worker::drain`] (rendering a frame).
+    static DRAINING: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
+}
+
+/// Clears [`DRAINING`] when the drain ends, even by a panic.
+#[cfg(not(target_arch = "wasm32"))]
+struct Draining;
+
+#[cfg(not(target_arch = "wasm32"))]
+impl Drop for Draining {
+    fn drop(&mut self) {
+        DRAINING.set(false);
+    }
+}
+
 impl Worker {
+    /// Render queued jobs, most important first, until the queue is empty.
+    ///
+    /// A pool thread waiting inside a render (on a parallel loop's join) runs other pool work
+    /// meanwhile, including the spawns of newer requests: each would start a whole frame render
+    /// on top of the one waiting, with all of its buffers. Dragging a layer asks for a new frame
+    /// every UI frame, so renders piled up on every thread until memory ran out (#647). A spawn
+    /// that lands inside a render returns at once instead; the render below it takes the queued
+    /// jobs when it is done, so each thread renders one frame at a time.
+    #[cfg(not(target_arch = "wasm32"))]
+    fn drain(&self) {
+        if DRAINING.get() {
+            return;
+        }
+        DRAINING.set(true);
+        let _reset = Draining;
+        while self.run_next() {}
+    }
+
     /// Render the queued job that matters most right now. `false` when the queue was empty.
     fn run_next(&self) -> bool {
         let Worker { queue, content_keys, .. } = self;
@@ -1234,6 +1267,80 @@ mod tests {
         assert!(!f.is_cached(&key));
     }
 
+    /// #647: a pool thread waiting inside a render (on a parallel loop) never starts another
+    /// frame render on top of it. Dragging a layer queues a frame every UI frame; renders used to
+    /// nest on every thread, each holding its buffers, until memory ran out.
+    #[test]
+    fn a_waiting_render_never_starts_another_on_its_thread() {
+        use effectcraft_engine::Session;
+        use effectcraft_engine::project::{Footage, FootageKind};
+        use effectcraft_engine::render::Image;
+        use effectcraft_engine::time::FrameRate;
+        use serde_json::json;
+        use std::cell::Cell;
+        use std::sync::atomic::AtomicUsize;
+        thread_local! {
+            static DEPTH: Cell<usize> = const { Cell::new(0) };
+        }
+        /// A decoder that works in parallel, as the renderer does: its thread joins the loop and
+        /// may wait for the parts other threads took.
+        #[derive(Default)]
+        struct Parallel {
+            calls: AtomicUsize,
+            deepest: AtomicUsize,
+        }
+        impl FootageSource for Parallel {
+            fn frame(&self, _: ItemId, _: &Footage, _: Tick) -> Option<Arc<Image>> {
+                let d = DEPTH.get() + 1;
+                DEPTH.set(d);
+                self.deepest.fetch_max(d, Ordering::SeqCst);
+                self.calls.fetch_add(1, Ordering::SeqCst);
+                (0..64).into_par_iter().for_each(|_| std::thread::sleep(std::time::Duration::from_millis(1)));
+                DEPTH.set(d - 1);
+                Some(Arc::new(Image::new(4, 4)))
+            }
+        }
+        let mut s = Session::default();
+        let cid = ItemId(s.execute("comp.new", json!({"name": "A", "width": 8, "height": 8, "duration": 10})).unwrap()["comp"].as_u64().unwrap());
+        let clip = Footage {
+            kind: FootageKind::Video,
+            width: 4,
+            height: 4,
+            has_video: true,
+            frame_rate: FrameRate::FPS_30,
+            duration: Tick::from_seconds_f64(10.0),
+            ..Default::default()
+        };
+        let item = Arc::make_mut(&mut s.project).add_item("clip.mov", Default::default(), None, ItemKind::Footage(clip));
+        s.execute("layer.addItem", json!({"item": item.0})).unwrap();
+        let footage = Arc::new(Parallel::default());
+        let src = RenderSource {
+            project: s.project.clone(),
+            footage: footage.clone(),
+            expr: None,
+            layer_cache: Arc::default(),
+            gpu: None,
+            gpu_display: false,
+            disk: None,
+        };
+        let f = Frames::default();
+        let content = f.content_of(&s.project, s.revision, cid);
+        let frames = 60;
+        for frame in 0..frames {
+            let key = FrameKey { revision: s.revision, content, comp: cid.0, frame, scale: 1000, view: 0, opts: 0 };
+            f.request(&src, key, cid, FrameRate::FPS_30.tick_of(frame), RenderOpts::default());
+        }
+        for _ in 0..4000 {
+            if f.inflight() == 0 {
+                break;
+            }
+            std::thread::sleep(std::time::Duration::from_millis(5));
+        }
+        assert_eq!(f.inflight(), 0, "every queued frame rendered");
+        assert!(footage.calls.load(Ordering::SeqCst) >= frames as usize);
+        assert_eq!(footage.deepest.load(Ordering::SeqCst), 1, "renders nested on a waiting thread");
+    }
+
     /// #284: scrubbing asks for a viewer frame at every step; a newer one drops the comp's viewer
     /// frames still waiting (they would all render first), but not prefetch frames or another
     /// comp's viewer frame.