Add wrappers for `VK_KHR_video{,_encode,_decode}_queue` #965

colinmarc · 2024-11-30T17:44:33Z

Closes #722, closes #916

I've been using these via internal wrappers for a good bit.

~~One downside (or is it a feature?) of the new impl crate::khr::video_decode_queue::Instance pattern is that I can't keep the wrappers in my project if I want to upgrade :)~~ EDIT: Stale observation, resolved below :)

MarijnS95 · 2024-12-02T10:57:55Z

Before looking, how is this supposed to compare to #916? That PR has a not-insignificant amount of basic review comments, and has stalled since.

When a PR is opened, and is supposed to supersede another PR, I expect a little attribution documenting what this PR does better than the old PR. I.e. more specifically, can I assume that all of the review comments there have been resolved in this PR, and won't need to be commented on again (e.g. because it was developed in isolation from that PR)?

MarijnS95

This PR is unfortunately forward in some places, but backwards in other places compared to !916. Please stick to the style used in other parts of the ash code-base, and take extra care when dealing with "returned" slices of structures containing pNext chains: the user should be able to set those before calling into the helper.

ash/src/extensions/khr/video_encode_queue.rs

ash/src/extensions/khr/video_decode_queue.rs

ash/src/extensions/khr/video_encode_queue.rs

ash/src/extensions/khr/video_decode_queue.rs

ash/src/extensions/khr/video_queue.rs

MarijnS95 · 2024-12-02T11:18:26Z

And don't forget a changelog entry 😰

MarijnS95 · 2024-12-02T11:39:37Z

One downside (or is it a feature?) of the new impl crate::khr::video_decode_queue::Instance pattern is that I can't keep the wrappers in my project if I want to upgrade :)

What is this supposed to mean? I don't think anything is "new" here besides the struct Instance being autogenerated, and having a restructured module hierarchy. impl blocks for types were always allowed in arbitrary modules, as long as those impl blocks live within the same crate.

No clue what kind of upgrade you are anticipating: if these wrappers are merged to ash, and make it into a release, you can remove them from your project?

colinmarc · 2024-12-02T11:49:02Z

What is this supposed to mean? I don't think anything is "new" here besides the struct Instance being autogenerated, and having a restructured module hierarchy. impl blocks for types were always allowed in arbitrary modules, as long as those impl blocks live within the same crate.

I'm just pointing to the fact that the wrappers have to live inside ash if they want to follow that pattern - it's not possible to impl the definition outside in some other crate, as you correctly point out. My linked in-tree wrapper was using the old pattern, or at least I thought it was, and the change in pattern prompted me to PR this instead of keeping it in-tree. Please don't read too much into it, it was just an observation.

MarijnS95 · 2024-12-02T11:53:13Z

I'm just pointing to the fact that the wrappers have to live inside ash if they want to follow that pattern - it's not possible to impl the definition outside in some other crate, as you correctly point out. My linked in-tree wrapper was using the old pattern, or at least I thought it was, and the change in pattern prompted me to PR this instead of keeping it in-tree. Please don't read too much into it, it was just an observation.

The pattern is identical. You could still define your own struct VideoQueueExt, using the same members or using ash::khr::video_queue::Device as its sole member with a Deref to be able to call the wrapper functions inside ash.

Defining impl on foreign types has never been supported by Rust, and is not something that has been changed by whatever "new' extension pattern you are referring to.

colinmarc · 2024-12-02T12:54:19Z

I think I got all your comments except the FnMut change to read_into_uninitialized_vector.

Defining impl on foreign types has never been supported by Rust, and is not something that has been changed by whatever "new' extension pattern you are referring to.

I'm sorry if I stepped on your toes somehow - please consider the observation retracted.

When a PR is opened, and is supposed to supersede another PR, I expect a little attribution documenting what this PR does better than the old PR. I.e. more specifically, can I assume that all of the review comments there have been resolved in this PR, and won't need to be commented on again (e.g. because it was developed in isolation from that PR)?

To be completely honest, I glanced at #916, but it didn't seem far enough along to work off of. This code came from a wrapper internal to my project, which I wrote before #916 existed. I understand that you don't want to review the ~same code twice, but if that's the case then feel free to just close this.

