From 3657915eeb279e134021c1070f7628b6173ff35e Mon Sep 17 00:00:00 2001 From: Robert Bragg Date: Fri, 3 Jun 2022 09:08:28 +0100 Subject: [PATCH 1/2] surface.acquire_texture: pass Option for timeout A std::time::Duration allows for timeouts to be specified more clearly in Rust using whatever units are convenient for the caller, and an Option also makes it clearer in case no timeout is wanted, as opposed to passing a bitwise !0 as special timeout value. Notably there was an impedance mismatch with the Vulkan backend that takes a 64bit timeout in nanoseconds and uses u64::MAX to indicate no timeout and the backend was not mapping a given u32::MAX into a u64::MAX --- wgpu-core/src/present.rs | 7 ++++++- wgpu-hal/examples/halmark/main.rs | 2 +- wgpu-hal/src/dx11/mod.rs | 2 +- wgpu-hal/src/dx12/mod.rs | 15 +++++++++++---- wgpu-hal/src/empty.rs | 2 +- wgpu-hal/src/gles/egl.rs | 2 +- wgpu-hal/src/gles/web.rs | 2 +- wgpu-hal/src/lib.rs | 11 ++++++++++- wgpu-hal/src/metal/surface.rs | 2 +- wgpu-hal/src/vulkan/instance.rs | 8 ++++++-- 10 files changed, 39 insertions(+), 14 deletions(-) diff --git a/wgpu-core/src/present.rs b/wgpu-core/src/present.rs index fa7a34bf9d..8f7182a9be 100644 --- a/wgpu-core/src/present.rs +++ b/wgpu-core/src/present.rs @@ -123,7 +123,12 @@ impl Global { let _ = device; let suf = A::get_surface_mut(surface); - let (texture_id, status) = match unsafe { suf.raw.acquire_texture(FRAME_TIMEOUT_MS) } { + let (texture_id, status) = match unsafe { + suf.raw + .acquire_texture(Some(std::time::Duration::from_millis( + FRAME_TIMEOUT_MS as u64, + ))) + } { Ok(Some(ast)) => { let clear_view_desc = hal::TextureViewDescriptor { label: Some("(wgpu internal) clear surface texture view"), diff --git a/wgpu-hal/examples/halmark/main.rs b/wgpu-hal/examples/halmark/main.rs index ff021a889d..d9638eb400 100644 --- a/wgpu-hal/examples/halmark/main.rs +++ b/wgpu-hal/examples/halmark/main.rs @@ -614,7 +614,7 @@ impl Example { let ctx = &mut self.contexts[self.context_index]; - let surface_tex = unsafe { self.surface.acquire_texture(!0).unwrap().unwrap().texture }; + let surface_tex = unsafe { self.surface.acquire_texture(None).unwrap().unwrap().texture }; let target_barrier0 = hal::TextureBarrier { texture: surface_tex.borrow(), diff --git a/wgpu-hal/src/dx11/mod.rs b/wgpu-hal/src/dx11/mod.rs index ff9c49e0b3..a77bb95919 100644 --- a/wgpu-hal/src/dx11/mod.rs +++ b/wgpu-hal/src/dx11/mod.rs @@ -124,7 +124,7 @@ impl crate::Surface for Surface { unsafe fn acquire_texture( &mut self, - timeout_ms: u32, + _timeout: Option, ) -> Result>, crate::SurfaceError> { todo!() } diff --git a/wgpu-hal/src/dx12/mod.rs b/wgpu-hal/src/dx12/mod.rs index 4b048ff009..089f8c9f43 100644 --- a/wgpu-hal/src/dx12/mod.rs +++ b/wgpu-hal/src/dx12/mod.rs @@ -527,7 +527,14 @@ impl SwapChain { self.raw } - unsafe fn wait(&mut self, timeout_ms: u32) -> Result { + unsafe fn wait( + &mut self, + timeout: Option, + ) -> Result { + let timeout_ms = match timeout { + Some(duration) => duration.as_millis() as u32, + None => winbase::INFINITE, + }; match synchapi::WaitForSingleObject(self.waitable, timeout_ms) { winbase::WAIT_ABANDONED | winbase::WAIT_FAILED => Err(crate::SurfaceError::Lost), winbase::WAIT_OBJECT_0 => Ok(true), @@ -690,7 +697,7 @@ impl crate::Surface for Surface { unsafe fn unconfigure(&mut self, device: &Device) { if let Some(mut sc) = self.swap_chain.take() { - let _ = sc.wait(winbase::INFINITE); + let _ = sc.wait(None); //TODO: this shouldn't be needed, // but it complains that the queue is still used otherwise let _ = device.wait_idle(); @@ -701,11 +708,11 @@ impl crate::Surface for Surface { unsafe fn acquire_texture( &mut self, - timeout_ms: u32, + timeout: Option, ) -> Result>, crate::SurfaceError> { let sc = self.swap_chain.as_mut().unwrap(); - sc.wait(timeout_ms)?; + sc.wait(timeout)?; let base_index = sc.raw.GetCurrentBackBufferIndex() as usize; let index = (base_index + sc.acquired_count) % sc.resources.len(); diff --git a/wgpu-hal/src/empty.rs b/wgpu-hal/src/empty.rs index be5708da38..0c546469b2 100644 --- a/wgpu-hal/src/empty.rs +++ b/wgpu-hal/src/empty.rs @@ -66,7 +66,7 @@ impl crate::Surface for Context { unsafe fn acquire_texture( &mut self, - timeout_ms: u32, + timeout: Option, ) -> Result>, crate::SurfaceError> { Ok(None) } diff --git a/wgpu-hal/src/gles/egl.rs b/wgpu-hal/src/gles/egl.rs index 06167ec841..fc3c331e9b 100644 --- a/wgpu-hal/src/gles/egl.rs +++ b/wgpu-hal/src/gles/egl.rs @@ -1194,7 +1194,7 @@ impl crate::Surface for Surface { unsafe fn acquire_texture( &mut self, - _timeout_ms: u32, //TODO + _timeout_ms: Option, //TODO ) -> Result>, crate::SurfaceError> { let sc = self.swapchain.as_ref().unwrap(); let texture = super::Texture { diff --git a/wgpu-hal/src/gles/web.rs b/wgpu-hal/src/gles/web.rs index 1c0ee6a9db..08f264e8ec 100644 --- a/wgpu-hal/src/gles/web.rs +++ b/wgpu-hal/src/gles/web.rs @@ -253,7 +253,7 @@ impl crate::Surface for Surface { unsafe fn acquire_texture( &mut self, - _timeout_ms: u32, + _timeout_ms: Option, //TODO ) -> Result>, crate::SurfaceError> { let sc = self.swapchain.as_ref().unwrap(); let texture = super::Texture { diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index 1ce7c657ba..83fc834601 100644 --- a/wgpu-hal/src/lib.rs +++ b/wgpu-hal/src/lib.rs @@ -193,10 +193,19 @@ pub trait Surface: Send + Sync { unsafe fn unconfigure(&mut self, device: &A::Device); + /// Returns the next texture to be presented by the swapchain for drawing + /// + /// A `timeout` of `None` means to wait indefinitely, with no timeout. + /// + /// # Portability + /// + /// Some backends can't support a timeout when acquiring a texture and + /// the timeout will be ignored. + /// /// Returns `None` on timing out. unsafe fn acquire_texture( &mut self, - timeout_ms: u32, + timeout: Option, ) -> Result>, SurfaceError>; unsafe fn discard_texture(&mut self, texture: A::SurfaceTexture); } diff --git a/wgpu-hal/src/metal/surface.rs b/wgpu-hal/src/metal/surface.rs index 124cf910f3..a0ca46ab35 100644 --- a/wgpu-hal/src/metal/surface.rs +++ b/wgpu-hal/src/metal/surface.rs @@ -216,7 +216,7 @@ impl crate::Surface for super::Surface { unsafe fn acquire_texture( &mut self, - _timeout_ms: u32, //TODO + _timeout_ms: Option, //TODO ) -> Result>, crate::SurfaceError> { let render_layer = self.render_layer.lock(); let (drawable, texture) = match autoreleasepool(|| { diff --git a/wgpu-hal/src/vulkan/instance.rs b/wgpu-hal/src/vulkan/instance.rs index 4a814fc16c..ad137f090e 100644 --- a/wgpu-hal/src/vulkan/instance.rs +++ b/wgpu-hal/src/vulkan/instance.rs @@ -707,10 +707,14 @@ impl crate::Surface for super::Surface { unsafe fn acquire_texture( &mut self, - timeout_ms: u32, + timeout: Option, ) -> Result>, crate::SurfaceError> { let sc = self.swapchain.as_mut().unwrap(); - let timeout_ns = timeout_ms as u64 * super::MILLIS_TO_NANOS; + + let timeout_ns = match timeout { + Some(duration) => duration.as_nanos() as u64, + None => u64::MAX, + }; // will block if no image is available let (index, suboptimal) = From ea233d233d88cfaf8717a52f94c093c0c06ee376 Mon Sep 17 00:00:00 2001 From: Robert Bragg Date: Fri, 3 Jun 2022 09:08:28 +0100 Subject: [PATCH 2/2] surface.acquire_texture: ignore timeout for Android < 11 Prior to Android 11 then Android's vkAcquireNextImageKHR implementation was non-conformant and didn't support timeouts and additionally would log a verbose warning if a timeout was requested. For reference this version of AcquireNextImageKHR doesn't support timeouts: https://android.googlesource.com/platform/frameworks/native/+/refs/tags/android-mainline-10.0.0_r13/vulkan/libvulkan/swapchain.cpp#1426 and this version does: https://android.googlesource.com/platform/frameworks/native/+/refs/tags/android-mainline-11.0.0_r45/vulkan/libvulkan/swapchain.cpp#1438 (i.e. timeout support was added in Android 11) This patch adds a dependency on the `android-properties` crate that provides a simple wrapper for the `__system_property_set` syscall so that the platform version can be read via the `ro.build.version.sdk` property and then for versions < 30 (corresponds to Android 11) any timeout given to `acquire_texture` will be ignored (and `u64::MAX` will be passed to Vulkan) --- Cargo.lock | 7 ++++++ wgpu-hal/Cargo.toml | 3 +++ wgpu-hal/src/vulkan/instance.rs | 42 ++++++++++++++++++++++++++++++++- wgpu-hal/src/vulkan/mod.rs | 1 + 4 files changed, 52 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index f66040ae51..0f5ee9f197 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -34,6 +34,12 @@ dependencies = [ "memchr", ] +[[package]] +name = "android-properties" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc7eb209b1518d6bb87b283c20095f5228ecda460da70b44f0802523dea6da04" + [[package]] name = "android_glue" version = "0.2.3" @@ -2101,6 +2107,7 @@ dependencies = [ name = "wgpu-hal" version = "0.12.0" dependencies = [ + "android-properties", "arrayvec", "ash", "bit-set", diff --git a/wgpu-hal/Cargo.toml b/wgpu-hal/Cargo.toml index caad696d40..020b39cc9a 100644 --- a/wgpu-hal/Cargo.toml +++ b/wgpu-hal/Cargo.toml @@ -90,6 +90,9 @@ wasm-bindgen = { version = "0.2" } web-sys = { version = "0.3", features = ["Window", "HtmlCanvasElement", "WebGl2RenderingContext"] } js-sys = { version = "0.3" } +[target.'cfg(target_os = "android")'.dependencies] +android-properties = "0.2" + [dependencies.naga] git = "https://github.com/gfx-rs/naga" rev = "571302e" diff --git a/wgpu-hal/src/vulkan/instance.rs b/wgpu-hal/src/vulkan/instance.rs index ad137f090e..c12ef1b5c5 100644 --- a/wgpu-hal/src/vulkan/instance.rs +++ b/wgpu-hal/src/vulkan/instance.rs @@ -214,10 +214,13 @@ impl super::Instance { /// - `raw_instance` must be created respecting `driver_api_version`, `extensions` and `flags` /// - `extensions` must be a superset of `required_extensions()` and must be created from the /// same entry, driver_api_version and flags. + /// - `android_sdk_version` is ignored and can be `0` for all platforms besides Android + #[allow(clippy::too_many_arguments)] pub unsafe fn from_raw( entry: ash::Entry, raw_instance: ash::Instance, driver_api_version: u32, + android_sdk_version: u32, extensions: Vec<&'static CStr>, flags: crate::InstanceFlags, has_nv_optimus: bool, @@ -283,6 +286,7 @@ impl super::Instance { entry, has_nv_optimus, driver_api_version, + android_sdk_version, }), extensions, }) @@ -557,6 +561,28 @@ impl crate::Instance for super::Instance { layers }; + #[cfg(target_os = "android")] + let android_sdk_version = { + // See: https://developer.android.com/reference/android/os/Build.VERSION_CODES + let mut prop = android_properties::getprop("ro.build.version.sdk"); + if let Some(val) = prop.value() { + match val.parse::() { + Ok(sdk_ver) => sdk_ver, + Err(err) => { + log::error!( + "Couldn't parse Android's ro.build.version.sdk system property ({val}): {err}" + ); + 0 + } + } + } else { + log::error!("Couldn't read Android's ro.build.version.sdk system property"); + 0 + } + }; + #[cfg(not(target_os = "android"))] + let android_sdk_version = 0; + let vk_instance = { let str_pointers = layers .iter() @@ -583,6 +609,7 @@ impl crate::Instance for super::Instance { entry, vk_instance, driver_api_version, + android_sdk_version, extensions, desc.flags, has_nv_optimus, @@ -711,11 +738,24 @@ impl crate::Surface for super::Surface { ) -> Result>, crate::SurfaceError> { let sc = self.swapchain.as_mut().unwrap(); - let timeout_ns = match timeout { + let mut timeout_ns = match timeout { Some(duration) => duration.as_nanos() as u64, None => u64::MAX, }; + // AcquireNextImageKHR on Android (prior to Android 11) doesn't support timeouts + // and will also log verbose warnings if tying to use a timeout. + // + // Android 10 implementation for reference: + // https://android.googlesource.com/platform/frameworks/native/+/refs/tags/android-mainline-10.0.0_r13/vulkan/libvulkan/swapchain.cpp#1426 + // Android 11 implementation for reference: + // https://android.googlesource.com/platform/frameworks/native/+/refs/tags/android-mainline-11.0.0_r45/vulkan/libvulkan/swapchain.cpp#1438 + // + // Android 11 corresponds to an SDK_INT/ro.build.version.sdk of 30 + if cfg!(target_os = "android") && self.instance.android_sdk_version < 30 { + timeout_ns = u64::MAX; + } + // will block if no image is available let (index, suboptimal) = match sc diff --git a/wgpu-hal/src/vulkan/mod.rs b/wgpu-hal/src/vulkan/mod.rs index b3e1a0aa7f..f79d9e00ee 100644 --- a/wgpu-hal/src/vulkan/mod.rs +++ b/wgpu-hal/src/vulkan/mod.rs @@ -87,6 +87,7 @@ struct InstanceShared { get_physical_device_properties: Option, entry: ash::Entry, has_nv_optimus: bool, + android_sdk_version: u32, driver_api_version: u32, }