Vulkan Overdraw Tracking #1994
Vulkan Overdraw Tracking #1994
Conversation
sean-purcell
force-pushed
the
overdraw-stencil
branch
from
5751e8e to
d8881f4
Compare
gapis/api/vulkan/overdraw.go
Outdated
| } | ||
| } | ||
|
|
||
| func (s *stencilOverdraw) add(ctx context.Context, after []uint64, capt *path.Capture, res replay.Result) { |
There was a problem hiding this comment.
Can capt here be different from s.capture? Do you need to pass it in both places? Here and in newStencilOverdraw
gapis/api/vulkan/overdraw.go
Outdated
| lastRenderPassArgs, lastRenderPassIdx, id, | ||
| mustAllocData, addCleanup, out) | ||
| if err != nil { | ||
|
|
There was a problem hiding this comment.
was the new line here intentional?
gapis/api/vulkan/overdraw.go
Outdated
| rewrite map[api.CmdID]replay.Result | ||
| } | ||
|
|
||
| func newStencilOverdraw(ctx context.Context, capt *path.Capture) *stencilOverdraw { |
There was a problem hiding this comment.
Oh yes, unused, oops
sean-purcell
force-pushed
the
overdraw-stencil
branch
from
1bb6092 to
7471934
Compare
cmd/gapit/screenshot.go
Outdated
| @@ -174,3 +182,25 @@ func (verb *screenshotVerb) frameCommand(ctx context.Context, capture *path.Capt | |||
| fmt.Printf("Frame Command: %v\n", eofEvents[verb.Frame].Command.GetIndices()) | |||
| return eofEvents[verb.Frame].Command, nil | |||
| } | |||
|
|
|||
| func rescaleBytes(ctx context.Context, data []byte, max int) { | |||
There was a problem hiding this comment.
Have you thought about just rescaling dynamically?
(0->Max) -> (0->1)
There was a problem hiding this comment.
Could be done as a follow-up but it would save some having to type.
That being said, someone may want to avoid rescaling as well.
There was a problem hiding this comment.
Never mind! I misread this, you are already doing what I had said.
Ignore me
| // unpackMapCustom takes a dense map of u32 -> structure, flattens the map into | ||
| // a slice, allocates the appropriate data using a custom provided allocation | ||
| // function and returns it as well as the length of the map. | ||
| func unpackMapCustom(alloc func(v ...interface{}) api.AllocResult, m interface{}) (api.AllocResult, uint32) { |
There was a problem hiding this comment.
Maybe unpackMapWithAllocator it describes what's custom about it a bit more clearly.
gapis/api/vulkan/overdraw.go
Outdated
| )).Ptr() | ||
| } | ||
|
|
||
| // TODO: determine if basePipelineHandle is an issue |
There was a problem hiding this comment.
You should be able to just pass in -1 / VK_NULL_HANDLE for base_pipeline_handle / base_pipeline_index.
Actually if you are passing in 0 for flags you MUST have -1 / VK_NULL_HANDLE for base_pipeline_*.
Alternatively, for a slight optimization, IF the original pipeline was created with VK_PIPELINE_CREATE_ALLOW_DERIVATIVES_BIT you could actaully set VK_PIPELINE_CREATE_DERIVATIVE_BIT, and point base_pipeline_handle to the original pipeline. That would improve pipeline compilation speed.
gapis/api/vulkan/overdraw.go
Outdated
| addCleanup func(func()), | ||
| out transform.Writer, | ||
| ) (stencilImage, error) { | ||
| // TODO: check if we're allowed to modify the command directly, since |
There was a problem hiding this comment.
You shouldn't modify the commands. This can cause problems later on, since these are shared data-structures. (If golang has a CONST keyword we would be using it here).
gapis/api/vulkan/overdraw.go
Outdated
| // TODO: Ideally this would be smarter, but without duplicating the | ||
| // state and mutating it, it's hard to tell what the right | ||
| // vkQueueSubmit to modify is. | ||
| lastSubmit := int64(after) |
There was a problem hiding this comment.
This logic should be improved to allow selecting a specific renderpass without counting on the early terminator. #2010 needs to be resolved first though.
gapis/api/vulkan/overdraw.go
Outdated
| VkImageLayout_VK_IMAGE_LAYOUT_GENERAL) | ||
| } | ||
| } else { | ||
| stencilDesc = MakeVkAttachmentDescription(a) |
There was a problem hiding this comment.
So we can store off some extra data on the side of VkCreatePhysicalDevice, and grab the supported depth formats. Then you will be able to look up what the valid/legal depth formats are.
This can be done as a follow-up.
| func (s *stencilOverdraw) add(ctx context.Context, extraCommands uint64, after []uint64, capt *path.Capture, res replay.Result) { | ||
| c, err := capture.ResolveFromPath(ctx, capt) | ||
| if err != nil { | ||
| res(nil, err) |
gapis/api/vulkan/overdraw.go
Outdated
| if err != nil { | ||
| res(nil, err) | ||
| } | ||
| lastSubmit := int64(after[0]) |
There was a problem hiding this comment.
Seems like this could be refactored into something more readable:
for lastSubmit := int64(after[0]); lastSubmit >= 0; lastSubmit-- {
switch (c.Commands[lastSubmit]).(type) {
case *VkQueueSubmit:
id := api.CmdID(uint64(lastSubmit) + extraCommands)
s.rewrite[id] = res
s.lastSubIdx[id] = api.SubCmdIdx(after[1:])
return
}
}
res(nil, &service.ErrDataUnavailable{
Reason: messages.ErrMessage("No last queue submission"),
})
|
Checking this on Windows now |
sean-purcell
force-pushed
the
overdraw-stencil
branch
from
21afd59 to
d0bb745
Compare
|
This was failing for me on Windows, (empty output). I ran with the validation layers attached, and saw a few issues. Can we resolve those before submitting. I sent them to you separately, because they are quite large for a github comment |
- Doesn't yet handle pre-existing depth/stencil attachments
sean-purcell
force-pushed
the
overdraw-stencil
branch
from
d0bb745 to
2e18ebb
Compare
Vulkan overdraw tracking using the stencil buffer (currently includes some commits that are necessary and haven't been merged to master yet).
gapit screenshot --overdrawproduces images that look like: