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

Separate linear sampler binding type? #1034

Closed
kvark opened this issue Aug 28, 2020 · 18 comments
Closed

Separate linear sampler binding type? #1034

kvark opened this issue Aug 28, 2020 · 18 comments
Assignees
Projects

Comments

@kvark
Copy link
Contributor

kvark commented Aug 28, 2020

Problem

Linear sampling (or "filtering") can only be done with textures that have GPUTextureComponentType.float. The shaders know about the component type of textures, but the shaders don't know if a sampler is filtering or not. So shader translation can't validate this, and outside of the shader, on the API side, we no longer know which textures are used with which samplers.

Solutions

  1. allow the filtering to be done on non-float textures, specify this as "undefined result" or something. It may be unfeasible if there is an UB in the driver/HW, and it's obviously not very portable (but the choice is provided for completeness!).
  2. have the shader translation to generate a map of texture-sampler pairs, which will then be used by the validation at the draw time (!) where all the bindings are known, and we can check the samplers.
  3. introduce a new GPUBindingType::"filtering-sampler" (in addition to regular "sampler" and "comparison-sampler") with the corresponding distinction in WGSL. This would mean:
    - createShaderModule can fail if we see that a filtering sampler is used with an incompatible texture
    - createBindGroupLayout can fail if the given sampler doesn't match the binding
    - no heavy validation at draw time

Details

If we go (3) one concern could be raised about how the translation from other languages, like SPIR-V, would know what type of the sampler to use. To some extent, it fits under the current umbrella of sampler diversity (we have a "comparison" sampler type that SPIR-V doesn't). As proposed in #889, we could generate multiple conflicting bindings for the SPIR-V sampler, and it will all work fine as long as only one of the multiple (regular/comparison/filtering) is used statically for any given entry point.

@kvark kvark added bug proposal wgsl WebGPU Shading Language Issues labels Aug 28, 2020
@kvark kvark added this to Needs Discussion in Main Aug 28, 2020
@Kangz
Copy link
Contributor

Kangz commented Aug 31, 2020

This is unfortunate. Do we have any data on whether 1) is consistent, or even allowed in the various APIs?

@Kangz
Copy link
Contributor

Kangz commented Aug 31, 2020

Also there's 4) in the shader round the texel coordinate before doing the texture() call so that not interpolation happens for integer textures.

@grorg grorg added this to For Next Meeting in WGSL Sep 1, 2020
@grorg
Copy link
Contributor

grorg commented Sep 1, 2020

Discussed at 2020-09-01 meeting.

@kainino0x
Copy link
Contributor

Also there's 4) in the shader round the texel coordinate before doing the texture() call so that not interpolation happens for integer textures.

This only works in cases where it's undefined between (filters, doesn't filter), but not where the result or behavior is actually undefined.

@kainino0x
Copy link
Contributor

From the WGSL call we determined that it isn't strictly necessary to have separate filtering and non-filtering sampler types in WGSL. We can determine whether a sampler is statically used with any int texture and use that to validate against the BGL during pipeline creation.

@kvark
Copy link
Contributor Author

kvark commented Sep 1, 2020

Want to clarify an argument @kainino0x had for not having a separate type in WGSL: generating it from SPIR-V would be difficult. I agree with this argument. The situation with the comparison samplers was difficult, because for each individual sample operation it was clear what kind of image/sampler is required. For linear samplers though, if we see that SPIR-V samples from a floating-point image, we don't know if it's meant to be used with a "linear" sampler or a regular one. So that introduce unnecessary friction.
TL;DR: I agree we can get away without a separate WGSL type, but still have the separate binding type.

@dneto0
Copy link
Contributor

dneto0 commented Sep 1, 2020

@kvark asked how to look up the behaviour for the various native APIs.

For Vulkan, the ability to use linear filtering depends on the kind of command (draw, copy), but mainly delegated to a property of the image format. See the tables in "Required format support" https://www.khronos.org/registry/vulkan/specs/1.1/html/vkspec.html#features-required-format-support

You want to see whether a particular image format (listed by enum) supports the VK_FORMAT_FEATURE_SAMPLED_IMAGE_FILTER_LINEAR_BIT functionality. The pattern I saw was:

  • no integer format (...UINT or ...SINT) format supported linear filtering
  • many ...SNORM and ...UNORM and ...SFLOAT formats supported linear filtering
    • but not all of them. For example, none of the image formats with 32-bit components supported it.

