From 3175a3c4cb378b51316f595dfab5e13e41fd2ce7 Mon Sep 17 00:00:00 2001 From: David Mann Date: Thu, 22 Oct 2026 21:42:40 +0200 Subject: [PATCH] Integration: keep failed compositor pipelines in the map (#8118) The parallel build skipped a non-map pipeline whose creation raised an error, so with new_with_format (errors logged, compositor still returned) its key was missing from kit.pipelines and the first draw panicked on the `self.kit.pipelines[&(kernel, acc)]` index. As before the PR, a failed non-map pipeline now keeps its invalid handle (draws report GPU errors); only a failed effect-map pipeline is skipped (all maps go to the CPU). Test: a_compositor_with_failed_pipelines_draws_without_panicking builds new_with_format(Rgb9e5Ufloat) on a device with an uncaptured-error handler and renders: it panicked at the index without the fix. The PR's test moves into tests/it (one test binary) and takes GPU_LOCK. Co-authored-by: 00xJS <56056764+00xJS@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/gpu/src/lib.rs | 12 ++++--- crates/gpu/tests/it/main.rs | 1 + crates/gpu/tests/it/pipeline_errors.rs | 50 ++++++++++++++++++++++++++ crates/gpu/tests/pipeline_errors.rs | 22 ------------ 4 files changed, 58 insertions(+), 27 deletions(-) create mode 100644 crates/gpu/tests/it/pipeline_errors.rs delete mode 100644 crates/gpu/tests/pipeline_errors.rs diff --git a/crates/gpu/src/lib.rs b/crates/gpu/src/lib.rs index ef7d1ec4fc..8dbfe54acb 100644 --- a/crates/gpu/src/lib.rs +++ b/crates/gpu/src/lib.rs @@ -733,13 +733,15 @@ impl Compositor { let (mut error, mut map_error) = (None, None); for (key, built, e) in built { let map = key.0.is_map(); + // A failed map pipeline turns all effect maps off below. A failed non-map pipeline + // keeps its (invalid) handle: draws look pipelines up by key, so dropping it would + // turn a validation error into a missing-key panic on the first draw. if let Some(e) = e { if map { - map_error.get_or_insert(e) - } else { - error.get_or_insert(e) - }; - continue; + map_error.get_or_insert(e); + continue; + } + error.get_or_insert(e); } let Some(p) = built else { continue }; if map { diff --git a/crates/gpu/tests/it/main.rs b/crates/gpu/tests/it/main.rs index f71972c5b9..2c070f19ce 100644 --- a/crates/gpu/tests/it/main.rs +++ b/crates/gpu/tests/it/main.rs @@ -6,6 +6,7 @@ static GPU_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); mod device_loss; mod parity; +mod pipeline_errors; mod timing; mod uniform_limits; mod vector_mask; diff --git a/crates/gpu/tests/it/pipeline_errors.rs b/crates/gpu/tests/it/pipeline_errors.rs new file mode 100644 index 0000000000..28d7a56791 --- /dev/null +++ b/crates/gpu/tests/it/pipeline_errors.rs @@ -0,0 +1,50 @@ +//! #6419: the compositor's pipelines are built on several threads. A pipeline the device can't +//! build is still an `Err` from `try_new_with_format`, not a panic from wgpu's default error +//! handler (error scopes are per thread). Skips when no GPU adapter exists. + +use std::sync::Arc; + +use photocraft_color::{Color, ColorMode, SampleType}; +use photocraft_doc::Document; +use photocraft_geom::Size; +use photocraft_gpu::{Compositor, render_to_vec}; + +use crate::GPU_LOCK; + +fn device() -> Option<(wgpu::Adapter, wgpu::Device, wgpu::Queue)> { + let instance = wgpu::Instance::default(); + let Ok(adapter) = pollster::block_on(instance.request_adapter(&wgpu::RequestAdapterOptions::default())) else { + eprintln!("skipping: no adapter"); + return None; + }; + let (device, queue) = pollster::block_on(adapter.request_device(&wgpu::DeviceDescriptor::default())).ok()?; + Some((adapter, device, queue)) +} + +#[test] +fn a_pipeline_built_on_another_thread_fails_as_an_error() { + let _lock = GPU_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let Some((adapter, device, _queue)) = device() else { return }; + // Never a render target, so every accumulation pipeline fails validation. + let Err(e) = Compositor::try_new_with_format(&device, wgpu::TextureFormat::Rgb9e5Ufloat) else { + panic!("a compositor accumulating into Rgb9e5Ufloat"); + }; + assert!(e.0.contains("couldn't build the compositor's pipelines"), "{e:?}"); + // The device is still usable for a compositor it can build. + assert!(Compositor::try_new_with_format(&device, Compositor::preferred_acc_format(&adapter)).is_ok()); +} + +/// `new_with_format` on a device whose errors go to the caller's handler: a pipeline that failed +/// to build stays in the compositor as an invalid handle, so a draw reports GPU errors instead of +/// panicking on a missing pipeline. +#[test] +fn a_compositor_with_failed_pipelines_draws_without_panicking() { + let _lock = GPU_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let Some((_adapter, device, queue)) = device() else { return }; + // As the app does (`DeviceHealth::watch`): GPU errors go to a handler, not wgpu's default panic. + device.on_uncaptured_error(Arc::new(|_: wgpu::Error| {})); + let mut comp = Compositor::new_with_format(&device, wgpu::TextureFormat::Rgb9e5Ufloat); + let doc = Document::with_background("t", Size::new(8, 8), ColorMode::Rgb, SampleType::U8, Color::rgba(0.5, 0.5, 0.5, 1.0)); + // Drawing into an unrenderable format can't succeed, but must not panic. + let _ = render_to_vec(&mut comp, &device, &queue, &doc, doc.bounds()); +} diff --git a/crates/gpu/tests/pipeline_errors.rs b/crates/gpu/tests/pipeline_errors.rs deleted file mode 100644 index 2ecbb8db74..0000000000 --- a/crates/gpu/tests/pipeline_errors.rs +++ /dev/null @@ -1,22 +0,0 @@ -//! #6419: the compositor's pipelines are built on several threads. A pipeline the device can't -//! build is still an `Err` from `try_new_with_format`, not a panic from wgpu's default error -//! handler (error scopes are per thread). Skips when no GPU adapter exists. - -use photocraft_gpu::Compositor; - -#[test] -fn a_pipeline_built_on_another_thread_fails_as_an_error() { - let instance = wgpu::Instance::default(); - let Ok(adapter) = pollster::block_on(instance.request_adapter(&wgpu::RequestAdapterOptions::default())) else { - eprintln!("skipping: no adapter"); - return; - }; - let Ok((device, _queue)) = pollster::block_on(adapter.request_device(&wgpu::DeviceDescriptor::default())) else { return }; - // Never a render target, so every accumulation pipeline fails validation. - let Err(e) = Compositor::try_new_with_format(&device, wgpu::TextureFormat::Rgb9e5Ufloat) else { - panic!("a compositor accumulating into Rgb9e5Ufloat"); - }; - assert!(e.0.contains("couldn't build the compositor's pipelines"), "{e:?}"); - // The device is still usable for a compositor it can build. - assert!(Compositor::try_new_with_format(&device, Compositor::preferred_acc_format(&adapter)).is_ok()); -}