WebGPU: Match WGSL syntax for texture component swizzle [chromium/src : main]
Fr (Gerrit)
Fr has uploaded the change for review![]( "Open in Gerrit")
Commit message
WebGPU: Match WGSL syntax for texture component swizzle
Change diff
Change information
- M third_party/blink/renderer/bindings/generated_in_modules.gni
- M third_party/blink/renderer/modules/webgpu/dawn_enum_conversions.cc
- M third_party/blink/renderer/modules/webgpu/dawn_enum_conversions.h
- M third_party/blink/renderer/modules/webgpu/gpu_texture.cc
- M third_party/blink/renderer/modules/webgpu/gpu_texture_view_descriptor.idl
- M third_party/dawn
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fr (Gerrit)
Fr added 1 comment![]( "Open in Gerrit")
Related details
- Kai Ninomiya
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kai Ninomiya (Gerrit)
Kai Ninomiya added 3 comments![]( "Open in Gerrit")
wgpu::ComponentSwizzle AsDawnEnum(const UChar& c) {nit: UChar is a (small) pure value, so it should be passed by-value instead of by-reference
const auto& swizzle = webgpu_desc->swizzle();What does this return if WebGPUExperimentalFeatures is disabled but the user passes a swizzle? I think the bindings ignore the user's request and just pass us `"rgba"`? In which case below could be just:
```
if (swizzle != "rgba") {
```
(If I'm wrong, then I think https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/webgpu/gpu_bind_group_layout.cc;l=33;drc=e7bd1b16aeb1d52e013ab5d04b4e607fdc2978b4 is also wrong)
if (kAllowedSwizzleChars.find(static_cast<char>(swizzle[i])) ==This is a lossy cast and I believe it will allow not only `'r' = 0x72` but also `'ɲ' = 0x0272`, etc.
How about changing `AsDawnEnum`'s `default` and use:
`if (AsDawnEnum(swizzle[i]) == wgpu::ComponentSwizzle::Undefined))`
That way we only have to worry about one place understanding the strings correctly.
This is probably worth having CTS coverage for.
Related details
- Fr
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fr (Gerrit)
Fr added 3 comments![]( "Open in Gerrit")
nit: UChar is a (small) pure value, so it should be passed by-value instead of by-reference
Done
What does this return if WebGPUExperimentalFeatures is disabled but the user passes a swizzle? I think the bindings ignore the user's request and just pass us `"rgba"`? In which case below could be just:
```
if (swizzle != "rgba") {
```(If I'm wrong, then I think https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/webgpu/gpu_bind_group_layout.cc;l=33;drc=e7bd1b16aeb1d52e013ab5d04b4e607fdc2978b4 is also wrong)
You're right! Let's simplify this code.
if (kAllowedSwizzleChars.find(static_cast<char>(swizzle[i])) ==This is a lossy cast and I believe it will allow not only `'r' = 0x72` but also `'ɲ' = 0x0272`, etc.
How about changing `AsDawnEnum`'s `default` and use:
`if (AsDawnEnum(swizzle[i]) == wgpu::ComponentSwizzle::Undefined))`
That way we only have to worry about one place understanding the strings correctly.This is probably worth having CTS coverage for.
Good catch for the lossy cast! I've used your suggestion and will update the CTS with your suggestion: https://github.com/gpuweb/cts/pull/4427#discussion_r2366955799
Related details
- Kai Ninomiya
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kai Ninomiya (Gerrit)
- Fr
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fr (Gerrit)
Fr voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
WebGPU: Match WGSL syntax for texture component swizzle
- M third_party/blink/renderer/bindings/generated_in_modules.gni
- M third_party/blink/renderer/modules/webgpu/dawn_enum_conversions.cc
- M third_party/blink/renderer/modules/webgpu/dawn_enum_conversions.h
- M third_party/blink/renderer/modules/webgpu/gpu_texture.cc
- M third_party/blink/renderer/modules/webgpu/gpu_texture_view_descriptor.idl
Code-Review: +1 by Kai Ninomiya
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fr (Gerrit)
Fr has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Corentin Wallez (Gerrit)
Corentin Wallez added 1 comment![]( "Open in Gerrit")
return false;This should have been true to keep returning textures when the feature is not enabled.
Related details
- Fr
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fr (Gerrit)
Fr added 1 comment![]( "Open in Gerrit")
return false;This should have been true to keep returning textures when the feature is not enabled.
I've opened https://chromium-review.googlesource.com/c/chromium/src/+/6983268 to fix it.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |