From 5a53a681aef67d782495db1509d9d23e02a9ebf1 Mon Sep 17 00:00:00 2001 From: daxpedda Date: Sat, 28 Oct 2023 00:24:37 +0200 Subject: [PATCH] Safe surface creation --- CHANGELOG.md | 4 +++ examples/common/src/framework.rs | 41 +++++++++++---------- examples/hello-triangle/src/main.rs | 3 +- examples/hello-windows/src/main.rs | 28 +++++++-------- examples/uniform-values/src/main.rs | 14 ++++---- wgpu/src/backend/web.rs | 2 +- wgpu/src/context.rs | 6 ++-- wgpu/src/lib.rs | 56 +++++++++++++++++++---------- wgpu/src/util/init.rs | 2 +- 9 files changed, 89 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 725f72b991f..b2c5477e4ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,10 @@ Bottom level categories: ## Unreleased +### Safe `Surface` creation + +It is now possible to safely create a `wgpu::Surface` with `Surface::create_surface()` and carries a lifetime to the passed `window`. `Surface::create_surface_from_raw()` can be used to produce a `Surface<'static>`, which remains `unsafe`. + For naga changelogs at or before v0.14.0. See [naga's changelog](naga/CHANGELOG.md). ## v0.18.0 (2023-10-25) diff --git a/examples/common/src/framework.rs b/examples/common/src/framework.rs index 094d5dc5972..24c69ade2a5 100644 --- a/examples/common/src/framework.rs +++ b/examples/common/src/framework.rs @@ -1,5 +1,6 @@ #[cfg(target_arch = "wasm32")] use std::str::FromStr; +use std::sync::OnceLock; #[cfg(not(target_arch = "wasm32"))] use std::time::Instant; #[cfg(target_arch = "wasm32")] @@ -12,6 +13,7 @@ use winit::{ event::{self, KeyEvent, WindowEvent}, event_loop::{ControlFlow, EventLoop}, keyboard::{Key, NamedKey}, + window::Window, }; #[allow(dead_code)] @@ -72,7 +74,7 @@ struct Setup { event_loop: EventLoop<()>, instance: wgpu::Instance, size: winit::dpi::PhysicalSize, - surface: wgpu::Surface, + surface: wgpu::Surface<'static>, adapter: wgpu::Adapter, device: wgpu::Device, queue: wgpu::Queue, @@ -100,7 +102,8 @@ async fn setup(title: &str) -> Setup { use winit::platform::windows::WindowBuilderExtWindows; builder = builder.with_no_redirection_bitmap(true); } - let window = builder.build(&event_loop).unwrap(); + static WINDOW: OnceLock = OnceLock::new(); + let window = WINDOW.get_or_init(|| builder.build(&event_loop).unwrap()); #[cfg(target_arch = "wasm32")] { @@ -169,26 +172,22 @@ async fn setup(title: &str) -> Setup { dx12_shader_compiler, gles_minor_version, }); - let (size, surface) = unsafe { - let size = window.inner_size(); - - #[cfg(any(not(target_arch = "wasm32"), target_os = "emscripten"))] - let surface = instance.create_surface(&window).unwrap(); - #[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))] - let surface = { - if let Some(offscreen_canvas_setup) = &offscreen_canvas_setup { - log::info!("Creating surface from OffscreenCanvas"); - instance.create_surface_from_offscreen_canvas( - offscreen_canvas_setup.offscreen_canvas.clone(), - ) - } else { - instance.create_surface(&window) - } + let size = window.inner_size(); + + #[cfg(any(not(target_arch = "wasm32"), target_os = "emscripten"))] + let surface = instance.create_surface(window).unwrap(); + #[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))] + let surface = { + if let Some(offscreen_canvas_setup) = &offscreen_canvas_setup { + log::info!("Creating surface from OffscreenCanvas"); + instance.create_surface_from_offscreen_canvas( + offscreen_canvas_setup.offscreen_canvas.clone(), + ) + } else { + instance.create_surface(window) } - .unwrap(); - - (size, surface) - }; + } + .unwrap(); let adapter = wgpu::util::initialize_adapter_from_env_or_default(&instance, Some(&surface)) .await .expect("No suitable GPU adapters found on the system!"); diff --git a/examples/hello-triangle/src/main.rs b/examples/hello-triangle/src/main.rs index fb836de68db..43d70cadd5e 100644 --- a/examples/hello-triangle/src/main.rs +++ b/examples/hello-triangle/src/main.rs @@ -10,7 +10,7 @@ async fn run(event_loop: EventLoop<()>, window: Window) { let instance = wgpu::Instance::default(); - let surface = unsafe { instance.create_surface(&window) }.unwrap(); + let surface = instance.create_surface(&window).unwrap(); let adapter = instance .request_adapter(&wgpu::RequestAdapterOptions { power_preference: wgpu::PowerPreference::default(), @@ -82,6 +82,7 @@ async fn run(event_loop: EventLoop<()>, window: Window) { surface.configure(&device, &config); + let window = &window; event_loop .run(move |event, target| { // Have the closure take ownership of the resources. diff --git a/examples/hello-windows/src/main.rs b/examples/hello-windows/src/main.rs index 43bd986965d..ae8f857b936 100644 --- a/examples/hello-windows/src/main.rs +++ b/examples/hello-windows/src/main.rs @@ -7,20 +7,20 @@ use winit::{ window::{Window, WindowId}, }; -struct ViewportDesc { - window: Window, +struct ViewportDesc<'window> { + window: &'window Window, background: wgpu::Color, - surface: wgpu::Surface, + surface: wgpu::Surface<'window>, } -struct Viewport { - desc: ViewportDesc, +struct Viewport<'window> { + desc: ViewportDesc<'window>, config: wgpu::SurfaceConfiguration, } -impl ViewportDesc { - fn new(window: Window, background: wgpu::Color, instance: &wgpu::Instance) -> Self { - let surface = unsafe { instance.create_surface(&window) }.unwrap(); +impl<'window> ViewportDesc<'window> { + fn new(window: &'window Window, background: wgpu::Color, instance: &wgpu::Instance) -> Self { + let surface = instance.create_surface(window).unwrap(); Self { window, background, @@ -28,7 +28,7 @@ impl ViewportDesc { } } - fn build(self, adapter: &wgpu::Adapter, device: &wgpu::Device) -> Viewport { + fn build(self, adapter: &wgpu::Adapter, device: &wgpu::Device) -> Viewport<'window> { let size = self.window.inner_size(); let caps = self.surface.get_capabilities(adapter); @@ -48,7 +48,7 @@ impl ViewportDesc { } } -impl Viewport { +impl Viewport<'_> { fn resize(&mut self, device: &wgpu::Device, size: winit::dpi::PhysicalSize) { self.config.width = size.width; self.config.height = size.height; @@ -62,11 +62,11 @@ impl Viewport { } } -async fn run(event_loop: EventLoop<()>, viewports: Vec<(Window, wgpu::Color)>) { +async fn run(event_loop: EventLoop<()>, viewports: &[(Window, wgpu::Color)]) { let instance = wgpu::Instance::default(); let viewports: Vec<_> = viewports - .into_iter() - .map(|(window, color)| ViewportDesc::new(window, color, &instance)) + .iter() + .map(|(window, color)| ViewportDesc::new(window, *color, &instance)) .collect(); let adapter = instance .request_adapter(&wgpu::RequestAdapterOptions { @@ -200,7 +200,7 @@ fn main() { } env_logger::init(); - pollster::block_on(run(event_loop, viewports)); + pollster::block_on(run(event_loop, &viewports)); } #[cfg(target_arch = "wasm32")] { diff --git a/examples/uniform-values/src/main.rs b/examples/uniform-values/src/main.rs index 50b7e7d6abd..2513293faf0 100644 --- a/examples/uniform-values/src/main.rs +++ b/examples/uniform-values/src/main.rs @@ -83,9 +83,9 @@ impl Default for AppState { } } -struct WgpuContext { - pub window: Window, - pub surface: wgpu::Surface, +struct WgpuContext<'window> { + pub window: &'window Window, + pub surface: wgpu::Surface<'window>, pub surface_config: wgpu::SurfaceConfiguration, pub device: wgpu::Device, pub queue: wgpu::Queue, @@ -94,12 +94,12 @@ struct WgpuContext { pub uniform_buffer: wgpu::Buffer, } -impl WgpuContext { - async fn new(window: Window) -> WgpuContext { +impl WgpuContext<'_> { + async fn new(window: &Window) -> WgpuContext { let size = window.inner_size(); let instance = wgpu::Instance::default(); - let surface = unsafe { instance.create_surface(&window) }.unwrap(); + let surface = instance.create_surface(window).unwrap(); let adapter = instance .request_adapter(&wgpu::RequestAdapterOptions { power_preference: wgpu::PowerPreference::HighPerformance, @@ -224,7 +224,7 @@ impl WgpuContext { } async fn run(event_loop: EventLoop<()>, window: Window) { - let mut wgpu_context = Some(WgpuContext::new(window).await); + let mut wgpu_context = Some(WgpuContext::new(&window).await); // (6) let mut state = Some(AppState::default()); let main_window_id = wgpu_context.as_ref().unwrap().window.id(); diff --git a/wgpu/src/backend/web.rs b/wgpu/src/backend/web.rs index a76fe5b142f..ace14d6b152 100644 --- a/wgpu/src/backend/web.rs +++ b/wgpu/src/backend/web.rs @@ -1109,7 +1109,7 @@ impl crate::context::Context for Context { fn instance_request_adapter( &self, - options: &crate::RequestAdapterOptions<'_>, + options: &crate::RequestAdapterOptions<'_, '_>, ) -> Self::RequestAdapterFuture { //TODO: support this check, return `None` if the flag is not set. // It's not trivial, since we need the Future logic to have this check, diff --git a/wgpu/src/context.rs b/wgpu/src/context.rs index bfcd4ae2ecb..c526d8ed49e 100644 --- a/wgpu/src/context.rs +++ b/wgpu/src/context.rs @@ -103,7 +103,7 @@ pub trait Context: Debug + WasmNotSend + WasmNotSync + Sized { ) -> Result<(Self::SurfaceId, Self::SurfaceData), crate::CreateSurfaceError>; fn instance_request_adapter( &self, - options: &RequestAdapterOptions<'_>, + options: &RequestAdapterOptions<'_, '_>, ) -> Self::RequestAdapterFuture; fn adapter_request_device( &self, @@ -1212,7 +1212,7 @@ pub(crate) trait DynContext: Debug + WasmNotSend + WasmNotSync { #[allow(clippy::type_complexity)] fn instance_request_adapter( &self, - options: &RequestAdapterOptions<'_>, + options: &RequestAdapterOptions<'_, '_>, ) -> Pin; fn adapter_request_device( &self, @@ -2070,7 +2070,7 @@ where fn instance_request_adapter( &self, - options: &RequestAdapterOptions<'_>, + options: &RequestAdapterOptions<'_, '_>, ) -> Pin { let future: T::RequestAdapterFuture = Context::instance_request_adapter(self, options); Box::pin(async move { diff --git a/wgpu/src/lib.rs b/wgpu/src/lib.rs index 2c4150d3146..42ac98c1e34 100644 --- a/wgpu/src/lib.rs +++ b/wgpu/src/lib.rs @@ -388,8 +388,9 @@ static_assertions::assert_impl_all!(SurfaceConfiguration: Send, Sync); /// [`GPUCanvasContext`](https://gpuweb.github.io/gpuweb/#canvas-context) /// serves a similar role. #[derive(Debug)] -pub struct Surface { +pub struct Surface<'window> { context: Arc, + surface: PhantomData<&'window ()>, id: ObjectId, data: Box, // Stores the latest `SurfaceConfiguration` that was set using `Surface::configure`. @@ -409,7 +410,7 @@ pub struct Surface { ))] static_assertions::assert_impl_all!(Surface: Send, Sync); -impl Drop for Surface { +impl Drop for Surface<'_> { fn drop(&mut self) { if !thread::panicking() { self.context.surface_drop(&self.id, self.data.as_ref()) @@ -1171,7 +1172,7 @@ pub use wgt::RequestAdapterOptions as RequestAdapterOptionsBase; /// /// Corresponds to [WebGPU `GPURequestAdapterOptions`]( /// https://gpuweb.github.io/gpuweb/#dictdef-gpurequestadapteroptions). -pub type RequestAdapterOptions<'a> = RequestAdapterOptionsBase<&'a Surface>; +pub type RequestAdapterOptions<'a, 'b> = RequestAdapterOptionsBase<&'a Surface<'b>>; #[cfg(any( not(target_arch = "wasm32"), all( @@ -1920,12 +1921,6 @@ impl Instance { /// If the specified display and window handle are not supported by any of the backends, then the surface /// will not be supported by any adapters. /// - /// # Safety - /// - /// - `raw_window_handle` must be a valid object to create a surface upon. - /// - `raw_window_handle` must remain valid until after the returned [`Surface`] is - /// dropped. - /// /// # Errors /// /// - On WebGL2: Will return an error if the browser does not support WebGL2, @@ -1936,12 +1931,29 @@ impl Instance { /// - On macOS/Metal: will panic if not called on the main thread. /// - On web: will panic if the `raw_window_handle` does not properly refer to a /// canvas element. - pub unsafe fn create_surface< + pub fn create_surface< + 'window, + W: raw_window_handle::HasWindowHandle + raw_window_handle::HasDisplayHandle, + >( + &self, + window: &'window W, + ) -> Result, CreateSurfaceError> { + unsafe { self.create_surface_from_raw(window) } + } + + /// See [`create_surface()`](Self::create_surface) for more details. + /// + /// # Safety + /// + /// - `raw_window_handle` must be a valid object to create a surface upon. + /// - `raw_window_handle` must remain valid until after the returned [`Surface`] is + /// dropped. + pub unsafe fn create_surface_from_raw< W: raw_window_handle::HasWindowHandle + raw_window_handle::HasDisplayHandle, >( &self, window: &W, - ) -> Result { + ) -> Result, CreateSurfaceError> { let raw_display_handle = window .display_handle() .map_err(|e| CreateSurfaceError { @@ -1961,6 +1973,7 @@ impl Instance { )?; Ok(Surface { context: Arc::clone(&self.context), + surface: PhantomData, id, data, config: Mutex::new(None), @@ -1976,7 +1989,7 @@ impl Instance { pub unsafe fn create_surface_from_core_animation_layer( &self, layer: *mut std::ffi::c_void, - ) -> Surface { + ) -> Surface<'static> { let surface = unsafe { self.context .as_any() @@ -1998,7 +2011,10 @@ impl Instance { /// /// - visual must be a valid IDCompositionVisual to create a surface upon. #[cfg(target_os = "windows")] - pub unsafe fn create_surface_from_visual(&self, visual: *mut std::ffi::c_void) -> Surface { + pub unsafe fn create_surface_from_visual( + &self, + visual: *mut std::ffi::c_void, + ) -> Surface<'static> { let surface = unsafe { self.context .as_any() @@ -2023,7 +2039,7 @@ impl Instance { pub unsafe fn create_surface_from_surface_handle( &self, surface_handle: *mut std::ffi::c_void, - ) -> Surface { + ) -> Surface<'static> { let surface = unsafe { self.context .as_any() @@ -2048,7 +2064,7 @@ impl Instance { pub unsafe fn create_surface_from_swap_chain_panel( &self, swap_chain_panel: *mut std::ffi::c_void, - ) -> Surface { + ) -> Surface<'static> { let surface = unsafe { self.context .as_any() @@ -2077,7 +2093,7 @@ impl Instance { pub fn create_surface_from_canvas( &self, canvas: web_sys::HtmlCanvasElement, - ) -> Result { + ) -> Result, CreateSurfaceError> { let surface = self .context .as_any() @@ -2088,6 +2104,7 @@ impl Instance { // TODO: This is ugly, a way to create things from a native context needs to be made nicer. Ok(Surface { context: Arc::clone(&self.context), + surface: PhantomData, #[cfg(any(not(target_arch = "wasm32"), feature = "webgl"))] id: ObjectId::from(surface.id()), #[cfg(any(not(target_arch = "wasm32"), feature = "webgl"))] @@ -2113,7 +2130,7 @@ impl Instance { pub fn create_surface_from_offscreen_canvas( &self, canvas: web_sys::OffscreenCanvas, - ) -> Result { + ) -> Result, CreateSurfaceError> { let surface = self .context .as_any() @@ -2124,6 +2141,7 @@ impl Instance { // TODO: This is ugly, a way to create things from a native context needs to be made nicer. Ok(Surface { context: Arc::clone(&self.context), + surface: PhantomData, #[cfg(any(not(target_arch = "wasm32"), feature = "webgl"))] id: ObjectId::from(surface.id()), #[cfg(any(not(target_arch = "wasm32"), feature = "webgl"))] @@ -4898,7 +4916,7 @@ impl Drop for SurfaceTexture { } } -impl Surface { +impl Surface<'_> { /// Returns the capabilities of the surface when used with the given adapter. /// /// Returns specified values (see [`SurfaceCapabilities`]) if surface is incompatible with the adapter. @@ -5287,7 +5305,7 @@ impl RenderBundle { } #[cfg(feature = "expose-ids")] -impl Surface { +impl Surface<'_> { /// Returns a globally-unique identifier for this `Surface`. /// /// Calling this method multiple times on the same object will always return the same value. diff --git a/wgpu/src/util/init.rs b/wgpu/src/util/init.rs index 987e8400fd7..3b28753e48e 100644 --- a/wgpu/src/util/init.rs +++ b/wgpu/src/util/init.rs @@ -80,7 +80,7 @@ pub fn initialize_adapter_from_env( /// Initialize the adapter obeying the WGPU_ADAPTER_NAME environment variable and if it doesn't exist fall back on a default adapter. pub async fn initialize_adapter_from_env_or_default( instance: &Instance, - compatible_surface: Option<&Surface>, + compatible_surface: Option<&Surface<'_>>, ) -> Option { match initialize_adapter_from_env(instance, compatible_surface) { Some(a) => Some(a),