From dc3e5245c2ca9a2f1e64a4452d7f3dccb1d3d064 Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Thu, 12 Nov 2026 11:29:57 +0530 Subject: [PATCH] #3493: show the frame again in the viewers once a LUT file changes The viewers replace their picture only when the frame key changes, and a frame rendered again after a LUT change had the same key, so the old look stayed on screen. The viewer frame keys carry the LUT generation (in memory only), which also leaves frames still rendering with the old LUT unasked for, so the refusal of late frames is no longer needed. --- crates/engine/src/footage/lut_reload.rs | 8 +++- crates/ui-egui/src/app/frame.rs | 4 +- crates/ui-egui/src/app/viewer_frames.rs | 50 ++++++++++++++++++++++++- crates/ui-egui/src/frames.rs | 16 +------- crates/ui-egui/src/frames/tests.rs | 23 ------------ 5 files changed, 60 insertions(+), 41 deletions(-) diff --git a/crates/engine/src/footage/lut_reload.rs b/crates/engine/src/footage/lut_reload.rs index 96cb14c4a..54211eac0 100644 --- a/crates/engine/src/footage/lut_reload.rs +++ b/crates/engine/src/footage/lut_reload.rs @@ -16,6 +16,12 @@ pub fn generation() -> u64 { effectcraft_effects::lut_generation() } +/// Stamp the LUT files in use now ([`effectcraft_effects::check_lut_files`]): the ones that +/// changed are read again by the next render. Returns [`generation`]. +pub fn check() -> u64 { + effectcraft_effects::check_lut_files() +} + /// How often the LUT files in use are stamped. pub const INTERVAL: Duration = Duration::from_secs(2); @@ -44,7 +50,7 @@ impl Watcher { let mut seen = effectcraft_effects::lut_generation(); // (ends once the watcher, the only sender, is dropped) while let Err(RecvTimeoutError::Timeout) = rx.recv_timeout(every) { - let now = effectcraft_effects::check_lut_files(); + let now = check(); if now != seen { seen = now; wake(); diff --git a/crates/ui-egui/src/app/frame.rs b/crates/ui-egui/src/app/frame.rs index c76e3f410..488b3c5ef 100644 --- a/crates/ui-egui/src/app/frame.rs +++ b/crates/ui-egui/src/app/frame.rs @@ -659,6 +659,8 @@ impl EffectcraftApp { /// A LUT file in use changed on disk (#3493): the frames kept were rendered with the old one. /// Renders read it again by themselves; the watcher wakes the interface when no render did. + /// The frame keys carry the generation ([`Self::frame_series`]), so the viewers show the + /// frames rendered again, and a frame still rendering with the old LUT is never asked for. fn tick_lut_files(&mut self, ctx: &egui::Context) { if self.lut_watcher.is_none() { let wake = ctx.clone(); @@ -667,7 +669,7 @@ impl EffectcraftApp { let generation = effectcraft_engine::lut_reload::generation(); if generation != self.lut_generation { self.lut_generation = generation; - self.frames.clear_stale(); + self.frames.clear(); } } diff --git a/crates/ui-egui/src/app/viewer_frames.rs b/crates/ui-egui/src/app/viewer_frames.rs index fe3ad8abb..d7c02a99c 100644 --- a/crates/ui-egui/src/app/viewer_frames.rs +++ b/crates/ui-egui/src/app/viewer_frames.rs @@ -128,7 +128,7 @@ impl EffectcraftApp { frame: 0, scale: (scale * 1000.0).round() as u32, view: self.view_hash(comp), - opts: frames::opts_hash(&self.frame_opts(comp, scale)) ^ self.roto_model_stamp(), + opts: frames::opts_hash(&self.frame_opts(comp, scale)) ^ self.roto_model_stamp() ^ self.lut_stamp(), } } @@ -142,6 +142,13 @@ impl EffectcraftApp { h.finish() | 1 } + /// Changes when a LUT file in use changed on disk (#3493): the frame on screen and the ones + /// still rendering were made with the old LUT, so the new ones must not share their keys + /// (the viewers show a frame again only when its key changes). 0 until a LUT file changes. + fn lut_stamp(&self) -> u64 { + self.lut_generation.wrapping_mul(0x9e37_79b9_7f4a_7c15) + } + /// The frame series the viewer of `comp` shows now (its scale, view and options, for the /// comp's current content): what the cache bars count. pub fn shown_series(&self, comp: ItemId) -> FrameKey { @@ -238,3 +245,44 @@ impl EffectcraftApp { request(&self.frames, &self.render_source(), key, comp, t, opts); } } + +#[cfg(test)] +mod tests { + /// #3493: the viewer shows the frame again once the LUT file it was graded with changes, + /// though its key was otherwise the same (same project revision, frame and options). + #[test] + fn the_viewer_shows_a_lut_file_changed_on_disk() { + let dir = tempfile::tempdir().unwrap(); + let lut = dir.path().join("viewer.cube"); + let write = |text: &str, secs: u64| { + std::fs::write(&lut, text).unwrap(); + let f = std::fs::File::options().write(true).open(&lut).unwrap(); + f.set_modified(std::time::UNIX_EPOCH + std::time::Duration::from_secs(secs)).unwrap(); + }; + write("LUT_1D_SIZE 2\n0 0 0\n0 0 0\n", 1_000); + let mut session = effectcraft_engine::Session::default(); + session.execute("comp.new", serde_json::json!({"width": 64, "height": 36, "duration": 2})).unwrap(); + session.execute("layer.newSolid", serde_json::json!({"color": "#808080"})).unwrap(); + session.execute("effect.apply", serde_json::json!({"layer": 1, "effect": "Apply Color LUT"})).unwrap(); + session.execute("prop.set", serde_json::json!({"layer": 1, "path": "effects/#1/lut", "value": lut.to_string_lossy()})).unwrap(); + let mut harness = egui_kittest::Harness::builder().with_size(egui::vec2(1200.0, 800.0)).build_eframe(|_| crate::EffectcraftApp::new(session)); + let centre = |h: &egui_kittest::Harness<'_, crate::EffectcraftApp>| { + h.state().viewer_image.as_ref().map(|img| img.pixels[img.pixels.len() / 2 + img.size[0] / 2].r()) + }; + let settle = |h: &mut egui_kittest::Harness<'_, crate::EffectcraftApp>, want: u8| { + for _ in 0..400 { + h.step(); + if centre(h) == Some(want) { + return; + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } + panic!("the viewer shows {:?}, not {want}", centre(h)); + }; + settle(&mut harness, 0); + write("LUT_1D_SIZE 2\n1 1 1\n1 1 1\n", 2_000); + // (what the app's watcher does every two seconds) + effectcraft_engine::lut_reload::check(); + settle(&mut harness, 255); + } +} diff --git a/crates/ui-egui/src/frames.rs b/crates/ui-egui/src/frames.rs index 6d99a8cd2..a0c2f94b5 100644 --- a/crates/ui-egui/src/frames.rs +++ b/crates/ui-egui/src/frames.rs @@ -298,9 +298,6 @@ struct Cache { playhead: Option, /// Scale of the frame on screen (the newest urgent request): frames of other scales go first. scale: Option, - /// Frames whose render started before this are not kept: a source file they read changed - /// since ([`Frames::clear_stale`]). - stale_before: Option, } impl Cache { @@ -771,7 +768,7 @@ impl Frames { #[cfg(target_arch = "wasm32")] let queue = Queue::default(); Frames { - cache: Arc::new(Mutex::new(Cache { map: HashMap::new(), order: VecDeque::new(), bytes: 0, gpu_bytes: 0, budget: 3 << 30, gpu_budget: GPU_BUDGET, gpu_pressure: None, gpu_adjusted: None, playhead: None, scale: None, stale_before: None })), + cache: Arc::new(Mutex::new(Cache { map: HashMap::new(), order: VecDeque::new(), bytes: 0, gpu_bytes: 0, budget: 3 << 30, gpu_budget: GPU_BUDGET, gpu_pressure: None, gpu_adjusted: None, playhead: None, scale: None })), inflight: Arc::new(Mutex::new(HashSet::new())), queue: Arc::new(Mutex::new(queue)), #[cfg(not(target_arch = "wasm32"))] @@ -1406,16 +1403,6 @@ impl Frames { } } - /// Drop every frame, and the ones still rendering when they finish: a source file changed on - /// disk (a LUT file, #3493), and a render that started before may have read the old one. - pub fn clear_stale(&self) { - // (marked first: a frame finishing while the cache is emptied is refused, not kept) - if let Ok(mut c) = self.cache.lock() { - c.stale_before = Some(web_time::Instant::now()); - } - self.clear(); - } - /// The playing preview's position (`None` when no preview plays): prefetch renders the /// frames it reaches first before the others, evictions spare the frames it shows next, and /// queued prefetch frames of its series it no longer reaches are dropped (#595). @@ -1912,7 +1899,6 @@ impl Worker { } if let Ok(mut c) = self.cache.lock() && (!img.is_gpu() || !self.gpu_retired.load(Ordering::Acquire)) - && c.stale_before.is_none_or(|at| t0 >= at) { c.insert_with(key, img, files, since); } diff --git a/crates/ui-egui/src/frames/tests.rs b/crates/ui-egui/src/frames/tests.rs index d6bf56a20..45398de21 100644 --- a/crates/ui-egui/src/frames/tests.rs +++ b/crates/ui-egui/src/frames/tests.rs @@ -218,29 +218,6 @@ fn retirement_reaches_existing_workers_and_keeps_cpu_frames_and_completion_live( assert!(frames.queue.lock().unwrap().running.is_empty()); } -/// #3493: a LUT file changed on disk drops the frames kept, and a frame whose render started -/// before is not kept when it finishes (it may have read the old file); one started after is. -#[test] -fn clear_stale_drops_kept_frames_and_ones_rendering() { - let frames = Frames::default(); - let worker = frames.worker(); - let kept = key(1, 0, 0); - insert(&frames, kept); - let (late, fresh) = (key(1, 1, 0), key(1, 2, 0)); - let started = web_time::Instant::now(); - frames.clear_stale(); - assert!(!frames.is_cached(&kept)); - for k in [late, fresh] { - frames.inflight.lock().unwrap().insert(k); - frames.queue.lock().unwrap().running.insert(k); - } - worker.finish_key(late, false, img(), None, None, started); - assert!(!frames.is_cached(&late), "a render started before the change"); - worker.finish_key(fresh, false, img(), None, None, web_time::Instant::now()); - assert!(frames.is_cached(&fresh), "a render started after it"); - assert_eq!(frames.inflight(), 0, "both jobs are released"); -} - fn key(content: u64, frame: i64, opts: u64) -> FrameKey { FrameKey { revision: content, content, comp: 1, frame, scale: 1000, view: 0, opts } }