[registry] New extension: VK_EXT_debug_marker

[baldurk]

I've done the initial writeup for a new extension VK_EXT_debug_marker. This pull request is for further discussion and feedback and to learn the next steps, I know there will be more work to do before it's in a final state.

It is equivalent to the functionality in GL_KHR_debug that isn't covered by VK_EXT_debug_report - object naming/tagging and marker regions/labels for annotating command buffers.

This would not be an extension expected to be implemented by any ICD, but by validation and debugging layers.

I based the structure/spec writing on VK_EXT_debug_report. I see that for the WSI extensions and for the new VK_KHR_sampler_mirror_clamp_to_edge there is a different structure used, where the functionality is included in-line in the specification as a new chapter and the appendices/ file is more about general information of the extension, issues, and such. I'm happy to restructure the work on this branch I would just want to know what is the right way to go about it.

Please let me know what you would want from me next, I'm available to do the legwork on getting this into shape and answering any questions/feedback about the content of the extension.

[courtney-g]

Where / how does a developer see these markers? Is that something that API dump needs to print?
Are the markers somehow placed in the actual command buffers (would seem not if this is just a validation layer feature).

[baldurk]

This would be up to whatever is consuming the markers. API dump could intercept and just print out the calls without any special processing. The validation layers might ignore the markers entirely as not useful, and instead only save the object names for friendlier error messages.

The primary purpose is for an external debugger or profiler to be able to generate a hierarchy for browsing the frame. As an example here's some markers from Dota being displayed:

[michaelworcester]

Ignore my deleted comment, shouldn't be drinking and gitting...

[oddhack]

I will take a look at it soon. As you've noticed, we expect that the actual spec changes, aside from asciidoc include:: framework, will exist on their own extension branch separate from the core version they're modifying. It doesn't look quite like that's happening here, but I've just skimmed the PR.
VK_KHR_sampler_mirror_clamp_to_edge was "special" in that it's something that should have been a core API Feature and people just didn't notice until too late to retrofit it into 1.0. It doesn't add to the API, only changes behavior of an existing token, but I still think it deserves its own extension branch. Hopefully we will not repeat that.

I expect we will learn a lot about the extension process from trying to integrate the first extensions authored outside Khronos, and you get to be something of an experimental subject. If you can, please advise of things you found unclear or hard to find or missing from the documentation on writing EXTs, so we can improve that.

Have you talked to the LunarG LoaderAndValidationLayers folks about implementing this as part of their SDK? That seems a natural place for it alongside debug_report.

[baldurk]

Yes, I have no objections to being a guinea pig for the extension process 😄. I thought it might be helpful to do this trial run with a fairly simple extension with no IHV impact. I'll certainly note down any thoughts on the process and share it, so far it's been pretty smooth.

I did do all of my work on a side branch on my fork, 1.0-VK_EXT_debug_marker. I branched off the 1.0.4 commit on the 1.0 branch. Unfortunately the github PR process only allows me to create pull requests with a destination branch that already exists on the target repo - so I've targeted to 1.0.

Perhaps the initial step of the process should be to request that a branch be created on KhronosGroup/Vulkan-Docs with a minimal change (maybe just the tag in vk.xml with just the extension number, no commands or types), and the PR can then be targeted at that?

As I understood it, the asciidoc changes would be merged into just the side branch as I have, but the vk.xml changes would be merged into 1.0 as well so that 1.0 is capable of generating any subset of core+all extensions. I see at the moment that 1.0 contains the wsi and debug_report extension details in vk.xml, and the other extension branches contain the bulk of the asciidoc work.

I've been working with LunarG and Valve to construct the extension, so they are aware and I presume may implement it in future validation layers. I already have an implementation working in renderdoc with Dota, and that's why I'm proposing the extension be standardised so that it can be used wider.

[courtney-g]

I like Michael's deleted comment about extending CreateInfo to tag or name an object. Would eliminate two function calls and the need for the VK_DEBUG_REPORT_OBJECT_TYPE enum. Can't tag physical devices that way, but I think everything else.

[baldurk]

Note: the VK_DEBUG_REPORT_OBJECT_TYPE enum is reused from the VK_EXT_debug_report extension (hence the name!) so it would not be eliminated in any case. I'd say that using the CreateInfo extending method would be less flexible - for the sake of removing just two function calls I think the downsides are significant.

