WGSL: allow bgra8unorm as texel format #3640
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch intends to add `bgra8unorm` as a valid texel format when the extension `bgra8unorm_storage` is enabled. `bgra8unorm_storage` corresponds to the WebGPU extension `bgra8unorm-storage`. Note that because in SPIRV there is no `bgra8` image format, the texel format `bgra8unorm` should be translated into `rgba8` in SPIRV, and in Vulkan backend an image with format rgba8unorm and component swizzling (`r` to `b` and `b` to `r`) will be bound to the descriptor set.
|
Previews, as seen when this build job started (23f55ba): |
VkImageView component mapping is not allowed for storage usage. However something similar should be possible to do with format reinterpretation (of a bgra8unorm image to rgba8unorm) and shader instrumentation to do the component mapping there as well. LGTM otherwise but we need to discuss it in the group (and check experimentally that the double component mapping works.) |
|
It's useful to know whether this feature is implementable on Vulkan, but it isn't designed to necessarily be implementable on Vulkan or D3D12. On those I believe we anticipate a lesser penalty for using rgba8unorm for canvases.
IIRC my "sometimes on Vulkan" statement was informed by gpuinfo reports. Here's the tail of a gpuinfo-vulkan-query output with the added requirement below. A ton of devices are lost, but a lot of AMD, Intel, and NVIDIA devices claim support. But if the spec doesn't allow it, maybe these are spurious results? add_rq('bgra8unorm storage', lambda info:
format_supported_with_optimal_tiling_features(info.fmts, vk.Format.B8G8R8A8_UNORM, vk.FormatFeature.STORAGE_IMAGE) or
format_supported_with_linear_tiling_features(info.fmts, vk.Format.B8G8R8A8_UNORM, vk.FormatFeature.STORAGE_IMAGE))gpuinfo-vulkan-query output |
Actually the support of using D3D12_FEATURE_DATA_FORMAT_SUPPORT formatInfo = {};
formatInfo.Format = DXGI_FORMAT_B8G8R8A8_UNORM;
device->CheckFeatureSupport(D3D12_FEATURE_FORMAT_SUPPORT, &formatInfo, sizeof(formatInfo))));
if (formatInfo.Support1 & D3D12_FORMAT_SUPPORT1_TYPED_UNORDERED_ACCESS_VIEW) {
printf("DXGI_FORMAT_B8G8R8A8_UNORM supports UAV on this GPU.");
}The usage of I've verified |
|
Interesting. I was only checking FL11_1. I should have checked FL12_1, but even on FL12_1 this is listed as "disallowed or not available": |
|
@RafaelCintron could you help double-check if my understanding in #3640 (comment) is correct? |
WGSL 2022-11-29 Minutes
|
dneto0
reviewed
Dec 6, 2022
The feature should be decided by the API folks.
|
The API spec already has this feature, so what we need to figure out is whether this is actually a gap that prevents it from being implemented.
|
|
If we're really not sure, we technically could go ahead and add this, under the assumption we might eventually need it, and then lift restrictions later if we can. The reason I'm hesitant to do this is, no equivalent exists in any other language. Cross compilers from HLSL/GLSL/SPIR-V/etc. would be out of luck without out-of-band information that they should pass bgra8unorm here instead of rgba8unorm. |
|
I just found that in Jiawei and Corentin's discussion they discovered the same constraints of Vulkan. Link for the record: |
|
I've just submitted the PR to allow PTAL, thanks! |
kainino0x
reviewed
Dec 7, 2022
wgsl/index.bs
Outdated
| @@ -3664,6 +3664,8 @@ which support the [[WebGPU#dom-gputextureusage-storage|WebGPU STORAGE]] usage. | |||
| These texel formats are used to parameterize the storage texture types defined | |||
| in [[#texture-storage]]. | |||
|
|
|||
| Note that `bgra8unorm` can only be used when the extension `bgra8unorm_storage` is enabled. | |||
There was a problem hiding this comment.
If we do go with this, this section needs to list the new WGSL extension (like the shader-f16 feature above it):
https://gpuweb.github.io/gpuweb/#bgra8unorm-storage
There was a problem hiding this comment.
We discussed this in the WGSL meeting on 2022-12-13.
The WGSL feature should not be guarded by an "enable". Please remove this sentence.
@Jiawei-Shao If that works, I'm in favor of it. But we do want to make sure it will work on Vulkan (or decide this extension won't target Vulkan). I'd like to understand how that format feature flag can be used. Actually, reading the proposed implementation after having dug into the Vulkan spec for a while, I bet format reinterpretation is how that flag can be used (that reinterpretation is valid). We need to find out whether we need to inject a swizzle into the shader:
If swizzling is required, we need to know the format at code generation time, which means we need the format in the shader, otherwise we can't generate code until pipeline creation (when we have the bind group layout). However if swizzling is not required, then I think #3667 is valid.
Unfortunately it sounds like format reinterpretation only reinterprets the bits, which would mean swizzling is required. I wonder if it's possible to just make the WebGPU application responsible for doing the reinterpretation, when it creates a texture view. This might be possible as long as all backends have the same behavior (bit reinterpretation with no swizzle):
|
I have an idea about this. It might mean that you can use it as a UAV but only when reinterpreted as an R32 format. There's a special carveout for this: |
WGSL 2022-12-06 Minutes
|
|
The discussion WGSL seems to have assumed that using texture view swizzling with storage textures is possible. But actually it isn't allowed. So the way to implement bgra8-unorm storage in Vulkan is to make storage bgra8unorm textures have viewFormats rgba8unorm, then creating rgba8unorm views when binding as storage. Finally in the shader I'm almost certains we'll need to add a swizzle when reading and writing to the texture (because storage textures are just using memory loads and not the texture units). The |
|
Regarding BGRA UNORM support in D3D12, it was added along with an iteration of relaxing format casting ( This sounds like we probably need to update some docs. I'll ping our doc author to get those tables updated with an additional footnote. |
|
That straightforwardly resolves the D3D12 mystery. Thanks! On the call @jenatali said D3D12 probably swizzles (when binding BGRA, the vec4 will be in the order RGBA), but he'll double check on this. This leaves the Vulkan mystery, which Google will try to find an answer to. |
It is true according to the test results in my patch to support |
|
Yes, D3D swizzles. I've also asked the doc team to update the format table docs, indicating that these are optionally supported by D3D12 (but not D3D11). |
|
Hi Myles!
We want to be able to generate any necessary swizzling code (Vulkan) or packing code (D3D12) at shader compilation time, so just having the info at pipeline creation time isn't enough.
This idea is very interesting. I'm not sure if it was discussed in the WGSL meeting where I wasn't present. I have a few concerns about it, though they don't necessarily block the idea.
|
kainino0x
reviewed
Dec 15, 2022
There was a problem hiding this comment.
Review comment: If we want to make this available in core we'll also need to remove the bgra8unorm-storage feature from the API spec. I think it should be in the same PR (though technically this PR is valid as is, it just implements a slightly different proposal)
Done |
kainino0x
approved these changes
Dec 15, 2022
|
LGTM but we can give other folks a chance to see Myles's comments before landing, just in case it inspires reconsideration (Also David still needs to re-review, officially) |
|
@litherum I also intended to inquire about this approach in one of previous meetings (not the most recent one) but couldn't articulate well. #3640 (comment) |
Aha! Early compilation strikes again! 😅😅😅 |
|
PTAL, thanks! |
|
I don't think this needs to wait, I'm just going to land it |
|
@litherum is it the case that I previously asked about these kind of discrepancies between the 2 tables in the pdf in the WebGPU matrix room (link to message). |
|
Regarding Vulkan, I think the list of formats in the "Formats without shader storage format" section wants to convey something else; and not restrict usage of @alan-baker thoughts? |
Was this suggestion discussed? |
Texture buffers are different than textures, and have different capabilities. |
WGSL 2023-01-10 Minutes
|
|
For the record @teoxoy has been discussing this here, it turns out there's some Vulkan support? I didn't read through the issue yet. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.