colinmarc · 2024-12-02T13:48:40Z

Ugh, I forgot that I split this into multiple commits last night - I've just been amending the last. I will clean it up, one sec.

MarijnS95

Defining impl on foreign types has never been supported by Rust, and is not something that has been changed by whatever "new' extension pattern you are referring to.

I'm sorry if I stepped on your toes somehow - please consider the observation retracted.

Okay. If there's something wrong with the ash API, note that I do need to understand the issue and the scope of it in case there is something we are doing wrong or could do better. I still don't understand your request/observation, so I cannot help (other than refuting statements in hopes of better understanding the point of contention).

To be completely honest, I glanced at #916, but it didn't seem far enough along to work off of.

That's the kind of information I'd like to see in a PR description. That way I know I don't have to directly correlate the review comments and confirm they were addressed as requested, for example. On the other hand, knowing that you would have looked at those review comments on a stale PR, and tried hard to preclude them from your PR in the first place, is a huge bonus.

This code came from a wrapper internal to my project, which I wrote before #916 existed.

All fine if you wrote the code before #916 existed, just wanted to know if you updated/modified the code before making this PR which happened after #916 existed (and was hard to miss given that it was mentioned in #722).

I understand that you don't want to review the ~same code twice, but if that's the case then feel free to just close this.

At least we actually made progress in this PR nice and quickly, and this looks all set to be merged before the next ash Release 🎉

Ugh, I forgot that I split this into multiple commits last night - I've just been amending the last. I will clean it up, one sec.

Don't worry, we'll probably end up squash-merging this as we do with any PR.

Changelog.md

ash/src/extensions/khr/video_queue.rs

MarijnS95 · 2024-12-02T13:41:28Z

ash/src/extensions/khr/video_encode_queue.rs

