Fix invalid data length calculated in get_query_pool_results() when called with WITH_AVAILABILITY flag

[matszczygiel]

Fixes #639

[MarijnS95]

Can you include a concise description of this issue - and why this solution is correct - in the commit message?

[MarijnS95]

Don't forget to add it to the changelog either.

[Ralith]

Note that we're still passing mem::size_of::<T>() as the stride. This means that for WITH_AVAILABILITY, T should probably be (u32, vk::Bool32) or similar, unlike your example in #639. Might bear documenting this explicitly.

[MarijnS95]

@Ralith I wondered if it makes sense to perhaps change the function signature to deal specifically with WITH_AVAILABILITY and WITH_STATUS? From your example I take it the stride should also be 8 (for u32 + bool32) here otherwise subsequent values start overwriting the availability/status result?

[Ralith]

mem::size_of::<(u32, vk::Bool32)>() is 8, so using that as T should Just Work without any additional measures in the interface--even without the change proposed in this PR.

[Ralith]

I think we should either add a stride argument or further remove the query_count argument, because I don't think it's useful to have a query count different than data.len() otherwise, unless we set stride to query_count / data.len(), but that seems a bit magical.

I lean towards dropping query_count in the spirit of greater type safety personally. Adding (or dynamically computing) stride would make it easier to select flags dynamically and decode results after the fact, but it's difficult to imagine a case where that is preferable.

[MarijnS95]

mem::size_of::<(u32, vk::Bool32)>() is 8, so using that as T should Just Work without any additional measures in the interface--even without the change proposed in this PR.

Meaning that this PR together with the example proposed in #639 doesn't work as intended, since the use of u32 there results in a stride of 4? My question is whether query N+1 would overwrite the AVAILABILITY bit of query N in that case.
Local testing indicates that that is indeed what happens; the availability result is overwritten by query value N+1.

Of course the caller can/should use a Vec<(u32, vk::Bool32)> here and immediately receive the type-safe results in a usable manner. Replace u32 with u64 for 64-bit results. And that immediately addresses / works around the "missing" stride field.

The request to come up with a more specific API is probably moot given that its use may be expanded over time - see WITH_STATUS_KHR which is being introduced through the VK_KHR_video_queue beta extension.
We can't check for T=u32 + TYPE_64 either, but that's unnecessary and the job of the validation layer.

I lean towards dropping query_count in the spirit of greater type safety personally. Adding (or dynamically computing) stride would make it easier to select flags dynamically and decode results after the fact, but it's difficult to imagine a case where that is preferable.

IIRC we discussed and approved/agreed on removing query_count before, since the user passes in a slice here and can hence easily change the length.

[matszczygiel]

OK. So what seems to be the solution is to just simply use a slice of (u32, vk::Bool32).
Should I close this PR ?

[MarijnS95]

@matszczygiel this PR is correct after you reword the title/commit message as suggested above, and at the same time you must use a tuple for T so that the stride is correct.

Don't forget to add a changelog entry either.

EDIT: Meanwhile @Ralith may want to submit a separate PR to remove query_count.

[matszczygiel]

@MarijnS95 I have changed the commit message. Also added the changelog entry.

[MarijnS95]

@Ralith Recapping what we wrote above... What do we get when we do not merge this change?

I think not merging this change forces the user to use the proper (u32, vk::Bool32) type for T, and automatically get the right stride in what is currently mem::size_of::<T>() as _? As such there's not really anything to fix?

Then separately we can remove query_count and utilize slice length.

[MarijnS95]

Suggested change

	- Fix invalid data length calculated inside of `get_query_pool_results` when called with `WITH_AVAILABILITY` flag (#640)
	- Data size in `get_query_pool_results()` is now based on total slice size rather than type size times query count to support `WITH_AVAILABILITY` when `T` is "wrong" (#640)

But as I wrote, I don't think this is very sensible in the end.

[Ralith]

Fine by me.

[MarijnS95]

Will close this then. I get the expected results on the current release using TYPE_64 | WITH_AVAILABILITY when passing a slice with T = (u64, vk::Bool32). Merging this would fix the wrong thing for the wrong reasons. #644 will only be a (breaking) API improvement.