Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vulkaninfo: Add video profiles support #1055

Conversation

aqnuep
Copy link
Contributor

@aqnuep aqnuep commented Dec 2, 2024

This PR accompanies KhronosGroup/Vulkan-Profiles#726 by adding video support to the outputs of vulkaninfo, including profiles JSON output.

This is marked draft for now, because I'd guess we do not want to merge this until the Vulkan-Profiles change is in (and thus the profile JSON schema is updated), but it is otherwise standalone.

@aqnuep aqnuep requested a review from charles-lunarg December 2, 2024 13:48
@ci-tester-lunarg
Copy link

CI Vulkan-Tools build queued with queue ID 314615.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build # 1579 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build # 1579 failed.

@lunarpapillo
Copy link
Contributor

The LunarG CI failure is due to an exposure on main due to an erroneous merge. I'll re-run it once the exposure is repaired.

@aqnuep aqnuep force-pushed the rastergrid/video-profiles-support branch from d9a938b to 0169ad3 Compare December 4, 2024 09:24
@ci-tester-lunarg
Copy link

CI Vulkan-Tools build queued with queue ID 316588.

@aqnuep aqnuep marked this pull request as ready for review December 4, 2024 09:24
@aqnuep
Copy link
Contributor Author

aqnuep commented Dec 4, 2024

This is now ready to be reviewed and merged as the Vulkan-Profiles changes are in.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build # 1585 running.

@aqnuep aqnuep force-pushed the rastergrid/video-profiles-support branch from 0169ad3 to 8d0dd9b Compare December 4, 2024 09:30
@ci-tester-lunarg
Copy link

CI Vulkan-Tools build queued with queue ID 316605.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build # 1586 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build # 1586 passed.

Copy link
Contributor

@charles-lunarg charles-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big change, but looks good.

My comments are exactly that, comments, not changes or requests. Code looks to be just about how I would have written it.

Comment on lines +377 to +380
# We need to add dumping utilities for enums declared in the video std headers and directly
# present in the Vulkan API structures. In order to do that we really have no choice but
# to parse the video.xml and generate the utilities based on the enum types defined there
videoRegistryFile = self.genOpts.registryFile.replace('vk.xml', 'video.xml')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found I was in a similar pick with the API Dump code gen. I found a way to run the same generator on both vk.xml and video.xml and get useful results. But for vulkaninfo, directly parsing video.xml is definitely preferred due to the architecture and needs of what you are printing.

flagTypes.add(flagType.name)

# Utility to get structure definition from structure name
def GetStructDef(name):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, looks like a design mistake of vulkaninfo requiring a bunch of list iteration instead of using a dictionary. But thats for me to fix in the future, since list iteration works fine when array sizes are low enough.

'''

# Setup video profile info
out += f'{" " * 20}VkVideoProfileInfoKHR profile_info{{\n'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{" " * 20} is a much nicer way of padding! I'll try to remember this trick next time I need to do a lot of that.

@charles-lunarg charles-lunarg merged commit 315964a into KhronosGroup:main Jan 6, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants