gpuweb / gpuweb

Where the GPU for the Web work happens!
https://webgpu.io
Other
4.82k stars 318 forks source link

Remove the need to hard-code texel format for `texture_storage_2d` in shaders #4298

Open rokcej opened 1 year ago

rokcej commented 1 year ago

When I'm using texture_storage_2d, I'm forced to hard-code the texel format (e.g. rgba8unorm) in the shader. Here's an example from my compute shader:

@group(0) @binding(8) var uRadiance: texture_storage_2d<rgba8unorm, write>;

Hard-coding rgba8unorm seems super redundant, since vec4<f32> is used when writing to the texture for several different formats. Why can't I just write something like texture_storage_2d<f32, write>, similar to normal texture bindings?

Here's the comment from @mehmetoguzderin on WebGPU's Matrix chat:

The bottleneck here is specific devices in Vulkan, where shaderStorageImageReadWithoutFormat and/or shaderStorageImageWriteWithoutFormat are necessary (that's why you see the more generic pattern in HLSL and Metal instead of GLSL's more specific format setting), even though SPIR-V supports the more generic typing. Although the support seems to be quite widespread in the latest drivers, this still needs consideration within the committee. If I am not missing an open issue in GitHub, this topic is not tracked right now, so maybe you can create an issue (or I can help with creating one).

shaderStorageImageReadWithoutFormat on Android: http://vulkan.gpuinfo.org/listdevicescoverage.php?feature=shaderStorageImageReadWithoutFormat&platform=android

shaderStorageImageWriteWithoutFormat on Android: http://vulkan.gpuinfo.org/listdevicescoverage.php?feature=shaderStorageImageWriteWithoutFormat&platform=android

mehmetoguzderin commented 1 year ago

In case I didn't forget anything while writing the comment, another consideration is how to go about qualifying f32 (unorm or snorm). I've run https://github.com/kainino0x/gpuinfo-vulkan-query by @kainino0x against these features and it probably needs to be an extension since shaderStorageImageReadWithoutFormat loses quite an amount of old devices (or emulated devices), whereas shaderStorageImageWriteWithoutFormat loses just an enumerable amount (shaderStorageImageWriteWithoutFormat after shaderStorageImageReadWithoutFormat does not lose any additional devices):

Requirement "shaderStorageImageWriteWithoutFormat" loses 3 (and partially loses 0) further deviceNames:
  In ALL reports (3 deviceNames):
    x PowerVR Rogue GE8300: 1 (5712)
    x PowerVR Rogue GE8320: 1 (7432)
    x PowerVR Rogue GX6250: 1 (3779)
  In SOME reports (0 deviceNames):
Requirement "shaderStorageImageReadWithoutFormat" loses 218 (and partially loses 7) further deviceNames: ...

Python snippet:

    add_rq('shaderStorageImageWriteWithoutFormat',
           lambda info: 'shaderStorageImageWriteWithoutFormat' in info.features)

    add_rq('shaderStorageImageReadWithoutFormat',
           lambda info: 'shaderStorageImageReadWithoutFormat' in info.features)

Full report as file is in following attachment: result-20230921-012253.txt

kainino0x commented 1 year ago

It's very interesting that shaderStorageImageWriteWithoutFormat seems to be basically ubiquitous. Last time we checked I don't think we had the analysis script, so we couldn't see that all of the devices that didn't have it would be unsupported anyway. Also it was 3.5 years ago before we dropped support for some old devices: https://github.com/gpuweb/gpuweb/pull/532#issuecomment-577435144

We should consider adding that to core WebGPU.

kainino0x commented 1 year ago

Nominating that change for milestone 1.

dneto0 commented 11 months ago

@alan-baker observed it would be annoying/bad to have the feature depend on the access mode. We now have read-only, read-write storage textures too. The read-side feature, shaderStorageImageReadWithoutFormat is much much less supported. Per the report, it loses 218 kinds of Vulkan devices cutting across Qualcomm, NVIDIA, Intel, AMD, Arm.

teoxoy commented 11 months ago

I was in process of pointing out the same thing, that this can only be added to core for write-only storage textures which could be seen as odd by users.

kainino0x commented 11 months ago

I think it's very worthwhile even if it can't be used with read-only/read-write. It can avoid unnecessary pipeline creation and reduces development friction since it's easier to do things like change the storage texture format. Syntactically it's pretty clean:

texture_storage_2d<rgba8unorm, write>
texture_storage_2d<rgba8unorm, read_write>
texture_storage_2d<f32, write>
texture_storage_2d<f32, read_write> // invalid
kdashg commented 11 months ago
WGSL 2023-11-28 Minutes * KG: For write-only storage textures common to not need the format. For read and read-write usages you do need the format, commonly. * DN: Sad to tie it to access mode. * TT: Most people will only use the write-only, so might be worthwhile still. * KG: what about “any”. * AB: Still need the shader component type in there, like for sampled types. Split it three ways: i32, u32, f32. * KG: Push to M2? * AB: Usefulness depends what you’re used to. HLSL never does it. GLSL does. But your engine might not be configured to plumb it through. Feels like a quality-of-life thing only. * JB: Kai points out opportunity to reduce # pipeline combinations. * KG: Think of this as “proposals considered”. * AB: Feature could cover the readonly. * DN: There are aesthetic concerns. * DN: Fingerprinting concern? * TT: Read without format is very uncommon. Think we could make write-without-format core. Only lose 3 PowerVR devices. * KG: Think the fingerprinting aspect: the feature is likely to align with other bundles of features. Are those PowerVR devices popular? * KG: **Put this aside for now**, **push to M2**. Group will not actively try to solve it in the M1 timeframe.
brendan-duncan commented 5 months ago

Adding my vote to this issue, it is a common source of WepGPU specific errors with Unity compute shaders that other supported platforms we support don't have.