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

Texture bindings with no sampler usage in WGSL should require float-unfilterable, not float, on derived bind group layouts #6527

Closed
ErichDonGubler opened this issue Nov 12, 2024 · 4 comments · Fixed by #6531
Assignees
Labels
area: correctness We're behaving incorrectly type: bug Something isn't working

Comments

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Nov 12, 2024

When a sampled texture binding is provided in WGSL, attempting to use the binding with a texture format that only accepts non-filterable floats (i.e., depth24plus):

  • JavaScript
     const adapter = await navigator.gpu.requestAdapter() 
     const device = await adapter.requestDevice({ requiredFeatures: ["timestamp-query"] }) 
    
     const colorAttachmentTexture = device.createTexture({
       format: "rgba8unorm", size: [1, 1], 
       usage: GPUTextureUsage.RENDER_ATTACHMENT,
     })
     const colorAttachmentView = colorAttachmentTexture.createView()
    
     const texture = device.createTexture({ format: "depth24plus", size: [1, 1], usage: GPUTextureUsage.TEXTURE_BINDING })
     const view = texture.createView()
    
     const module = device.createShaderModule({
       code: `
     @group(0) @binding(0)
     var letMeUseNonFilterableFloatPlease: texture_2d<f32>;
    
     // Perfunctory vertex shader.
     @vertex
     fn vs_main(
       @builtin(vertex_index) idx: u32
     ) -> @builtin(position) vec4f {
       const pos: array<f32, 3> = array(-1.0, 0.0, 1.0);
       // Consume the texture, so its binding slot is valid, but don't use a sampler.
       let numLevels = textureNumLevels(letMeUseNonFilterableFloatPlease);
       return vec4(pos[idx] * f32(numLevels), 1.0, 1.0, 0.0);
     }
    
     // Perfunctory fragment shader.
     @fragment
     fn fs_main() -> @location(0) vec4f {
       return vec4(1.);
     }
     `
     })
    
     const renderPipeline = device.createRenderPipeline({
       vertex: {
     	  module, 
       },
       fragment: {
     	module,
     	targets: [
     	  {
     		format: "rgba8unorm",    
     	  },
     	],
       },
       layout: 'auto',
     })
    
     const bindGroup = device.createBindGroup({
       layout: renderPipeline.getBindGroupLayout(0),
       entries: [
     	{
     		binding: 0,
     		resource: view,
     	},
     	],
     })
    
     const encoder = device.createCommandEncoder()
     const renderPass = encoder.beginRenderPass({
       colorAttachments: [
     	{
     		view: colorAttachmentView,
     	  loadOp: "clear",
     	  storeOp: "store",
     	}
       ],
     })
     renderPass.setPipeline(renderPipeline)
     renderPass.setBindGroup(0, bindGroup)
     renderPass.draw(3)
     renderPass.end()
     device.queue.submit([encoder.finish()])
  • Rust
     // TODO: Will convert to Rust source code on request!

…errors with:

Texture binding 0 expects sample type = Float { filterable: true }, but given a view with format = Depth24Plus

…but the WebGPU spec. states in the algorithm for default bind group layout1 in the section titled Default pipeline layout (section 10.1.1, step 4.9.2, Else branch):

To create a default pipeline layout for GPUPipelineBase pipeline, run the following device timeline steps:

  1. For each GPUProgrammableStage stageDesc in the descriptor used to create pipeline:

    1. For each resource resource statically used by entryPoint:

      1. If resource is for a sampled texture binding:

        1. If resource is a depth texture binding:

          Else if the sampled type of resource is:

          • f32 and there exists a static use of resource by stageDesc in a texture builtin function call that also uses a sampler

We need to accept non-filterable floats in cases like these.

Footnotes

  1. N.B. that WGPU calls default bind group layouts "derived bind group layouts".

@ErichDonGubler ErichDonGubler added type: bug Something isn't working area: correctness We're behaving incorrectly labels Nov 12, 2024
@ErichDonGubler ErichDonGubler changed the title Texture bindings with no sampler usage should require float-unfilterable, not float Texture bindings with no sampler usage in WGSL should require float-unfilterable, not float Nov 12, 2024
@ErichDonGubler ErichDonGubler changed the title Texture bindings with no sampler usage in WGSL should require float-unfilterable, not float Texture bindings with no sampler usage in WGSL should require float-unfilterable, not float, on derived bind group layouts Nov 12, 2024
@ErichDonGubler
Copy link
Member Author

From @jimblandy during our triage meeting todya: We might be able to use something like naga::valid::FunctionInfo::sampling{,_set} members to plumb this information upwards, if we don't already do the proper analysis. Then wgpu_core needs to consult it when deriving bind group layouts.

@ErichDonGubler
Copy link
Member Author

Downstream suffering: Firefox bug 1917099, Scthe/nanite-webgpu#6, Scthe/nanite-webgpu#4.

@Scthe
Copy link

Scthe commented Nov 12, 2024

Hi, author of nanite-webgpu (and frostbitten-hair-webgpu) here. This is probably my app's fault, I did what worked on Chrome and decided to wait for full webgpu support in Firefox release before fixing it. More info: Scthe/nanite-webgpu#4 (comment) . I don't know if what's done in my shader is permitted by wgsl spec.

@ErichDonGubler
Copy link
Member Author

ErichDonGubler commented Nov 12, 2024

@Scthe: Actually, I believe your app is in the right here; just populated the OP with full info, including a citation of the WebGPU spec., after @teoxoy and I did some thinking together today. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: correctness We're behaving incorrectly type: bug Something isn't working
Projects
Status: Done
2 participants