texture_2d_depth for loading depth values might be unnecessary?

[magcius]

We currently have a special requirement that all depth textures must use a special texture_2d_depth type in the WGSL shader. While it's common to have special shadow sampler types for doing comparison tests, it's not common for shaders just loading depth values to require it. This is somewhat of an annoying requirement for engines, though not an impossible one; I'd like to see if it is strictly necessary.

This requirement seems to originate from the Metal Shading Language Specification, which says:

You must declare textures with depth formats as one of the following texture data types:
depth2d<T, access a = access::sample>
depth2d_array<T, access a = access::sample>
depthcube<T, access a = access::sample>
depthcube_array<T, access a = access::sample>
depth2d_ms<T, access a = access::read>
depth2d_ms_array<T, access a = access::read>

However, I looked at the common shader tools for Metal, and found:

• glsl-optimizer only outputs depth2d for comparison samplers: https://github.com/aras-p/glsl-optimizer/blob/d78c3d2f249aa870368ad320905bc954c47704f6/src/glsl/ir_print_metal_visitor.cpp#L471-L474
• SPIRV-Cross only outputs depth2d if comparison operations were used on the samplers: https://github.com/KhronosGroup/SPIRV-Cross/blob/c2500e504d2b823d73d2f129e4f4f050e9618ecb/spirv_msl.cpp#L13498-L13540

These two tools cover a substantial amount of games shipping on Unity and Unreal. So, it seems that these operations are "de facto" spec'd to work.

I think this could be evidence that we could drop the texture_2d_depth requirement for simply loading depth textures, though it could still be required for doing comparison sampling.

The second issue is that loading from a depth texture seems to have undefined values for the second, third and fourth components in loads, as seen in the Metal Shading Language Specification:

For a texture with the MTLPixelFormatR8Unorm pixel format, the default values for unspecified normalized components are G = 0.0, B = 0.0, and A = 1.0. For a texture with depth or stencil pixel format (such as MTLPixelFormatDepth24Unorm_Stencil8 or MTLPixelFormatStencil8), the default value for an unspecified component is undefined.

(This is a bizarre contradiction in the spec. depth2d's special load function only returns one component, there are no "unspecified components", so how could they even be undefined? This gives me some confidence that the texture loads are actually spec'd.)

On new enough devices, we could use texture swizzles, but that doesn't work for all platforms we want to support. However, given that I expect loading the G/B/A channels from a depth texture to be relatively rare in content, I think it could be solved by compiling an extra shader at pipeline creation time on these devices only if they access G/B/A of a depth texture load. But others can respond with the difficulty of such an approach.

[magcius]

See also: #1198 , 82500965

[kvark]

Thank you for the investigation work!
I'd split this question in 2:

1. should we be able to have a regular GPUTextureSampleType == "float" binding for depth textures in the bind group layout?
2. if not, should we consider "texture_2d" in WGSL to be compatible with the GPUTextureSampleType == "depth" binding in the layout? In this case, the shader generator can see the BGL and put depth2d in the resulting MSL. So we can do this even without assuming anything extra about Metal valid usage.

[magcius]

Good question. I would clearly prefer if GPUTextureSampleType == "depth" could go away as well, but a simple step 1 might just be just removing texture_2d_depth from WGSL.

Are there any other requirements that mandate GPUTextureSampleType == "depth"?

[kvark]

No other requirements. It's only a separate type on MSL.
As a small side bonus to either of these choices, texture_depth_multisampled_2d will be terminated :)

[Kangz]

I think we still need GPUTextureSampleType == "depth" for comparison sampling, as that can only be done with depth2d. 1) would be nice. This way you only need to care about the "depth" sample type for comparison sampling. FYI @litherum

[magcius]

Yeah, I am definitely OK with keeping "depth" for comparison sampling.

[kainino0x]

