From 444836f7471a2742f280b17faab2562486249cc7 Mon Sep 17 00:00:00 2001 From: Robert Bragg Date: Sat, 4 Jun 2022 17:05:40 +0100 Subject: [PATCH] Acquire texture: `Option` timeouts (#2724) * 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 * 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-core/src/present.rs | 7 ++++- wgpu-hal/Cargo.toml | 3 ++ 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 | 48 +++++++++++++++++++++++++++++-- wgpu-hal/src/vulkan/mod.rs | 1 + 13 files changed, 90 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f66040ae5..0f5ee9f19 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-core/src/present.rs b/wgpu-core/src/present.rs index fa7a34bf9..8f7182a9b 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/Cargo.toml b/wgpu-hal/Cargo.toml index caad696d4..020b39cc9 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/examples/halmark/main.rs b/wgpu-hal/examples/halmark/main.rs index ff021a889..d9638eb40 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 ff9c49e0b..a77bb9591 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 4b048ff00..089f8c9f4 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 be5708da3..0c546469b 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 06167ec84..fc3c331e9 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 1c0ee6a9d..08f264e8e 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 1ce7c657b..83fc83460 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 124cf910f..a0ca46ab3 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 4a814fc16..c12ef1b5c 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, @@ -707,10 +734,27 @@ 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 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) = diff --git a/wgpu-hal/src/vulkan/mod.rs b/wgpu-hal/src/vulkan/mod.rs index b3e1a0aa7..f79d9e00e 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, }