vkCmdPushConstants() in MatrixStack::record() can sometimes cause Vulkan validation-error VUID-01795

[drywolf]

Describe the bug

TLDR:
Minimal repro code https://github.com/drywolf/vsg-View-vkCmdPushConstants-bug
(see vsg-View-PushConst-bug-minimal.cpp)

---

Some context:

• in our VulkanSceneGraph production code, we are rendering off-screen (without a vsg::Window)
• our renderer uses just one single vsg::CommandGraph to record/submit all of our Vulkan drawing/commands
• we are using multiple vsg::RenderGraphs that are children to this vsg::CommandGraph
• some of the vsg::RenderGraphs are rendering 3D scenes/geometry
  • these passes are making use of vsg::View/vsg::Camera/vsg::MatrixTransform/etc.
  • (very similar to the code in vsgExamples / vsgXchange-assimp)
• and some other vsg::RenderGraphs are simply rendering a Fullscreen triangle/a Fullscreen GLSL shader
  • each of those passes is using its own individual vsg::Framebuffer+ vsg::Image attachments
  • typically the fullscreen-shaders are reading from a GLSL sampler2D and are writing to the bound vsg::Image attachments
  • (e.g. for doing color-effects on previously rendered RGBA images of a scene)

The Bug:

Recently we have found that under some circumstances in our app, we are getting VUID-vkCmdPushConstants-offset-01795 Vulkan validation-errors.
It took some debugging into the VulkanSceneGraph code itself to figure out what was causing it.
We are rendering Fullscreen-Passes using pretty much the exact same VSG mechanisms & C++ classes that are otherwise used for the 3D scene rendering.
Because of that, when VSG encounters our Fullscreen-Passes, it also calls the following code:

• void RecordTraversal::apply(const Command& command) (source)
  • which calls vsg::StateStack::record() (source)
    • which calls MatrixStack::record() (source)
      • which ultimately calls:
        vkCmdPushConstants(commandBuffer, pipeline, stageFlags, offset, sizeof(newmatrix), newmatrix.data());

As long as our Fullscreen-Shaders did not use any Push-Constants, the check for if (pipeline == 0 || stageFlags == 0) prevented the vkCmdPushConstants() call for the projection & model-view matrices from happening.
But recently we added a Fullscreen-Pass that was using Push-Constants to pass a single vsg::vec4 to the fullscreen-shader.
In this case we started seeing the VUID-vkCmdPushConstants-offset-01795 validation error.

VUID-vkCmdPushConstants-offset-01795(ERROR / SPEC): msgNum: 666667206 - Validation Error: [ VUID-vkCmdPushConstants-offset-01795 ] Object 0: handle = 0x1ea94cfd920, type = VK_OBJECT_TYPE_COMMAND_BUFFER; | MessageID = 0x27bc88c6 | vkCmdPushConstants(): VK_SHADER_STAGE_FRAGMENT_BIT, VkPushConstantRange in VkPipelineLayout 0x84c0580000000017[] overlapping offset = 0 and size = 64, do not contain VK_SHADER_STAGE_FRAGMENT_BIT. The Vulkan spec states: For each byte in the range specified by offset and size and for each shader stage in stageFlags, there must be a push constant range in layout that includes that byte and that stage (https://vulkan.lunarg.com/doc/view/1.3.250.0/windows/1.3-extensions/vkspec.html#VUID-vkCmdPushConstants-offset-01795)
    Objects: 1
        [0] 0x1ea94cfd920, type: 6, name: NULL
VUID-vkCmdPushConstants-offset-01795(ERROR / SPEC): msgNum: 666667206 - Validation Error: [ VUID-vkCmdPushConstants-offset-01795 ] Object 0: handle = 0x1ea94cfd920, type = VK_OBJECT_TYPE_COMMAND_BUFFER; | MessageID = 0x27bc88c6 | vkCmdPushConstants(): VK_SHADER_STAGE_FRAGMENT_BIT, VkPushConstantRange in VkPipelineLayout 0x84c0580000000017[] overlapping offset = 64 and size = 64, do not contain VK_SHADER_STAGE_FRAGMENT_BIT. The Vulkan spec states: For each byte in the range specified by offset and size and for each shader stage in stageFlags, there must be a push constant range in layout that includes that byte and that stage (https://vulkan.lunarg.com/doc/view/1.3.250.0/windows/1.3-extensions/vkspec.html#VUID-vkCmdPushConstants-offset-01795)
    Objects: 1
        [0] 0x1ea94cfd920, type: 6, name: NULL

---

PS: the vsg::Commands overload of void RecordTraversal::apply() is calling into exactly the same function-callstack as shown above.
(it can lead to the exact same problem)

To Reproduce

Steps to reproduce the behavior:

1. clone the GIT repo containing the bug-repro code
   • https://github.com/drywolf/vsg-View-vkCmdPushConstants-bug
2. configure and build the code
3. see the README.md and/or BugScenario code in vsg-View-PushConst-bug.cpp
4. run the vsg-View-PushConst-bug.exe executable
5. see Vulkan validation errors VUID-01795 in console
   • also in some scenarios you will experience a crash

Expected behavior

• the vkCmdPushConstants() in MatrixStack::record() should only be called when actively requested/configured
  • in other words: the 2x mat4 push-constants that are used specifically only for the VSG pbr/phong/flat shaders, should only be set when one of these shader-sets is currently active
• how this ought to be configured/controlled from the C++ code needs to be discussed ?!
  • could be set per ShaderSet (as mentioned above)
  • but could also be set per StateGroup, per RenderPass/RenderGraph, PipelineLayout, ... ?!

[drywolf]

I now added a stripped down version of the VUID-01795 error repro-code to the repository:

• vsg-View-PushConst-bug-minimal.cpp

The original vsg-View-PushConst-bug.cpp code has some additional code for experimentation + it also contains a BugScenario that seems to reveal a completely separate / different crash-bug in BindGraphicsPipeline::record()

[drywolf]

how this ought to be configured/controlled from the C++ code needs to be discussed ?!

I imagine the most correct & least invasive way to fix this, would be to have some kind of meta-data/trait attached to the PushConstantRanges pushConstantRanges; stored in the vsg::PipelineLayout.
That way, this PushConstantRange meta-data/trait could then be handled in CommandBuffer::setCurrentPipelineLayout() in the same way as is already done for the _currentPushConstantStageFlags.

And finally this meta-data/trait could be fetched during MatrixStack::record() and be used to decide if vkCmdPushConstants() should be called for the projection/view-model matrices or not.

---

PS: Maybe the code that is currently in MatrixStack::record() could even be pulled out of State.h and this handling of "PushConstant-Traits" be made into a wider-reaching functionality (to make the handling of the projection/model-view matrices less deeply coupled to the core VSG code)
But this is more a question for future architecture improvements rather than fixing the above issue.

---

PSS: @robertosfield
In this code I noticed that only the first element of pushConstantRanges is used.
I would have expected this to be checking/combining the stage-flags of all the pushConstantRanges in the layout, not just the first one ?!
Does this code work correctly when using multiple pushConstangRanges, or would this maybe also need fixing/a more complete implementation?

[robertosfield]

I'm not in position to look into this, but hopefully will be able to reproduce it and ponder on how best to resolve it in the next few weeks.