[replay] Vulkan dump resources command-buffer clones reuse mutated attachments

[MartynJacques-Arm]

Describe the replay bug:
If an attachment is mutated by a prior-dumped draw and is used by the current draw, then the current draw being dumped reads those mutations, and its attachments are corrupted. For example

// original API calls
CmdBuf
 RenderPass
  DrawOne // blend
  DrawTwo // blend
 RenderPass
CmdBuf

becomes

// dump attachments of DrawOne
CmdBuf
 RenderPass
  DrawOne      <- dump attachments
 RenderPass
CmdBuf

// dump attachments of DrawTwo
CmdBuf
 RenderPass
  DrawOne
  DrawTwo      <- dump attachments
 RenderPass
CmdBuf

The execution of "dump attachments of DrawTwo" reads the results of the prior command buffer replay and blends into those results, instead of the original attachment state.

[bradgrantham-lunarg]

@fabian-lunarg Another issue for our ongoing engagement. Please add priority as you desire

[panos-lunarg]

Can you please describe in more detail how the execution of the first command buffer affects the execution of the second command buffer?
What/which are the results of the first command buffer that affect/blend into the results of the second?

[MartynJacques-Arm]

Sure. Let's say we have two draws in a command buffer, each draw adds 0.25 to the RGB channel which start at 0.0.

After the first draw dump, the triangle should be 0.25. After the second, it should be 0.5.

What actually happens is the second draw dump is 0.75, because the 0.25 from the first cb clone is used as the starting point for the second cb clone.

Similarly, I've also seen this when an image is copied, then a shader reads that copy, modifies the sampled colour, and writes the result back to the original image. When the draws are dumped, the modifications compound as each command buffer clone executes because the image is not reset to its original pre-command buffer execution state.

I'm happy to put together a minimal repro if needed.

[panos-lunarg]

Sure. Let's say we have two draws in a command buffer, each draw adds 0.25 to the RGB channel which start at 0.0.

After the first draw dump, the triangle should be 0.25. After the second, it should be 0.5.

What actually happens is the second draw dump is 0.75, because the 0.25 from the first cb clone is used as the starting point for the second cb clone.

The render pass does not clear the color attachment?

Similarly, I've also seen this when an image is copied, then a shader reads that copy, modifies the sampled colour, and writes the result back to the original image. When the draws are dumped, the modifications compound as each command buffer clone executes because the image is not reset to its original pre-command buffer execution state.

Yes. Initially there was logic that kept copies of the resources that are detected as modifiable inside shaders (storage images and buffers). The contents were restored between submissions. This logic most likely has gone stale and might not work correctly any more.

[MartynJacques-Arm]

The render pass does not clear the color attachment?

Not in this case. Although, it is something I dreamed up to illustrate the issue

Yes. Initially there was logic [...]

Yeah, I did come across BackUpMutableResources and RevertMutableResources but without definitions

[panos-lunarg]

Not in this case. Although, it is something I dreamed up to illustrate the issue

How is the color attachment cleared? By the host?
In any case @fabian-lunarg is working on a PR for a couple other issues which happens to solve this problem

[fabian-lunarg]

I created a repro-case, as I understood the issue and tried to make it visible.

the sample draws two additive blended triangles next to each other, with slight overlap.
dumping both draws on current dev re-draws the 1st triangle, so it appears wrong/ too bright:

originally tried solving a different issue with #3289 .
luckily it happens to also fix the wrong re-draws:

[MartynJacques-Arm]

Thanks Fabian. I've tested this with my original API sequence and the issue is no longer there.

I tested with some other content and I don't think the fix survives command buffer boundaries. For example:

  CB0:
      clear swapchain image to black

  CB1:
      begin render pass(loadOp=LOAD, finalLayout=COLOR_ATTACHMENT_OPTIMAL)
      draw grey triangle (+0.25 additive)
      end render pass

  CB2:
      begin render pass(loadOp=LOAD, finalLayout=PRESENT_SRC)
      draw same grey triangle (+0.25 additive)
      end render pass

If I dump all draws, for the final draw, I'd expect the colour value to be 0.5 but I'm seeing 0.75

[fabian-lunarg]

many thanks @MartynJacques-Arm !
was able to reproduce+confirm the follow-up case too.
it's neither fixed nor caused by #3289 (already fails on dev too).
it's very similar but slightly different case, need some additional time to investigate
and decide if it makes sense and try to fix within #3289 ... or leave the issue opened and follow-up with a dedicated PR.

[fabian-lunarg]

added a commit to #3289, now also fixing the command-buffer-boundary case.

[MartynJacques-Arm]

Thanks again @fabian-lunarg. I can confirm this now works properly across command buffers, I'm no longer seeing the accumulated values.

I did unfortunately run into an issue with the latest PR though. I have an app that uses queries and it looks like the fix doesn't play nicely with them e.g.

[...]
vkCmdResetQueryPool
[...]
vkCmdBeginQuery
vkCmdDraw
vkCmdEndQuery
[...]

Crashes here when dumping:

[...]
GFXReconstruct VulkanReplayDumpResourcesBase::OverrideCmdEndQuery
GFXReconstruct VulkanReplayDumpResources::Process_vkCmdEndQuery
GFXReconstruct VulkanReplayConsumer::Process_vkCmdEndQuery
GFXReconstruct VulkanDecoder::Decode_vkCmdEndQuery
GFXReconstruct VulkanDecoder::DecodeFunctionCall
[...]

[fabian-lunarg]

@MartynJacques-Arm many thanks, think I can already see the issue.
a previous commit dealt w. a similar case for unbalanced debug labels/markers.

edit:

tldr;
'should' be fine now :)

another commit has been added to #3289
the issue, as far as I could tell, was the cloned dump-resource submission could end a query they did not start (themselves).
a similar issue surfaced before for vkCmdEndDebugUtilsLabelEXT, but many drivers would just tolerate it.
e.g. mesa changed that at some point (22.2.5 crashes and 26.1.1 was fine)