Merge pull request #1326 from tylerjenningsw/fix/gpu-frame-validation

GPU: contain frame-local validation failures (refs #1319)

crates/gpu/src/context.rs

@@ -74,6 +74,8 @@ struct Lease {
     pool: Weak<Mutex<Pool>>,
     /// The [`Enc`] that took it (see [`Pool::owned`]).
     owner: u64,
+    /// The pool's [`Pool::generation`] when it was taken.
+    generation: u64,
 }
 
 impl std::fmt::Debug for Lease {
@@ -87,7 +89,7 @@ impl Drop for Lease {
         if let Some(pool) = self.pool.upgrade()
             && let Ok(mut p) = pool.lock()
         {
-            p.release_owned(self.texture.clone(), self.owner);
+            p.release_owned(self.texture.clone(), self.owner, self.generation);
         }
     }
 }
@@ -118,6 +120,8 @@ pub(crate) struct Pool {
     /// working texture of another `Enc` before it is gone.
     owned: HashMap<u64, HashMap<(u32, u32), Vec<wgpu::Texture>>>,
     next_owner: u64,
+    /// Raised by [`Pool::discard`]: a texture taken before it never comes back to the pool.
+    generation: u64,
 }
 
 /// In-`Enc` texture reuse ([`Pool::owned`]); tests turn it off to compare.
@@ -276,7 +280,10 @@ impl Pool {
         }
     }
 
-    fn release_owned(&mut self, t: wgpu::Texture, owner: u64) {
+    fn release_owned(&mut self, t: wgpu::Texture, owner: u64, generation: u64) {
+        if generation != self.generation {
+            return;
+        }
         if let Some(m) = self.owned.get_mut(&owner) {
             m.entry((t.width(), t.height())).or_default().push(t);
             return;
@@ -306,6 +313,18 @@ impl Pool {
         self.free.clear();
         self.bytes = 0;
     }
+
+    /// After a failed frame (#1319): drop every kept texture, the free ones and those waiting
+    /// for an open encoder or kept by one, and the textures still out never return. The
+    /// encoders' bookkeeping stays, so reuse remains ordered.
+    fn discard(&mut self) {
+        self.clear();
+        self.released.clear();
+        for kept in self.owned.values_mut() {
+            kept.clear();
+        }
+        self.generation = self.generation.wrapping_add(1);
+    }
 }
 
 /// Uniform parameter block shared by every kernel (see `shaders/common.wgsl`).
@@ -1015,8 +1034,9 @@ impl GpuContext {
     }
 
     fn texture(&self, w: u32, h: u32) -> wgpu::Texture {
+        // Named with its size, so a GPU error about it says which texture failed (#1319).
         self.device.create_texture(&wgpu::TextureDescriptor {
-            label: None,
+            label: Some(&format!("working image {w}×{h}")),
             size: wgpu::Extent3d { width: w.max(1), height: h.max(1), depth_or_array_layers: 1 },
             mip_level_count: 1,
             sample_count: 1,
@@ -1031,10 +1051,10 @@ impl GpuContext {
     /// one is zeroed.
     fn pooled(&self, w: u32, h: u32, owner: u64) -> (GpuImage, bool) {
         let (w, h) = (w.max(1), h.max(1));
-        let reused = self.pool.lock().ok().and_then(|mut p| p.take_owned(owner, w, h).or_else(|| p.take(w, h)));
+        let (reused, generation) = self.pool.lock().map(|mut p| (p.take_owned(owner, w, h).or_else(|| p.take(w, h)), p.generation)).unwrap_or((None, u64::MAX));
         let was_reused = reused.is_some();
         let texture = reused.unwrap_or_else(|| self.texture(w, h));
-        let lease = Arc::new(Lease { texture: texture.clone(), pool: Arc::downgrade(&self.pool), owner });
+        let lease = Arc::new(Lease { texture: texture.clone(), pool: Arc::downgrade(&self.pool), owner, generation });
         (GpuImage { texture, width: w, height: h, _lease: Some(lease), levels: None }, was_reused)
     }
 
@@ -1150,6 +1170,21 @@ impl GpuContext {
             s.clear();
         }
     }
+
+    /// After a failed frame: forget every texture and buffer kept for reuse, since any of them
+    /// may be one the device rejected (#1319). Later frames allocate afresh.
+    pub fn discard_kept(&self) {
+        self.clear_uploads();
+        if let Ok(mut p) = self.pool.lock() {
+            p.discard();
+        }
+        if let Ok(mut z) = self.zeros.lock() {
+            z.clear();
+        }
+        if let Ok(mut c) = self.particle_states.lock() {
+            c.clear();
+        }
+    }
 }
 
 /// Records GPU work for one render (a command encoder submitted at readback or the end).

crates/gpu/src/lib.rs

@@ -97,6 +97,29 @@ pub use wgpu;
 
 use crate::context::Enc;
 
+/// Why [`Gpu::within_memory`] discarded a frame.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum FrameError {
+    /// The GPU context is retired (device lost, a readback timed out, an uncaptured error).
+    Retired(String),
+    /// Out of video memory (or a driver reporting a failed allocation as an internal error),
+    /// with any validation errors it went on to cause.
+    Memory(String),
+    /// The device rejected a command with no allocation failure before it (an invalid texture
+    /// or parameters, #1319): a compositor bug, not memory pressure.
+    Validation(String),
+}
+
+impl std::fmt::Display for FrameError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            FrameError::Retired(e) | FrameError::Memory(e) | FrameError::Validation(e) => f.write_str(e),
+        }
+    }
+}
+
+impl std::error::Error for FrameError {}
+
 /// The GPU compositor (cheap to clone; one device shared by all clones).
 #[derive(Clone)]
 pub struct Gpu {
@@ -265,33 +288,42 @@ impl Gpu {
         }
     }
 
-    /// Run work in native thread-local out-of-memory and internal-error scopes. A failed
-    /// result is discarded and cached allocations are released; callers can retry with a fresh
-    /// CPU renderer. Some drivers report a failed allocation as an internal error rather than
-    /// out-of-memory (Adreno's `ERROR_INITIALIZATION_FAILED`, #591), so both count.
+    /// Run work in native thread-local out-of-memory, internal-error and validation scopes. A
+    /// failed result is discarded and the textures and buffers kept for reuse are released;
+    /// callers can retry with a fresh CPU renderer. Some drivers report a failed allocation as an
+    /// internal error rather than out-of-memory (Adreno's `ERROR_INITIALIZATION_FAILED`, #591),
+    /// so both count. A failed allocation leaves an invalid texture whose use is then a
+    /// validation error (`Texture::create_view`, #1319): caught here, it fails this frame only
+    /// instead of retiring the GPU compositor for the session as an uncaptured error.
     /// 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, String> {
-        self.ctx.check_health()?;
+    pub fn within_memory<T>(&self, f: impl FnOnce() -> T) -> Result<T, FrameError> {
+        self.ctx.check_health().map_err(FrameError::Retired)?;
         #[cfg(not(target_arch = "wasm32"))]
         {
             let oom = self.ctx.device.push_error_scope(wgpu::ErrorFilter::OutOfMemory);
             let internal = self.ctx.device.push_error_scope(wgpu::ErrorFilter::Internal);
+            let validation = self.ctx.device.push_error_scope(wgpu::ErrorFilter::Validation);
             let out = f();
+            let validation = pollster::block_on(validation.pop());
             let internal = pollster::block_on(internal.pop());
             let oom = pollster::block_on(oom.pop());
-            match internal.or(oom) {
-                Some(e) => {
-                    self.ctx.clear_uploads();
-                    Err(e.to_string())
-                }
-                None => self.ctx.check_health().map(|()| out),
+            let memory = oom.is_some() || internal.is_some();
+            // 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);
             }
+            // Its textures go back to the pool when dropped: drop them before emptying it.
+            drop(out);
+            self.ctx.discard_kept();
+            let errors = errors.join("; then: ");
+            Err(if memory { FrameError::Memory(errors) } else { FrameError::Validation(errors) })
         }
         #[cfg(target_arch = "wasm32")]
         {
             let out = f();
-            self.ctx.check_health().map(|()| out)
+            self.ctx.check_health().map(|()| out).map_err(FrameError::Retired)
         }
     }
 