It seemed pretty nuanced.

@kvark
Copy link
Contributor Author

kvark commented Sep 1, 2020

I'm full aware of the VK_FORMAT_FEATURE_SAMPLED_IMAGE_FILTER_LINEAR_BIT, and I've been specifying it for WebGPU in #1027. My question the following: supposing the user does a linear sample of an integer texture. Would they get UB, or would the sampler just behave as if it's nearest?

@dneto0
Copy link
Contributor

dneto0 commented Sep 2, 2020

Since it's clearly out of spec, then I would assume UB.

It seems to me that validation layers would be fully able to complain.

@kvark
Copy link
Contributor Author

kvark commented Sep 2, 2020

@dneto0 wait, does that mean that linear sampling of, say VK_FORMAT_R32_SFLOAT is also UB? This format doesn't have VK_FORMAT_FEATURE_SAMPLED_IMAGE_FILTER_LINEAR_BIT guaranteed either.

@dneto0
Copy link
Contributor

dneto0 commented Sep 8, 2020

@dneto0 wait, does that mean that linear sampling of, say VK_FORMAT_R32_SFLOAT is also UB? This format doesn't have VK_FORMAT_FEATURE_SAMPLED_IMAGE_FILTER_LINEAR_BIT guaranteed either.

Yes, that's the way I read those tables too. Yes, this was suprising to me.

@kvark
Copy link
Contributor Author

kvark commented Sep 8, 2020

Filed KhronosGroup/Vulkan-Docs#1362 to get some clarification from the Vulkan spec.
It seems too harsh for Vulkan to declare this UB, given that no other API considers this to be UB (I think?).

@kvark
Copy link
Contributor Author

kvark commented Sep 14, 2020

So there is still this problem of some formats with Float texture component type can't be legally sampled. In addition to the Vulkan issue, it would be great to hear from @litherum and @damyanp about the guarantees Metal and D3D12 provide, respectively, when linearly sampling from a floating point format that doesn't have the relevant capability on the current platform.

In order to solve that at the API level, we figured (on the call) that we could split the "float" texture component type into "float" and "filtered-float". This would allow the createBindGroup to validate the format for being capable to filter on sampling.

@damyanp
Copy link

damyanp commented Sep 14, 2020

TL;DR: undefined behavior in D3D12

I'm not sure I've completely followed the request here. In HLSL it is not valid to try and sample from an integer texture - so Texture2D, for example will fail to compile if you try and sample from it.

However, if you bind an integer texture to something expecting a float texture, or if you try and sample from one of the non-sampleable formats, then you get undefined behavior.

Details of which formats are sampleable or not can be found here: https://microsoft.github.io/DirectX-Specs/d3d/archive/D3D11_3_FunctionalSpec.htm#19.1.4%20D3D11.3%20Format%20List

@kvark
Copy link
Contributor Author

kvark commented Sep 15, 2020

@damyanp the integer textures situation is rather clear. What is not clear is what happens if you linearly sample a float component texture (like R32F) with linear sampling, where the texture format doesn't have this capability. Is it still total UB?

@damyanp
Copy link

damyanp commented Sep 15, 2020

Yes, UB.

@Kangz
Copy link
Contributor

Kangz commented Sep 15, 2020

And not just undefined value? Damn :(

@kdashg kdashg moved this from Needs Discussion to Needs Specification in Main Sep 15, 2020
@dj2 dj2 removed the wgsl WebGPU Shading Language Issues label Sep 15, 2020
@dj2 dj2 removed this from For Next Meeting in WGSL Sep 15, 2020
@kvark kvark self-assigned this Sep 15, 2020
@kvark
Copy link
Contributor Author

kvark commented Jan 4, 2021

Fixed by #1223

@kvark kvark closed this as completed Jan 4, 2021
@kainino0x kainino0x moved this from Needs Specification to Specification Done in Main Jan 19, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this issue Sep 6, 2022
)

So that remapping after transfer will yield the same mapped
contents. This was already the case for MapWrite, but not for
MapRead, since MapRead may flush buffer updates into the mapping.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Main
Specification Done
Development

Successfully merging a pull request may close this issue.

7 participants