re: OP / question 1: @litherum would it be possible to get input from the Metal team on why this restriction exists? (that depth-format textures must be bound as depth* in MSL even if they're sampled as color - especially given the contradiction of the "second issue" above - and the lack of this restriction in other APIs)

[magcius]

FYI, from more porting experience: because you already are forced to specify viewDimension in the pipeline layout, I don't think it's too onerous to require specifying depth at the same time.

I'd be OK with moving forward with removing texture_2d_depth and just using the pipeline layout to write the correct texture type in the shader, which sounds like it wouldn't require correspondence with the Metal team. But it would be a nice future to allow float as well.

[kvark]

@magcius I agree, having this outcome would be totally fine. The reason @kainino0x tries to dig the Metal behavior is that - if it turns out to be valid, then we wouldn't need to do any of the extra implementation work to glue the BGL that disagrees with the shader. So if we can get this answer soon-ish, it would help for sure.

[kdashg]

WGSL meeting minutes 2021-09-14

• DM: Previously we decided all depth textures had to be bound as depth bindings. This is inconvenient. Not how it’s done anywhere except Metal. Metal doesn’t have the binding types that we have. Observed by @magcius that the porting paths on Metal, in the cases when comparison is not use, will map to regular texture, and not using depth-texture. Seems to work for everyone for now.
• AB: But that’s not documented as the valid behaviour.
• DM: Want to layout the options
  • 1. Ask clarification from Metal: is the desired pattern supportable?
  • 2. Allow the shader to see regular texture 2D, provided the binding is the regular depth binding. (Does not require input from Metal. Solution is in the shader compiler only.)
• JG: On Metal the compilation needs the pipeline layout, to say which samplers are dept samplers. We probably want to wait for MM for both.
• RM: I’ll ask the Metal team, try to get feedback.

[Kangz]

@litherum did you have any chance to make the list of builtins that would need to maybe be emulated in the future for depth textures?

[litherum]

Looking at the docs, there are 2 results:

1. Non-depth textures have write() functions, but depth textures do not
2. gather() for non-depth textures takes an enum class component {x, y, z, w} argument, but gather() for depth textures does not. It seems unlikely that specifying any value other than component::x on depth textures will work portably.

[kainino0x]

Meeting: We didn't have time to finish this discussion but seemed likely we'll need to go with the pipeline-creation-time code generation solution above due to write()/gather().

[kainino0x]

We also need to decide whether to return vec4<f32>(D, X, X, X) like for stencil or specify more tightly. I think we would keep the Xs because otherwise we would need to do pipeline-time compilation on all backends, not just Metal.

Technically with the pipeline-time compilation being proposed above I think we could precisely specify behavior for both depth and stencil, but it doesn't seem necessary.

[kvark]

Thoughts from the meeting:
If gather() is a problem for depth seen as f32, then it's also a problem for stencil, which has to be viewed as u32. So we have to figure out how valid is this gather() anyway. If it's valid to do, we have an option to expose depth as a regular f32 texture (in both the shader and the pipeline layout). If gather() is invalid, we may need to consider having a "stencil" binding type in the pipeline layout, so that we can at least forbid gather() on Y,Z,W, and we can initialize those to (0, 0, 1) when loading data.

[kainino0x]

Meeting: overall agreement on the pipeline-creation-time code generation being an acceptable approach, the open question is whether we should instead just take advantage of the de facto behavior of Metal and just allow depth textures to be bound to regular "float" binding points.

IIUC:

1. (Current) Require depth textures to be bound to "depth" type bind points. Require shader to use texture_depth_2d* for "depth" type bind points.
2. (Pipeline-creation-time code generation) Require depth textures to be bound to "depth" type bind points. Allow shader to use texture_depth_2d* OR texture_2d* for "depth" type bind points.
3. (De facto) Allow depth textures to be bound to "float"/"unfilterable-float" type bind points. Allow shader to use texture_depth_2d* OR texture_2d* for "depth" type bind points. (3b: Instead require shader to use texture_depth_2d* for "depth" type bind points.)

action item @litherum: determine whether we need to guard against users using gather() on the wrong component (need to know for both depth AND stencil, I think). If so, that prevents option 3 and we have to do option 1/2 (and same for stencil).

[kainino0x]

3. Allow depth textures to be bound to "float"/"unfilterable-float" type bind points.

Based on #1266 (comment) + #744 (comment) I think actually we would only be able to bind depth textures to "unfilterable-float" type bind points.

[Kangz]

From the last meeting we have general agreement to remove allow binding depth textures to texture_2d<f32>, the only difficult is the gather operation on components that are not the first one. And we uncovered the same issue for stencil. Things we need to figure out are:

• For depth do we still allow using them as texture_2d<f32>? (the discussion is leaning yes I think)
• Do we allow gather operations on depth texture bindings? (that's dependent on @litherum's investigation)
• Do we need to add a stencil sample type to prevent stencil textures to be gathered on incorrectly?

@litherum have you been able to check if it is a problem on Metal to use stencil / depth with gather operations not on the first component?

I'm hoping that it may be ok on stencil, so we can keep it the way it currently is. For depth textures turned texture_2d<f32>, we know that at pipeline compilation time, and so do we know the component of the textureGather call (it has to be a constant or pipeline constant), so we can emit an error.

[kdashg]

WebGPU meeting minutes 2022-02-16

• MM: idea here - think we had consensus on this months ago but didn't write resolution - only one popular human writable shading language has a type for depth textures. Others don't. When transliterating shaders, source shader likely won't have types for depth textures. Ease-of-use concern. Want to produce WGSL, but source shader doesn't' have depth texture type.
• MM: general consensus - allow color texture types in the shader to be bound to depth textures, can then sample from them. In API, continue to have bind point be depth_texture bind point.
• MM: 1) In WGSL-to-Metal you'll have info about what are depth textures at the API level. Can produce valid MSL.
• 2. Not needed when translating to HLSL.
• 3. Doesn't delete the depth_texture type from WGSL; you just don't need to use it when binding a depth texture. Methods on depth texture types are different than color texture types. Don't have to use the type if you don't need the extra methods.
• MM: think this satisfies everyone's concerns.
• DM: it was the gather operations where we stopped. DOn't know how Metal will handle gather + stencil textures.
• MM: stencil textures are just textures. Wrote test app, they do work.
• DM: they also work on depth textures seen as depth32 textures?
• MM: don't remember.
• DM: depth textures also have combined formats. Shader does the gather - still need to do it as a gather on a depth+stencil format.
• KG: so you want to make sure this works by law rather than de facto?
• MM: it's valid ot bind stencil texture to color attachment.
• KG: for depth though. You're not supposed to use those as color. Should we double check, or are you pretty confident?
• MM: I think we should assume it works.
• KN: concern with stencil - not that it's not allowed - but that G,B color channels might not be defined.
• MM: it's not defined. I ran test on different hardware, they produced different results. Don't remember 100%.
• KN: if depth works the same way - legal, but not defined results for G,B,A channels - we need to at pipeline creation time transform these in some way so that we hide the undefined results.
• KG: same thing we need to do for stencil.
• DM: we don't have a stencil binding type.
• KG: thought we decided that.
• KN: right.
• MM: not a security concern. "sample this" is something the compiler can see, and expect all the results are defined.
• KN: wanted to ensure it's legal with depth so we can define it the same way.
• MM: we should do that.
• DM: so - do we have to have a depth binding used for all the depth textures? or can i use f32 binding type?
• KG: if just sampling - can use f32 type. depth comparisons -> need depth binding type.
• KN: that's distinction between options (2) and (3). can the mismatch be between the pipeline and the shader? or texture and pipeline?
• DM: that question has been in the air for months. If pipeline knows about depth - we can polyfill everything.
• KG: so this would require pipeline time compilation on Metal.
• MM: would require producing MSL when you have the pipeline layout. Already true.
• KG: should we just ditch depth and see how far we get?
• KN: option (2) then. Fine choice; concern that it's still annoying. Have to get pipeline layout correct.
• MM: Kai's comment from Dec 15 2021 (?). OK. DM's asking whether option (3) is acceptable, should follow up on that.
• KN: I'm happy to do option (2) and consider (3) post-V1.
• KG: I thought we were focusing on (3).
• DM: thought we had all the info we need.
• MM: info missing - if you write MSL program that doesn't use depth texture type, and bind depth texture to it, and call gather() - what happens? I don't know.
• KG: we suspect it behaves like an R32F texture.
• MM: not necessarily. Program I described, if you bind stencil texture - behaves differently.
• KN: only on the other channels?
• MM: yes.
• KN: I strongly suspect depth would be the same. Don't know whether it's legal / spec'd as not legal.
• MM: spec's clear here. Depth texture requires depth type in shader. Option (3)'s against Metal's spec.
• KN: I agree
• KG: but we know it works.
• MM: we know it works for sample operations. No evidence that gather() operations de facto work. The combination is rare.
• DM: concern - if we decide option (3) is fine, all the work going into option (2) is wasted.
• MM: will anyone implement this in next couple weeks?
• DM: option (3) doesn't require impl - just allowing it.
• KN: if you can come back with an answer in a couple of weeks, we can leave this open.

