Desktop: Fix crash caused by submitting work mid surface-reconfigure (#4317)

Implement wgpu-sync

Cargo.lock

@@ -6951,6 +6951,7 @@ dependencies = [
  "vello",
  "web-sys",
  "wgpu",
+ "wgpu-sync",
 ]
 
 [[package]]
@@ -7017,6 +7018,13 @@ dependencies = [
  "wgpu-types",
 ]
 
+[[package]]
+name = "wgpu-sync"
+version = "0.0.0"
+dependencies = [
+ "wgpu",
+]
+
 [[package]]
 name = "wgpu-types"
 version = "29.0.3"

Cargo.toml

@@ -14,6 +14,7 @@ members = [
 	"frontend/wrapper",
 	"libraries/dyn-any",
 	"libraries/math-parser",
+	"libraries/wgpu-sync",
 	"node-graph/libraries/graphene-hash",
 	"node-graph/libraries/*",
 	"node-graph/nodes/*",
@@ -98,6 +99,7 @@ graphene-std = { path = "node-graph/nodes/gstd" }
 interpreted-executor = { path = "node-graph/interpreted-executor" }
 node-macro = { path = "node-graph/node-macro" }
 wgpu-executor = { path = "node-graph/libraries/wgpu-executor" }
+wgpu-sync = { path = "libraries/wgpu-sync" }
 graphite-proc-macros = { path = "proc-macros" }
 graphite-editor = { path = "editor" }
 graphene-canvas-utils = { path = "node-graph/libraries/canvas-utils" }

desktop/src/render/state.rs

@@ -1,13 +1,12 @@
 use wgpu::PresentMode;
 
 use crate::window::Window;
-use crate::wrapper::{WgpuContext, WgpuExecutor};
+use crate::wrapper::{WgpuContext, WgpuCurrentSurfaceTexture, WgpuExecutor, WgpuSurface};
 
 #[derive(derivative::Derivative)]
 #[derivative(Debug)]
 pub(crate) struct RenderState {
-	surface: wgpu::Surface<'static>,
-	context: WgpuContext,
+	surface: WgpuSurface,
 	executor: WgpuExecutor,
 	config: wgpu::SurfaceConfiguration,
 	render_pipeline: wgpu::RenderPipeline,
@@ -162,12 +161,11 @@ impl RenderState {
 			cache: None,
 		});
 
-		let wgpu_executor = WgpuExecutor::with_context(context.clone()).expect("Failed to create WgpuExecutor");
+		let executor = WgpuExecutor::with_context(context).expect("Failed to create WgpuExecutor");
 
 		Self {
 			surface,
-			context,
-			executor: wgpu_executor,
+			executor,
 			config,
 			render_pipeline,
 			transparent_texture,
@@ -193,12 +191,6 @@ impl RenderState {
 		self.desired_width = width;
 		self.desired_height = height;
 		self.surface_outdated = true;
-
-		if width > 0 && height > 0 && (self.config.width != width || self.config.height != height) {
-			self.config.width = width;
-			self.config.height = height;
-			self.surface.configure(&self.context.device, &self.config);
-		}
 	}
 
 	pub(crate) fn bind_viewport_texture(&mut self, viewport_texture: std::sync::Arc<wgpu::Texture>) {
@@ -254,6 +246,14 @@ impl RenderState {
 		if !self.surface_outdated {
 			return Ok(());
 		}
+
+		// Apply resize once per presented frame.
+		if self.desired_width > 0 && self.desired_height > 0 && (self.config.width != self.desired_width || self.config.height != self.desired_height) {
+			self.config.width = self.desired_width;
+			self.config.height = self.desired_height;
+			self.surface.configure(&self.executor.context().device, &self.config);
+		}
+
 		let ui_scale = if let Some(ui_texture) = &self.ui_texture
 			&& (self.desired_width != ui_texture.width() || self.desired_height != ui_texture.height())
 		{
@@ -266,21 +266,19 @@ impl RenderState {
 			self.render_overlays(scene);
 		}
 
-		let (output, suboptimal) = match self.surface.get_current_texture() {
-			wgpu::CurrentSurfaceTexture::Success(t) => (t, false),
-			// wgpu reports the swapchain no longer matches the underlying surface; present this frame and reconfigure after present, since `Surface::configure` panics while an acquired `SurfaceTexture` is still alive
-			wgpu::CurrentSurfaceTexture::Suboptimal(t) => (t, true),
-			// Window is minimized or behind another window: skip the frame silently and try again once it becomes visible
-			wgpu::CurrentSurfaceTexture::Occluded => return Ok(()),
-			wgpu::CurrentSurfaceTexture::Lost => return Err(RenderError::SurfaceLost),
-			wgpu::CurrentSurfaceTexture::Outdated => return Err(RenderError::SurfaceOutdated),
-			wgpu::CurrentSurfaceTexture::Timeout => return Err(RenderError::SurfaceTimeout),
-			wgpu::CurrentSurfaceTexture::Validation => return Err(RenderError::SurfaceValidation),
+		let (surface_texture, suboptimal) = match self.surface.get_current_texture(&self.executor.context().queue) {
+			WgpuCurrentSurfaceTexture::Success(t) => (t, false),
+			WgpuCurrentSurfaceTexture::Suboptimal(t) => (t, true),
+			WgpuCurrentSurfaceTexture::Occluded => return Ok(()),
+			WgpuCurrentSurfaceTexture::Lost => return Err(RenderError::SurfaceLost),
+			WgpuCurrentSurfaceTexture::Outdated => return Err(RenderError::SurfaceOutdated),
+			WgpuCurrentSurfaceTexture::Timeout => return Err(RenderError::SurfaceTimeout),
+			WgpuCurrentSurfaceTexture::Validation => return Err(RenderError::SurfaceValidation),
 		};
 
-		let view = output.texture.create_view(&wgpu::TextureViewDescriptor::default());
+		let view = surface_texture.texture.create_view(&wgpu::TextureViewDescriptor::default());
 
-		let mut encoder = self.context.device.create_command_encoder(&wgpu::CommandEncoderDescriptor { label: Some("Render Encoder") });
+		let mut encoder = self.executor.context().device.create_command_encoder(&wgpu::CommandEncoderDescriptor { label: Some("Render Encoder") });
 
 		{
 			let mut render_pass = encoder.begin_render_pass(&wgpu::RenderPassDescriptor {
@@ -318,12 +316,12 @@ impl RenderState {
 				tracing::warn!("No bind group available - showing clear color only");
 			}
 		}
-		self.context.queue.submit(std::iter::once(encoder.finish()));
+		surface_texture.queue.submit(std::iter::once(encoder.finish()));
 		window.pre_present_notify();
-		output.present();
+		surface_texture.present();
 
 		if suboptimal {
-			self.surface.configure(&self.context.device, &self.config);
+			self.surface.configure(&self.executor.context().device, &self.config);
 		}
 
 		if ui_scale.is_some() {
@@ -340,7 +338,7 @@ impl RenderState {
 		let overlays_texture_view = self.overlays_texture.as_ref().unwrap_or(&self.transparent_texture).create_view(&wgpu::TextureViewDescriptor::default());
 		let ui_texture_view = self.ui_texture.as_ref().unwrap_or(&self.transparent_texture).create_view(&wgpu::TextureViewDescriptor::default());
 
-		let bind_group = self.context.device.create_bind_group(&wgpu::BindGroupDescriptor {
+		let bind_group = self.executor.context().device.create_bind_group(&wgpu::BindGroupDescriptor {
 			layout: &self.render_pipeline.get_bind_group_layout(0),
 			entries: &[
 				wgpu::BindGroupEntry {

desktop/src/window.rs

@@ -1,6 +1,7 @@
 use crate::consts::APP_NAME;
 use crate::event::AppEventScheduler;
 use crate::wrapper::messages::MenuItem;
+use crate::wrapper::{WgpuInstance, WgpuSurface};
 use std::collections::HashMap;
 use std::sync::Arc;
 use winit::cursor::{CursorIcon, CustomCursor, CustomCursorSource};
@@ -86,8 +87,8 @@ impl Window {
 		self.winit_window.request_redraw();
 	}
 
-	pub(crate) fn create_surface(&self, instance: &wgpu::Instance) -> wgpu::Surface<'static> {
-		instance.create_surface(self.winit_window.clone()).unwrap()
+	pub(crate) fn create_surface(&self, instance: &WgpuInstance) -> WgpuSurface {
+		instance.create_surface(self.winit_window.clone()).expect("Failed to create surface")
 	}
 
 	pub(crate) fn pre_present_notify(&self) {

desktop/wrapper/src/lib.rs

@@ -11,8 +11,11 @@ pub use graphite_editor::consts::{DOUBLE_CLICK_MILLISECONDS, FILE_EXTENSION};
 pub use wgpu_executor::WgpuBackends;
 pub use wgpu_executor::WgpuContext;
 pub use wgpu_executor::WgpuContextBuilder;
+pub use wgpu_executor::WgpuCurrentSurfaceTexture;
 pub use wgpu_executor::WgpuExecutor;
 pub use wgpu_executor::WgpuFeatures;
+pub use wgpu_executor::WgpuInstance;
+pub use wgpu_executor::WgpuSurface;
 
 mod handle_desktop_wrapper_message;
 mod intercept_editor_message;

libraries/wgpu-sync/Cargo.toml

@@ -0,0 +1,10 @@
+[package]
+name = "wgpu-sync"
+description = "Helper for working with wgpu in a multi-threaded context"
+version.workspace = true
+edition.workspace = true
+authors.workspace = true
+license.workspace = true
+
+[dependencies]
+wgpu = { workspace = true }

libraries/wgpu-sync/src/lib.rs

@@ -0,0 +1,206 @@
+//! Wraps wgpu types to provide synchronization against surface configuration.
+//! Everything sharing a [`Instance`] is synchronized against that instance's [`Surface`]s.
+//! [`Surface::configure`] takes a write lock, and all other operations take a read lock.
+//!
+//! [`wgpu::Surface::configure`] recreates the swapchain and waits for the GPU to idle.
+//! A concurrent `submit`, `get_current_texture`, or `present` makes that
+//! wait fail (validation error, panic, or driver crash on the unsafe hal usage).
+//!
+//! [`Instance`] and [`Adapter`] wrapper types can be dereferenced to the underlying wgpu type.
+//! Their `create_surface`/`request_adapter`/`request_device` methods shadow the wgpu ones.
+//! These methods return wrapper types that synchronize against the parent [`Instance`].
+//! Be aware that using the underlying wgpu versions directly (through deref) results in unsynchronized objects.
+//!
+//! Guards hold their read lock for their whole lifetime.
+//! While holding a [`SurfaceTextureGuard`] reuse its [`QueueGuard`] via [`SurfaceTextureGuard::queue`] to avoid deadlock.
+
+use std::ops::Deref;
+use std::sync::{Arc, PoisonError, RwLock, RwLockReadGuard, RwLockWriteGuard};
+
+#[derive(Clone, Debug)]
+struct Lock(Arc<RwLock<()>>);
+
+impl Lock {
+	fn new() -> Self {
+		Self(Arc::new(RwLock::new(())))
+	}
+
+	fn read(&self) -> RwLockReadGuard<'_, ()> {
+		self.0.read().unwrap_or_else(PoisonError::into_inner)
+	}
+
+	fn write(&self) -> RwLockWriteGuard<'_, ()> {
+		self.0.write().unwrap_or_else(PoisonError::into_inner)
+	}
+}
+
+#[derive(Clone, Debug)]
+pub struct Instance {
+	raw: wgpu::Instance,
+	lock: Lock,
+}
+
+impl Instance {
+	pub fn new(raw: wgpu::Instance) -> Self {
+		Self { raw, lock: Lock::new() }
+	}
+
+	pub fn create_surface(&self, target: impl Into<wgpu::SurfaceTarget<'static>>) -> Result<Surface, wgpu::CreateSurfaceError> {
+		Ok(Surface {
+			raw: self.raw.create_surface(target)?,
+			lock: self.lock.clone(),
+		})
+	}
+
+	pub async fn request_adapter(&self, options: &wgpu::RequestAdapterOptions<'_, '_>) -> Result<Adapter, wgpu::RequestAdapterError> {
+		Ok(Adapter {
+			raw: self.raw.request_adapter(options).await?,
+			lock: self.lock.clone(),
+		})
+	}
+
+	pub async fn enumerate_adapters(&self, backends: wgpu::Backends) -> Vec<Adapter> {
+		self.raw
+			.enumerate_adapters(backends)
+			.await
+			.into_iter()
+			.map(|adapter| Adapter {
+				raw: adapter,
+				lock: self.lock.clone(),
+			})
+			.collect()
+	}
+}
+
+impl Deref for Instance {
+	type Target = wgpu::Instance;
+	fn deref(&self) -> &wgpu::Instance {
+		&self.raw
+	}
+}
+
+#[derive(Clone, Debug)]
+pub struct Adapter {
+	raw: wgpu::Adapter,
+	lock: Lock,
+}
+
+impl Adapter {
+	pub async fn request_device(&self, desc: &wgpu::DeviceDescriptor<'_>) -> Result<(wgpu::Device, Queue), wgpu::RequestDeviceError> {
+		let (device, queue) = self.raw.request_device(desc).await?;
+		Ok((device, Queue { raw: queue, lock: self.lock.clone() }))
+	}
+}
+
+impl Deref for Adapter {
+	type Target = wgpu::Adapter;
+	fn deref(&self) -> &wgpu::Adapter {
+		&self.raw
+	}
+}
+
+#[derive(Clone, Debug)]
+pub struct Queue {
+	raw: wgpu::Queue,
+	lock: Lock,
+}
+
+impl Queue {
+	pub fn submit<I: IntoIterator<Item = wgpu::CommandBuffer>>(&self, command_buffers: I) -> wgpu::SubmissionIndex {
+		self.lock().submit(command_buffers)
+	}
+
+	pub fn lock(&self) -> QueueGuard<'_> {
+		QueueGuard {
+			raw: &self.raw,
+			_guard: self.lock.read(),
+		}
+	}
+
+	pub fn write_buffer(&self, buffer: &wgpu::Buffer, offset: wgpu::BufferAddress, data: &[u8]) {
+		self.lock().write_buffer(buffer, offset, data);
+	}
+
+	pub fn write_texture(&self, texture: wgpu::TexelCopyTextureInfo<'_>, data: &[u8], data_layout: wgpu::TexelCopyBufferLayout, size: wgpu::Extent3d) {
+		self.lock().write_texture(texture, data, data_layout, size);
+	}
+}
+
+pub struct QueueGuard<'a> {
+	raw: &'a wgpu::Queue,
+	_guard: RwLockReadGuard<'a, ()>,
+}
+
+impl Deref for QueueGuard<'_> {
+	type Target = wgpu::Queue;
+	fn deref(&self) -> &wgpu::Queue {
+		self.raw
+	}
+}
+
+#[derive(Debug)]
+pub struct Surface {
+	raw: wgpu::Surface<'static>,
+	lock: Lock,
+}
+
+impl Surface {
+	pub fn configure(&self, device: &wgpu::Device, config: &wgpu::SurfaceConfiguration) {
+		let _guard = self.lock.write();
+		self.raw.configure(device, config);
+	}
+
+	pub fn get_current_texture<'a>(&self, queue: &'a Queue) -> CurrentSurfaceTexture<'a> {
+		debug_assert!(Arc::ptr_eq(&self.lock.0, &queue.lock.0), "queue must come from the same `Instance` as this surface");
+		let guard = queue.lock();
+		let raw = self.raw.get_current_texture();
+		match raw {
+			wgpu::CurrentSurfaceTexture::Success(raw) => CurrentSurfaceTexture::Success(SurfaceTextureGuard { raw, queue: guard }),
+			wgpu::CurrentSurfaceTexture::Suboptimal(raw) => CurrentSurfaceTexture::Suboptimal(SurfaceTextureGuard { raw, queue: guard }),
+			wgpu::CurrentSurfaceTexture::Occluded => CurrentSurfaceTexture::Occluded,
+			wgpu::CurrentSurfaceTexture::Lost => CurrentSurfaceTexture::Lost,
+			wgpu::CurrentSurfaceTexture::Outdated => CurrentSurfaceTexture::Outdated,
+			wgpu::CurrentSurfaceTexture::Timeout => CurrentSurfaceTexture::Timeout,
+			wgpu::CurrentSurfaceTexture::Validation => CurrentSurfaceTexture::Validation,
+		}
+	}
+
+	pub fn get_capabilities(&self, adapter: &wgpu::Adapter) -> wgpu::SurfaceCapabilities {
+		self.raw.get_capabilities(adapter)
+	}
+}
+
+#[derive(Debug)]
+pub enum CurrentSurfaceTexture<'a> {
+	Success(SurfaceTextureGuard<'a>),
+	Suboptimal(SurfaceTextureGuard<'a>),
+	Occluded,
+	Lost,
+	Outdated,
+	Timeout,
+	Validation,
+}
+
+pub struct SurfaceTextureGuard<'a> {
+	raw: wgpu::SurfaceTexture,
+	pub queue: QueueGuard<'a>,
+}
+
+impl SurfaceTextureGuard<'_> {
+	pub fn present(self) {
+		self.raw.present();
+	}
+}
+
+impl Deref for SurfaceTextureGuard<'_> {
+	type Target = wgpu::SurfaceTexture;
+	fn deref(&self) -> &Self::Target {
+		&self.raw
+	}
+}
+
+impl std::fmt::Debug for SurfaceTextureGuard<'_> {
+	fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+		f.debug_struct("SurfaceTexture").field("raw", &self.raw).finish()
+	}
+}

node-graph/libraries/canvas-utils/src/wasm.rs

@@ -7,7 +7,7 @@ use web_sys::js_sys::{Object, Reflect};
 use web_sys::wasm_bindgen::{JsCast, JsValue};
 use web_sys::{CanvasRenderingContext2d, HtmlCanvasElement, window};
 #[cfg(feature = "wgpu")]
-use wgpu_executor::WgpuExecutor;
+use wgpu_executor::{WgpuCurrentSurfaceTexture, WgpuExecutor, WgpuSurface};
 
 const CANVASES_OBJECT_KEY: &str = "imageCanvases";
 
@@ -52,13 +52,13 @@ impl Canvas for CanvasHandle {
 }
 
 #[cfg(feature = "wgpu")]
-pub struct CanvasSurfaceHandle(CanvasHandle, Option<Arc<wgpu::Surface<'static>>>);
+pub struct CanvasSurfaceHandle(CanvasHandle, Option<Arc<WgpuSurface>>);
 #[cfg(feature = "wgpu")]
 impl CanvasSurfaceHandle {
 	pub fn new() -> Self {
 		Self(CanvasHandle::new(), None)
 	}
-	fn surface(&mut self, executor: &WgpuExecutor) -> &wgpu::Surface<'_> {
+	fn surface(&mut self, executor: &WgpuExecutor) -> &WgpuSurface {
 		if self.1.is_none() {
 			let canvas = self.0.get().canvas.clone();
 			let surface = executor
@@ -115,9 +115,9 @@ impl CanvasSurface for CanvasSurfaceHandle {
 			},
 		);
 
-		let surface_texture = match surface.get_current_texture() {
-			wgpu::CurrentSurfaceTexture::Success(t) | wgpu::CurrentSurfaceTexture::Suboptimal(t) => t,
-			other => panic!("Failed to get surface texture: {other:?}"),
+		let surface_texture = match surface.get_current_texture(&context.queue) {
+			WgpuCurrentSurfaceTexture::Success(t) | WgpuCurrentSurfaceTexture::Suboptimal(t) => t,
+			_ => panic!("Failed to get surface texture"),
 		};
 
 		encoder.copy_texture_to_texture(
@@ -136,7 +136,7 @@ impl CanvasSurface for CanvasSurfaceHandle {
 			source_texture.size(),
 		);
 
-		context.queue.submit([encoder.finish()]);
+		surface_texture.queue.submit([encoder.finish()]);
 		surface_texture.present();
 	}
 }

node-graph/libraries/wgpu-executor/Cargo.toml

@@ -18,6 +18,7 @@ node-macro = { workspace = true }
 glam = { workspace = true }
 anyhow = { workspace = true }
 wgpu = { workspace = true }
+wgpu-sync = { workspace = true }
 futures = { workspace = true }
 web-sys = { workspace = true }
 vello = { workspace = true }

node-graph/libraries/wgpu-executor/src/context.rs

@@ -1,4 +1,5 @@
-use wgpu::{Adapter, Backends, Device, Features, Instance, Queue};
+use wgpu::{Backends, Device, Features};
+use wgpu_sync::{Adapter, Instance, Queue};
 
 #[derive(Debug, Clone)]
 pub struct Context {
@@ -77,10 +78,10 @@ impl ContextBuilder {
 }
 impl ContextBuilder {
 	fn build_instance(&self) -> Instance {
-		Instance::new(wgpu::InstanceDescriptor {
+		Instance::new(wgpu::Instance::new(wgpu::InstanceDescriptor {
 			backends: self.backends,
 			..wgpu::InstanceDescriptor::new_without_display_handle()
-		})
+		}))
 	}
 	#[cfg(target_family = "wasm")]
 	async fn request_adapter(&self, instance: &Instance) -> Option<Adapter> {

node-graph/libraries/wgpu-executor/src/lib.rs

@@ -4,8 +4,6 @@ pub mod shader_runtime;
 mod texture_cache;
 pub mod texture_conversion;
 
-use std::sync::Arc;
-
 use crate::shader_runtime::ShaderRuntime;
 use crate::texture_cache::TextureCache;
 use anyhow::Result;
@@ -14,6 +12,7 @@ use core_types::color::SRGBA8;
 use futures::lock::Mutex;
 use glam::UVec2;
 use graphene_application_io::{ApplicationIo, EditorApi};
+use std::sync::Arc;
 use vello::{AaConfig, AaSupport, RenderParams, Renderer, RendererOptions, Scene};
 use wgpu::{Origin3d, TextureAspect};
 
@@ -25,6 +24,10 @@ pub use pipeline::PipelineCache as WgpuPipelineCache;
 pub use rendering::RenderContext;
 pub use wgpu::Backends as WgpuBackends;
 pub use wgpu::Features as WgpuFeatures;
+pub use wgpu_sync::CurrentSurfaceTexture as WgpuCurrentSurfaceTexture;
+pub use wgpu_sync::Instance as WgpuInstance;
+pub use wgpu_sync::Queue as WgpuQueue;
+pub use wgpu_sync::Surface as WgpuSurface;
 
 const TEXTURE_CACHE_SIZE: u64 = 256 * 1024 * 1024; // 256 MiB
 
@@ -88,7 +91,11 @@ impl WgpuExecutor {
 				};
 				renderer.override_image(&image_brush.image, Some(texture_view));
 			}
-			renderer.render_to_texture(&self.context().device, &self.context().queue, scene, &texture_view, &render_params)?;
+
+			{
+				let queue = self.context().queue.lock();
+				renderer.render_to_texture(&self.context().device, &queue, scene, &texture_view, &render_params)?;
+			}
 			for (image_brush, _) in context.resource_overrides.iter() {
 				renderer.override_image(&image_brush.image, None);
 			}

node-graph/libraries/wgpu-executor/src/texture_conversion.rs

@@ -150,12 +150,12 @@ impl<'i> Convert<List<Raster<GPU>>, &'i WgpuExecutor> for List<Raster<GPU>> {
 impl<'i> Convert<List<Raster<GPU>>, &'i WgpuExecutor> for List<Raster<CPU>> {
 	async fn convert(self, _: Footprint, executor: &'i WgpuExecutor) -> List<Raster<GPU>> {
 		let device = &executor.context().device;
-		let queue = &executor.context().queue;
+		let queue = executor.context().queue.lock();
 		let list = self
 			.into_iter()
 			.map(|row| {
 				let (image, attributes) = row.into_parts();
-				let texture = upload_to_texture(device, queue, &image);
+				let texture = upload_to_texture(device, &queue, &image);
 
 				Item::from_parts(Raster::new_gpu(texture), attributes)
 			})
@@ -170,8 +170,8 @@ impl<'i> Convert<List<Raster<GPU>>, &'i WgpuExecutor> for List<Raster<CPU>> {
 impl<'i> Convert<Raster<GPU>, &'i WgpuExecutor> for Raster<CPU> {
 	async fn convert(self, _: Footprint, executor: &'i WgpuExecutor) -> Raster<GPU> {
 		let device = &executor.context().device;
-		let queue = &executor.context().queue;
-		let texture = upload_to_texture(device, queue, &self);
+		let queue = executor.context().queue.lock();
+		let texture = upload_to_texture(device, &queue, &self);
 
 		queue.submit([]);
 		Raster::new_gpu(texture)