Acquire texture: Option<std::time::Duration> timeouts
#2724
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cwfitzgerald
requested changes
Jun 4, 2022
rib
force-pushed
the
acquire_texture_duration_timeouts
branch
from
9105c39
to
abd6f05
Compare
cwfitzgerald
requested changes
Jun 4, 2022
Gordon-F
reviewed
Jun 4, 2022
rib
force-pushed
the
acquire_texture_duration_timeouts
branch
from
abd6f05
to
e982fd4
Compare
|
ah, oops I also missed removing the the |
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
rib
force-pushed
the
acquire_texture_duration_timeouts
branch
from
e982fd4
to
b60aaf1
Compare
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)
auto-merge was automatically disabled
June 4, 2022 12:26
Head branch was pushed to by a user without write access
rib
force-pushed
the
acquire_texture_duration_timeouts
branch
from
b60aaf1
to
ea233d2
Compare
|
btw just pushed a last second tweak to include the value of the property in the log message in case of parse failure, as suggested |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The Vulkan backend wasn't correctly interpreting the u32 timeout value for Surface
acquire_textureconsidering the special meaning foru32::MAXthat should mean 'no timeout'.The Vulkan
vkAcquireNextImageKHRAPI takes a 64bit timeout in nanoseconds andu32::MAXtimeout in milliseconds should have been converted to au64::MAX.Prior to Android 11 then Android's
vkAcquireNextImageKHRimplementation is also non-conformant and doesn't support timeouts (and will also output verbose log messages if timeouts are given).To make the intent clearer, this updates the
acquire_textureAPI to take astd::time::Durationthat allows the caller to use whatever units are convenient. Additionally the duration is wrapped in anOptionso it's clearer to see when no timeout is wanted without needing to reserve a special value to indicate this.On Android < 11 then any given timeout is now ignored (as with the egl / web / metal backends) and
u64::MAXis always passed tovkAcquireNextImageKHRFor 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)
Testing
I have tested this via an egui Android application I have been working on recently, and this can also be tested with this minimal winit + wgpu Android application: https://github.com/rib/agdk-rust/tree/main/examples/na-winit-wgpu and these same patches based on the 0.12 branch of wgpu: https://github.com/rib/wgpu/tree/acquire_texture_duration_timeouts_v0.12