[litherum]

I just wrote the simplest little test program I could - all it does is fill a 2D depth texture with data, bind it to a texture2d in MSL, and call gather() on it in a compute shader. I then ran it on the 2 devices I have in front of me: an AMD GPU and an M1 machine.

Running texture.gather(mySampler, float2(0.5, 0.5), 0, component::????):

(assume the 4 relevant samples in the depth texture are named "cell1", "cell2", "cell3", and "cell4"):

GPU	component::x	component::y	component::z	component::w
AMD Radeon Pro Vega 56	[cell1, cell2, cell3, cell4]	[cell1, cell2, cell3, cell4]	[cell1, cell2, cell3, cell4]	[cell1, cell2, cell3, cell4]
Apple M1	[cell1, cell2, cell3, cell4]	[cell1, cell2, cell3, cell4]	[cell1, cell2, cell3, cell4]	[1.0, 1.0, 1.0, 1.0]

Note how [cell1, cell1, cell1, 1.0] is not either of the acceptable formats for either sampling stencil textures or sampling depth textures. Given that I found this edge case literally immediately after starting investigating this, I don't think we should expect gather() on a depth texture bound as a non-depth type to give any sensible results in the y, z, and w components in practice.

[kainino0x]

Note how [cell1, cell1, cell1, 1.0] is not either