crates/gpu/src/particles.rs

@@ -33,6 +33,13 @@ pub(crate) struct Checkpoints {
     clock: u64,
 }
 
+impl Checkpoints {
+    /// Forget every kept state (after a failed frame, whose buffers may be invalid).
+    pub(crate) fn clear(&mut self) {
+        self.list.clear();
+    }
+}
+
 fn pipes(g: &GpuContext) -> &Pipes {
     g.particles.get_or_init(|| {
         let d = &g.device;

crates/gpu/src/tests.rs

@@ -864,6 +864,71 @@ fn within_memory_returns_the_work_when_there_is_room() {
     assert_eq!((img.width, img.height), (64, 32));
 }
 
+/// 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 {
+    let scope = g.ctx.device.push_error_scope(wgpu::ErrorFilter::Validation);
+    let too_wide = g.ctx.device.limits().max_texture_dimension_2d.saturating_add(1);
+    let t = g.ctx.device.create_texture(&wgpu::TextureDescriptor {
+        label: Some("deliberately too wide"),
+        size: wgpu::Extent3d { width: too_wide, height: 1, depth_or_array_layers: 1 },
+        mip_level_count: 1,
+        sample_count: 1,
+        dimension: wgpu::TextureDimension::D2,
+        format: wgpu::TextureFormat::Rgba8Unorm,
+        usage: wgpu::TextureUsages::TEXTURE_BINDING,
+        view_formats: &[],
+    });
+    assert!(pollster::block_on(scope.pop()).is_some(), "the device rejected the texture");
+    t
+}
+
+/// A frame that makes a view of an invalid texture ("Texture with '' label is invalid", #1319)
+/// fails alone: the validation error doesn't reach the device's handler (which retires the
+/// compositor for the session, and panics in tests), and the next frame composites on the GPU,
+/// read back, like the CPU.
+#[test]
+fn a_view_of_an_invalid_texture_fails_the_frame_not_the_compositor() {
+    hold_gpu_lock();
+    let Some(g) = Gpu::headless() else { return };
+    let rejected = rejected_texture(&g);
+    let err = g.within_memory(|| rejected.create_view(&Default::default())).expect_err("the frame fails");
+    assert!(matches!(&err, crate::FrameError::Validation(e) if e.contains("deliberately too wide")), "{err:?}");
+    assert!(g.ctx.check_health().is_ok(), "the compositor stays on: {:?}", g.ctx.check_health());
+    let s = blend_scene(BitDepth::Bpc32, BlendMode::Normal);
+    let cpu = Renderer::new(&s.p, &Pattern, RenderOpts { backend: Backend::Cpu, ..opts() }).comp_frame_cpu(s.cid, Tick::ZERO);
+    let mut r = Renderer::new(&s.p, &Pattern, RenderOpts { backend: Backend::Gpu, ..opts() });
+    r.accel = Some(&g);
+    let img = g.within_memory(|| g.render(&r, s.cid, Tick::ZERO)).expect("the next frame isn't rejected").expect("the GPU renders it");
+    let mut d = diff(&cpu, &img, tolerance(BitDepth::Bpc32));
+    d.quantized = false;
+    check("after a rejected frame", Some(d), 0.0);
+}
+
+/// No texture of a failed frame is reused: not one released while another encoder is still
+/// open (it waits for that encoder), nor one taken before the failure and handed back after.
+#[test]
+fn a_failed_frame_returns_no_texture_to_the_pool() {
+    hold_gpu_lock();
+    let Some(g) = Gpu::headless() else { return };
+    let rejected = rejected_texture(&g);
+    let earlier = g.within_memory(|| crate::context::Enc::new(&g.ctx).image(48, 16)).expect("an earlier frame");
+    let mut other = crate::context::Enc::new(&g.ctx);
+    other.encoder();
+    g.within_memory(|| (crate::context::Enc::new(&g.ctx).image(64, 32), rejected.create_view(&Default::default()))).expect_err("the frame fails");
+    drop(earlier);
+    drop(other);
+    let (hits, _) = g.ctx.pool_stats();
+    let (a, b) = g
+        .within_memory(|| {
+            let mut e = crate::context::Enc::new(&g.ctx);
+            (e.image(64, 32), e.image(48, 16))
+        })
+        .expect("the next frame");
+    assert_eq!(((a.width, a.height), (b.width, b.height)), ((64, 32), (48, 16)));
+    assert_eq!(g.ctx.pool_stats().0, hits, "no texture of the failed frame was reused");
+}
+
 /// A buffer whose allocation fails (here: over the device's buffer size limit; on Adreno an
 /// internal `ERROR_INITIALIZATION_FAILED`) is reported to the caller's error scope instead of
 /// panicking in `create_buffer_init`'s mapped range on a frame thread (#591).

crates/ui-egui/src/frames.rs

@@ -31,7 +31,7 @@ use effectcraft_engine::project::{Comp, ItemId, ItemKind, LayerSource, Node, Pro
 use effectcraft_engine::render::disk_cache::{self, DiskCache};
 use effectcraft_engine::render::{CancellableFootage, ExprHost, FootageSource, LayerCache, RenderOpts, Renderer};
 use effectcraft_engine::time::Tick;
-use effectcraft_gpu::{DisplayFrame, Gpu};
+use effectcraft_gpu::{DisplayFrame, FrameError, Gpu};
 use rayon::prelude::*;
 
 /// Comp identities (and the project snapshots behind them) the RAM preview keeps frames of.
@@ -1318,11 +1318,16 @@ impl Worker {
 
     /// A failed scoped attempt retries on CPU. Retired contexts cannot recover by reducing
     /// their budget; only a healthy device's allocation failure uses the upstream backoff.
-    fn gpu_attempt_failed(&self, gpu: &Gpu, e: &str) {
+    fn gpu_attempt_failed(&self, gpu: &Gpu, e: &FrameError) {
         if gpu.context().check_health().is_err() {
             log::warn!("the GPU context is retired ({e}); the frame renders on the CPU");
             return;
         }
+        // Not memory pressure: keeping fewer GPU frames wouldn't help (#1319).
+        if let FrameError::Validation(_) = e {
+            log::warn!("the GPU rejected the frame ({e}); it renders on the CPU, the next frame tries the GPU again");
+            return;
+        }
         log::warn!("the GPU ran out of memory ({e}); the frame renders on the CPU and fewer GPU frames are kept");
         if let Ok(mut c) = self.cache.lock() {
             c.gpu_out_of_memory();

docs/architecture.md

@@ -345,7 +345,10 @@ as NaNs (a kernel that skips pixels fails the oracle) and validation errors pani
 (`ui-egui::frames`); headless renders, the CLI (unless `--gpu`) and CI use the CPU. Outside
 tests, device errors are logged rather than fatal. A viewer frame that runs out of video memory
 (`Gpu::within_memory`) renders on the CPU, the uploaded layers and pooled textures are freed, and
-the RAM preview keeps half as many GPU frames from then on. A frame whose render panics is
+the RAM preview keeps half as many GPU frames from then on. The same guard catches a frame's
+validation errors (an invalid texture, often left by a failed allocation): that frame renders on
+the CPU and the kept textures are freed, but the compositor stays on and, with no allocation
+failure before it, the RAM preview keeps its GPU frames (#1319). A frame whose render panics is
 rendered again on the CPU or released, never left in progress. Native readbacks and timing waits
 share a finite deadline; failure retires pending readbacks exactly once, and late results are
 discarded. The desktop executable and browser page own their shared device's error/loss handlers and forward