Vulkan 1.2.175: Provisional Video Extensions

[MarijnS95]

Depends on #431 (which depends on #429)

Introducing Vulkan 1.2.175 with raw bindings for the new provisional Vulkan Video Extension. No "safe" bindings are included yet; those can be written if someone ends up needing these, or after the API is stabilized. For now it'll allow users to experiment around with the "raw" (raw pointer) API and unblock future updates.

The Vulkan-Headers declaration consist in part of C headers, presumably due to lack of bitfield support in structs in vk.xml. Fortunately bindings can be generated effortlessly with bindgen.

TODO:

• Bindgen currently runs at build-time. Use a feature toggle to regenerate bindings in development and on the CI only, relieving crate users of both this dependency and the Vulkan-Headers repo. We'll check in pregenerated bindings just like how the generator output is checked in today;
• Redo trait requirements for pNext. This extension introduces nested trait extensions. We have the usual:

  <type category="struct" name="VkVideoProfileKHR" structextends="VkQueryPoolCreateInfo,VkFormatProperties2,VkImageCreateInfo,VkImageViewCreateInfo,VkBufferCreateInfo">

  But then:

  <type category="struct" name="VkVideoDecodeH264ProfileEXT" structextends="VkVideoProfileKHR">

  Since VkVideoProfileKHR extends existing root structs no ExtendsVkVideoProfileKHR trait is generated for it. However, VkVideoDecodeH264ProfileEXT includes pNext and will - due to structextends="VkVideoProfileKHR" - only implement ExtendsVkVideoProfileKHR which doesn't exist. For now all traits are emitted to solve this build error, but it does not allow VkVideoDecodeH264ProfileEXT to be put in VkVideoProfileKHRBuilder::push_next. Nor does it allow any of the "parent" structs for VkVideoProfileKHR to be put in VkVideoDecodeH264ProfileEXT either unless they implement ExtendsVkVideoProfileKHR.

[MaikKlein]

The Vulkan-Headers declaration consist in part of C headers, presumably due to lack of bitfield support in structs in vk.xml. Fortunately bindings can be generated effortlessly with bindgen.

There is at least one examples of bitfields in vk.xml VkTransformMatrixKHR. Also bindgen will most likely handle bitfields rust differently than we do. Currently we handwrite those structs.

Also what about the naming scheme? What if the next release includes those types in the xml? Will the generated types have the same naming scheme as from bindgen? Do we get a name clash and need to remove vulkan video from bindgen?

I am not even sure we should update to the latest spec, I feel like they should fix the xml. I'll open an issue for it.

[MarijnS95]

There is at least one examples of bitfields in vk.xml VkTransformMatrixKHR.

Ah great, so they have the possibility to declare these in XML after all.

Also bindgen will most likely handle bitfields rust differently than we do. Currently we handwrite those structs.

It's a C header and bindgen should generate structs compatible with that layout? It wouldn't work if it doesn't perform what is advertised.

Also what about the naming scheme? What if the next release includes those types in the xml? Will the generated types have the same naming scheme as from bindgen? Do we get a name clash and need to remove vulkan video from bindgen?

If the next release moves everything back to XML we can remove the bindgen part, yes. We might also want to parse the XML and include headers based on what is defined instead of hardcoding those in wrapper.h:

        <type category="include" name="vk_video/vulkan_video_codec_h264std.h">#include "vk_video/vulkan_video_codec_h264std.h"</type>
        <type requires="vk_video/vulkan_video_codec_h264std.h" name="StdVideoH264ProfileIdc"/>
        <type category="include" name="vk_video/vulkan_video_codec_h264std_decode.h">#include "vk_video/vulkan_video_codec_h264std_decode.h"</type>
        <type requires="vk_video/vulkan_video_codec_h264std_decode.h" name="StdVideoDecodeH264PictureInfo"/>
        <type requires="vk_video/vulkan_video_codec_h264std_decode.h" name="StdVideoDecodeH264ReferenceInfo"/>
        <type requires="vk_video/vulkan_video_codec_h264std_decode.h" name="StdVideoDecodeH264Mvc"/>

I am not even sure we should update to the latest spec, I feel like they should fix the xml. I'll open an issue for it.

I don't want to block future spec updates, and I do feel that Ash - as the most popular Rust Vulkan binding crate - should stay relatively up to date to allow users to experiment with the latest spec without hassle. We can also disable this extension entirely but it seems quite trivial to support it now and we haven't really built and public API like a vk/extensions/khr wrapper yet. Perhaps time to look back at how we dealt with the provisional KHR_ray_tracing extension being superseded by the official release? We wanted to make experimental extension support more explicit one way or another?

