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.

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

Primitive clipping at the rasterization stage is inconsistent between APIs #3638

Closed
Kangz opened this issue Nov 25, 2022 · 6 comments
Closed
Milestone

Comments

@Kangz
Copy link
Contributor

Kangz commented Nov 25, 2022

Looking to enable tests around depth clipping and clamping in Dawn, there seems to be yet another inconsistency between APIs.

Because the frag_depth should always be clamped to the viewport, then @builtin(frag_depth) implies that MTLDepthClipMode.clamp is used. @kainino0x previously made this great table, but experimental results show that Metal with MTLDepthClipMode.clamp doesn't do clipping of the fragment at the rasterization stage if @builtin(frag_depth) is used.

More precisely, the following WGSL code with MTLDepthClipMode.Clamp (and trivially the pipeline from it) draws a point on Metal which prevents implementing unclippedDepth = false with frag_depth like on the other APIs:

        @vertex fn vs() -> @builtin(position) vec4<f32> {
            return vec4<f32>(0.0, 0.0, 5.0, 1.0);
        }

        @fragment fn fs() -> @builtin(frag_depth) f32 {
            return 0.5;
        }

The way to fix this inconsistency is to pass the viewport bounds to the shader and either:

  • Use MTLDepthClipMode.clamp and discard at the start of the FS if the position is outside of the viewport.
  • Use MTLDepthClipMode.clip and clamp the resulting frag_depth. (what we opted to do for Vulkan).

However since this changes what's in the investigation table, maybe we need to revisit the exact depth clipping behavior?

@Kangz Kangz added this to the V1.0 milestone Nov 25, 2022
@kainino0x
Copy link
Contributor

kainino0x commented Nov 29, 2022

That's a case I didn't think to test :(. It's surprising to me that the fragment output could affect clipping. I suppose reading the Vulkan spec defined my mental model of how z clipping works - it specifies it as a primitive geometry clip, afaik never a depth test / fragment discard. It's likely I misinterpreted some test results as clips when really I was seeing discards.

In Metal it sounds like it is a clip if you don't write frag_depth, but a discard if you do, which is reasonable since writing frag_depth has special consequences already due to the early depth test. That said, I do wonder if this Metal behavior is different between Apple GPUs and Intel/AMD/NVIDIA GPUs (since they implement D3D12).

@kainino0x kainino0x added the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Nov 29, 2022
@kdashg kdashg changed the title Primtive clipping at the rasterization stage is inconsistent between APIs Primitive clipping at the rasterization stage is inconsistent between APIs Nov 29, 2022
@Kangz
Copy link
Contributor Author

Kangz commented Nov 29, 2022

That's a case I didn't think to test :(. It's surprising to me that the fragment output could affect clipping.

I wrote the issue in a bit of a confused way but fragment output doesn't affect clipping. It is that we need to clamp it to the viewport on Metal like we do on Vulkan. The way the investigation suggested to do that was to use MTLDepthClipMode.clamp but that causes the rasterizer to clip and not clamp.

@kainino0x
Copy link
Contributor

kainino0x commented Nov 30, 2022

Hm. I thought there must be some difference because the depth_clip_clamp tests I wrote passed on Metal but not on Vulkan. I don't remember whether I used the Intel or Apple GPU for that result. They're not passing for me now, on an Apple GPU, with Chrome Canary.

OK, I think I understand the problem now. To restate, we thought MTLDepthClipModeClip kept clamping on (like D3D), but now you're seeing results that it doesn't (like unextended Vulkan, or OpenGL; this makes sense since Metal usually copies OpenGL semantics, and also because the naming of MTLDepthClipModeClip/Clamp strongly implies it). In your test case, MTLDepthClipModeClip doesn't work because we need clamping, and MTLDepthClipModeClamp doesn't work because we need clipping.

I wonder if there was a driver bug when I was testing Metal's behavior.

In any case, D3D still doesn't let us disable clamping, so I don't think we can change our API, unless we can think of a way to emulate it? (Immediate thought is to use ~infinite viewport z bounds, but I am pretty sure this messes up too many other things, if it's even possible.)

@austinEng
Copy link
Contributor

Just checked: the existing depth_clip_clamp tests pass on Metal on both Intel and AMD GPUs. They fail on M1.

@kainino0x
Copy link
Contributor

Sigh, but thanks for doing that investigation! Clears things up a lot.

@Kangz
Copy link
Contributor Author

Kangz commented Dec 8, 2022

Closing, since we agreed in the group that we'd just require clamping of frag_depth in Metal the same way we do for Vulkan.

@Kangz Kangz closed this as completed Dec 8, 2022
@kainino0x kainino0x removed the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants