diff --git a/crates/gpu/src/lib.rs b/crates/gpu/src/lib.rs index 05e12217da..f9b6f11e57 100644 --- a/crates/gpu/src/lib.rs +++ b/crates/gpu/src/lib.rs @@ -684,9 +684,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}"))), } } @@ -694,6 +694,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"), @@ -728,19 +740,42 @@ 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(); + // 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); + continue; + } + error.get_or_insert(e); + } + 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(), @@ -773,7 +808,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 { 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()); +}