Safe surface creation (#4597)

CHANGELOG.md

@@ -47,6 +47,14 @@ Bottom level categories:
 - Log vulkan validation layer messages during instance creation and destruction: By @exrook in [#4586](https://github.com/gfx-rs/wgpu/pull/4586)
 - `TextureFormat::block_size` is deprecated, use `TextureFormat::block_copy_size` instead: By @wumpf in [#4647](https://github.com/gfx-rs/wgpu/pull/4647)
 
+#### Safe `Surface` creation
+
+It is now possible to safely create a `wgpu::Surface` with `Surface::create_surface()` by letting `Surface` hold a lifetime to `window`.
+
+Passing an owned value `window` to `Surface` will return a `Surface<'static>`. Shared ownership over `window` can still be achieved with e.g. an `Arc`. Alternatively a reference could be passed, which will return a `Surface<'window>`.
+
+`Surface::create_surface_from_raw()` can be used to continue producing a `Surface<'static>` without any lifetime requirements over `window`, which also remains `unsafe`.
+
 #### Naga
 
 - Introduce a new `Scalar` struct type for use in Naga's IR, and update all frontend, middle, and backend code appropriately. By @jimblandy in [#4673](https://github.com/gfx-rs/wgpu/pull/4673).

examples/common/src/framework.rs

@@ -1,3 +1,5 @@
+use std::sync::Arc;
+
 use wgpu::{Instance, Surface, WasmNotSend, WasmNotSync};
 use wgpu_test::GpuTestConfiguration;
 use winit::{
@@ -90,7 +92,7 @@ fn init_logger() {
 
 struct EventLoopWrapper {
     event_loop: EventLoop<()>,
-    window: Window,
+    window: Arc<Window>,
 }
 
 impl EventLoopWrapper {
@@ -98,7 +100,7 @@ impl EventLoopWrapper {
         let event_loop = EventLoop::new().unwrap();
         let mut builder = winit::window::WindowBuilder::new();
         builder = builder.with_title(title);
-        let window = builder.build(&event_loop).unwrap();
+        let window = Arc::new(builder.build(&event_loop).unwrap());
 
         #[cfg(target_arch = "wasm32")]
         {
@@ -121,7 +123,7 @@ impl EventLoopWrapper {
 ///
 /// As surface usage varies per platform, wrapping this up cleans up the event loop code.
 struct SurfaceWrapper {
-    surface: Option<wgpu::Surface>,
+    surface: Option<wgpu::Surface<'static>>,
     config: Option<wgpu::SurfaceConfiguration>,
 }
 
@@ -141,9 +143,9 @@ impl SurfaceWrapper {
     ///
     /// We cannot unconditionally create a surface here, as Android requires
     /// us to wait until we recieve the `Resumed` event to do so.
-    fn pre_adapter(&mut self, instance: &Instance, window: &Window) {
+    fn pre_adapter(&mut self, instance: &Instance, window: Arc<Window>) {
         if cfg!(target_arch = "wasm32") {
-            self.surface = Some(unsafe { instance.create_surface(&window).unwrap() });
+            self.surface = Some(instance.create_surface(window).unwrap());
         }
     }
 
@@ -163,7 +165,7 @@ impl SurfaceWrapper {
     /// On all native platforms, this is where we create the surface.
     ///
     /// Additionally, we configure the surface based on the (now valid) window size.
-    fn resume(&mut self, context: &ExampleContext, window: &Window, srgb: bool) {
+    fn resume(&mut self, context: &ExampleContext, window: Arc<Window>, srgb: bool) {
         // Window size is only actually valid after we enter the event loop.
         let window_size = window.inner_size();
         let width = window_size.width.max(1);
@@ -173,7 +175,7 @@ impl SurfaceWrapper {
 
         // We didn't create the surface in pre_adapter, so we need to do so now.
         if !cfg!(target_arch = "wasm32") {
-            self.surface = Some(unsafe { context.instance.create_surface(&window).unwrap() });
+            self.surface = Some(context.instance.create_surface(window).unwrap());
         }
 
         // From here on, self.surface should be Some.
@@ -252,7 +254,7 @@ struct ExampleContext {
 }
 impl ExampleContext {
     /// Initializes the example context.
-    async fn init_async<E: Example>(surface: &mut SurfaceWrapper, window: &Window) -> Self {
+    async fn init_async<E: Example>(surface: &mut SurfaceWrapper, window: Arc<Window>) -> Self {
         log::info!("Initializing wgpu...");
 
         let backends = wgpu::util::backend_bits_from_env().unwrap_or_default();
@@ -357,8 +359,7 @@ async fn start<E: Example>(title: &str) {
     init_logger();
     let window_loop = EventLoopWrapper::new(title);
     let mut surface = SurfaceWrapper::new();
-    let context = ExampleContext::init_async::<E>(&mut surface, &window_loop.window).await;
+    let context = ExampleContext::init_async::<E>(&mut surface, window_loop.window.clone()).await;
-
     let mut frame_counter = FrameCounter::new();
 
     // We wait to create the example until we have a valid surface.
@@ -384,7 +385,7 @@ async fn start<E: Example>(title: &str) {
 
             match event {
                 ref e if SurfaceWrapper::start_condition(e) => {
-                    surface.resume(&context, &window_loop.window, E::SRGB);
+                    surface.resume(&context, window_loop.window.clone(), E::SRGB);
 
                     // If we haven't created the example yet, do so now.
                     if example.is_none() {

examples/hello-triangle/src/main.rs

@@ -12,7 +12,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(),
@@ -84,6 +84,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.

examples/hello-windows/src/main.rs

@@ -1,6 +1,6 @@
 #![cfg_attr(target_arch = "wasm32", allow(dead_code))]
 
-use std::collections::HashMap;
+use std::{collections::HashMap, sync::Arc};
 use winit::{
     event::{Event, WindowEvent},
     event_loop::EventLoop,
@@ -8,9 +8,9 @@ use winit::{
 };
 
 struct ViewportDesc {
-    window: Window,
+    window: Arc<Window>,
     background: wgpu::Color,
-    surface: wgpu::Surface,
+    surface: wgpu::Surface<'static>,
 }
 
 struct Viewport {
@@ -19,8 +19,8 @@ struct Viewport {
 }
 
 impl ViewportDesc {
-    fn new(window: Window, background: wgpu::Color, instance: &wgpu::Instance) -> Self {
+    fn new(window: Arc<Window>, background: wgpu::Color, instance: &wgpu::Instance) -> Self {
-        let surface = unsafe { instance.create_surface(&window) }.unwrap();
+        let surface = instance.create_surface(window.clone()).unwrap();
         Self {
             window,
             background,
@@ -62,7 +62,7 @@ impl Viewport {
     }
 }
 
-async fn run(event_loop: EventLoop<()>, viewports: Vec<(Window, wgpu::Color)>) {
+async fn run(event_loop: EventLoop<()>, viewports: Vec<(Arc<Window>, wgpu::Color)>) {
     let instance = wgpu::Instance::default();
     let viewports: Vec<_> = viewports
         .into_iter()
@@ -180,6 +180,7 @@ fn main() {
                     .with_inner_size(winit::dpi::PhysicalSize::new(WINDOW_SIZE, WINDOW_SIZE))
                     .build(&event_loop)
                     .unwrap();
+                let window = Arc::new(window);
                 window.set_outer_position(winit::dpi::PhysicalPosition::new(
                     WINDOW_PADDING + column * WINDOW_OFFSET,
                     WINDOW_PADDING + row * (WINDOW_OFFSET + WINDOW_TITLEBAR),

examples/uniform-values/src/main.rs

@@ -16,6 +16,7 @@
 //! The usage of the uniform buffer within the shader itself is pretty self-explanatory given
 //! some understanding of WGSL.
 
+use std::sync::Arc;
 // We won't bring StorageBuffer into scope as that might be too easy to confuse
 // with actual GPU-allocated WGPU storage buffers.
 use encase::ShaderType;
@@ -84,8 +85,8 @@ impl Default for AppState {
 }
 
 struct WgpuContext {
-    pub window: Window,
+    pub window: Arc<Window>,
-    pub surface: wgpu::Surface,
+    pub surface: wgpu::Surface<'static>,
     pub surface_config: wgpu::SurfaceConfiguration,
     pub device: wgpu::Device,
     pub queue: wgpu::Queue,
@@ -95,11 +96,11 @@ struct WgpuContext {
 }
 
 impl WgpuContext {
-    async fn new(window: Window) -> WgpuContext {
+    async fn new(window: Arc<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.clone()).unwrap();
         let adapter = instance
             .request_adapter(&wgpu::RequestAdapterOptions {
                 power_preference: wgpu::PowerPreference::HighPerformance,
@@ -223,7 +224,7 @@ impl WgpuContext {
     }
 }
 
-async fn run(event_loop: EventLoop<()>, window: Window) {
+async fn run(event_loop: EventLoop<()>, window: Arc<Window>) {
     let mut wgpu_context = Some(WgpuContext::new(window).await);
     // (6)
     let mut state = Some(AppState::default());
@@ -351,6 +352,7 @@ fn main() {
         .with_inner_size(winit::dpi::LogicalSize::new(900, 900))
         .build(&event_loop)
         .unwrap();
+    let window = Arc::new(window);
     #[cfg(not(target_arch = "wasm32"))]
     {
         env_logger::builder().format_timestamp_nanos().init();

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,

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,
@@ -1239,7 +1239,7 @@ pub(crate) trait DynContext: Debug + WasmNotSend + WasmNotSync {
     #[allow(clippy::type_complexity)]
     fn instance_request_adapter(
         &self,
-        options: &RequestAdapterOptions<'_>,
+        options: &RequestAdapterOptions<'_, '_>,
     ) -> Pin<InstanceRequestAdapterFuture>;
     fn adapter_request_device(
         &self,
@@ -2103,7 +2103,7 @@ where
 
     fn instance_request_adapter(
         &self,
-        options: &RequestAdapterOptions<'_>,
+        options: &RequestAdapterOptions<'_, '_>,
     ) -> Pin<InstanceRequestAdapterFuture> {
         let future: T::RequestAdapterFuture = Context::instance_request_adapter(self, options);
         Box::pin(async move {

wgpu/src/lib.rs

@@ -27,6 +27,7 @@ use std::{
 use context::{Context, DeviceRequest, DynContext, ObjectId};
 use parking_lot::Mutex;
 
+use raw_window_handle::{HasDisplayHandle, HasWindowHandle};
 pub use wgt::{
     AdapterInfo, AddressMode, AstcBlock, AstcChannel, Backend, Backends, BindGroupLayoutEntry,
     BindingType, BlendComponent, BlendFactor, BlendOperation, BlendState, BufferAddress,
@@ -379,6 +380,10 @@ impl Drop for Sampler {
 pub type SurfaceConfiguration = wgt::SurfaceConfiguration<Vec<TextureFormat>>;
 static_assertions::assert_impl_all!(SurfaceConfiguration: Send, Sync);
 
+trait WgpuSurfaceRequirement: WasmNotSend + WasmNotSync {}
+
+impl<T: WasmNotSend + WasmNotSync> WgpuSurfaceRequirement for T {}
+
 /// Handle to a presentable surface.
 ///
 /// A `Surface` represents a platform-specific surface (e.g. a window) onto which rendered images may
@@ -387,9 +392,9 @@ static_assertions::assert_impl_all!(SurfaceConfiguration: Send, Sync);
 /// This type is unique to the Rust API of `wgpu`. In the WebGPU specification,
 /// [`GPUCanvasContext`](https://gpuweb.github.io/gpuweb/#canvas-context)
 /// serves a similar role.
-#[derive(Debug)]
+pub struct Surface<'window> {
-pub struct Surface {
     context: Arc<C>,
+    _surface: Option<Box<dyn WgpuSurfaceRequirement + 'window>>,
     id: ObjectId,
     data: Box<Data>,
     // Stores the latest `SurfaceConfiguration` that was set using `Surface::configure`.
@@ -400,6 +405,28 @@ pub struct Surface {
     // been created is is additionally wrapped in an option.
     config: Mutex<Option<SurfaceConfiguration>>,
 }
+
+// This custom implementation is required because [`Surface::_surface`] doesn't
+// require [`Debug`](fmt::Debug), which we should not require from the user.
+impl<'window> fmt::Debug for Surface<'window> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("Surface")
+            .field("context", &self.context)
+            .field(
+                "_surface",
+                &if self._surface.is_some() {
+                    "Some"
+                } else {
+                    "None"
+                },
+            )
+            .field("id", &self.id)
+            .field("data", &self.data)
+            .field("config", &self.config)
+            .finish()
+    }
+}
+
 #[cfg(any(
     not(target_arch = "wasm32"),
     all(
@@ -409,7 +436,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 +1198,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,11 +1947,9 @@ 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
+    /// If a reference is passed in `window`, the returned [`Surface`] will
-    ///
+    /// hold a lifetime to it. Owned values will return a [`Surface<'static>`]
-    /// - `raw_window_handle` must be a valid object to create a surface upon.
+    /// instead.
-    /// - `raw_window_handle` must remain valid until after the returned [`Surface`] is
-    ///   dropped.
     ///
     /// # Errors
     ///
@@ -1936,12 +1961,37 @@ 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>(
-        W: raw_window_handle::HasWindowHandle + raw_window_handle::HasDisplayHandle,
+        &self,
-    >(
+        window: W,
+    ) -> Result<Surface<'window>, CreateSurfaceError>
+    where
+        W: HasWindowHandle + HasDisplayHandle + WasmNotSend + WasmNotSync + 'window,
+    {
+        let mut surface = unsafe { self.create_surface_from_raw(&window) }?;
+        surface._surface = Some(Box::new(window));
+        Ok(surface)
+    }
+
+    /// An alternative version to [`create_surface()`](Self::create_surface)
+    /// which has no lifetime requirements to `window` and doesn't require
+    /// [`Send`] or [`Sync`] (on non-Wasm targets). This makes it `unsafe`
+    /// instead and always returns a [`Surface<'static>`].
+    ///
+    /// 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>(
         &self,
         window: &W,
-    ) -> Result<Surface, CreateSurfaceError> {
+    ) -> Result<Surface<'static>, CreateSurfaceError>
+    where
+        W: HasWindowHandle + HasDisplayHandle,
+    {
         let raw_display_handle = window
             .display_handle()
             .map_err(|e| CreateSurfaceError {
@@ -1963,6 +2013,7 @@ impl Instance {
         }?;
         Ok(Surface {
             context: Arc::clone(&self.context),
+            _surface: None,
             id,
             data,
             config: Mutex::new(None),
@@ -1978,7 +2029,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()
@@ -1988,6 +2039,7 @@ impl Instance {
         };
         Surface {
             context: Arc::clone(&self.context),
+            _surface: None,
             id: ObjectId::from(surface.id()),
             data: Box::new(surface),
             config: Mutex::new(None),
@@ -2000,7 +2052,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()
@@ -2010,6 +2065,7 @@ impl Instance {
         };
         Surface {
             context: Arc::clone(&self.context),
+            _surface: None,
             id: ObjectId::from(surface.id()),
             data: Box::new(surface),
             config: Mutex::new(None),
@@ -2025,7 +2081,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()
@@ -2035,6 +2091,7 @@ impl Instance {
         };
         Surface {
             context: Arc::clone(&self.context),
+            _surface: None,
             id: ObjectId::from(surface.id()),
             data: Box::new(surface),
             config: Mutex::new(None),
@@ -2050,7 +2107,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()
@@ -2060,6 +2117,7 @@ impl Instance {
         };
         Surface {
             context: Arc::clone(&self.context),
+            _surface: None,
             id: ObjectId::from(surface.id()),
             data: Box::new(surface),
             config: Mutex::new(None),
@@ -2079,7 +2137,7 @@ impl Instance {
     pub fn create_surface_from_canvas(
         &self,
         canvas: web_sys::HtmlCanvasElement,
-    ) -> Result<Surface, CreateSurfaceError> {
+    ) -> Result<Surface<'static>, CreateSurfaceError> {
         let surface = self
             .context
             .as_any()
@@ -2090,6 +2148,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: None,
             #[cfg(any(not(target_arch = "wasm32"), feature = "webgl"))]
             id: ObjectId::from(surface.id()),
             #[cfg(any(not(target_arch = "wasm32"), feature = "webgl"))]
@@ -2115,7 +2174,7 @@ impl Instance {
     pub fn create_surface_from_offscreen_canvas(
         &self,
         canvas: web_sys::OffscreenCanvas,
-    ) -> Result<Surface, CreateSurfaceError> {
+    ) -> Result<Surface<'static>, CreateSurfaceError> {
         let surface = self
             .context
             .as_any()
@@ -2126,6 +2185,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: None,
             #[cfg(any(not(target_arch = "wasm32"), feature = "webgl"))]
             id: ObjectId::from(surface.id()),
             #[cfg(any(not(target_arch = "wasm32"), feature = "webgl"))]
@@ -4913,7 +4973,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.
@@ -5303,7 +5363,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.

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<Adapter> {
     match initialize_adapter_from_env(instance, compatible_surface) {
         Some(a) => Some(a),