vkGetLatencyTimingsNV is inconvenient to use with VkGetLatencyMarkerInfoNV

[MarijnS95]

Hi, Ash (Vulkan bindings for Rust) maintainer here 👋

I recently landed a wrapper for VK_NV_low_latency2 in our bindings, and have just been made aware of a flaw in these bindings.

As it turns out the length for VkGetLatencyMarkerInfoNV::pTimings is encoded in the pTimingCount parameter of vkGetLatencyTimingsNV, rather than in the struct. This results in the following disadvantages (especially annoying considering our generator):

1. the pTimings pointer has no annotation of being an array (nor where to find the length);
2. The VkGetLatencyMarkerInfoNV is not self-descriptive about its contents;
3. Specifically for our generator this means we assume pTimings is a pointer to a single object, not an array;
4. A "useless/empty" pLatencyMarkerInfo has to be passed (it has pNext, more on that later) just to query the length: normally this parameter is NULL;
5. Upon querying a known-length array of VkLatencyTimingsFrameReportNV, the caller must also initialize those structs as they also have an sType/pNext.

Now in this Vulkan wrapper I'd typically create a stack-local VkGetLatencyMarkerInfoNV, pass that into the Vulkan function twice, and return the user an array of VkLatencyTimingsFrameReportNV. However, because VkGetLatencyMarkerInfoNV is extendible via pNext, we must expose this to the user by allowing them to set the pNext pointer.
The same applies to VkLatencyTimingsFrameReportNV: because the user could extend these structs as well, we need to let them provide an array of them with pNext set so that they can optionally query additional information if there ever is an extension to this struct.

What do I expect out of this?

Since the extension has been released the API is set in stone and can no longer be changed. The issues highlighted above do not necessitate new functions to improve the experience, but I do want to bring awareness to this design, and ask a few questions about them so that I can better design my API wrappers if this is an intended pattern (used more often going forward).

Solve the count problem

I believe that the count member could have been moved into VkGetLatencyMarkerInfoNV, so that vk.xml can reference this as the source for array length, and so that the struct becomes self-descriptive. If we pass a mutable pointer to VkGetLatencyMarkerInfoNV into vkGetLatencyTimingsNV (when pTimings = NULL), that call could update the count member to the right size?

Alternatively the pTimings member could have been a direct argument of vkGetLatencyTimingsNV, obviating the need for struct VkGetLatencyMarkerInfoNV altogether?

Use of pNext

Probably tying into the second alternative above: it seems both structs were designed with extensibility in mind, but this does bloat the interface. Was it necessary to have pNext on both VkGetLatencyMarkerInfoNV and VkLatencyTimingsFrameReportNV? Not having it on the former allows us to flatten the arguments into the function.

[chansen-nv]

Thank you for bringing this to our attention! I've reviewed the issue here and yes - pTimingCount should have been a part of VkGetLatencyMarkerInfoNV. I've created an MR to fix this and updated the revision number for VK_NV_low_latency2. You should see the updates in the vk.xml soon and this change will be reflected in our next NVIDIA driver release.

[MarijnS95]

@chansen-nv thank you so much for the quick response and action, looking forward to the changes!

[oddhack]

@MarijnS95 could you take a look at internal MR 6287? It's failing the ash-compile CI stage and we should get that cleared up before the next spec update (which will probably not be until next week, since there wasn't much merged this week).

[MarijnS95]

@oddhack done! It is only failing on compiling our function-wrapper since the function parameters changed. After correcting that it compiles fine. I can work around it in ash by e.g. reverting/removing that wrapper from the master branch temporarily, if that helps you keep the CI green upstream.

I've left some feedback on the MR too, but it is ready to be merged.

[oddhack]

This should be fixed in the 1.3.271 spec update.