Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix display of depth-stencil image sampler ImageViews #1001

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix display of depth-stencil image sampler ImageViews #1001

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions core/image/convert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"fmt"

"github.com/google/gapid/core/data/protoutil"
"github.com/google/gapid/core/stream/fmts"
"github.com/google/gapid/gapis/database"
)

Expand Down Expand Up @@ -86,6 +87,15 @@ func Convert(data []byte, width, height, depth int, srcFmt, dstFmt *Format) ([]b
}

func convertDirect(data []byte, width, height, depth int, srcFmt, dstFmt *Format) ([]byte, error) {

// 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)
}

Comment on lines +91 to +98
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

srcKey, dstKey := srcFmt.Key(), dstFmt.Key()
if srcKey == dstKey {
return data, nil // No conversion required.
Expand Down