This doesn't match any of the items in your table, is there a typo?

I don't think we should expect gather() on a depth texture bound as a non-depth type to give any sensible results in the y, z, and w components in practice.

The premise is that it's OK if we don't have portable behavior - the spec already makes loose guarantees for stencil with S, X, X, X.
We can expand this section to describe some partially-defined behavior for depth and gather operations as well (e.g. gather works on component::x, but implementation-defined results for anything else).

I think the question we want to answer is whether it's safe, i.e. won't return uninitialized data or cause undefined behavior. The Metal shading language spec just says it's undefined. But it seems unlikely there are more than a few fixed behaviors.

[kainino0x]

Went ahead and filed the new proposal as #3115.

[litherum]

I'm not sure this argument has been made before, but option 2 has a problem. Consider a shader like this:

fn foo(tex: texture2d) {
    sample(tex, ...);
}
fn bar() {
    foo(texture1);
    foo(texture2);
}

Imagine that texture1 has been bound with a depth bind point, and texture2 has been bound with a texture_2d bind point. The shader itself typechecks successfully, but if we want to use Metal's internal depth types for the texture2, we'll end up having to duplicate the body of foo. In the worse case, we'll have to duplicate functions 2^n times, where n is the number of textures bound to the pipeline.

After some more discussion, we're now comfortable with option 3 instead of option 2, provided the spec indicates that operations which can observe/return the y/z/w channels will observe/return uninitialized and nonportable data. (Sorry for this to come in so late.)