[MaikKlein]

It's a C header and bindgen should generate structs compatible with that layout? It wouldn't work if it doesn't perform what is advertised.

I am not talking about ABI, I am talking about semantic versioning.

• Do the structs follow the same naming convention as in ash?
• Are the fields snake case?
• Is the Vk stripped from the types?

Stuff like that.

It is possible to simply not require semantic versioning for these things.

There is already an issue for it KhronosGroup/Vulkan-Docs#1505

[MarijnS95]

It is possible to simply not require semantic versioning for these things.

Sounds about what we did with KHR_ray_tracing, but we'll probably have to communicate that better somehow. A very simple way is to hide this all behind an experimental_vulkan_video feature, and document that it is free to change or disappear with any normal semver release?

[MaikKlein]

Sounds about what we did with KHR_ray_tracing, but we'll probably have to communicate that better somehow. A very simple way is to hide this all behind an experimental_vulkan_video feature, and document that it is free to change or disappear with any normal semver release?

Sounds reasonable, but lets wait for a reply first. I wish Rust and had an experimental/unstable attribute for this.

[MarijnS95]

• Do the structs follow the same naming convention as in ash?

There are no KHR or BIT or EXT postfixes so there's nothing to change here;

• Are the fields snake case?

Yes, all enum variants and struct fields appear to be snake case;

• Is the Vk stripped from the types?

They are prefixed with StdVideo and used like that by generated code. There appears to be no VK_ prefix except in the version/string preprocessor definitions.

[MarijnS95]

Sounds reasonable, but lets wait for a reply first. I wish Rust and had an experimental/unstable attribute for this.

We'll probably split off and merge all the way up to .174 and leave this open for visibility. We can make a move whenever the XML is fixed, the next header release contains something interesting, or someone really needs it (and potentially puts in extra effort to make it workable).

[MarijnS95]

@MaikKlein Well, we have an answer. Unsurprisingly these header types (data layout) are part of the H.264/5 spec and not specific to Vulkan, and it's very unlikely for them to be mapped out to XML (even though that should theoretically be possible).

I feel like cleaning this up (hiding bindgen behind a feature toggle) and going ahead with this anyway, we have the headers working already with minimal hassle. One possible improvement is collecting the list of headers/contents from XML instead of hardcoding them in wrapper.h.

[MaikKlein]

Ok there has been another update

I believe all the StdIdc types that are included by value are enumerants, while the other Std structures are all passed by pointer. Either way language bindings do need access to define the Std* APIs themselves, of course. The Video TSG is aware of the issue and will be discussing it this week. It is a provisional extension and can easily change.

This sounds good. If the pregenerated bindgen types are not too much work we can still get them in now.

[MarijnS95]

Yep, the structs should all be passed by value and could be turned into *const c_void if we want to avoid this. Enumerants like StdVideoH264ProfileIdc however are directly used by the XML bindings, in structs as well. The headers go out of their way to use sized uint<bits>_t in the structs despite being bitfields but the enumerants have no size annotation and could depend on the target architecture's native int size.

So we'll need the headers for now, disable vk_video altogether, or hope that at least the enums are moved to XML?

[MaikKlein]

So we'll need the headers for now, disable vk_video altogether, or hope that at least the enums are moved to XML?

Lets not go out of our ways to make provisional extensions work. (Unless it is an easy change) Lets wait until most of these definitions will be in the xml. I guess we can ignore it for now?

[MarijnS95]

Lets not go out of our ways to make provisional extensions work. (Unless it is an easy change) Lets wait until most of these definitions will be in the xml. I guess we can ignore it for now?

Yes, let's do that. Let's get the open PRs (+ dependencies) for .171 in, then I'll submit a separate PR updating up till .174 and one that disables generation for the video extensions, so that we can continue pulling in new extensions without being bothered by this. I'll rebase this and keep it somewhat buildable so that folks wishing to play with the Video extensions can do so from Rust.

[MarijnS95]

We're generating C bindings using bindgen in the generator now as it's supposed to be, instead of at runtime using build.rs. However, this depends on machine-specific stdint.h which is a bit ahead on my distro versus the CI (glibc 2.33 vs 2.31), leading to generation mismatches. Unfortunately bindgen (and clang/LLVM under the hood) can't (easily?) be told to treat uintX_t types as Rusts uX, otherwise we don't need it.

