KhronosGroup / Vulkan-Docs

The Vulkan API Specification and related tools
Other
2.7k stars 452 forks source link

Missing `vulkan_video_codecs_common` requirement for video headers #2179

Open Dav1dde opened 11 months ago

Dav1dde commented 11 months ago

The video headers, e.g.:

        <type category="include" name="vk_video/vulkan_video_codec_h264std.h">#include "vk_video/vulkan_video_codec_h264std.h"</type>
        <type category="include" name="vk_video/vulkan_video_codec_h264std_decode.h">#include "vk_video/vulkan_video_codec_h264std_decode.h"</type>

include a file vk_video/vulkan_video_codecs_common.h, but this is file is missing as a type and not marked as a requirement of the include types.

I think such dependencies should be correctly modeled in vk.xml for external generators, this currently breaks glad: https://github.com/Dav1dde/glad/issues/434

oddhack commented 11 months ago

There is an conscious separation between the Std types and the Vulkan API. All the Std stuff gets punted off into codec headers generated from video.xml, that have their own internal structure. The Vulkan spec itself knows nothing about them, except that it needs to include the appropriate codec header to get the types defined - these are external APIs like Android AHardwareBuffer or Metal MTLTexture_id.

Ideally the downstream generators will reflect that separation of concerns. We've had to iterate on the video XML a couple of times to try and get the exact header structure the video TSG is after, and it's not impossible that additional iteration will be required.

oddhack commented 11 months ago

Tagging @aqnuep for the Video TSG.

aqnuep commented 11 months ago

There's been a recent change to the way the video std headers are generated which should take care of the appropriate dependent includes there.

As @oddhack noted, these headers should be treated more like platform headers.

Looking at the linked issue, I think your problem should be fixed by simply having the Vulkan headers coming with an SDK or any other package (which should also include the Vulkan video headers) available, just like with other platform headers, and wouldn't have to worry about generating them from video.xml.

Dav1dde commented 11 months ago

So far glad took care including all API specific headers, this is especially relevant for the header-only loader it generates, which includes everything in a single header file (including khrplatform and the video headers if necessary).

I could now relax this and require a vulkan sdk to be installed globally (which needs to include said headers), but I would like to avoid this if possible, I think this makes glad quite a bit less useful and more annoying to deal with (especially since now I have a dependency between used version of the specifications to generate the code and the vulkan sdk, a mismatch wouldn't compile).

If the vk.xml includes requirements transitively for included sdk headers this would be the easiest to deal with for 3rd party providers. I do understand though that this is an additional maintenance burden including a source of errors.

Alternatively I'll have to manually track this or parse video.xml or guess by 'parsing' the header includes. Parsing the video.xml being the proper solution, which may also help for other languages than C, but also comes with a lot of added complexity.

I hope this clears my situation up a bit, I was initially hoping for the simple additional requires in vk.xml but I understand if you don't want have to add this 'workaround'.

aqnuep commented 11 months ago

@Dav1dde, just to better understand the underlying issue about how glad uses the vk.xml, and thus for us to be able to come up with a solution that would work for you...

Is the only problem that there is no mention of the include of the vulkan_video_codecs_common header in the vk.xml itself, or there is a more general issue with the vk.xml depending on the Vulkan Video Std headers in general, because they are external to the base set of Vulkan headers and thus what glad normally generates from the vk.xml?

The transitive relationship itself across these headers is indeed defined in video.xml, not in vk.xml, but AFAICT even before the recent change glad would have required the Vulkan Video Std headers to be available externally due to the dependency on vulkan_video_codec_h264std et al. How was that handled so far by glad without having the Vulkan SDK headers installed, or glad otherwise having a local copy of those headers?

Dav1dde commented 11 months ago

Is the only problem that there is no mention of the include of the vulkan_video_codecs_common header in the vk.xml itself

This is the issue, the video headers themselves are fine, because they are properly required by other types. But the common is listed nowhere. So glad automatically pulls in the video headers but it does not know about the common header and that's why it's missing in the end.

How was that handled so far by glad without having the Vulkan SDK headers installed, or glad otherwise having a local copy of those headers?

https://github.com/Dav1dde/glad/blob/2348b07c1ab4504d60398713781d8a57880234fa/glad/generator/c/__init__.py#L257-L303

I have a builtin list of links where to resolve the headers, glad also packages a snapshot of all files (specs, headers) which I update every now and then.

aqnuep commented 11 months ago

Just out of curiosity, could glad simply manually add the dependency on vulkan_video_codecs_common, or just automatically include all files in the vk_video directory?

I'm asking because while we can technically modify vk.xml to include a reference to it, but strictly speaking such a dependency would really be redundant with the information already in the video.xml.

Dav1dde commented 11 months ago

Just out of curiosity, could glad simply manually add the dependency on vulkan_video_codecs_common

Yeah this would be my first workaround. At some point I'd probably look into processing the video.xml as well. But either solutions arent perfect. 1) I have to keep a manual list of references or 2) it's a lot more complex.