[magcius]

Yes, you'll have to either inline or duplicate functions, however, the number of functions you'll need is the number of times called with different texture type arguments, not the number bound to the pipeline. You already require the pipeline layout at shader compilation time, which will tell you the texture type for a given binding.

[kainino0x]

Option 2 gives implementations the freedom to choose between rewriting the shader, and relying on de facto behavior, by adding validation that makes the first possible. It also means that, if we add texture operations in the future that aren't safe on depth textures, we can reject those at pipeline creation time (because we know whether they're used with depth textures or not). So far, feedback has been that the extra validation is acceptable from a developer standpoint.

For these reasons, I think we should keep the previous resolution of option 2.

[litherum]

the number of functions you'll need is the number of times called with different texture type arguments, not the number bound to the pipeline

Right - that makes this worse, not better. In general, function calls are expected to be more numerous than bindings.

Here's an example program which describes one thing I'm worried about:

fn foo(textureA : texture2d, textureB : texture2d, textureC : texture2d) {
    sample(textureA, ...);
    sample(textureB, ...);
    sample(textureC, ...);
}

fn bar() {
    foo(depthTexture, someTexture, someOtherTexture);
    foo(someTexture, depthTexture, someOtherTexture);
    foo(someTexture, someOtherTexture, depthTexture);
    foo(depthTexture, depthTexture, someTexture);
    ... etc etc etc ...
}

Above, we end up having to make a lot of copies of foo() (either by inlining or specializing). Every parameter might need to use the depth type or the non-depth type. I believe @dneto0 has historically expressed a desire to avoid this kind of blowup.

Here's another example of something else I'm worried about:

fn a(myTexture: texture2d) {
    sample(myTexture, ...);
}
fn b(myTexture: texture2d) {
    ...
    a(myTexture);
    ...
}
fn c(myTexture: texture2d) {
    ...
    b(myTexture);
    ...
}
fn d(myTexture: texture2d) {
    ...
    c(myTexture);
    c(myOtherTexture);
    ...
}

This describes how the blowup is global, rather than local - a() needs to be inlined/specialized, so that makes b() have to be inlined/specialized, so that makes c() have to be inlined/specialized. The bulk of the shader can be inside b() and c(), which means we can end up essentially multiplying the compiled size of the entire shader, just because someone added a single function call c(myOtherTexture) somewhere.

The conclusion that I'm coming to is that, if the information about what is bound to a depth texture is present in the bind group layout, but isn't present in the shader source, it's too easy for an author to accidentally create a binary size footgun.

freedom to choose between rewriting the shader, and relying on de facto behavior

This decision seems false to me. If compilers are going to be relying on the de-facto behavior in any situation, then option 2 doesn't seem to have much value. If the de-facto behavior can be trusted, then option 3 is strictly better.

The value of option 2 is "the compiler will intentionally blow up the binary size of your program to generate texture operations that follow the letter of the law."

I'm making a judgement call here: blowing up the binary size of the program is more harmful than relying on the de-facto behavior. And, if no compilers are actually going to be blowing up the binary size of the program, then it seems unfortunate to require authors to supply information which won't end up being used.

Do Tint or Naga currently plan on blowing up the binary size of the program based on GPUTextureSampleType.depth?

if we add texture operations in the future that aren't safe on depth textures, we can reject those at pipeline creation time

GPUTextureSampleType.depth could be re-added at the point that we add these new operations. Enforcing this is no worse than the existing texture analysis we have to do today.

[kainino0x]

GPUTextureSampleType.depth could be re-added at the point that we add these new operations. Enforcing this is no worse than the existing texture analysis we have to do today.

This is a key observation. Given this I'm pretty sure I'm convinced it makes sense to do option 3.

[kainino0x]

Meeting: Re-resolved on option 3.