Fix display of depth-stencil image sampler ImageViews #1001
Conversation
mark-lunarg
requested review from
pmuetschard,
ianelliottus and
mikes-lunarg
| // Source format of depth/stencil images may be packed into a depth-only representation that | ||
| // differs in size from the original format | ||
| if srcFmt.Name == "VK_FORMAT_X8_D24_UNORM_PACK32" { | ||
| srcFmt = NewUncompressed("VK_FORMAT_X8_D24_UNORM_PACK32", fmts.D_U24_NORM) | ||
| } else if srcFmt.Name == "VK_FORMAT_D16_UNORM_S8_UINT" { | ||
| srcFmt = NewUncompressed("VK_FORMAT_D16_UNORM_S8_UINT", fmts.D_U16_NORM) | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think this is the right place to put this. It looks like we are pretending the srcFmt is different for these two cases only for the purpose of conversion. If D_U{16,24}_NORM is the actual format of the data, we should make sure it's reported as such.
There was a problem hiding this comment.
@pmuetschard, I can see about changing the original format of the surface, but did not choose this path originally due to the comments in commit 2bba7444f, I interpreted this to mean that the original format of the surface must be maintained. This pattern is also used in a handful of places, for example, wherever getDepthImageFormatFromVulkanFormat() is called or in tranform_overdraw.go or tranform_read_framebuffer.go. With this in mind, is the right place to put this change in the capture code where the original surface data is saved?
There was a problem hiding this comment.
Sorry for the delay in getting back to you. Thanks for the pointers, it helps clear things up a bit as to what is going on here.
I'm still not quite convinced we need this "translation", but assuming we do, I think we should push it up the stack. (e.g. the name field in the proto is meant only for display purposes, so having our core library look at it is surprising.) Since this is called from the texture resource resolve, we should handle it at that level.
However, it makes me wonder, why does the texture's data not match the expected size? From the screenshot, a 2048x2048 texture with format VK_FORMAT_X8_D24_UNORM_PACK32 should have a size of 2k*2k*4, not 2k*2k*3, or am I reading the spec wrong? It seems to me that we are not serializing this data correctly and cutting out the X8 parts - this makes me think it's an MEC bug and this fix just works around it, but would make stuff blow up if we were to pass a true VK_FORMAT_X8_D24_UNORM_PACK32 image.
There was a problem hiding this comment.
@pmuetschard, I imagined that the authors didn't want to 'waste' the space for copying/saving/resolving the X channel in this class of formats, though it seems like it'd be much more straightforward to just save the original data in its original format. If you wish, I could look at what it'd take to go that route, or we move it to the texture resource resolve level as mentioned.
|
Pascal, is this PR still alive ? |
The depth component of combined depth/stencil image formats is stored separately in a packed format. When resolving these packed formats, the new packed format size must be taken into account instead of the original combined format size.
Fixes issue 204190897.