extensions/khr: Add VK_KHR_pipeline_binary extension

[MarijnS95]

No description provided.

[MarijnS95]

Our no_std build complains:

error[E0412]: cannot find type `Vec` in this scope
  --> ash/src/extensions/khr/pipeline_binary.rs:66:19
   |
66 |     ) -> VkResult<Vec<u8>> {
   |                   ^^^ not found in this scope
   |
help: consider importing this struct
   |
3  | use alloc::vec::Vec;
   |

It's quite unfortunate that the clippy::std_instead_of_alloc lint didn't warn that Vec from the prelude is in std (this lint only triggers if use std::vec::Vec is explicitly imported). CC @i509VCB who landed this feature, I haven't found an upstream tracking issue but maybe you know about this?

[MarijnS95]

Not sure what to do here. We could map NOT_ENOUGH_SPACE_KHR to INCOMPLETE to make read_into_uninitialized_vector() try again, though we could also be changing it at some point to return the incomplete but partial result to the user, which is invalid here.

If we don't, we can still use read_into_uninitialized_vector() to help us call vkGetPipelineBinaryDataKHR() twice. On NOT_ENOUGH_SPACE_KHR - which is unexpected - that error is bubbled up to the caller (and they wouldn't know how to handle it, since the wrapper is the one that was dealing with querying and preallocating for the requested size initially...).

[MarijnS95]

Mapped to INCOMPLETE and rewrote the documentation, also to reference the upstream issue (non-public repo).

@Ralith I think this is ready for merging unless you find anything glaring, and I'll follow up and clean up depending on how the upstream discussion pans out.

[MarijnS95]

After upstream clarification, VK_NOT_ENOUGH_SPACE_KHR docs are accidentally missing and will be added. There's a fundamental difference with VK_INCOMPLETE that's already described at vkGetPipelineBinaryDataKHR() and supposedly the reason why this new return value was added:

and on return pPipelineBinaryDataSize is overwritten with the size of the data, in bytes, that is required to store the binary.

So even if the previously requested size is too small, the function doesn't write data (leaves it MaybeUninit 👀) but uses pPipelineBinaryDataSize to communicate the required space again, so that the user doesn't have to call it yet another time.

This is unfortunately inconsistent from other binary-array-returning functions that predate the addition of this error return. They have their own ways of documenting "special behaviour"...

---

I think, but didn't ask, that this facilitates a pattern to preallocate static arrays on the stack with some predetermined upper bound. If less space is required only one API call is needed, otherwise the next call can be done immediately with the right size after heap-allocating something bigger. That pattern isn't possible with VK_INCOMPLETE (and that would write data twice, too, for functions where partial results may still be valid).

[MarijnS95]

I wrote a new variant of read_into_uninitialized_binary_vector() to deal with this.

[MarijnS95]

Also added some debug_assert!(). None of the clauses invoke undefined behaviour, but do point to an incorrect implementation when returning VK_ERROR_NOT_ENOUGH_SPACE_KHR.

---

Separate from this all, read_into_uninitialized_vector() was initially designed (see its doc-comment) te deal with functions that arbitrarily change their count at runtime between calls, hence the infinite loop.

This behaviour is not expected for most functions currently using read_into_uninitialized_vector(), but reuse it for conciseness due to also returning VK_INCOMPLETE. These calls could benefit from the same "maximum of 2 calls" strategy (with debug asserts) and otherwise bubble VK_INCOMPLETE back to the user. Unfortunately they cannot easily use a preallocated upper-bound Vec like I did in read_into_uninitialized_binary_vector(), because the Vulkan call wouldn't immediately tell us what the desired count is if it returned VK_INCOMPLETE. We'd need a third call for that.

[Ralith]

These calls could benefit from the same "maximum of 2 calls" strategy

What's the benefit here? Just hypothetical improved detection of implementation bugs? Seems like unnecessary churn.

[Ralith]

Shouldn't vk::PipelineBinaryKeyKHR be an out parameter, to allow for extension? If this is for some reason undesired, then the lifetime should be 'static.

[Ralith]

"twice until" is weird word choice. Maybe "at most twice"?

[Ralith]

This seems to be missing the code to actually expand the Vec.

Suggested change

	err_code = f(&mut count, data.as_mut_ptr().cast());
	data.reserve(count);
	err_code = f(&mut count, data.as_mut_ptr().cast());

[Ralith]

nit: with_capacity may allocate extra space, and we might as well use it

Suggested change

	let mut count = 4096;
	let mut data = Vec::<u8>::with_capacity(count);
	let mut data = Vec::<u8>::with_capacity(4096);
	let mut count = data.capacity();

(and adjust the debug_assert accordingly)

[Ralith]

Do we know that binaries are, in practice, usually smaller than this?