From 86323b1843b6ff729b71c894a868b67d9b11767d Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Sun, 14 Apr 2024 14:05:59 +0200 Subject: [PATCH 1/6] documentation & naming fixes --- wgpu-core/src/any_surface.rs | 2 +- wgpu/src/lib.rs | 56 ++++++++++++++++++++++++------------ 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/wgpu-core/src/any_surface.rs b/wgpu-core/src/any_surface.rs index 94edfc4433..6342a8037b 100644 --- a/wgpu-core/src/any_surface.rs +++ b/wgpu-core/src/any_surface.rs @@ -8,7 +8,7 @@ use std::mem::ManuallyDrop; use std::ptr::NonNull; struct AnySurfaceVtable { - // We oppurtunistically store the backend here, since we now it will be used + // We opportunistically store the backend here, since we now it will be used // with backend selection and it can be stored in static memory. backend: Backend, // Drop glue which knows how to drop the stored data. diff --git a/wgpu/src/lib.rs b/wgpu/src/lib.rs index 3bf77fd10b..3a46b30d48 100644 --- a/wgpu/src/lib.rs +++ b/wgpu/src/lib.rs @@ -366,9 +366,19 @@ static_assertions::assert_impl_all!(SurfaceConfiguration: Send, Sync); /// serves a similar role. pub struct Surface<'window> { context: Arc, - _surface: Option>, + + /// Optionally, keep the source of the handle used for the surface alive. + /// + /// This is useful for platforms where the surface is created from a window and the surface + /// would become invalid when the window is dropped. + _handle_source: Option>, + + /// Wgpu-core surface id. id: ObjectId, - data: Box, + + /// Additional surface data returned by [`DynContext::instance_create_surface`]. + surface_data: Box, + // Stores the latest `SurfaceConfiguration` that was set using `Surface::configure`. // It is required to set the attributes of the `SurfaceTexture` in the // `Surface::get_current_texture` method. @@ -385,15 +395,15 @@ impl<'window> fmt::Debug for Surface<'window> { f.debug_struct("Surface") .field("context", &self.context) .field( - "_surface", - &if self._surface.is_some() { + "_handle_source", + &if self._handle_source.is_some() { "Some" } else { "None" }, ) .field("id", &self.id) - .field("data", &self.data) + .field("data", &self.surface_data) .field("config", &self.config) .finish() } @@ -405,7 +415,8 @@ static_assertions::assert_impl_all!(Surface<'_>: Send, Sync); impl Drop for Surface<'_> { fn drop(&mut self) { if !thread::panicking() { - self.context.surface_drop(&self.id, self.data.as_ref()) + self.context + .surface_drop(&self.id, self.surface_data.as_ref()) } } } @@ -1967,6 +1978,8 @@ impl Instance { /// Creates a new surface targeting a given window/canvas/surface/etc.. /// + /// Internally, this creates surfaces for all backends that are enabled for this instance. + /// /// See [`SurfaceTarget`] for what targets are supported. /// See [`Instance::create_surface_unsafe`] for surface creation with unsafe target variants. /// @@ -1977,7 +1990,7 @@ impl Instance { target: impl Into>, ) -> Result, CreateSurfaceError> { // Handle origin (i.e. window) to optionally take ownership of to make the surface outlast the window. - let handle_origin; + let handle_source; let target = target.into(); let mut surface = match target { @@ -1987,14 +2000,14 @@ impl Instance { inner: CreateSurfaceErrorKind::RawHandle(e), })?, ); - handle_origin = Some(window); + handle_source = Some(window); surface }?, #[cfg(any(webgpu, webgl))] SurfaceTarget::Canvas(canvas) => { - handle_origin = None; + handle_source = None; let value: &wasm_bindgen::JsValue = &canvas; let obj = std::ptr::NonNull::from(value).cast(); @@ -2013,7 +2026,7 @@ impl Instance { #[cfg(any(webgpu, webgl))] SurfaceTarget::OffscreenCanvas(canvas) => { - handle_origin = None; + handle_source = None; let value: &wasm_bindgen::JsValue = &canvas; let obj = std::ptr::NonNull::from(value).cast(); @@ -2032,13 +2045,15 @@ impl Instance { } }; - surface._surface = handle_origin; + surface._handle_source = handle_source; Ok(surface) } /// Creates a new surface targeting a given window/canvas/surface/etc. using an unsafe target. /// + /// Internally, this creates surfaces for all backends that are enabled for this instance. + /// /// See [`SurfaceTargetUnsafe`] for what targets are supported. /// See [`Instance::create_surface`] for surface creation with safe target variants. /// @@ -2053,9 +2068,9 @@ impl Instance { Ok(Surface { context: Arc::clone(&self.context), - _surface: None, + _handle_source: None, id, - data, + surface_data: data, config: Mutex::new(None), }) } @@ -2229,7 +2244,7 @@ impl Adapter { &self.id, self.data.as_ref(), &surface.id, - surface.data.as_ref(), + surface.surface_data.as_ref(), ) } @@ -4833,7 +4848,7 @@ impl Surface<'_> { DynContext::surface_get_capabilities( &*self.context, &self.id, - self.data.as_ref(), + self.surface_data.as_ref(), &adapter.id, adapter.data.as_ref(), ) @@ -4872,7 +4887,7 @@ impl Surface<'_> { DynContext::surface_configure( &*self.context, &self.id, - self.data.as_ref(), + self.surface_data.as_ref(), &device.id, device.data.as_ref(), config, @@ -4891,8 +4906,11 @@ impl Surface<'_> { /// If a SurfaceTexture referencing this surface is alive when the swapchain is recreated, /// recreating the swapchain will panic. pub fn get_current_texture(&self) -> Result { - let (texture_id, texture_data, status, detail) = - DynContext::surface_get_current_texture(&*self.context, &self.id, self.data.as_ref()); + let (texture_id, texture_data, status, detail) = DynContext::surface_get_current_texture( + &*self.context, + &self.id, + self.surface_data.as_ref(), + ); let suboptimal = match status { SurfaceStatus::Good => false, @@ -4955,7 +4973,7 @@ impl Surface<'_> { .downcast_ref::() .map(|ctx| unsafe { ctx.surface_as_hal::( - self.data.downcast_ref().unwrap(), + self.surface_data.downcast_ref().unwrap(), hal_surface_callback, ) }) From 2326cf3045a6b5b9ded020cad233136d03f528c1 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Sun, 14 Apr 2024 23:16:40 +0200 Subject: [PATCH 2/6] wgpu_core surface creation now creates surfaces for all enabled backends, remove any_surface again --- wgpu-core/src/any_surface.rs | 95 ---------- wgpu-core/src/device/global.rs | 4 +- wgpu-core/src/hal_api.rs | 20 +-- wgpu-core/src/hub.rs | 2 +- wgpu-core/src/instance.rs | 305 ++++++++++++++++++--------------- wgpu-core/src/lib.rs | 1 - wgpu-core/src/present.rs | 6 +- wgpu-core/src/resource.rs | 4 +- wgpu/src/backend/wgpu_core.rs | 6 +- 9 files changed, 192 insertions(+), 251 deletions(-) delete mode 100644 wgpu-core/src/any_surface.rs diff --git a/wgpu-core/src/any_surface.rs b/wgpu-core/src/any_surface.rs deleted file mode 100644 index 6342a8037b..0000000000 --- a/wgpu-core/src/any_surface.rs +++ /dev/null @@ -1,95 +0,0 @@ -use wgt::Backend; - -/// The `AnySurface` type: a `Arc` of a `A::Surface` for any backend `A`. -use crate::hal_api::HalApi; - -use std::fmt; -use std::mem::ManuallyDrop; -use std::ptr::NonNull; - -struct AnySurfaceVtable { - // We opportunistically store the backend here, since we now it will be used - // with backend selection and it can be stored in static memory. - backend: Backend, - // Drop glue which knows how to drop the stored data. - drop: unsafe fn(*mut ()), -} - -/// An `A::Surface`, for any backend `A`. -/// -/// Any `AnySurface` is just like an `A::Surface`, except that the `A` type -/// parameter is erased. To access the `Surface`, you must downcast to a -/// particular backend with the \[`downcast_ref`\] or \[`take`\] methods. -pub struct AnySurface { - data: NonNull<()>, - vtable: &'static AnySurfaceVtable, -} - -impl AnySurface { - /// Construct an `AnySurface` that owns an `A::Surface`. - pub fn new(surface: A::Surface) -> AnySurface { - unsafe fn drop_glue(ptr: *mut ()) { - unsafe { - _ = Box::from_raw(ptr.cast::()); - } - } - - let data = NonNull::from(Box::leak(Box::new(surface))); - - AnySurface { - data: data.cast(), - vtable: &AnySurfaceVtable { - backend: A::VARIANT, - drop: drop_glue::, - }, - } - } - - /// Get the backend this surface was created through. - pub fn backend(&self) -> Backend { - self.vtable.backend - } - - /// If `self` refers to an `A::Surface`, returns a reference to it. - pub fn downcast_ref(&self) -> Option<&A::Surface> { - if A::VARIANT != self.vtable.backend { - return None; - } - - // SAFETY: We just checked the instance above implicitly by the backend - // that it was statically constructed through. - Some(unsafe { &*self.data.as_ptr().cast::() }) - } - - /// If `self` is an `Arc`, returns that. - pub fn take(self) -> Option { - if A::VARIANT != self.vtable.backend { - return None; - } - - // Disable drop glue, since we're returning the owned surface. The - // caller will be responsible for dropping it. - let this = ManuallyDrop::new(self); - - // SAFETY: We just checked the instance above implicitly by the backend - // that it was statically constructed through. - Some(unsafe { *Box::from_raw(this.data.as_ptr().cast::()) }) - } -} - -impl Drop for AnySurface { - fn drop(&mut self) { - unsafe { (self.vtable.drop)(self.data.as_ptr()) } - } -} - -impl fmt::Debug for AnySurface { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "AnySurface<{}>", self.vtable.backend) - } -} - -#[cfg(send_sync)] -unsafe impl Send for AnySurface {} -#[cfg(send_sync)] -unsafe impl Sync for AnySurface {} diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index 9c54dfc193..7a5dc5278f 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -1972,7 +1972,7 @@ impl Global { }; let caps = unsafe { - let suf = A::get_surface(surface); + let suf = A::surface_as_hal(surface); let adapter = &device.adapter; match adapter.raw.adapter.surface_capabilities(suf.unwrap()) { Some(caps) => caps, @@ -2058,7 +2058,7 @@ impl Global { // https://github.com/gfx-rs/wgpu/issues/4105 match unsafe { - A::get_surface(surface) + A::surface_as_hal(surface) .unwrap() .configure(device.raw(), &hal_config) } { diff --git a/wgpu-core/src/hal_api.rs b/wgpu-core/src/hal_api.rs index 179024baed..f1a40b1cff 100644 --- a/wgpu-core/src/hal_api.rs +++ b/wgpu-core/src/hal_api.rs @@ -11,7 +11,7 @@ pub trait HalApi: hal::Api + 'static + WasmNotSendSync { fn create_instance_from_hal(name: &str, hal_instance: Self::Instance) -> Instance; fn instance_as_hal(instance: &Instance) -> Option<&Self::Instance>; fn hub(global: &Global) -> &Hub; - fn get_surface(surface: &Surface) -> Option<&Self::Surface>; + fn surface_as_hal(surface: &Surface) -> Option<&Self::Surface>; } impl HalApi for hal::api::Empty { @@ -25,7 +25,7 @@ impl HalApi for hal::api::Empty { fn hub(_: &Global) -> &Hub { unimplemented!("called empty api") } - fn get_surface(_: &Surface) -> Option<&Self::Surface> { + fn surface_as_hal(_: &Surface) -> Option<&Self::Surface> { unimplemented!("called empty api") } } @@ -46,8 +46,8 @@ impl HalApi for hal::api::Vulkan { fn hub(global: &Global) -> &Hub { &global.hubs.vulkan } - fn get_surface(surface: &Surface) -> Option<&Self::Surface> { - surface.raw.downcast_ref::() + fn surface_as_hal(surface: &Surface) -> Option<&Self::Surface> { + surface.vulkan.as_ref() } } @@ -67,8 +67,8 @@ impl HalApi for hal::api::Metal { fn hub(global: &Global) -> &Hub { &global.hubs.metal } - fn get_surface(surface: &Surface) -> Option<&Self::Surface> { - surface.raw.downcast_ref::() + fn surface_as_hal(surface: &Surface) -> Option<&Self::Surface> { + surface.metal.as_ref() } } @@ -88,8 +88,8 @@ impl HalApi for hal::api::Dx12 { fn hub(global: &Global) -> &Hub { &global.hubs.dx12 } - fn get_surface(surface: &Surface) -> Option<&Self::Surface> { - surface.raw.downcast_ref::() + fn surface_as_hal(surface: &Surface) -> Option<&Self::Surface> { + surface.dx12.as_ref() } } @@ -110,7 +110,7 @@ impl HalApi for hal::api::Gles { fn hub(global: &Global) -> &Hub { &global.hubs.gl } - fn get_surface(surface: &Surface) -> Option<&Self::Surface> { - surface.raw.downcast_ref::() + fn surface_as_hal(surface: &Surface) -> Option<&Self::Surface> { + surface.gl.as_ref() } } diff --git a/wgpu-core/src/hub.rs b/wgpu-core/src/hub.rs index 6fdc0c2e57..eb57411d98 100644 --- a/wgpu-core/src/hub.rs +++ b/wgpu-core/src/hub.rs @@ -241,7 +241,7 @@ impl Hub { if let Element::Occupied(ref surface, _epoch) = *element { if let Some(ref mut present) = surface.presentation.lock().take() { if let Some(device) = present.device.downcast_ref::() { - let suf = A::get_surface(surface); + let suf = A::surface_as_hal(surface); unsafe { suf.unwrap().unconfigure(device.raw()); //TODO: we could destroy the surface here diff --git a/wgpu-core/src/instance.rs b/wgpu-core/src/instance.rs index 20e67d5f71..b4752397ca 100644 --- a/wgpu-core/src/instance.rs +++ b/wgpu-core/src/instance.rs @@ -1,13 +1,12 @@ +use std::collections::HashMap; use std::sync::Arc; use crate::{ - any_surface::AnySurface, api_log, device::{queue::Queue, resource::Device, DeviceDescriptor}, global::Global, hal_api::HalApi, - id::markers, - id::{AdapterId, DeviceId, Id, Marker, QueueId, SurfaceId}, + id::{markers, AdapterId, DeviceId, Id, Marker, QueueId, SurfaceId}, present::Presentation, resource::{Resource, ResourceInfo, ResourceType}, resource_log, LabelHelpers, DOWNLEVEL_WARNING_MESSAGE, @@ -21,6 +20,7 @@ use thiserror::Error; pub type RequestAdapterOptions = wgt::RequestAdapterOptions; type HalInstance = ::Instance; +type HalSurface = ::Surface; #[derive(Clone, Debug, Error)] #[error("Limit '{name}' value {requested} is better than allowed {allowed}")] @@ -113,31 +113,36 @@ impl Instance { } pub(crate) fn destroy_surface(&self, surface: Surface) { - fn destroy(instance: &Option, surface: AnySurface) { - unsafe { - if let Some(suf) = surface.take::() { - instance.as_ref().unwrap().destroy_surface(suf); + fn destroy(instance: &Option, mut surface: Option>) { + if let Some(surface) = surface.take() { + unsafe { + instance.as_ref().unwrap().destroy_surface(surface); } } } - match surface.raw.backend() { - #[cfg(vulkan)] - Backend::Vulkan => destroy::(&self.vulkan, surface.raw), - #[cfg(metal)] - Backend::Metal => destroy::(&self.metal, surface.raw), - #[cfg(dx12)] - Backend::Dx12 => destroy::(&self.dx12, surface.raw), - #[cfg(gles)] - Backend::Gl => destroy::(&self.gl, surface.raw), - _ => unreachable!(), - } + #[cfg(vulkan)] + destroy::(&self.vulkan, surface.vulkan); + #[cfg(metal)] + destroy::(&self.metal, surface.metal); + #[cfg(dx12)] + destroy::(&self.dx12, surface.dx12); + #[cfg(gles)] + destroy::(&self.gl, surface.gl); } } pub struct Surface { pub(crate) presentation: Mutex>, pub(crate) info: ResourceInfo, - pub(crate) raw: AnySurface, + + #[cfg(vulkan)] + pub vulkan: Option>, + #[cfg(metal)] + pub metal: Option>, + #[cfg(dx12)] + pub dx12: Option>, + #[cfg(gles)] + pub gl: Option>, } impl Resource for Surface { @@ -163,7 +168,7 @@ impl Surface { &self, adapter: &Adapter, ) -> Result { - let suf = A::get_surface(self).ok_or(GetSurfaceSupportError::Unsupported)?; + let suf = A::surface_as_hal(self).ok_or(GetSurfaceSupportError::Unsupported)?; profiling::scope!("surface_capabilities"); let caps = unsafe { adapter @@ -203,7 +208,7 @@ impl Adapter { } pub fn is_surface_supported(&self, surface: &Surface) -> bool { - let suf = A::get_surface(surface); + let suf = A::surface_as_hal(surface); // If get_surface returns None, then the API does not advertise support for the surface. // @@ -461,13 +466,21 @@ pub enum RequestAdapterError { #[derive(Clone, Debug, Error)] #[non_exhaustive] pub enum CreateSurfaceError { - #[error("No backend is available")] - NoSupportedBackend, - #[error(transparent)] - InstanceError(#[from] hal::InstanceError), + #[error("The backend {0} was not enabled on the instance.")] + BackendNotEnabled(Backend), + #[error("Failed to create surface for any enabled backend.")] + InstanceError(HashMap), } impl Global { + /// Creates a new surface targeting the given display/window handles. + /// + /// Internally attempts to create hal surfaces for all enabled backends. + /// TODO DO NOT MERGE: DESCRIBE BONKERS ERROR HANDLING + /// + /// id_in: + /// - If `Some`, the id to assign to the surface. A new one will be generated otherwise. + /// /// # Safety /// /// - `display_handle` must be a valid object to create a surface upon. @@ -483,51 +496,84 @@ impl Global { profiling::scope!("Instance::create_surface"); fn init( + errors: &mut HashMap, + any_created: &mut bool, + backend: Backend, inst: &Option, display_handle: raw_window_handle::RawDisplayHandle, window_handle: raw_window_handle::RawWindowHandle, - ) -> Option> { - inst.as_ref().map(|inst| unsafe { - match inst.create_surface(display_handle, window_handle) { - Ok(raw) => Ok(AnySurface::new::(raw)), - Err(e) => Err(e), + ) -> Option> { + inst.as_ref().and_then(|inst| { + match unsafe { inst.create_surface(display_handle, window_handle) } { + Ok(raw) => { + *any_created = true; + Some(raw) + } + Err(err) => { + log::debug!( + "Instance::create_surface: failed to create surface for {:?}: {:?}", + backend, + err + ); + errors.insert(backend, err); + None + } } }) } - let mut hal_surface: Option> = None; - - #[cfg(vulkan)] - if hal_surface.is_none() { - hal_surface = - init::(&self.instance.vulkan, display_handle, window_handle); - } - #[cfg(metal)] - if hal_surface.is_none() { - hal_surface = - init::(&self.instance.metal, display_handle, window_handle); - } - #[cfg(dx12)] - if hal_surface.is_none() { - hal_surface = - init::(&self.instance.dx12, display_handle, window_handle); - } - #[cfg(gles)] - if hal_surface.is_none() { - hal_surface = init::(&self.instance.gl, display_handle, window_handle); - } - - let hal_surface = hal_surface.ok_or(CreateSurfaceError::NoSupportedBackend)??; + let mut errors = HashMap::default(); + let mut any_created = false; let surface = Surface { presentation: Mutex::new(None), info: ResourceInfo::new("", None), - raw: hal_surface, + + #[cfg(vulkan)] + vulkan: init::( + &mut errors, + &mut any_created, + Backend::Vulkan, + &self.instance.vulkan, + display_handle, + window_handle, + ), + #[cfg(metal)] + metal: init::( + &mut errors, + &mut any_created, + Backend::Metal, + &self.instance.metal, + display_handle, + window_handle, + ), + #[cfg(dx12)] + dx12: init::( + &mut errors, + &mut any_created, + Backend::Dx12, + &self.instance.dx12, + display_handle, + window_handle, + ), + #[cfg(gles)] + gl: init::( + &mut errors, + &mut any_created, + Backend::Gl, + &self.instance.gl, + display_handle, + window_handle, + ), }; - #[allow(clippy::arc_with_non_send_sync)] - let (id, _) = self.surfaces.prepare(id_in).assign(Arc::new(surface)); - Ok(id) + if any_created { + #[allow(clippy::arc_with_non_send_sync)] + let (id, _) = self.surfaces.prepare(id_in).assign(Arc::new(surface)); + Ok(id) + } else { + Err(CreateSurfaceError::InstanceError(errors)) + } } /// # Safety @@ -538,58 +584,72 @@ impl Global { &self, layer: *mut std::ffi::c_void, id_in: Option, - ) -> SurfaceId { + ) -> Result { profiling::scope!("Instance::create_surface_metal"); let surface = Surface { presentation: Mutex::new(None), info: ResourceInfo::new("", None), - raw: { - let hal_surface = self - .instance - .metal - .as_ref() - .map(|inst| { - // we don't want to link to metal-rs for this - #[allow(clippy::transmute_ptr_to_ref)] - inst.create_surface_from_layer(unsafe { std::mem::transmute(layer) }) - }) - .unwrap(); - AnySurface::new::(hal_surface) - }, + metal: Some(self.instance.metal.as_ref().map_or( + Err(CreateSurfaceError::BackendNotEnabled(Backend::Metal)), + |inst| { + // we don't want to link to metal-rs for this + #[allow(clippy::transmute_ptr_to_ref)] + Ok(inst.create_surface_from_layer(unsafe { std::mem::transmute(layer) })) + }, + )?), + #[cfg(dx12)] + dx12: None, + #[cfg(vulkan)] + vulkan: None, + #[cfg(gles)] + gl: None, }; let (id, _) = self.surfaces.prepare(id_in).assign(Arc::new(surface)); - id + Ok(id) } #[cfg(dx12)] - /// # Safety - /// - /// The visual must be valid and able to be used to make a swapchain with. - pub unsafe fn instance_create_surface_from_visual( + fn instance_create_surface_dx12( &self, - visual: *mut std::ffi::c_void, id_in: Option, - ) -> SurfaceId { - profiling::scope!("Instance::instance_create_surface_from_visual"); - + create_surface_func: impl FnOnce(&HalInstance) -> HalSurface, + ) -> Result { let surface = Surface { presentation: Mutex::new(None), info: ResourceInfo::new("", None), - raw: { - let hal_surface = self - .instance + dx12: Some(create_surface_func( + self.instance .dx12 .as_ref() - .map(|inst| unsafe { inst.create_surface_from_visual(visual as _) }) - .unwrap(); - AnySurface::new::(hal_surface) - }, + .ok_or(CreateSurfaceError::BackendNotEnabled(Backend::Dx12))?, + )), + #[cfg(metal)] + metal: None, + #[cfg(vulkan)] + vulkan: None, + #[cfg(gles)] + gl: None, }; let (id, _) = self.surfaces.prepare(id_in).assign(Arc::new(surface)); - id + Ok(id) + } + + #[cfg(dx12)] + /// # Safety + /// + /// The visual must be valid and able to be used to make a swapchain with. + pub unsafe fn instance_create_surface_from_visual( + &self, + visual: *mut std::ffi::c_void, + id_in: Option, + ) -> Result { + profiling::scope!("Instance::instance_create_surface_from_visual"); + self.instance_create_surface_dx12(id_in, |inst| unsafe { + inst.create_surface_from_visual(visual as _) + }) } #[cfg(dx12)] @@ -600,25 +660,11 @@ impl Global { &self, surface_handle: *mut std::ffi::c_void, id_in: Option, - ) -> SurfaceId { + ) -> Result { profiling::scope!("Instance::instance_create_surface_from_surface_handle"); - - let surface = Surface { - presentation: Mutex::new(None), - info: ResourceInfo::new("", None), - raw: { - let hal_surface = self - .instance - .dx12 - .as_ref() - .map(|inst| unsafe { inst.create_surface_from_surface_handle(surface_handle) }) - .unwrap(); - AnySurface::new::(hal_surface) - }, - }; - - let (id, _) = self.surfaces.prepare(id_in).assign(Arc::new(surface)); - id + self.instance_create_surface_dx12(id_in, |inst| unsafe { + inst.create_surface_from_surface_handle(surface_handle) + }) } #[cfg(dx12)] @@ -629,27 +675,11 @@ impl Global { &self, swap_chain_panel: *mut std::ffi::c_void, id_in: Option, - ) -> SurfaceId { + ) -> Result { profiling::scope!("Instance::instance_create_surface_from_swap_chain_panel"); - - let surface = Surface { - presentation: Mutex::new(None), - info: ResourceInfo::new("", None), - raw: { - let hal_surface = self - .instance - .dx12 - .as_ref() - .map(|inst| unsafe { - inst.create_surface_from_swap_chain_panel(swap_chain_panel as _) - }) - .unwrap(); - AnySurface::new::(hal_surface) - }, - }; - - let (id, _) = self.surfaces.prepare(id_in).assign(Arc::new(surface)); - id + self.instance_create_surface_dx12(id_in, |inst| unsafe { + inst.create_surface_from_swap_chain_panel(swap_chain_panel as _) + }) } pub fn surface_drop(&self, id: SurfaceId) { @@ -657,11 +687,15 @@ impl Global { api_log!("Surface::drop {id:?}"); - fn unconfigure(global: &Global, surface: &AnySurface, present: &Presentation) { - let hub = HalApi::hub(global); - if let Some(hal_surface) = surface.downcast_ref::() { + fn unconfigure( + global: &Global, + surface: &Option>, + present: &Presentation, + ) { + if let Some(surface) = surface { + let hub = HalApi::hub(global); if let Some(device) = present.device.downcast_ref::() { - hub.surface_unconfigure(device, hal_surface); + hub.surface_unconfigure(device, surface); } } } @@ -669,15 +703,16 @@ impl Global { let surface = self.surfaces.unregister(id); let surface = Arc::into_inner(surface.unwrap()) .expect("Surface cannot be destroyed because is still in use"); + if let Some(present) = surface.presentation.lock().take() { #[cfg(vulkan)] - unconfigure::(self, &surface.raw, &present); + unconfigure::(self, &surface.vulkan, &present); #[cfg(metal)] - unconfigure::(self, &surface.raw, &present); + unconfigure::(self, &surface.metal, &present); #[cfg(dx12)] - unconfigure::(self, &surface.raw, &present); + unconfigure::(self, &surface.dx12, &present); #[cfg(gles)] - unconfigure::(self, &surface.raw, &present); + unconfigure::(self, &surface.gl, &present); } self.instance.destroy_surface(surface); } @@ -785,7 +820,7 @@ impl Global { adapters.retain(|exposed| exposed.info.device_type == wgt::DeviceType::Cpu); } if let Some(surface) = compatible_surface { - let surface = &A::get_surface(surface); + let surface = &A::surface_as_hal(surface); adapters.retain(|exposed| unsafe { // If the surface does not exist for this backend, // then the surface is not supported. diff --git a/wgpu-core/src/lib.rs b/wgpu-core/src/lib.rs index 5454f0d682..52c92ef64b 100644 --- a/wgpu-core/src/lib.rs +++ b/wgpu-core/src/lib.rs @@ -48,7 +48,6 @@ unused_qualifications )] -pub mod any_surface; pub mod binding_model; pub mod command; mod conv; diff --git a/wgpu-core/src/present.rs b/wgpu-core/src/present.rs index 2f274cd554..6965d24c3e 100644 --- a/wgpu-core/src/present.rs +++ b/wgpu-core/src/present.rs @@ -157,7 +157,7 @@ impl Global { #[cfg(not(feature = "trace"))] let _ = device; - let suf = A::get_surface(surface.as_ref()); + let suf = A::surface_as_hal(surface.as_ref()); let (texture_id, status) = match unsafe { suf.unwrap() .acquire_texture(Some(std::time::Duration::from_millis( @@ -324,7 +324,7 @@ impl Global { .textures .remove(texture.info.tracker_index()); let mut exclusive_snatch_guard = device.snatchable_lock.write(); - let suf = A::get_surface(&surface); + let suf = A::surface_as_hal(&surface); let mut inner = texture.inner_mut(&mut exclusive_snatch_guard); let inner = inner.as_mut().unwrap(); @@ -418,7 +418,7 @@ impl Global { .lock() .textures .remove(texture.info.tracker_index()); - let suf = A::get_surface(&surface); + let suf = A::surface_as_hal(&surface); let exclusive_snatch_guard = device.snatchable_lock.write(); match texture.inner.snatch(exclusive_snatch_guard).unwrap() { resource::TextureInner::Surface { mut raw, parent_id } => { diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index 0c5f712326..eb55fac9d6 100644 --- a/wgpu-core/src/resource.rs +++ b/wgpu-core/src/resource.rs @@ -1023,7 +1023,9 @@ impl Global { profiling::scope!("Surface::as_hal"); let surface = self.surfaces.get(id).ok(); - let hal_surface = surface.as_ref().and_then(|surface| A::get_surface(surface)); + let hal_surface = surface + .as_ref() + .and_then(|surface| A::surface_as_hal(surface)); hal_surface_callback(hal_surface) } diff --git a/wgpu/src/backend/wgpu_core.rs b/wgpu/src/backend/wgpu_core.rs index edc24cee38..95b56fdf0c 100644 --- a/wgpu/src/backend/wgpu_core.rs +++ b/wgpu/src/backend/wgpu_core.rs @@ -554,12 +554,12 @@ impl crate::Context for ContextWgpuCore { raw_window_handle, } => unsafe { self.0 - .instance_create_surface(raw_display_handle, raw_window_handle, None)? + .instance_create_surface(raw_display_handle, raw_window_handle, None) }, #[cfg(metal)] SurfaceTargetUnsafe::CoreAnimationLayer(layer) => unsafe { - self.0.instance_create_surface_metal(layer, None) + self.0.instance_create_surface_metal(layer, None)? }, #[cfg(dx12)] @@ -578,7 +578,7 @@ impl crate::Context for ContextWgpuCore { self.0 .instance_create_surface_from_swap_chain_panel(swap_chain_panel, None) }, - }; + }?; Ok(( id, From dc7bce3e8e7e77bbd4310ff9358e240cf1e946a7 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 15 Apr 2024 00:06:33 +0200 Subject: [PATCH 3/6] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb7c17a6b9..a46810eb32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -152,6 +152,7 @@ Bottom level categories: - Failing to set the device lost closure will call the closure before returning. By @bradwerth in [#5358](https://github.com/gfx-rs/wgpu/pull/5358). - Use memory pooling for UsageScopes to avoid frequent large allocations. by @robtfm in [#5414](https://github.com/gfx-rs/wgpu/pull/5414) - Fix deadlocks caused by recursive read-write lock acquisitions [#5426](https://github.com/gfx-rs/wgpu/pull/5426). +- Fix surfaces being only compatible with first backend enabled on an instance, causing failures when manually specifying an adapter. By @Wumpf in [#5535](https://github.com/gfx-rs/wgpu/pull/5535). #### Naga - In spv-in, remove unnecessary "gl_PerVertex" name check so unused builtins will always be skipped. By @Imberflur in [#5227](https://github.com/gfx-rs/wgpu/pull/5227). From 764efa4f6f18993e4f25e0a127e07c1a94d43cc7 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 15 Apr 2024 00:18:27 +0200 Subject: [PATCH 4/6] mac fixe --- wgpu/src/backend/wgpu_core.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wgpu/src/backend/wgpu_core.rs b/wgpu/src/backend/wgpu_core.rs index 95b56fdf0c..edf5bc5e26 100644 --- a/wgpu/src/backend/wgpu_core.rs +++ b/wgpu/src/backend/wgpu_core.rs @@ -559,7 +559,7 @@ impl crate::Context for ContextWgpuCore { #[cfg(metal)] SurfaceTargetUnsafe::CoreAnimationLayer(layer) => unsafe { - self.0.instance_create_surface_metal(layer, None)? + self.0.instance_create_surface_metal(layer, None) }, #[cfg(dx12)] From 21f26f2b3e9d2d96660a1dfcf61a7d9b1cb1419a Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 15 Apr 2024 23:12:20 +0200 Subject: [PATCH 5/6] document surface creation failure handling --- wgpu-core/src/instance.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/wgpu-core/src/instance.rs b/wgpu-core/src/instance.rs index b4752397ca..4719a2d945 100644 --- a/wgpu-core/src/instance.rs +++ b/wgpu-core/src/instance.rs @@ -469,14 +469,18 @@ pub enum CreateSurfaceError { #[error("The backend {0} was not enabled on the instance.")] BackendNotEnabled(Backend), #[error("Failed to create surface for any enabled backend.")] - InstanceError(HashMap), + FailedToCreateSurfaceForAnyBackend(HashMap), } impl Global { /// Creates a new surface targeting the given display/window handles. /// /// Internally attempts to create hal surfaces for all enabled backends. - /// TODO DO NOT MERGE: DESCRIBE BONKERS ERROR HANDLING + /// + /// Fails only if creation for surfaces for all enabled backends fails in which case + /// the error for each enabled backend is listed. + /// Vice versa, if creation for any backend succeeds, success is returned. + /// Surface creation errors are logged to the debug log in any case. /// /// id_in: /// - If `Some`, the id to assign to the surface. A new one will be generated otherwise. @@ -572,7 +576,9 @@ impl Global { let (id, _) = self.surfaces.prepare(id_in).assign(Arc::new(surface)); Ok(id) } else { - Err(CreateSurfaceError::InstanceError(errors)) + Err(CreateSurfaceError::FailedToCreateSurfaceForAnyBackend( + errors, + )) } } From 43ba92675960295d1b92123cc5c9daffb10bc8b0 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Tue, 16 Apr 2024 23:58:12 +0200 Subject: [PATCH 6/6] fix canvas_get_context_returned_null --- wgpu-core/src/instance.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wgpu-core/src/instance.rs b/wgpu-core/src/instance.rs index 4719a2d945..6d350598cb 100644 --- a/wgpu-core/src/instance.rs +++ b/wgpu-core/src/instance.rs @@ -468,7 +468,7 @@ pub enum RequestAdapterError { pub enum CreateSurfaceError { #[error("The backend {0} was not enabled on the instance.")] BackendNotEnabled(Backend), - #[error("Failed to create surface for any enabled backend.")] + #[error("Failed to create surface for any enabled backend: {0:?}")] FailedToCreateSurfaceForAnyBackend(HashMap), }