SaschaWillems / Vulkan

C++ examples for the Vulkan graphics API
MIT License
10.35k stars 2.03k forks source link

subpasses example uses incorrect input attachment fragment shader indices #295

Closed nanley closed 7 years ago

nanley commented 7 years ago

According to 14.4. Fragment Input Attachment Interface,

A subpass input variable identified with an InputAttachmentIndex decoration of i reads from the input attachment indicated by pInputAttachments[i] member of VkSubpassDescription

However, the fragment shaders set input_attachment_index equal to the index of the attachment of the render pass (pInputAttachments[j].attachment, 0 <= j < inputAttachmentCount). Decreasing the index by 1 in all the fragment shaders fixes the issue.

Unfortunately, it doesn't seem like the validation layer catches this issue although there seems to be code for it at layers/core_validation.cpp:2652 . Do you think this issue has enough information to file an issue with the validation layers?

SaschaWillems commented 7 years ago

What platform (os, driver, GPU)? I have tested the example on 4 GPUs and 3 different platforms and all worked fine. If this is actually wrong and not caught by recent validation layers then yes, this may be wrong or missing from the layers. I'd have to take a look at this first, but dunno when I get a chance to look at it.

nanley commented 7 years ago

I'm running the example on Linux, using Mesa 17 and a Skylake Intel GPU. It's interesting that other GPUs don't have this issue, perhaps they don't make use of the shader input attachment index.

SaschaWillems commented 7 years ago

I also do test on Linux (Ubuntu) with a Haswell CPU and it works just fine there. Same with other GPUs (AMD, NVIDIA). Did you check with other examples/apps that use subpass input attachments like vkQuake?

But after taking a quick look you seem to be right. The SPIR-V decorators are there but at least on NV the index actually doesn't matter.

nanley commented 7 years ago

Interesting datapoint about HSW. vkQuake gameplay looks correct, but there is flickering on the menu screen. The shader used in that game actually implements both the behavior found in 14.4 (as I understand it) and the subpasses example. The shader input attachment index is 0, which matches the index of the attachment reference in the subpass and the attachment reference's attachment field.

SaschaWillems commented 7 years ago

I can confirm this with several devices (Intel IGP on Ubuntu and an Android Phone). Without your changes, the example does not render correct, so I'll change this as per your initial post.

And yes, I think this should be reported by the validation layers, so it may be worth an issue over at the Khronos repo.

eero-t commented 7 years ago

I can verify that it now renders everything on SKL GT4e. It looks same as in screenshot, except for some minor variation in colors (e.g. the upper part of the pillar on right is blueish, not reddish): https://github.com/SaschaWillems/Vulkan/blob/master/screenshots/subpasses.jpg

SaschaWillems commented 7 years ago

Color variation is due to randomized light positions and colors, other than that it works for me on an Intel IGP (Mesa) and an Android mobile with these changes, so I consider this (hopefully) fixed.

eero-t commented 7 years ago

Could you add random seed option to the demos and take the screenshots with a (documented) fixed seed value? That would help in validating the results, especially if validation gets automated.