Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly handle user callbacks in surface_configure #4227

Merged
merged 2 commits into from
Oct 11, 2023
Merged

Properly handle user callbacks in surface_configure #4227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3cf239
Properly handle user callbacks in surface_configure
nical Oct 10, 2023
4e270b7
Add a changelog entry
nical Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ By @teoxoy in [#4185](https://github.com/gfx-rs/wgpu/pull/4185)
- `Queue::on_submitted_work_done` callbacks will now always be called after all previous `BufferSlice::map_async` callbacks, even when there are no active submissions. By @cwfitzgerald in [#4036](https://github.com/gfx-rs/wgpu/pull/4036).
- Fix `clear` texture views being leaked when `wgpu::SurfaceTexture` is dropped before it is presented. By @rajveermalviya in [#4057](https://github.com/gfx-rs/wgpu/pull/4057).
- Add `Feature::SHADER_UNUSED_VERTEX_OUTPUT` to allow unused vertex shader outputs. By @Aaron1011 in [#4116](https://github.com/gfx-rs/wgpu/pull/4116).
- Fix a panic in `surface_configure`. By @nical in [#4220](https://github.com/gfx-rs/wgpu/pull/4220) and [#4227](https://github.com/gfx-rs/wgpu/pull/4227)

#### Vulkan
- Fix enabling `wgpu::Features::PARTIALLY_BOUND_BINDING_ARRAY` not being actually enabled in vulkan backend. By @39ali in[#3772](https://github.com/gfx-rs/wgpu/pull/3772).
Expand Down
231 changes: 116 additions & 115 deletions wgpu-core/src/device/global.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2262,152 +2262,153 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
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);
// 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 device = match device_guard.get(device_id) {
Ok(device) => device,
Err(_) => break DeviceError::Invalid.into(),
};
if !device.valid {
break DeviceError::Invalid.into();
}
let device = match device_guard.get(device_id) {
Ok(device) => device,
Err(_) => break DeviceError::Invalid.into(),
};
if !device.valid {
break DeviceError::Invalid.into();
}

#[cfg(feature = "trace")]
if let Some(ref trace) = device.trace {
trace
.lock()
.add(trace::Action::ConfigureSurface(surface_id, config.clone()));
}
#[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,
};
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 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);
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);
}
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);
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 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 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)
}

Expand Down