It means the name must be known at creation time and can't be changed if the object is recycled and used for something else, e.g. for a system that creates a number of objects in a pool. It also adds a new restriction that other APIs don't impose - the name must be known at creation time. That means e.g. the name must be passed around from its source to the creation call, rather than set separately (which is much easier to disable with a #define in non-debug builds). The tag is also deliberately generic so there's no guarantee that it would always be used for information known at creation time.

[kspagnoli-nv]

Hi there - Kyle from the NVIDIA software tools team here. I was discussing the details of this proposed extension with some colleagues and we saw advantages in both methods. We liked that the "create info extension method" offered the following features:

• Object type agnostic (future proof against unknown objects)
• Type-safety
• Potentially less overhead
• Fits other Vulkan patterns

Whereas the "explicit call method" offered:

• Ability to rename objects
• Ability to name objects that aren't explicitly created (queues, swap chain images, etc.)
• Parity with existing OpenGL extension

The ability to rename objects seems like a pretty big deal but I'm curious how many people will actually pool higher level objects (image, buffers, etc.) and instead rely on pooling the underlying device memory. Creating a new image or buffer with existing device memory should be very cheap.

That being said, our tools won't have any problem supporting either method. Personally, I find the "create info extension method" to be the cleaner method but can see the merits of either approach.

[baldurk]

It's worth mentioning again that the object type enum used is from debug_report, which does not have the type-safe alternative of using create info. For that reason alone, any extension that adds a new object in the future will almost certainly add to that enum. Especially if debug_report becomes core in a future Vulkan revision.

We would need ISV input to understand how important renaming is, but I would lean in the direction of allowing it rather than disallowing it. I would not like to implement the create info method and then find out later that there is a good use-case for the more expressive form of separate functions - then we end up with two equivalent ways to do the same thing, which in my view is strongly undesirable.

To clarify the point I made before about requiring names to be known at creation time - imagine a codebase where the creation of a texture used for a framebuffer is specified in a high-level material system. If the name must be known at creation time, the name must be passed down from the high-level source to the vulkan API surface in a single call, which is fairly ugly/difficult to compile out entirely for non-debug builds. In contrast if the naming happens in a separate call, then the creation can remain the same in either build and all that's required is to compile out the separate naming call from the root.

I would agree that both approaches have merits. My personal view leans towards the pragmatic - I'd characterise the advantages of using create info are primarily aesthetic (cleaner, type-safe and potentially more future proof), and the advantages of separate functions is pragmatic (more flexible).

Hopefully this reads OK. I'm also open to both approaches but I want to make a fair argument for the one I see as the better course 😄.

[kspagnoli-nv]

Thanks for the detailed reply, Baldur! I agree that a singular method to name an object is preferred; hence the need to hash it out now. I was on the fence before - leaning towards the immutable names approach - but you have some really good points here and have convinced me that the pragmatic approach is more flexible if only one method is desired.

BTW - Are there plans to preserve any of this discussion similar to the "Issues" section that OpenGL extensions typically have? A summary of "Should we have a creation time structure to specify an object's name?" could be worth including.

[baldurk]

Yes I agree the issues should be preserved. That's maybe a question for @oddhack - as I mentioned in the original text, I based the structure of the extension on the debug_report branch which just had an extra appendix added with everything, but the WSI extensions have their details added as a chapter (Chapter 29 in the 1.0 + wsi spec ), with an appendix (E.2, E.3, E.4 etc) containing more meta-data information like the issues and version history.

I'm happy to reformat, create that appendix and note this issue somewhere - I just want to be sure that's the right thing to do!

[kspagnoli-nv]

Some other debug marker APIs have the ability to specify a color. Is this something that should be supported by this extension?

Also, should a more flexible marker begin API be considered? Similar to VkRenderPassBeginInfo.

For example:

typedef struct VkDebugMarkerBeginInfoEXT {
    VkStructureType        sType;       /* = VK_STRUCTURE_TYPE_DEBUG_MARKER_BEGIN_INFO_EXT */
    const void*            pNext;
    const char*            pName;
    float                  color[4];    /* optional */
} VkDebugMarkerBeginInfoEXT;

VKAPI_ATTR void VKAPI_CALL vkCmdDebugMarkerBeginEXT(
    VkCommandBuffer                    commandBuffer,
    const VkDebugMarkerBeginInfoEXT*   pDebugMarkerBeginInfo);

[baldurk]

I like that proposal, it grants additional flexibility and I imagine that most codebases will have common utility code to cover the calling of these functions anyway. Perhaps this should also be used for the 'insert' non-hierarchical marker which could make use of the same struct with a slight rename:

VKAPI_ATTR void VKAPI_CALL vkCmdDebugMarkerInsertEXT(
    VkCommandBuffer               commandBuffer,
    const VkDebugMarkerInfoEXT*   pDebugMarkerInfo);

If that seems reasonable to include as well, I will write up language describing this and add it to this branch.

[kspagnoli-nv]

Thanks Baldur -- I agree on the uniformity. That being said, do you think it makes sense to apply the same pattern to the name and tag APIs? Something like:

VKAPI_ATTR void VKAPI_CALL vkDebugMarkerSetObjectTagEXT(
    VkCommandBuffer               commandBuffer,
    const VkDebugMarkerObjectTagInfoEXT*   pDebugMarkerObjectTagInfo);

[baldurk]

I don't know what the guidance is on when extensible structs should be used vs. inlined parameters - maybe someone from Khronos can chime in on that?

It does seem reasonable to me though, particularly since none of these functions are called at high frequencies and regardless since they're for debugging they're not performance sensitive.

[baldurk]

Bumping this again post-GDC.

If anyone else has any opinions or guidance on the tradeoff between extensible structs vs in-line parameters that would be appreciated. It may make sense to consider the command buffer functions vkCmdDebugMarkerBeginEXT and vkCmdDebugMarkerInsertEXT separately from vkDebugMarkerSetObjectTagEXT and vkDebugMarkerSetObjectNameEXT. It seems a fair direction to me but I'm not sure if there are some larger guidelines to be aware of to keep the API consistent.

Also (ideally after the above issue is settled) I can make the discussed language changes but I'm not clear on how the extension text should be structured. As mentioned before I based this branch on the debug report branch since the extensions are quite similar, but the debug report branch only adds an appendix with its new commands/spec language - compared to the WSI branches which add their new commands/language into the main body and have an appendix which includes things like the issue discussion which we would like to preserve. To avoid redundant work - should I change the format to the WSI style at the same time?

[courtney-g]

FYI - I kept the VK_EXT_debug_report extension in it's own appendix to minimize possible conflicts with the spec and other extensions.

[baldurk]

I've rebased this branch on top of the latest 1.0 branch. I then updated it given a few assumptions about how things should be organised and arranged, and how the parameters-vs-extensible-structs balance should be resolved. Hopefully this is on the right lines and won't need re-worked, but it's a fairly small extension so further edits won't be hard. I will leave it as is assuming it's correct unless I hear otherwise.

• I moved the previous appendix into a proper chapter (I just titled it Debugging since I imagine when debug report gets rolled into core in a future revision this extension and it will both end up in the same chapter).
• I added a new appendix that matches the WSI/VK_KHR_sampler_mirror_clamp_to_edge extension appendices. This tracks the issues raised so far (I hope I have done an accurate job of transcribing/summarising them) as well as some examples and a revision history.
• I changed all new functions to take extensible structs instead of parameters.
• The marker begin/insert structure now contains a color member which can be used to optionally provide a color for markers. If set to {0,0,0,0} it's ignored, to allow for convenient zero-initialisation of the structure.

Further feedback is of course welcome, I will go back to waiting to hear what happens next 😄.

[oddhack]

Thanks. As the LunarG guys have probably communicated to you, the holdup with accepting the branch isn't technical in nature, it's that we've been sorting through a bunch of issues around copyrights and licenses and contributor agreements, in order to make sure the externally generated branches to the spec actually protect everyone's contributions properly, both inside and outside of Khronos. Unfortunately as with anything involving lawyers, this is taking much longer than anyone would like :-( There are some early signs of progress - e.g. (almost) all the KFUL (MIT) licenses in the tree on scripts and such have been changed to Apache 2.0 as of today - but the big holdup remains tweaking the specification copyright to explicitly allow for, and getting a CLA in place around, extension branches. I hope there will be progress during next week's F2F meeting.

[baldurk]

Thanks! I hadn't heard about the copyright/license issues, but that does make sense.

[dennis-hamester]

The functions in this extension take the VkDebugMarker*InfoEXT structs via non-const pointers. Is this intended, or just an oversight?

[krOoze]

@dennishamester Prolly bug? Anyway, this PR+branch is stale. Maybe make an Issue instead for better visibility.

[dennis-hamester]

@krOoze Thanks and done (#513)

[oddhack]

Closing this - extension is published & side issue is opened separately.