Fix buffer size for D24_UNORM_S8_INT format#940
Conversation
Vulkan expects depth component of D24_UNORM format to be padded to 32 bits. This change adds an 8-bit padding after the depth channel.
|
Looks like the newly added tests use depth format that is not supported by the bot environment which causes the Ubuntu builds to fail. What should we do with those? |
dj2
left a comment
dj2
left a comment
There was a problem hiding this comment.
Isn't the D24UNORM_S8UINT format already 32 bits as there is 24bits of depth and 8 bits of stencil? Adding another 8 bits makes it 40 bits doesn't it?
| private: | ||
| bool is_padding_ = false; | ||
| bool is_packable_ = false; | ||
| FormatComponentType name_ = FormatComponentType::kR; |
There was a problem hiding this comment.
Why did the default values change?
| 3U, | ||
| { | ||
| {FormatComponentType::kD, FormatMode::kUNorm, 24}, | ||
| {FormatComponentType::kX, FormatMode::kUNorm, 8}, |
There was a problem hiding this comment.
What does this represent? This is no longer 32 bits, it's 40 bits isn't it?
|
Yeah, I am not convinced this is correct, but I don't yet understand why (without your PR) the |
|
I think the issue is actually elsewhere in Amber. I have been trying to figure this out for a few hours and I could still be wrong. See here: amber/src/vulkan/transfer_image.cc Line 221 in cc1dec6 I think Amber is copying the depth/stencil buffer to the host and back, and messing things up a bit when it does so. This is why the framebuffer ends up with the wrong image (the depth buffer was changed by Amber between each draw). It just so happens that making the format bigger means that we have enough room for things to work, but I don't think that is the real fix. I think the real fix is to adjust the size of the host buffer when we need space for stencil data. Probably here: amber/src/vulkan/transfer_image.cc Line 152 in cc1dec6 Or perhaps we shouldn't even change the size of the host buffer, as it is technically correct. Also, could the values be probed? If so, it would be strange if the stencil data came after the depth data. We could instead somehow copy the depth and stencil data separately, and then combine them into the host buffer (and vice-versa when copying back to the device). This seems harder. |
3 participants
Vulkan expects depth component of D24_UNORM format to be padded to 32 bits. This change adds an 8-bit padding after the depth channel.