-
Notifications
You must be signed in to change notification settings - Fork 976
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.
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
hal: cargo feature to allow using VK_GOOGLE_display_timing
unsafely
#6149
Conversation
4bbe4b0
to
5a7a205
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can activate the feature differently, but the code itself is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks interesting as a start but I wonder how useful it is without also providing the results from vkGetPastPresentationTimingGOOGLE()
. Or are you driving these timings from Choreographer
callbacks?
Co-authored-by: Marijn Suijten <marijns95@gmail.com>
Co-authored-by: Marijn Suijten <marijns95@gmail.com>
Co-authored-by: Marijn Suijten <marijns95@gmail.com>
I have documented this in the PR on the feature now - is it clearer. Essentially, because this isn't a fully supported feature, I expect users to manually set these things up themselves (i.e. call With the aim of keeping this change as focused as possible, I only provided the absolute bare minimum of functionality to allow experimenting with this extension outside of wgpu - i.e. the functionality which could not be accessed from the outside. |
I'm experimenting with this in my android-pacing branch, and indeed have it working as expected on Android DJMcNab/vello@2673d0c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks!
hal: cargo feature to allow using `VK_GOOGLE_display_timing` unsafely (gfx-rs#6149) * Expose the raw swapchain from a vulkan `Surface` * Allow setting the present timing information on hal Vulkan * Fix clippy without the feature enabled * CHANGELOG * Revert inadvertently formatted Cargo.toml * Move display timing to a feature * Update the changelog * Whitespace and doc wording tweaks * Apply suggestions from code review Co-authored-by: Marijn Suijten <marijns95@gmail.com> * Revert inadvertent formatting changes again * Remove unused qualification Co-authored-by: Marijn Suijten <marijns95@gmail.com> * Address review feedback * Fix flow of sentence and follow intra-doc-link * Add more docs to `set_next_present_time`, and rename * Also rename `next_present_times` * Apply suggestions from code review Co-authored-by: Marijn Suijten <marijns95@gmail.com> --------- Co-authored-by: Marijn Suijten <marijns95@gmail.com>
Connections
Description
In many applications, you need to do frame pacing properly to get acceptable latency performance.
On Android, this is exposed through the VK_GOOGLE_display_timing device extension. However, for applications using
wgpu
, it is currently impossible to use this extension for three reasons, each of which are addressed in this PR:vkGetPastPresentationTimingGOOGLE
present
call with theVkPresentTimesInfoGOOGLE
The solution here addresses the first of these by adding a
raw_swapchain
method towgpu_hal::vulkan::Surface
.The remaining two are addressed using targeted hacks, both protected by the new perma-unstable
wgpu-hal
featureunstable_vulkan_google_display_timing
. The extension is always enabled if available when the feature is set, and the additional present information for the "next" presentation is cached on thewgpu-hal::vulkan::Surface
object, which will be accessed usingwgpu::Surface::as_hal
.This PR does not change the high-level
wgpu
API, and instead requires users of this feature to add a dependency onwgpu-hal
themselves.Testing
This feature is currently not tested. My intention is to integrate this PR with Vello before this is merged.
I don't think it is possible to automate testing of this PR, as we do not have any tests running on Android, and only MoltenVK implements this outside of Android.
Checklist
cargo fmt
.cargo clippy
. If applicable, add:--target wasm32-unknown-unknown
N/A--target wasm32-unknown-emscripten
N/Acargo xtask test
to run tests. These kill my editor and any terminal emulator at the moment, but I can't imagine this changes anything.CHANGELOG.md
. See simple instructions inside file.