+    pub unsafe fn get_encoded_video_session_parameters(
+        &self,
+        session_parameters_info: &vk::VideoEncodeSessionParametersGetInfoKHR<'_>,
+        info: &mut vk::VideoEncodeSessionParametersFeedbackInfoKHR<'_>,


Nit: prefer to match this to the signature declaration:

Suggested change

info: &mut vk::VideoEncodeSessionParametersFeedbackInfoKHR<'_>,

feedback_info: &mut vk::VideoEncodeSessionParametersFeedbackInfoKHR<'_>,

Also, I think this is allowed to be None? Perhaps that's only useful to initially query the size of data, and/or to only query data?

Likewise, it seems like pData is expected to be NULL when the application wants to determine of any of the requested parameters were overridden, without needing to query the parameters. See the NOTE at https://registry.khronos.org/vulkan/specs/latest/man/html/vkGetEncodedVideoSessionParametersKHR.html#_description.

I pushed a version with a separate _len method for determining the length of the output slice, and optional feedback_info on both methods. It required a new RawMutPtr helper. (Happy to adjust if you want something different.)

We discussed this function in chat. It is used to query three "distinct" things:

pFeedbackInfo;

pDataSize;

pData.

Of those, we identified two patterns that appear to be useful:

Query everything. This is already provided by the current implementation of this PR, and conveniently makes use of read_into_uninitialized_vector(). My only "optimization request" would be to pass pFeedbackInfo only during the first or second call, so that the driver doesn't have to (walk the optional pNext chain and) fill the struct(s) repeatedly;

Query pFeedbackInfo only, and optionally pData. This implies that the first call should provide pDataSize too, so that there is no third call needed for that (i.e. instead of adding a pFeedbackInfo=NULL variant based on read_into_uninitialized_vector()).

I didn't list this, but there is a third pattern that I am not sure is useful: query pData exclusively. This is identical to the ail of pattern 2., where a read_into_uninitialized_vector()-based variant would let ash perform the pDataSize query oblivious to the user.

Since this either spawns a lot of variants of the same function, or hides possible optimizations from the user (insofar returning preallocated Vecs isn't considered suboptimal...), we think having a _len() variant of this function - despite not needing to initialize the data - is the solution. If both getters carry feedback_info: Option<&mut ...>, the caller can decide when and where to query this.

However, this implies that the array getter takes &mut [MaybeUninit<u8>] to not unnecessarily force the callers to (zero-)initialize a &mut [u8] when it is going to be overwritten anyway.

@Ralith what is your thought on this, and the provided implementation/resolution?

@Friz64 I believe your input is also valuable on the API of this function. See also my additional concerns in #965 (review).

Discussed this some more in chat. I don't have a ton of context for this API, or really a very strong opinion, but it seems like the current draft is close to a sweet spot, preserving consistency with two-call patterns elsewhere, not constraining callers, and not doing redundant work. I concur with @MarijnS95 that tweaking the non-_len version to return an &mut [u8] containing the initialized (sub)set of the input slice on success would be a nice convenience to add.

ash/src/extensions/khr/video_queue.rs

colinmarc · 2024-12-02T17:40:25Z

ash/src/lib.rs

+}
+
+impl<'r, T> RawMutPtr<T> for Option<&'r mut T> {
+    unsafe fn as_raw_mut_ptr(self) -> *mut T {


I just want to call out that this doesn't work if you don't consume self, because &mut T is not Copy (that would be aliasing). However, that makes this slightly inconsistent with regards to RawPtr (just above).

I wonder if trait RawPtr should take self by value too, given that Option<&T> is Copy (and we don't have &Option<&T>).

Locally this violates clippy::wrong_self_convention though.

Ah yeah, I missed that lint. I could change both to to_... or just do RawMutPtr::to_raw_mut_ptr and leave RawPtr as it is.

Since both are private helpers, I'm not opposed to unifying both into a to_.

colinmarc · 2024-12-02T17:54:50Z

Okay. If there's something wrong with the ash API, note that I do need to understand the issue and the scope of it in case there is something we are doing wrong or could do better. I still don't understand your request/observation, so I cannot help (other than refuting statements in hopes of better understanding the point of contention).

Nothing is wrong with the ash API. I was being lazy and keeping the Video*QueueExt wrapper internal to my project rather than upstreaming it. The confusion stems from this:

The pattern is identical. You could still define your own struct VideoQueueExt, using the same members or using ash::khr::video_queue::Device as its sole member with a Deref to be able to call the wrapper functions inside ash.

I didn't know that - I didn't have a deep understanding of what bits are codegen and what are manual and how they interact, and I had previously built my internal wrapper by copy/pasting ash code and tweaking the signatures. :)

Because the struct definition moved into codegen and implementing foreign structs is illegal, it didn't work anymore to just copy/paste and tweak the signatures. Thus, the observation that the change made it impossible to write external wrappers. In actuality, it is still very possible, you just have to actually know how it works.

No clue what kind of upgrade you are anticipating: if these wrappers are merged to ash, and make it into a release, you can remove them from your project?

I also have no idea what I meant by upgrade when I wrote that 48 hours ago 😅

Changelog.md

ash/src/extensions/khr/video_encode_queue.rs

MarijnS95 · 2024-12-06T13:21:28Z

ash/src/extensions/khr/video_encode_queue.rs

+        &self,
+        session_parameters_info: &vk::VideoEncodeSessionParametersGetInfoKHR<'_>,
+        feedback_info: Option<&mut vk::VideoEncodeSessionParametersFeedbackInfoKHR<'_>>,
+        out: &mut [u8],


My gripe with this API was forcing the caller to initialize the byte-slice. &mut [MaybeUninit<u8>] seems more fitting, and it allows callers to pass Vec::spare_capacity_mut() directly after reserving enough capacity.

Looks like there's also a new helper for that: MaybeUninit::uninit_array

I wouldn't call it new: I've been waiting for years for those helpers to stabilize 😅. Likewise for slice_assume_init_*(), and slice_as_mut_ptr() to replace the cast below.

ash/src/extensions/khr/video_encode_queue.rs

MarijnS95 · 2024-12-06T13:26:56Z

ash/src/lib.rs

+}
+
+impl<'r, T> RawMutPtr<T> for Option<&'r mut T> {
+    unsafe fn as_raw_mut_ptr(self) -> *mut T {


I wonder if trait RawPtr should take self by value too, given that Option<&T> is Copy (and we don't have &Option<&T>).

Locally this violates clippy::wrong_self_convention though.

ash/src/lib.rs

ash/src/extensions/khr/video_encode_queue.rs

This adds the following extensions: - VK_KHR_video_queue - VK_KHR_video_encode_queue - VK_KHR_video_decode_queue

MarijnS95

I hate giving repeated "changes requested" reviews like this (it's a common source of editor/submitter fatigue), but I just keep spotting new things that I either missed, or weren't there before the last pushes...

Happy to accept-changes / resolve them myself though! Seems to be mostly copy-paste typos while attempting to replicate constructs from other files.

ash/src/extensions/khr/video_queue.rs

ash/src/lib.rs

ash/src/extensions/khr/video_queue.rs

MarijnS95

I am happy with this, just need some more time to think about get_encoded_video_session_parameters_len().

I guess that's where the len naming comes from. And as we discussed, this is the variant that could also be used to query just feedback_info and optionally have the length around. That function name (in hindsight) sits a bit weird with me.

One alternative is having just ``get_encoded_video_session_parameters(), which takes both out` and `feedback_info` as `Option`, and unconditionally returns the length?

That length should be useless to the caller if they gave us a Some() slice in out, because we'll still validate the length for them.

And utilizing an unsafe implementation of the yet-unstable slice_assume_init_*(), we could return their Option<&mut [u8]> back to them, signifying that it was initialized.

colinmarc · 2024-12-06T20:47:13Z

I hate giving repeated "changes requested" reviews like this (it's a common source of editor/submitter fatigue), but I just keep spotting new things that I either missed, or weren't there before the last pushes...

Happy to accept-changes / resolve them myself though! Seems to be mostly copy-paste typos while attempting to replicate constructs from other files.

I get the desire to have the codebase consistent, no worries. Sorry about all the copy pasta.

Are there plans to codegen more of this stuff? That would obviously help with the consistency.

MarijnS95 · 2024-12-06T21:15:37Z

Are there plans to codegen more of this stuff? That would obviously help with the consistency.

There is (see open issues) but as you've hopefully noticed by the abundance of (requested) consistency yet subtle differences all over the place, you probably understand that this is hard.

colinmarc mentioned this pull request Nov 30, 2024

Safe wrapper for the video extension #722

Open

MarijnS95 requested changes Dec 2, 2024

View reviewed changes

colinmarc force-pushed the video-queue branch 2 times, most recently from 4f4dff2 to 0078caa Compare December 2, 2024 12:53

colinmarc force-pushed the video-queue branch from 0078caa to 335546a Compare December 2, 2024 12:58

colinmarc requested a review from MarijnS95 December 2, 2024 13:30

colinmarc force-pushed the video-queue branch 3 times, most recently from 115b221 to a6fcdfe Compare December 2, 2024 13:45

colinmarc force-pushed the video-queue branch from a6fcdfe to d3487a2 Compare December 2, 2024 13:52

MarijnS95 approved these changes Dec 2, 2024

View reviewed changes

colinmarc force-pushed the video-queue branch from d3487a2 to 5d2e75d Compare December 2, 2024 16:40

colinmarc commented Dec 2, 2024

View reviewed changes

colinmarc force-pushed the video-queue branch from 5d2e75d to a3f2c4d Compare December 2, 2024 18:00

MarijnS95 added the Blocking a release label Dec 3, 2024

MarijnS95 added this to the Ash 0.39 with Vulkan 1.4 milestone Dec 3, 2024

MarijnS95 removed the Blocking a release label Dec 3, 2024

colinmarc force-pushed the video-queue branch from a3f2c4d to 533389d Compare December 3, 2024 22:28

MarijnS95 reviewed Dec 6, 2024

View reviewed changes

MarijnS95 requested a review from Ralith December 6, 2024 13:35

MarijnS95 reviewed Dec 6, 2024

View reviewed changes

ash/src/extensions/khr/video_encode_queue.rs Show resolved Hide resolved

colinmarc force-pushed the video-queue branch from 0b0f8cf to 5fb4c69 Compare December 6, 2024 19:30

extensions/khr: Add video encode/decode queue extensions

f492cd5

This adds the following extensions: - VK_KHR_video_queue - VK_KHR_video_encode_queue - VK_KHR_video_decode_queue

colinmarc force-pushed the video-queue branch from 5fb4c69 to f492cd5 Compare December 6, 2024 19:33

MarijnS95 requested changes Dec 6, 2024

View reviewed changes

MarijnS95 reviewed Dec 6, 2024

View reviewed changes

ash/src/extensions/khr/video_queue.rs Outdated Show resolved Hide resolved

MarijnS95 reviewed Dec 6, 2024

View reviewed changes

ash/src/extensions/khr/video_queue.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

f17b15f

MarijnS95 approved these changes Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wrappers for `VK_KHR_video{,_encode,_decode}_queue` #965

Add wrappers for `VK_KHR_video{,_encode,_decode}_queue` #965

colinmarc commented Nov 30, 2024 •

edited by MarijnS95

Loading

MarijnS95 commented Dec 2, 2024 •

edited

Loading

MarijnS95 left a comment

MarijnS95 commented Dec 2, 2024

MarijnS95 commented Dec 2, 2024

colinmarc commented Dec 2, 2024

MarijnS95 commented Dec 2, 2024

colinmarc commented Dec 2, 2024

colinmarc commented Dec 2, 2024

MarijnS95 left a comment

MarijnS95 Dec 2, 2024

MarijnS95 Dec 2, 2024

MarijnS95 Dec 2, 2024

colinmarc Dec 2, 2024

MarijnS95 Dec 2, 2024

MarijnS95 Dec 6, 2024

MarijnS95 Dec 6, 2024

Ralith Dec 7, 2024 •

edited

Loading

colinmarc Dec 2, 2024

MarijnS95 Dec 6, 2024

colinmarc Dec 7, 2024

MarijnS95 Dec 7, 2024

colinmarc commented Dec 2, 2024 •

edited

Loading

MarijnS95 Dec 6, 2024

colinmarc Dec 6, 2024

MarijnS95 Dec 6, 2024 •

edited

Loading

MarijnS95 Dec 6, 2024

MarijnS95 left a comment

MarijnS95 left a comment •

edited

Loading

colinmarc commented Dec 6, 2024

MarijnS95 commented Dec 6, 2024

	info: &mut vk::VideoEncodeSessionParametersFeedbackInfoKHR<'_>,
	feedback_info: &mut vk::VideoEncodeSessionParametersFeedbackInfoKHR<'_>,

Add wrappers for VK_KHR_video{,_encode,_decode}_queue #965

Are you sure you want to change the base?

Add wrappers for VK_KHR_video{,_encode,_decode}_queue #965

Conversation

colinmarc commented Nov 30, 2024 • edited by MarijnS95 Loading

MarijnS95 commented Dec 2, 2024 • edited Loading

MarijnS95 left a comment

Choose a reason for hiding this comment

MarijnS95 commented Dec 2, 2024

MarijnS95 commented Dec 2, 2024

colinmarc commented Dec 2, 2024

MarijnS95 commented Dec 2, 2024

colinmarc commented Dec 2, 2024

colinmarc commented Dec 2, 2024

MarijnS95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinmarc commented Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarijnS95 Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarijnS95 left a comment

Choose a reason for hiding this comment

MarijnS95 left a comment • edited Loading

Choose a reason for hiding this comment

colinmarc commented Dec 6, 2024

MarijnS95 commented Dec 6, 2024

Add wrappers for `VK_KHR_video{,_encode,_decode}_queue` #965

Add wrappers for `VK_KHR_video{,_encode,_decode}_queue` #965

colinmarc commented Nov 30, 2024 •

edited by MarijnS95

Loading

MarijnS95 commented Dec 2, 2024 •

edited

Loading

Ralith Dec 7, 2024 •

edited

Loading

colinmarc commented Dec 2, 2024 •

edited

Loading

MarijnS95 Dec 6, 2024 •

edited

Loading

MarijnS95 left a comment •

edited

Loading