From 3568ede54e79f0b08612d0a693bdfbe289771390 Mon Sep 17 00:00:00 2001 From: 00xJ0 <56056764+00xJ0@users.noreply.github.com> Date: Tue, 20 Oct 2026 18:13:04 -0700 Subject: [PATCH 1/3] GPU compositor: build its pipelines in parallel; DX12's FXC compiles one at a time and took ~8 s on one core (#6419) --- crates/gpu/src/lib.rs | 52 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 9 deletions(-) diff --git a/crates/gpu/src/lib.rs b/crates/gpu/src/lib.rs index 334944daad..e2cf262ef3 100644 --- a/crates/gpu/src/lib.rs +++ b/crates/gpu/src/lib.rs @@ -642,9 +642,9 @@ impl Compositor { /// to the CPU compositor. Where errors only arrive asynchronously (WebGPU), creation is assumed /// to have worked. pub fn try_new_with_format(device: &wgpu::Device, acc_format: wgpu::TextureFormat) -> Result { - match first_error(device, || Self::new_with_format(device, acc_format)) { - (comp, None) => Ok(comp), - (_, Some(e)) => Err(Unsupported(format!("couldn't build the compositor's pipelines: {e}"))), + match first_error(device, || Self::build(device, acc_format)) { + ((comp, None), None) => Ok(comp), + ((_, Some(e)), _) | (_, Some(e)) => Err(Unsupported(format!("couldn't build the compositor's pipelines: {e}"))), } } @@ -652,6 +652,18 @@ impl Compositor { /// `Rgba16Float` on adapters that can't render to `Rgba32Float` (e.g. some Intel Vulkan drivers), /// which otherwise panics at render-pipeline creation. See `Compositor::preferred_acc_format`. pub fn new_with_format(device: &wgpu::Device, acc_format: wgpu::TextureFormat) -> Self { + let (comp, error) = Self::build(device, acc_format); + if let Some(e) = error { + log::error!("GPU compositor: couldn't build its pipelines: {e}"); + } + comp + } + + /// The compositor and the first error raised building its (non-map) pipelines. They are + /// built in parallel, each inside its own error scope on the thread that builds it (wgpu's + /// scopes are per thread): on DX12, FXC compiles one shader at a time and the ~30 pipelines + /// took about 8 s on one core (#6419). + fn build(device: &wgpu::Device, acc_format: wgpu::TextureFormat) -> (Self, Option) { let module = device.create_shader_module(wgpu::ShaderModuleDescriptor { label: Some("pc_compose"), source: wgpu::ShaderSource::Wgsl(SHADER.into()) }); let bgl0 = device.create_bind_group_layout(&wgpu::BindGroupLayoutDescriptor { label: Some("pc_compose_uniforms"), @@ -686,19 +698,40 @@ impl Compositor { cache: None, })) }; - let mut pipelines: HashMap<_, _> = - Kernel::DRAWN.iter().filter(|k| !k.is_map()).filter_map(|&k| Some(((k, acc_format), pipeline(k, acc_format)?))).collect(); // Effect maps render to R32Float / R16Float, which some adapters (e.g. GL without float // render targets) can't: then layer effects use the CPU compositor and the rest stays here. - let maps = Kernel::DRAWN.iter().filter(|k| k.is_map()).flat_map(|&k| [(k, MAP32), (k, MAP16)]); - let (maps, map_error) = first_error(device, || maps.filter_map(|key| Some((key, pipeline(key.0, key.1)?))).collect::>()); + let keys = Kernel::DRAWN.iter().filter(|k| !k.is_map()).map(|&k| (k, acc_format)); + let keys: Vec<_> = keys.chain(Kernel::DRAWN.iter().filter(|k| k.is_map()).flat_map(|&k| [(k, MAP32), (k, MAP16)])).collect(); + let built = fx::par_map(keys, |key| { + let (built, error) = first_error(device, || pipeline(key.0, key.1)); + (key, built, error) + }); + let (mut pipelines, mut maps) = (HashMap::new(), Vec::new()); + let (mut error, mut map_error) = (None, None); + for (key, built, e) in built { + let map = key.0.is_map(); + if let Some(e) = e { + if map { + map_error.get_or_insert(e) + } else { + error.get_or_insert(e) + }; + continue; + } + let Some(p) = built else { continue }; + if map { + maps.push((key, p)); + } else { + pipelines.insert(key, p); + } + } if let Some(e) = &map_error { log::info!("GPU compositor: no effect-map pipelines ({e}); layer effects use the CPU"); } else { pipelines.extend(maps); } let dummy = Tex::new(device, "pc_compose_dummy", 1, 1, wgpu::TextureFormat::Rgba8Unorm, wgpu::TextureUsages::TEXTURE_BINDING).view; - Self { + let comp = Self { kit: Kit { pipelines, bgl0, bgl1, dummy }, pool: Vec::new(), residents: HashMap::new(), @@ -726,7 +759,8 @@ impl Compositor { health: None, timer: None, } - .with_texture_limit(device.limits().max_texture_dimension_2d) + .with_texture_limit(device.limits().max_texture_dimension_2d); + (comp, error) } fn with_texture_limit(mut self, max: u32) -> Self { From d383afe428fa41d4ee34ed88e953eefdc4969b0e Mon Sep 17 00:00:00 2001 From: 00xJ0 <56056764+00xJ0@users.noreply.github.com> Date: Tue, 20 Oct 2026 18:40:26 -0700 Subject: [PATCH 2/3] Test: a compositor pipeline that fails on a worker thread is an error, not a panic (#6419) --- crates/gpu/tests/pipeline_errors.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 crates/gpu/tests/pipeline_errors.rs diff --git a/crates/gpu/tests/pipeline_errors.rs b/crates/gpu/tests/pipeline_errors.rs new file mode 100644 index 0000000000..2ecbb8db74 --- /dev/null +++ b/crates/gpu/tests/pipeline_errors.rs @@ -0,0 +1,22 @@ +//! #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()); +} From 3175a3c4cb378b51316f595dfab5e13e41fd2ce7 Mon Sep 17 00:00:00 2001 From: David Mann Date: Thu, 22 Oct 2026 21:42:40 +0200 Subject: [PATCH 3/3] 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()); -}