I've seen this happen on other projects too, and the solution is generally to discard changes to these files if the CI disagrees. It's going to be annoying though given the frequency at which I import and regenerate newer Vulkan headers, having to not forget to revert specific changes to ash/src/vk/video.rs. Suggestions welcome!

[MaikKlein]

Generation works on Windows too but unfortunately the data layout (alignment/padding) shifts in a few structs. This has to be investigated in more detail as I have no idea if there is a true platform difference, or if Linux or Windows is wrong is now getting the wrong layout.

This is a bit worrying, do you have an example where the padding is different? We would need a tracking issue for it and maybe reach out to the bindgen folks. I can't think of a reason why the padding would be different.

[MarijnS95]

This is a bit worrying, do you have an example where the padding is different? We would need a tracking issue for it and maybe reach out to the bindgen folks. I can't think of a reason why the padding would be different.

The problem is with:

typedef struct StdVideoH265HrdFlags {
    uint32_t nal_hrd_parameters_present_flag : 1;
    uint32_t vcl_hrd_parameters_present_flag : 1;
    uint32_t sub_pic_hrd_params_present_flag : 1;
    uint32_t sub_pic_cpb_params_in_pic_timing_sei_flag : 1;
    uint8_t  fixed_pic_rate_general_flag; // each bit represents a sublayer, bit 0 - vps_max_sub_layers_minus1
    uint8_t  fixed_pic_rate_within_cvs_flag; // each bit represents a sublayer, bit 0 - vps_max_sub_layers_minus1
    uint8_t  low_delay_hrd_flag; // each bit represents a sublayer, bit 0 - vps_max_sub_layers_minus1
} StdVideoH265HrdFlags;

On Linux (currently checked in to this PR) the first four bitfields are modeled as a single u8 (which would be enough to hold all the fields). On Windows however, clang/bindgen recognizes that the type is specified as uint32_t and reserves four bytes for it:

