From 8430692fc7354a3fae7aa66ba8e80b9a1b78d4dc Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Mon, 9 Oct 2023 09:34:22 +0200 Subject: [PATCH 1/4] [PATCH] Handle user callbacks in surface_configure --- wgpu-core/src/device/global.rs | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index 62fc7bdf78..bb7d4b1af1 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -2104,15 +2104,19 @@ impl Global { Ok(()) } - log::info!("configuring surface with {:?}", config); - let hub = A::hub(self); - let mut token = Token::root(); + // User callbacks must not be called while we are holding locks. + let mut user_callbacks = None; - let (mut surface_guard, mut token) = self.surfaces.write(&mut token); - let (adapter_guard, mut token) = hub.adapters.read(&mut token); - let (device_guard, mut token) = hub.devices.read(&mut token); + log::info!("configuring surface with {:?}", config); let error = 'outer: loop { + let hub = A::hub(self); + let mut token = Token::root(); + + let (mut surface_guard, mut token) = self.surfaces.write(&mut token); + let (adapter_guard, mut token) = hub.adapters.read(&mut token); + let (device_guard, mut token) = hub.devices.read(&mut token); + let device = match device_guard.get(device_id) { Ok(device) => device, Err(_) => break DeviceError::Invalid.into(), @@ -2184,8 +2188,13 @@ impl Global { } // Wait for all work to finish before configuring the surface. - if let Err(e) = device.maintain(hub, wgt::Maintain::Wait, &mut token) { - break e.into(); + match device.maintain(hub, wgt::Maintain::Wait, &mut token) { + Ok((closures, _)) => { + user_callbacks = Some(closures); + } + Err(e) => { + break e.into(); + } } // All textures must be destroyed before the surface can be re-configured. @@ -2233,6 +2242,10 @@ impl Global { return None; }; + if let Some(callbacks) = user_callbacks { + callbacks.fire(); + } + Some(error) } From c53b3a023332d0a71fb0e09dd3d2b9ec75255d6a Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Wed, 11 Oct 2023 09:35:00 +0200 Subject: [PATCH 2/4] Properly handle user callbacks in surface_configure (#4227) * Properly handle user callbacks in surface_configure * Add a changelog entry --- wgpu-core/src/device/global.rs | 229 +++++++++++++++++---------------- 1 file changed, 117 insertions(+), 112 deletions(-) diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index bb7d4b1af1..92327d62bd 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -2104,148 +2104,153 @@ impl Global { Ok(()) } - // User callbacks must not be called while we are holding locks. - let mut user_callbacks = None; - log::info!("configuring surface with {:?}", config); let error = 'outer: loop { let hub = A::hub(self); let mut token = Token::root(); - let (mut surface_guard, mut token) = self.surfaces.write(&mut token); - let (adapter_guard, mut token) = hub.adapters.read(&mut token); - let (device_guard, mut token) = hub.devices.read(&mut token); - - let device = match device_guard.get(device_id) { - Ok(device) => device, - Err(_) => break DeviceError::Invalid.into(), - }; - #[cfg(feature = "trace")] - if let Some(ref trace) = device.trace { - trace - .lock() - .add(trace::Action::ConfigureSurface(surface_id, config.clone())); - } - - let surface = match surface_guard.get_mut(surface_id) { - Ok(surface) => surface, - Err(_) => break E::InvalidSurface, - }; + // User callbacks must not be called while we are holding locks. + let user_callbacks; + { + let (mut surface_guard, mut token) = self.surfaces.write(&mut token); + let (adapter_guard, mut token) = hub.adapters.read(&mut token); + let (device_guard, mut token) = hub.devices.read(&mut token); - let caps = unsafe { - let suf = A::get_surface(surface); - let adapter = &adapter_guard[device.adapter_id.value]; - match adapter.raw.adapter.surface_capabilities(&suf.unwrap().raw) { - Some(caps) => caps, - None => break E::UnsupportedQueueFamily, + let device = match device_guard.get(device_id) { + Ok(device) => device, + Err(_) => break DeviceError::Invalid.into(), + }; + if !device.valid { + break DeviceError::Invalid.into(); } - }; - let mut hal_view_formats = vec![]; - for format in config.view_formats.iter() { - if *format == config.format { - continue; - } - if !caps.formats.contains(&config.format) { - break 'outer E::UnsupportedFormat { - requested: config.format, - available: caps.formats, - }; - } - if config.format.remove_srgb_suffix() != format.remove_srgb_suffix() { - break 'outer E::InvalidViewFormat(*format, config.format); + #[cfg(feature = "trace")] + if let Some(ref trace) = device.trace { + trace + .lock() + .add(trace::Action::ConfigureSurface(surface_id, config.clone())); } - hal_view_formats.push(*format); - } - if !hal_view_formats.is_empty() { - if let Err(missing_flag) = - device.require_downlevel_flags(wgt::DownlevelFlags::SURFACE_VIEW_FORMATS) - { - break 'outer E::MissingDownlevelFlags(missing_flag); + let surface = match surface_guard.get_mut(surface_id) { + Ok(surface) => surface, + Err(_) => break E::InvalidSurface, + }; + + let caps = unsafe { + let suf = A::get_surface(surface); + let adapter = &adapter_guard[device.adapter_id.value]; + match adapter.raw.adapter.surface_capabilities(&suf.unwrap().raw) { + Some(caps) => caps, + None => break E::UnsupportedQueueFamily, + } + }; + + let mut hal_view_formats = vec![]; + for format in config.view_formats.iter() { + if *format == config.format { + continue; + } + if !caps.formats.contains(&config.format) { + break 'outer E::UnsupportedFormat { + requested: config.format, + available: caps.formats, + }; + } + if config.format.remove_srgb_suffix() != format.remove_srgb_suffix() { + break 'outer E::InvalidViewFormat(*format, config.format); + } + hal_view_formats.push(*format); } - } - let num_frames = present::DESIRED_NUM_FRAMES - .clamp(*caps.swap_chain_sizes.start(), *caps.swap_chain_sizes.end()); - let mut hal_config = hal::SurfaceConfiguration { - swap_chain_size: num_frames, - present_mode: config.present_mode, - composite_alpha_mode: config.alpha_mode, - format: config.format, - extent: wgt::Extent3d { - width: config.width, - height: config.height, - depth_or_array_layers: 1, - }, - usage: conv::map_texture_usage(config.usage, hal::FormatAspects::COLOR), - view_formats: hal_view_formats, - }; + if !hal_view_formats.is_empty() { + if let Err(missing_flag) = + device.require_downlevel_flags(wgt::DownlevelFlags::SURFACE_VIEW_FORMATS) + { + break 'outer E::MissingDownlevelFlags(missing_flag); + } + } - if let Err(error) = validate_surface_configuration(&mut hal_config, &caps) { - break error; - } + let num_frames = present::DESIRED_NUM_FRAMES + .clamp(*caps.swap_chain_sizes.start(), *caps.swap_chain_sizes.end()); + let mut hal_config = hal::SurfaceConfiguration { + swap_chain_size: num_frames, + present_mode: config.present_mode, + composite_alpha_mode: config.alpha_mode, + format: config.format, + extent: wgt::Extent3d { + width: config.width, + height: config.height, + depth_or_array_layers: 1, + }, + usage: conv::map_texture_usage(config.usage, hal::FormatAspects::COLOR), + view_formats: hal_view_formats, + }; - // Wait for all work to finish before configuring the surface. - match device.maintain(hub, wgt::Maintain::Wait, &mut token) { - Ok((closures, _)) => { - user_callbacks = Some(closures); + if let Err(error) = validate_surface_configuration(&mut hal_config, &caps) { + break error; } - Err(e) => { - break e.into(); + + // Wait for all work to finish before configuring the surface. + match device.maintain(hub, wgt::Maintain::Wait, &mut token) { + Ok((closures, _)) => { + user_callbacks = closures; + } + Err(e) => { + break e.into(); + } } - } - // All textures must be destroyed before the surface can be re-configured. - if let Some(present) = surface.presentation.take() { - if present.acquired_texture.is_some() { - break E::PreviousOutputExists; + // All textures must be destroyed before the surface can be re-configured. + if let Some(present) = surface.presentation.take() { + if present.acquired_texture.is_some() { + break E::PreviousOutputExists; + } } - } - // TODO: Texture views may still be alive that point to the texture. - // this will allow the user to render to the surface texture, long after - // it has been removed. - // - // https://github.com/gfx-rs/wgpu/issues/4105 + // TODO: Texture views may still be alive that point to the texture. + // this will allow the user to render to the surface texture, long after + // it has been removed. + // + // https://github.com/gfx-rs/wgpu/issues/4105 - match unsafe { - A::get_surface_mut(surface) - .unwrap() - .raw - .configure(&device.raw, &hal_config) - } { - Ok(()) => (), - Err(error) => { - break match error { - hal::SurfaceError::Outdated | hal::SurfaceError::Lost => E::InvalidSurface, - hal::SurfaceError::Device(error) => E::Device(error.into()), - hal::SurfaceError::Other(message) => { - log::error!("surface configuration failed: {}", message); - E::InvalidSurface + match unsafe { + A::get_surface_mut(surface) + .unwrap() + .raw + .configure(&device.raw, &hal_config) + } { + Ok(()) => (), + Err(error) => { + break match error { + hal::SurfaceError::Outdated | hal::SurfaceError::Lost => { + E::InvalidSurface + } + hal::SurfaceError::Device(error) => E::Device(error.into()), + hal::SurfaceError::Other(message) => { + log::error!("surface configuration failed: {}", message); + E::InvalidSurface + } } } } + + surface.presentation = Some(present::Presentation { + device_id: Stored { + value: id::Valid(device_id), + ref_count: device.life_guard.add_ref(), + }, + config: config.clone(), + num_frames, + acquired_texture: None, + }); } - surface.presentation = Some(present::Presentation { - device_id: Stored { - value: id::Valid(device_id), - ref_count: device.life_guard.add_ref(), - }, - config: config.clone(), - num_frames, - acquired_texture: None, - }); + user_callbacks.fire(); return None; }; - if let Some(callbacks) = user_callbacks { - callbacks.fire(); - } - Some(error) } From 3565d640994158ac074de65c298a35fe8483d87b Mon Sep 17 00:00:00 2001 From: Duncan Fairbanks Date: Thu, 29 Feb 2024 17:44:09 -0800 Subject: [PATCH 3/4] HACK: roll back the device.valid check This came from a PR that is way harder to cherry pick. --- wgpu-core/src/device/global.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index 92327d62bd..a04e98cc61 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -2121,9 +2121,9 @@ impl Global { Ok(device) => device, Err(_) => break DeviceError::Invalid.into(), }; - if !device.valid { - break DeviceError::Invalid.into(); - } + // if !device.valid { + // break DeviceError::Invalid.into(); + // } #[cfg(feature = "trace")] if let Some(ref trace) = device.trace { From 3ee424fc5ba0053eb7a2c8a292a5da3510e425a9 Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Sun, 5 Nov 2023 21:09:41 -0500 Subject: [PATCH 4/4] Fix Panic in Surface Configure (#4635) --- wgpu-core/src/device/global.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index a04e98cc61..fb5065dba6 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -2107,12 +2107,12 @@ impl Global { log::info!("configuring surface with {:?}", config); let error = 'outer: loop { - let hub = A::hub(self); - let mut token = Token::root(); - // User callbacks must not be called while we are holding locks. let user_callbacks; { + let hub = A::hub(self); + let mut token = Token::root(); + let (mut surface_guard, mut token) = self.surfaces.write(&mut token); let (adapter_guard, mut token) = hub.adapters.read(&mut token); let (device_guard, mut token) = hub.devices.read(&mut token);