Fix D3D12 pixel history missing stencil copy.

[jristic]

• When viewing pixel history on a depth stencil, the depth value was copied but the stencil value was not.

Description

Before:

After:

[Zorro666]

Thanks for finding this fix in this instance here.
The Vulkan implementation handles this inside CopyImagePixel().
On Vullkan CopyImagePixel() copies the depth & stencil data if the target has both values.

Doing a quick look at the code I think this is relevant and the code might need updating in D3D12PixelHistoryPerFragmentCallback::PreDraw() on lines lines 1991 and 2045 where the depth is being copied but it does not look to be copying the stencil.

As a discussion point should we change the D3D12 CopyImagePixel() behaviour to copy depth & stencil if both exist to match the Vulkan implementation?
or
Update these couple of places with copy'n'paste code blocks

Both approaches solve the problem

[jristic]

I agree that the current shape of the code isn't ideal, especially with how the second half of this function would handle the stencil copy but since the depth stencil is the primary target here we early out and don't use that code. I'm also not a fan of how I'm using an addition to the offset of sizeof(float) because the depth copy has a side effect on the offset, but I didn't want to refactor the function since I don't understand in what scenarios the first check for IsDepthFormat would succeed but not cause the early out of the function to occur.
I'll take another look at the Vulkan implementation and give it some more thought.

[jristic]

I see what you're saying, both the CmdCopyImageToBuffer and dispatch paths of CopyImagePixel in the vulkan implementation handle stencil automatically if it is present. However, this makes me wonder why the original author chose to deviate from that if they were otherwise quite closely matching the two implementations.

In addition, this is somewhat unrelated but I wonder why there are two paths for copying, CopyTextureRegion and a dispatch, for non-mssa vs. msaa targets, if the latter copy type can handle both cases.

[jristic]

For the D3D12PixelHistoryPerFragmentCallback copies, I believe the stencil is intentionally not copied because it is not representative of the stencil values written by the capture and instead contains the stencil values written by the pixel history algorithm itself in order to facilitate the per-fragment capture.

[skarolewics]

Hi, chiming in as the original author of the D3D12 pixel history code. :) Any divergences from the Vulkan implementation are largely due to API differences, or situations where PSO/rootsig setup, etc. required more attention (iirc the Vulkan impl relied on shader patching for some usage, as one example). Or places where I goofed and missed something, heh.

Bugs still in the D3D12 impl are largely due to limited code coverage for automated testing. Thank you for discovering and fixing some issues!

[jristic]

Thanks for the context! I can see how the API would make the addition of a stencil copy introduce duplicated logic in both paths of CopyImagePixel, does it make the most sense to keep it a separate call to the function then (meaning the original change as posted)? Especially since in some cases of the depth copy we don't actually want the stencil value (the D3D12PixelHistoryPerFragmentCallback pass).
I'm open to either way of doing it, let me know whatever makes the most sense.

[Zorro666]

Thank you for the discussion (I have had computer problems and not been able to reply until now).
I have been working on some small bits of extra automated test validation on the Vulkan and D3D12 implementations to augment this work (this PR and other scenarios).

I think in the context of this change for D3D12 it doesn't make sense to copy the Vulkan implementation and instead do the stencil copy when it is required.

Re-reviewing the implementation and running extra automated tests locally, I think this PR is what we need.

The other depth copies on lines 1990 & 2044 are in the per fragment history loop where the stencil value can't be obtained (it is used to select the fragment).

I will complete my automated tests and verify this change with the tests and then merge it.

[skarolewics]

D3D12 uses planar depth stencil formats, so copying stencil is a different subresource to select than depth. So it's not as simple as reading the green channel from a single sample. I agree, keeping it as a separate copy call will be more straightforward.

[Zorro666]

Just for reference here is PR with small test extension for Vk and D3D12 pixel history. The test PR validates this change (test fails before this change and passes after the code change).

The new test also identified some potential new D3D12 pixel history bugs where the post mod stencil value is the wrong value: in one case it is 0 when it should be -1 or -2 and in another case incorrect depth/stencil value on the first event in the history. Have disabled that part of the new test until the code is fixed.