Implement vulkan video extensions #916
Conversation
There was a problem hiding this comment.
Thanks for getting started on this.
It would however be nice to mark your PR as draft until it is ready for a review, to save the maintainers some time pointing out basic inconsistencies. For example, other parts of the ash codebase seem to have been used as template to implement new functions, yet modified in such a way that they are unnecessarily inconsistent.
Once those are merged this should be much closer to a mergable state. Thanks :)
| video_session: vk::VideoSessionKHR, | ||
| ) -> usize { | ||
| let mut memory_requirements_count = mem::MaybeUninit::uninit(); | ||
| let _ = (self.fp.get_video_session_memory_requirements_khr)( |
There was a problem hiding this comment.
This is the first time in ash that a Result gets ignored. With zero (comment) explanations why.
| @@ -90,6 +90,7 @@ By disabling the default `std` feature, this crate compiles in a [`no_std` envir | |||
| - Added `VK_KHR_calibrated_timestamps` device extension (#890) | |||
| - Added `VK_KHR_maintenance6` device extension (#891) | |||
| - Added `VK_NV_copy_memory_indirect` device extension (#892) | |||
| - Added support for vulkan video extensions (#916) | |||
There was a problem hiding this comment.
This can't retroactively be part of the 0.30.0 release that was already published. Please move this line under ## [Unreleased] - ReleaseDate above, and list out the extension names explicitly.
There was a problem hiding this comment.
VK_KHR_video_encode_queue also has one instance-level function that hasn't been wrapped here?
| @@ -0,0 +1,49 @@ | |||
| //! <https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_KHR_video_encode_queue.html> | |||
|
|
|||
| use crate::prelude::VkResult; | |||
There was a problem hiding this comment.
The idea of a prelude is to always import *.
| pub unsafe fn get_encoded_video_session_parameters( | ||
| &self, | ||
| video_session_parameters_info: &vk::VideoEncodeSessionParametersGetInfoKHR<'_>, | ||
| ) -> VkResult<(vk::VideoEncodeSessionParametersFeedbackInfoKHR<'_>, Vec<u8>)> { |
There was a problem hiding this comment.
vk::VideoEncodeSessionParametersFeedbackInfoKHR<'_> is a struct with a p_next chain that allows the user to request additional info. It should be a &mut function argument instead of being output-only.
| video_format_info: &vk::PhysicalDeviceVideoFormatInfoKHR<'_>, | ||
| ) -> usize { | ||
| let mut memory_requirements_count = mem::MaybeUninit::uninit(); | ||
| let _ = (self.fp.get_physical_device_video_format_properties_khr)( |
There was a problem hiding this comment.
We've never ignored Vulkan errors, and you have no reason to do so either? A low-level wrapper should pass all the info that it gets right back to the caller and not hide anything.
| &mut count, | ||
| out.as_mut_ptr(), | ||
| ).result() | ||
| } |
There was a problem hiding this comment.
Missing an assert that count wasn't updated to value != out.len().
| ) | ||
| } | ||
|
|
||
| // Retrieve the number of elements to pass to [`get_video_session_memory_requirements`][Self::get_video_session_memory_requirements] |
| &mut count, | ||
| out.as_mut_ptr(), | ||
| ).result() | ||
| } |
There was a problem hiding this comment.
Same assert is missing here.
| (self.fp.bind_video_session_memory_khr)( | ||
| self.handle, | ||
| video_session, | ||
| bind_session_memory_infos.len() as _, |
There was a problem hiding this comment.
Only khr/acceleration_structures.rs is erroneously using typeless casts. Please specify u32 here explicitly.
Closes #722
Added support for the video extensions.