mirror of
https://github.com/storytold/photocraft.git
synced 2026-10-24 09:57:08 -06:00
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
00xJS
Claude Opus 5.5
parent
87905d448f
commit
3175a3c4cb
@@ -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 {
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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());
|
||||
}
|
||||
@@ -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());
|
||||
}
|
||||
Reference in New Issue
Block a user