Merge pull request #3363 from FredApps/fix/3357-gpu-fallback-exit

#3357: release retired GPU resources before CPU fallback

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<T>(&self, f: impl FnOnce() -> T) -> Result<T, FrameError> {
-        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<String> = [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<T>(&self, out: T) -> Result<T, FrameError> {
+        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))
+            }
         }
     }
 

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 {

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<E>(&self, comp_id: ItemId, t: Tick, scope: impl FnOnce(&dyn Accelerator) -> Result<Option<Image>, E>) -> Result<Image, E> {
+        self.comp_frame_with(comp_id, t, true, scope)
+    }
+
+    fn comp_frame_with<E>(&self, comp_id: ItemId, t: Tick, scoped: bool, scope: impl FnOnce(&dyn Accelerator) -> Result<Option<Image>, E>) -> Result<Image, E> {
+        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

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<f32>) -> Option<crate::Buf> {
@@ -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<std::sync::Arc<crate::Image>> {
+        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::<Option<crate::Image>, _>("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<Option<crate::Image>, ()> { 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");
+}

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)