-
Notifications
You must be signed in to change notification settings - Fork 311
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
2nd, 3rd, and 4th components of sampled depth/stencil textures are undefined in Metal #1198
Comments
@litherum @grorg do you or the Metal team have more insight into this issue? i.e. "the default value for an unspecified component is undefined". Does this mean it is actually undefined and may leak information from other GPU memory? Or does it simply mean that it may return a "safe" value that differs depending on the Metal implementation / texture compression? |
A strawman proposal: all depth & stencil textures have to be seen as "texture_depth_xx" by the shader (name up to bikeshed with regards to stencil presense). So WGSL -> MSL compiler can figure out that only one component is sampled, and replace the rest with 0, 0, 0, 1. In addition, we can add |
Talked about this with editors. Here is an expanded suggestion.
It's already misleading for saying the textures can only be compare-sampled. I think we can instead do something like:
So this binding type would be the only binding you could have for a texture that has a depth/stencil format. The shader compiler would therefore know precisely when it's invalid to get the 2nd, 3rd, etc component from a texture. For stencil, we could add a separate function like I believe these measures would fully address the problem @austinEng found. |
#970 says that:
so we may also need a compile-time check that you don't use a texture with both a comparison-sampler and a non-comparison-sampler |
extra data point: This turns out to also be a problem on D3D12 for some GPUs/drivers @RafaelCintron. See https://bugs.chromium.org/p/dawn/issues/detail?id=574 |
I'm pretty sure this is not actually true, see archaeology here: #970 (comment) |
The latest buzz (which is actually quite old) here is that for stencil textures the users will need to create a view with "stencil" aspect and bind it as "uint" binding. Given that some platforms don't correctly sample 2nd/3rd/4th components, the WebGPU implementations would have to provide an extra bit of data (for each uint texture binding in the layout) saying whether it's for stencil or not. (credit goes to @Kangz for suggesting that). I think this is doable. @austinEng are there any concerns left unaddressed by this path? |
I think such a workaround would work though it is a bit unfortunate given that it needs to be done for every uint texture, regardless of whether it's stencil or not. This seems fine though given that this behavior is wrong on D3D12 and should hopefully get fixed in drivers, and newer versions of MacOS can workaround the issue with swizzling. One remaining clarification: Is it allowed to bind a |
it's inconsistent, yes, but |
Well, |
That's true, I guess we could allow binding depth to |
I thought Then stencil aspects can just be used as |
I felt like there was a reason to force us using "depth" bind point for depth textures, but I'm not able to recall/find it now. |
I think it was just the ,0,0,1 issue. |
Well, it's still sort of an issue then. Supporting this edge case means we'll have to provide that extra bit of information on every |
IMHO as long as yzw is made of "safe" values and we know that eventually the data will be 0, 0, 1 on all platforms, I think this is fine. Unfortunately WebGPU cannot paper over every hardware difference, especially in its first version. However we can steer the ecosystem towards fixing this differences. WebGL has a similar thing were some tests are not required to pass to be conformant, because of hardware differences or driver bugs. |
I think this discussion got dropped but is unfinished. The concern is about the cost of ensuring this 0, 0, 1 on all "float" textures, given that they may actually be depth textures in disguise. If we allow depth to be bound as "float", then we'd have to 1) provide a bit of information for every texture, and 2) do the run-time check every time YZW are accessed on it. This is both run-time and implementation time overhead (on Metal). The alternative is to say that the depth format textures (with depth aspect) can only be bound as depth sample component. I think that would still allow people to do everything they need? |
Another alternative is to eat the slight non-portability with text saying sampling depth/stencil textures in disguise SHOULD produce (X, 0, 0, 1). Then on all platforms that support texture swizzling make it happen, and over time the few macOS versions that don't support it will disappear, at which point we can change to MUST. |
I though that one of the reasons we added |
|
Comparison sampling, but I thought it was also required for normal sampling of depth textures? If we have type information that it's a depth texture, then we can also prevent loading any component other than depth. In fact, sampling We determine whether it is used for comparison or not based on static analysis of whether it is used with |
Texture swizzling is not compatible with MTLTextureUsageRenderTarget, and for most use cases of stencil textures you'll want it to be a render target. So I suppose option 4 doesn't really work. |
@kvark for option (7), why a validation error? Why not just have the implementation set the yzw channels in this case? With this modification, option (7) seems better than option (3). Compilers compiling from other languages to WGSL wouldn't have to care, since there would be no difference in the shading language. Then, when we compile WGSL to MSL, we'll do it with the pipeline layout, so we'll know which texture requires having its yzw channels clobbered. (It would be yet one more analysis where we have to figure out which texture operations occur on which texture, though.) That being said, we're leaning toward option 5. There aren't many use cases of stencil buffers which require actually sampling from it in a shader. Most use cases just use it as a render target, which would continue to work. And, if we discover a use case, we can always add it in later. |
That would be a transformation in the shader, but the shader may have already been generated. My very tentative preference is toward 5 now, and 7 later? (but maybe any of 1/3/6/7.) I would like 3/6 best, but I agree with the issue Myles raised in the meeting; they require WGSL generators to synthesize type data they don't normally have. 7 is a bit fancier than the analysis we already require for sampler/texture pairs, but seems feasible. (We would have to be strict and require that the yzw channels be discarded immediately, though; after that they immediately become very hard to track through the program.) |
Not sure why I haven't considered this, but yes, @litherum , we should just make the generated shader to set the proper yzw channels. I think this makes (7) even better than I anticipated, and we should go with it. |
Ah, I forgot this only affects Metal, where we always have the pipeline layout at shader generation time. Option 7b, I'll call it, sounds good to me. (Generate .xxxx or vec4(x,0,0,1) or whatever) Slightly concerned well run into this behavior on other backends as well, but if that would indeed be out of spec, then we can just do workarounds. |
Is 1) completely dismissed? This is a small corner case that developers are not very likely to run into (programs work on Metal today) that should resolve itself overtime. It would be unfortunate to make our binding model even more inflexible because of it. For example 2) 3) 6) and 7) prevent having pipelines agnostic at whether data comes from a stencil textures or an
Where do you see this? I can't find anything in the Metal docs. |
@Kangz the amount of extra work for 7b is rather small, and I don't think the "having pipelines agnostic" argument is particularly strong, because they are already very limited for depth textures. |
It’s caught by the validation layer. It’s probably in the docs somewhere, too. |
I'm trying to figure out how the analysis would work for 7B. I think it's fundamentally a different kind of analysis that the one for uniformity or filterable textures. For the uniformity analysis, you gather constraints. You determine all the constraints inside a function, see if any of them conflict, summarize those constraints at all the call sites of the function. For filterable textures, you also gather constraints. If both the texture and sampler being used with it are global variables, then you can check for a conflict immediately. If either one of them is an function parameter, you then reform that constraint in terms of the calling function, and repeat this all the way to the entry point. For the analysis in 7b, we're not gathering constraints. Instead, we're trying to figure out, at the site of every I think we've allowed authors to take pointers to textures, so one could write something like this:
So, when we emit code for the Specializing the function seems unfortunate, because of the exponential blowup. Another option would be to internally map WGSL's Another thing we could do is forbid WGSL authors from taking pointers to textures, but being able to do that is pretty useful from an author's perspective. So it would be a big shame to lose a large, useful language feature just because of this corner case that most authors aren't going to hit anyway. Philosophically, the way you know what code to emit for a given expression is represented in the type system. So, I kind of feel like, if we wanted to statically know, at every call site to |
Regarding option 1, the spec says "the default value for an unspecified component is undefined" (emphasis mine). So, if the WGSL author writes a program with this:
The downstream MSL compiler would be totally within its rights to entirely eliminate the bounds check of the array access. Regardless of whether WGSL specifies what the But, it would be really bad if every call to (In reality, there's no way for the MSL compiler to know whether or not the texture is a stencil texture or not, so it's probably safe. It's unsafe if you follow the letter of the law of C++, though.) So, I think the conclusion is: the stencil-ness (or possibly "scalar-ness" re: option 6) needs to either be part of WGSL's type system, or we forbid stencil textures for ending up as resources in a shader. |
The argument is that we need to be pragmatic as to what to paper over and what to fix by adding inflexibility in the API. For depth textures we don't have a choice because MSL requires a different type be used. For sampling of stencil textures the only problem is undefined values for the .yzw components. This is a minuscule portability issue and we're going to add more garbage to our already baroque binding model to fix it? It doesn't seem worth the effort when there are many other much bigger portability pitfalls in the API (the robust access and invalid draw behavior for example, but also just rasterization, filtering and viewport precision, etc). Let's be pragmatic and live this one alone, we collectively already spend too much time on it when we could have shrugged and moved on to more important things.
Yeah the compiler can't take advantage of that UB, so really it isn't a safety problem, just the tiny portability issue. |
It's Complicated: https://llvm.org/docs/LangRef.html#undefined-values (NB: This is LLVM not C++ per se) |
I'd emphasize that distinction! I looked through the C++14 spec. (There are some close calls. First: 26.6.2.8 for the description of valarray::min
Second: For A::fetch_key
) So, that chimeric behaviour of LLVM "undef", where each time it's observed it may have a different value, is very much LLVM-specific. Since WGSL can't know that a stencil value is bound to something being accessed, then the MSL compiler can't know. And so the LLVM "undef"-ness shouldn't kick in. I'm with @Kangz on this one. Metal isn't even compatible with itself, so that's a strong argument to me there isn't a burning need to cement this particular corner in WebGPU. I think we should note the issue, and exhort Apple to fix Metal implementations. |
If we choose to not do anything about working around this aspect of Metal, do we even need the |
Depth textures are a different league. They have different types in SPIR-V and MSL (versus float). If you want to consider removing the binding type depth textures, you might as well remove the whole |
Echoing something I said on the call, the appeal of (5) is that we can have more time to introduce this nicely (possibly, as an optional feature). It's not hard-blocking user workflows, since they can do a copy from stencil to an |
WGSL meeting minutes 2021-07-13
|
Interesting note from #1924 (comment) - the decision here is not just about Stencil, as we thought. It also affects depth textures visible to shaders as regular textures, which may be even a bigger deal than Stencil views (e.g. it's related to the ability to resolve MSAA depth). |
Resolved (I think?): For both depth and stencil, just have the G/B/A channels have unspecified values, don't try to make this 100% portable. Same as WebGL. Try to tighten it up over time with newer devices. |
WGSL meeting notes: (previously) MM: (listed 3 main directions)
MM: Didn’t we resolve this in yesterday’s WebGPU meeting? We agreed to leave this unspecified. |
FWIW I think it was in the WGSL agenda because of option 2:
|
We were testing over 2 million cases. This was, unsurprisingly, slow. Reduce down to 24,000 cases.
In WGSL, performing a
textureSample
ortextureLoad
operation on atexture_sampled_2d<type>
yields avec4<type>
. This is true even for textures that have fewer than 4 components ex.)r8uint
,r16float
,rg8unorm
. It is possible to bind a texture withn
components totexture_sampled_2d<type>
as long as the texture's component type matches thetype
in the shader. You can bind a texture of typedepth32float
totexture_sampled_2d<float>
.Sampling the "unspecified" components yields default values. i.e. accessing the
b
component of anrg
texture yields some default value.According to the Metal Shading Language Specification (EDIT:) 2.3 section 6.10 / 2.4 section 6.12:
On the contrary, in all of the other APIs (Vulkan, D3D12, OpenGL too), when sampling a depth texture (it has 1 component), accessing the "unspecified" components
.g
,.b
, and.a
yield 0, 0, and 1, respectively.In recent versions of MacOS (10.15+) and iOS (13.0+),
MTLTextureSwizzleChannels
can be set on texture view creation to force the unspecified components to some component index, or 0, or 1. Unfortunately, on older Apple platforms, mitigating the undefined values in Metal would involve binding extra data to the shader to indicate whether at draw-time a texture is a depth/stencil texture in order to emulate correct values for the unspecified components. We could also recompile pipelines and pass the data as MTLFunctionConstants, but that may be a worse solution.If we want to avoid binding extra data per-sampled-texture or recompiling pipelines, an alternative solution could be to add even more binding types and WGSL types:
sampled-depth-texture
andsampled-stencil-texture
. This way, a WGSL compiler knows at compile-time if a texture is a depth/stencil texture and can insert shader code to emulate the unspecified components.Note: When I say "sampling a depth texture" I mean sampling the values out of a depth texture bound to
texture_sampled_2d<f32>
. This is not the same as usingtexture_depth_2d
which is for comparison sampling.The text was updated successfully, but these errors were encountered: