diff --git a/crates/gpu/src/lib.rs b/crates/gpu/src/lib.rs index b087a2ceb..ddbc8d54a 100644 --- a/crates/gpu/src/lib.rs +++ b/crates/gpu/src/lib.rs @@ -301,7 +301,12 @@ impl Gpu { /// Browser callers use the host/owned-device failure bridge and bounded readbacks instead /// of blocking their event loop on an error-scope future. pub fn within_memory(&self, f: impl FnOnce() -> T) -> Result { - self.ctx.check_health().map_err(FrameError::Retired)?; + if let Err(error) = self.ctx.check_health() { + // Queued jobs may still own this context after the UI detached it. Release its + // working caches before CPU fallback adds host-memory pressure (#3357). + self.ctx.discard_kept(); + return Err(FrameError::Retired(error)); + } #[cfg(not(target_arch = "wasm32"))] { let oom = self.ctx.device.push_error_scope(wgpu::ErrorFilter::OutOfMemory); @@ -315,7 +320,7 @@ impl Gpu { // The allocation failure first: a validation error is often its consequence. let errors: Vec = [oom, internal, validation].into_iter().flatten().map(|e| e.to_string()).collect(); if errors.is_empty() { - return self.ctx.check_health().map(|()| out).map_err(FrameError::Retired); + return self.finish_memory_attempt(out); } // Its textures go back to the pool when dropped: drop them before emptying it. drop(out); @@ -326,7 +331,21 @@ impl Gpu { #[cfg(target_arch = "wasm32")] { let out = f(); - self.ctx.check_health().map(|()| out).map_err(FrameError::Retired) + self.finish_memory_attempt(out) + } + } + + /// Retirement without a scoped error (for example, a readback timeout) must release + /// the same retained resources as a rejected allocation before the caller retries. + fn finish_memory_attempt(&self, out: T) -> Result { + match self.ctx.check_health() { + Ok(()) => Ok(out), + Err(error) => { + // Dropping the result can return leased textures to the pool. + drop(out); + self.ctx.discard_kept(); + Err(FrameError::Retired(error)) + } } } diff --git a/crates/gpu/src/tests.rs b/crates/gpu/src/tests.rs index 9ab9fd49d..7b85bfc85 100644 --- a/crates/gpu/src/tests.rs +++ b/crates/gpu/src/tests.rs @@ -998,6 +998,49 @@ fn within_memory_returns_the_work_when_there_is_room() { assert_eq!((img.width, img.height), (64, 32)); } +/// A timeout can retire a context without any device error scope reporting a failure. +/// Queued jobs keep that context alive, so CPU fallback must not wait for its last owner +/// to drop before releasing cached uploads and working textures (#3357). +fn retired_attempt_releases_kept_resources(retire_before: bool) { + hold_gpu_lock(); + let made = Gpu::try_headless(); + let Some(g) = require_gpu("retired memory attempt", made.as_ref()) else { return }; + let buf = Arc::new(effectcraft_effects::Buf { img: Image::new(16, 16), offset: [0.0; 2], scale: 1.0 }); + drop(g.ctx.upload_buf(&buf).expect("a small cached upload")); + drop(crate::context::Enc::new(&g.ctx).image(32, 16)); + assert_eq!(g.ctx.upload_entries(), 1); + assert!(g.ctx.pool_retained_bytes() > 0); + // An earlier frame can return a lease after retirement; it must not refill the pool. + let earlier = crate::context::Enc::new(&g.ctx).image(48, 16); + let retire = g.ctx.device_failure_notifier(); + if retire_before { + retire("synthetic readback retirement".into()); + } + let called = std::cell::Cell::new(false); + let result = g.within_memory(|| { + called.set(true); + let out = crate::context::Enc::new(&g.ctx).image(64, 16); + retire("synthetic readback retirement".into()); + out + }); + assert!(matches!(result, Err(crate::FrameError::Retired(_)))); + assert_eq!(called.get(), !retire_before); + assert_eq!(g.ctx.upload_entries(), 0, "CPU fallback must not retain uploaded layers"); + assert_eq!(g.ctx.pool_retained_bytes(), 0, "CPU fallback must not retain working textures"); + drop(earlier); + assert_eq!(g.ctx.pool_retained_bytes(), 0, "late leases cannot refill a retired pool"); +} + +#[test] +fn within_memory_releases_caches_when_already_retired() { + retired_attempt_releases_kept_resources(true); +} + +#[test] +fn within_memory_releases_caches_when_retired_during_work() { + retired_attempt_releases_kept_resources(false); +} + /// A texture the device rejected (deliberately too wide), made outside any frame under a scope /// of its own: the view a frame then makes of it is the reported validation error. fn rejected_texture(g: &Gpu) -> wgpu::Texture { diff --git a/crates/render/src/pipeline.rs b/crates/render/src/pipeline.rs index 1d9efc795..f856db82c 100644 --- a/crates/render/src/pipeline.rs +++ b/crates/render/src/pipeline.rs @@ -500,6 +500,22 @@ impl<'a> Renderer<'a> { /// Render a composition at comp time `t` (transparent background, comp size × scale). /// Top-level frames go to the GPU when [`Self::active_accel`] is set and handles them. pub fn comp_frame(&self, comp_id: ItemId, t: Tick) -> Image { + self.comp_frame_with(comp_id, t, false, |a| Ok::<_, std::convert::Infallible>(a.comp_frame(self, comp_id, t))).unwrap_or_else(|never| match never {}) + } + + /// Render a frame, validating the accelerated attempt before starting any CPU fallback. + /// The caller's scope must run only the accelerator, not this whole method: a GPU fault + /// must release failed resources before a potentially large CPU render (#3357). + /// A scope error returns immediately; the caller can then retry with a fresh CPU renderer + /// without accelerator-produced cache entries. Healthy declines and Auto timing retain + /// the same behavior as [`Self::comp_frame`]. Its fallback disables acceleration for + /// effects too: GPU work must never escape the supplied error scope. + pub fn comp_frame_scoped(&self, comp_id: ItemId, t: Tick, scope: impl FnOnce(&dyn Accelerator) -> Result, E>) -> Result { + self.comp_frame_with(comp_id, t, true, scope) + } + + fn comp_frame_with(&self, comp_id: ItemId, t: Tick, scoped: bool, scope: impl FnOnce(&dyn Accelerator) -> Result, E>) -> Result { + let cpu = if scoped { self.with_accel(None) } else { self.with_accel(self.accel) }; // Classic 3D runs composite on the GPU too (M12.7), so Auto sends 3D comps there as well; // Auto renders each comp on whichever compositor measured faster ([`AutoPick`]). if self.depth == 0 @@ -508,24 +524,24 @@ impl<'a> Renderer<'a> { let auto = self.frame_auto(a, comp_id, false); if auto.is_none_or(|(p, key)| p.choose(key)) { let t0 = web_time::Instant::now(); - if let Some(img) = a.comp_frame(self, comp_id, t) { + if let Some(img) = scope(a)? { if let Some((p, key)) = auto { p.record(key, true, t0.elapsed().as_secs_f64() * 1e3); } - return img; + return Ok(img); } if let Some((p, key)) = auto { p.declined(key); } } let t0 = web_time::Instant::now(); - let img = self.comp_frame_cpu(comp_id, t); + let img = cpu.comp_frame_cpu(comp_id, t); if let Some((p, key)) = auto { p.record(key, false, t0.elapsed().as_secs_f64() * 1e3); } - return img; + return Ok(img); } - self.comp_frame_cpu(comp_id, t) + Ok(cpu.comp_frame_cpu(comp_id, t)) } /// The Auto timing history and key for a top-level frame of `comp` (`None` unless the diff --git a/crates/render/src/tests/auto.rs b/crates/render/src/tests/auto.rs index 22f1a6215..5cc503173 100644 --- a/crates/render/src/tests/auto.rs +++ b/crates/render/src/tests/auto.rs @@ -14,12 +14,13 @@ struct MockAccel { handle: bool, calls: AtomicUsize, pick: AutoPick, + effect_calls: AtomicUsize, } impl MockAccel { /// Construct a renderer stand-in without making assumptions about scheduling or speed. fn new(handle: bool) -> MockAccel { - MockAccel { handle, calls: Default::default(), pick: Default::default() } + MockAccel { handle, calls: Default::default(), pick: Default::default(), effect_calls: Default::default() } } /// Number of full-frame requests dispatched to this accelerator. fn calls(&self) -> usize { @@ -42,6 +43,7 @@ impl Accelerator for MockAccel { }) } fn supports_effect(&self, _id: &str) -> bool { + self.effect_calls.fetch_add(1, Ordering::SeqCst); false } fn effects(&self, _chain: &[crate::FxStep], _buf: &crate::Buf, _levels: Option) -> Option { @@ -160,3 +162,87 @@ fn auto_pick_follows_changing_costs() { assert!(pick.stats(AutoKey::new(ItemId(7), 1.0, true)).is_none()); assert!(pick.stats(AutoKey::new(ItemId(7), 0.5, false)).is_none()); } + +/// Counts actual CPU source reads and rejects any CPU fallback inside a GPU error scope. +struct ScopedFootage { + in_scope: std::sync::atomic::AtomicBool, + reads: AtomicUsize, +} +impl crate::FootageSource for ScopedFootage { + fn frame(&self, _: ItemId, f: &effectcraft_project::Footage, _: Tick) -> Option> { + assert!(!self.in_scope.load(Ordering::SeqCst), "CPU fallback ran before GPU validation/cleanup"); + self.reads.fetch_add(1, Ordering::SeqCst); + Some(std::sync::Arc::new(crate::Image::filled(f.width, f.height, [1.0, 0.0, 0.0, 1.0]))) + } +} + +fn scoped_project() -> (Project, ItemId) { + let (mut p, cid) = project(); + let comp = p.comp(cid).unwrap().clone(); + let footage = effectcraft_project::Footage { + path: "synthetic.png".into(), + kind: effectcraft_project::FootageKind::Still, + width: 50, + height: 50, + has_video: true, + duration: Tick::from_seconds_f64(2.0), + ..Default::default() + }; + let item = p.add_item("Source", Label::Red, None, ItemKind::Footage(footage)); + let mut layer = build::layer(&mut p, &comp, "Source", LayerSource::Footage { item }, (50, 50), None); + super::add_effect_200(&mut p, &mut layer, "ec.blur.gaussian", &[]); + p.comp_mut(cid).unwrap().layers = vec![layer].into(); + (p, cid) +} + +/// #3357: a declined GPU frame must not allocate a CPU frame only to discard it when the +/// enclosing GPU scope reports a fault. The caller retries on a fresh CPU renderer once. +#[test] +fn failed_gpu_scope_returns_before_cpu_fallback() { + let (p, cid) = scoped_project(); + for handled in [false, true] { + let a = MockAccel::new(handled); + let src = ScopedFootage { in_scope: false.into(), reads: 0.into() }; + let mut r = Renderer::new(&p, &src, RenderOpts { backend: Backend::Gpu, ..Default::default() }); + r.accel = Some(&a); + let result = r.comp_frame_scoped(cid, Tick::ZERO, |a| { + src.in_scope.store(true, Ordering::SeqCst); + let frame = a.comp_frame(&r, cid, Tick::ZERO); + drop(frame); + src.in_scope.store(false, Ordering::SeqCst); + Err::, _>("device retired") + }); + assert_eq!(result.unwrap_err(), "device retired"); + assert_eq!(src.reads.load(Ordering::SeqCst), 0); + let frame = r.with_accel(None).comp_frame(cid, Tick::ZERO); + assert!(frame.get(100, 50)[3] > 0.99); + assert_eq!(src.reads.load(Ordering::SeqCst), 1); + assert_eq!(a.calls(), 1); + } +} + +/// A healthy decline closes its scope before CPU work, and still records Auto timings. +#[test] +fn healthy_gpu_decline_renders_cpu_once_outside_scope() { + let (p, cid) = scoped_project(); + let a = MockAccel::new(false); + let src = ScopedFootage { in_scope: false.into(), reads: 0.into() }; + let mut r = Renderer::new(&p, &src, RenderOpts { backend: Backend::Auto, ..Default::default() }); + r.accel = Some(&a); + let frame = r + .comp_frame_scoped(cid, Tick::ZERO, |a| { + src.in_scope.store(true, Ordering::SeqCst); + let frame = a.comp_frame(&r, cid, Tick::ZERO); + src.in_scope.store(false, Ordering::SeqCst); + Ok::<_, ()>(frame) + }) + .unwrap(); + assert!(frame.get(100, 50)[3] > 0.99); + assert_eq!(src.reads.load(Ordering::SeqCst), 1); + assert_eq!(a.calls(), 1); + assert_eq!(a.pick.stats(AutoKey::new(cid, 1.0, false)).unwrap().cpu_frames, 1); + // Auto's next frame stays on CPU, without entering a GPU scope at all. + r.comp_frame_scoped(cid, Tick::ZERO, |_| -> Result, ()> { panic!("Auto CPU choice must not enter GPU error scopes") }).unwrap(); + assert_eq!(src.reads.load(Ordering::SeqCst), 2); + assert_eq!(a.effect_calls.load(Ordering::SeqCst), 0, "CPU effects must not use the GPU outside its scope"); +} diff --git a/crates/ui-egui/src/frames.rs b/crates/ui-egui/src/frames.rs index 8bfcd73e8..72d1edba6 100644 --- a/crates/ui-egui/src/frames.rs +++ b/crates/ui-egui/src/frames.rs @@ -1926,7 +1926,7 @@ impl Worker { } if let Some(g) = gpu { // The GPU composites and the frame is read back. - return match g.within_memory(|| r.comp_frame(job.comp, job.t)) { + return match r.comp_frame_scoped(job.comp, job.t, |a| g.within_memory(|| a.comp_frame(&r, job.comp, job.t))) { Ok(img) => { self.gpu_succeeded(); cpu_frame(&img, &job.src.project, &job.opts)