@@ -2795,6 +2787,7 @@ fn bindgen_test_layout_StdVideoH265SubLayerHrdParameters() {
 pub struct StdVideoH265HrdFlags {
     pub _bitfield_align_1: [u8; 0],
     pub _bitfield_1: __BindgenBitfieldUnit<[u8; 1usize]>,
+    pub __bindgen_padding_0: [u8; 3usize],
     pub fixed_pic_rate_general_flag: u8,
     pub fixed_pic_rate_within_cvs_flag: u8,
     pub low_delay_hrd_flag: u8,

(Plus the necessary bindgen_test_layout_StdVideoH265HrdFlags() changes below).

I don't exactly know the rules bitfields and their overal size in the struct. It feels Windows is more correct, but for all I know there might as well be different rules when targeting Linux or Windows.

[MaikKlein]

Awesome work, I want to get this in before the next release.

But there is one thing I am a bit uncomfortable with and that is semver with provisional extensions.

I know that there ash hasn't done anything for it in the past, but that was mainly due to me not knowing that they even added provisional extensions to the vk.xml.

We have an experimental folder, which is "exempt" from semver, but vkvideo here is all over the place. Any thoughts what we can do about it?

We technically could add a cfg/feature flag for all types/functions from provisional extensions. Any idea how much work that would be? #343

[MarijnS95]

We have an experimental folder, which is "exempt" from semver, but vkvideo here is all over the place. Any thoughts what we can do about it?

Technically the Video extensions aren't all over, except in the autogenerated bindings which are public. The experimental folder only contains handwritten helpers/wrappers for extensions, not the underlying bindings themselves. I'll see if I can easily get the generated output behind a cfg tomorrow.

[MaikKlein]

Technically the Video extensions aren't all over, except in the autogenerated bindings which are public.

But there are quite a few types, bitflags, commands which were generated from the provisional beta extension. Like VkVideoCodecOperationFlagBitsKHR for example. So it is not just the native stuff from bindgen.

[MarijnS95]

@MaikKlein Having looked into this some, properly guarding provisional API with features requires substantial changes to the generator. Extensions only declare what types/enums/commands they need, but the definitions sit elsewhere without provisional=true attribute. For commands this is already taken care of by storing them in a hashmap and generating the functions and impl block with the extension (making it really easy to move this all to a feature-guarded provisional_extensions.rs instead of generating a #[cfg] on every block), but the same has to be done for types and enums.

Unfortunately too, extending existing enums with constants happens within the extension and provisional_extensions.rs too, meaning we cannot go for an intermediary solution where the types remain unguarded in definitions.rs, as their StructType is only defined behind a #[cfg].

This might mean moving every extension-specific type out from definitions.rs into (provisional_)extensions.rs in the end. Care has to be taken when a type is "required" by multiple extensions, and it won't really make the generator more pretty either.

[MaikKlein]

Yeah that is what I thought :(

I think we can still get it in and make a release, we just have be careful about breaking changes and properly update our versioning in ash. We really need to prioritize the new generator.

I'll have another look later today, but don't think I have any other concerns.

[MarijnS95]

Yeah that is what I thought :(

I think we can still get it in and make a release, we just have be careful about breaking changes and properly update our versioning in ash. We really need to prioritize the new generator.

Breaking changes are allowed on most-significant version changes. In our case the minor version field is most-significant because we're still on 0., so it's fine despite being a little annoying.

Would be cool to have a look at the new generator and refactor some of the changes made to the old one into there. What's the status now? Any substantial changes to be made first, or local changes in-progress?

[MarijnS95]

@MaikKlein Gave this another look in GodBolt: https://godbolt.org/z/bev9eEzdv. The structs really have different layout on Clang/GCC versus MSVC, and that's apparently allowed. This should probably be "fixed" (just use the smallest integer type) in Vulkan headers, or Ash needs per-target and per-platform bindgen output. Not to mention all the trouble we'll be causing with cross compilations though at least clang on Windows seems to adhere to what MSVC is doing.

[Friz64]

I think we should definitely raise an issue with the Vulkan folks before implementing unusual hacks.

By the C standard it is only allowed to use int, signed int and unsigned int (which usually matches with the uint32_t used) for bit fields, but in practice implementations allow for more flexibility, including the smallest integer type as you mentioned. Whether that should be part of the Vulkan Headers is unclear.

Additionally, the layout could be somehow explicitly defined as with e.g. VkAccelerationStructureInstanceKHR. The important difference is that the layout doesn't actually differ in practice with structs like that in the Vulkan specification.

Also note that there is a RFC adding support for bit fields, but I don't expect it to land in the near future: rust-lang/rfcs#3113

[MarijnS95]

I think we should definitely raise an issue with the Vulkan folks before implementing unusual hacks.

It doesn't seem unusual to have per-target and/or per-platform bindgen files, especially if the target platform defines different layout rules for the same struct. However, I wonder if the Video TSG is aware of this discrepancy - will raise an issue - EDIT: KhronosGroup/Vulkan-Docs#1571.

Additionally, the layout could be somehow explicitly defined as with e.g. VkAccelerationStructureInstanceKHR.

Not sure that'd help. This struct is in a header file and compilers won't be able to read this documentation.

Also note that there is a RFC adding support for bit fields, but I don't expect it to land in the near future: rust-lang/rfcs#3113

I wonder if, as a temporary solution, bindgen could also treat this per-platform discrepancy with some #[cfg], which I assume the RFC would implicitly deal with under the hood too when compiled for different platforms.

[Friz64]

It doesn't seem unusual to have per-target and/or per-platform bindgen files

Certainly not the perfect solution. But if there are no alternatives (for now) it is the only one, fair.

Not sure that'd help. This struct is in a header file and compilers won't be able to read this documentation.

Meant that à la "If a compiler produces code that diverges from that pattern, applications must employ another method to set values according to the correct bit pattern.".

But yeah, don't know if something like that could also be applied to the Vulkan Video types.

---

Let's wait and see what the Khronos issue tells us.

[MarijnS95]

Meant that à la "If a compiler produces code that diverges from that pattern, applications must employ another method to set values according to the correct bit pattern.".

That could work, unfortunately VkAccelerationStructureInstanceKHR does not define these two as a single field in vk.xml opening up potential problems - but at least they together sum to the size of a single int leaving no issues in practice. As mentioned in Vulkan-Docs these are simple booleans anyway, that are probably better modeled by a bitflags structure like the rest of Vulkan.

[Friz64]

probably better modeled by a bitflags structure like the rest of Vulkan

Yes! This would be by far the best solution, i'd be great if that went through :)

[MarijnS95]

probably better modeled by a bitflags structure like the rest of Vulkan

Yes! This would be by far the best solution, i'd be great if that went through :)

Looks like we're getting padding bits instead: KhronosGroup/Vulkan-Docs#1571 (comment)

[Friz64]

Huh okay, probably the least amount of effort on their side, i'm fine with it ¯_(ツ)_/¯