WebGPU: Implement chromium-experimental-sampling-resource-table [chromium/src : main]
Corentin Wallez (Gerrit)
Corentin Wallez has uploaded the change for review![]( "Open in Gerrit")
Commit message
WebGPU: Implement chromium-experimental-sampling-resource-table
Change diff
Change information
- M gpu/command_buffer/service/webgpu_decoder_impl.cc
- M third_party/blink/renderer/bindings/generated_in_modules.gni
- M third_party/blink/renderer/bindings/idl_in_modules.gni
- M third_party/blink/renderer/modules/webgpu/BUILD.gn
- M third_party/blink/renderer/modules/webgpu/dawn_enum_conversions.cc
- M third_party/blink/renderer/modules/webgpu/gpu_command_encoder.cc
- M third_party/blink/renderer/modules/webgpu/gpu_compute_pass_encoder.cc
- M third_party/blink/renderer/modules/webgpu/gpu_compute_pass_encoder.h
- M third_party/blink/renderer/modules/webgpu/gpu_compute_pass_encoder.idl
- M third_party/blink/renderer/modules/webgpu/gpu_device.cc
- M third_party/blink/renderer/modules/webgpu/gpu_device.h
- M third_party/blink/renderer/modules/webgpu/gpu_device.idl
- M third_party/blink/renderer/modules/webgpu/gpu_pipeline_layout.cc
- M third_party/blink/renderer/modules/webgpu/gpu_pipeline_layout_descriptor.idl
- M third_party/blink/renderer/modules/webgpu/gpu_render_pass_descriptor.idl
- A third_party/blink/renderer/modules/webgpu/gpu_resource_table.cc
- A third_party/blink/renderer/modules/webgpu/gpu_resource_table.h
- A third_party/blink/renderer/modules/webgpu/gpu_resource_table.idl
- M third_party/blink/renderer/modules/webgpu/gpu_supported_features.cc
- M third_party/blink/renderer/modules/webgpu/gpu_supported_features.idl
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Loko Kung (Gerrit)
Loko Kung voted and added 2 comments![]( "Open in Gerrit")
Votes added by Loko Kung
| Code-Review | +1 |
2 comments
// TODO(https://crbug.com/435317394): The extension struct exposed by webgpu.hnit. Use the*
Related details
- Corentin Wallez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fr (Gerrit)
Fr added 5 comments![]( "Open in Gerrit")
[RuntimeEnabled=WebGPUExperimentalResourceTable] void setResourceTable(GPUResourceTable table);Proposal says `setResourceTable(GPUResourceTable? table)` so that the resource table can be unset by passing null to setResourceTable. Is that intended?
exception_state.ThrowTypeError(Shall we have a TODO to address this?
wgpu::BindingResource dawn_resource = AsDawnType(resource, exception_state);Throwing an exception on ExceptionState does not abort C++ execution, you should have something like:
```cpp
wgpu::BindingResource dawn_resource = AsDawnType(resource, exception_state);
if (exception_state.HadException()) {
return;
}
```
Same for `insert()` below
[RaisesException] void update(GPUIndex32 slot, GPUBindingResource resource);
[RaisesException] GPUIndex32 insert(GPUBindingResource resource);
[RaisesException] void remove(GPUIndex32 slot);
Use `undefined` not `void` for consistency with other WebGPU methods.
Corentin Wallez (Gerrit)
Corentin Wallez added 7 comments![]( "Open in Gerrit")
// TODO(https://crbug.com/435317394): The extension struct exposed by webgpu.hCorentin Walleznit. Use the*
Done
[RuntimeEnabled=WebGPUExperimentalResourceTable] void setResourceTable(GPUResourceTable table);Proposal says `setResourceTable(GPUResourceTable? table)` so that the resource table can be unset by passing null to setResourceTable. Is that intended?
It's a leftover from a previous version of the proposal that I forgot to fix. I guess we can keep like this and see later fixed to handle null in the implementation though.
Corentin WallezNit: remove empty line
Done
// Copyright 2019 The Chromium AuthorsCorentin Walleznit. 2026
Done
exception_state.ThrowTypeError(Shall we have a TODO to address this?
We are likely never going to add external textures to resource tables. Because they are composite bindings they are too difficult to handle.
wgpu::BindingResource dawn_resource = AsDawnType(resource, exception_state);Throwing an exception on ExceptionState does not abort C++ execution, you should have something like:
```cpp
wgpu::BindingResource dawn_resource = AsDawnType(resource, exception_state);
if (exception_state.HadException()) {
return;
}
```Same for `insert()` below
Done
[RaisesException] void update(GPUIndex32 slot, GPUBindingResource resource);
[RaisesException] GPUIndex32 insert(GPUBindingResource resource);
[RaisesException] void remove(GPUIndex32 slot);
Use `undefined` not `void` for consistency with other WebGPU methods.
Looking at `gpu_buffer.idl` and `gpu_compute_pass_encoder.idl` we seem to use `void` consistently in Blink so keeping as is.
Related details
- Fr
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Corentin Wallez (Gerrit)
Corentin Wallez voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Fr
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fr (Gerrit)
Fr voted and added 3 comments![]( "Open in Gerrit")
Votes added by Fr
| Code-Review | +1 |
3 comments
[RuntimeEnabled=WebGPUExperimentalResourceTable] void setResourceTable(GPUResourceTable table);Corentin WallezProposal says `setResourceTable(GPUResourceTable? table)` so that the resource table can be unset by passing null to setResourceTable. Is that intended?
It's a leftover from a previous version of the proposal that I forgot to fix. I guess we can keep like this and see later fixed to handle null in the implementation though.
It seems like you now check `nullptr` in `GPUComputePassEncoder::setResourceTable`. Why not in the IDL then?
[RaisesException] void update(GPUIndex32 slot, GPUBindingResource resource);
[RaisesException] GPUIndex32 insert(GPUBindingResource resource);
[RaisesException] void remove(GPUIndex32 slot);
Corentin WallezUse `undefined` not `void` for consistency with other WebGPU methods.
Looking at `gpu_buffer.idl` and `gpu_compute_pass_encoder.idl` we seem to use `void` consistently in Blink so keeping as is.
- Corentin Wallez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Corentin Wallez (Gerrit)
Corentin Wallez voted and added 2 comments![]( "Open in Gerrit")
Votes added by Corentin Wallez
| Commit-Queue | +1 |
2 comments
[RuntimeEnabled=WebGPUExperimentalResourceTable] void setResourceTable(GPUResourceTable table);Corentin WallezProposal says `setResourceTable(GPUResourceTable? table)` so that the resource table can be unset by passing null to setResourceTable. Is that intended?
FrIt's a leftover from a previous version of the proposal that I forgot to fix. I guess we can keep like this and see later fixed to handle null in the implementation though.
It seems like you now check `nullptr` in `GPUComputePassEncoder::setResourceTable`. Why not in the IDL then?
The code checks it to call `SetResourceTable(nullptr)` which is actually supported in Dawn at the moment and unsets the resource table for the rest of the compute encoder, so it's handling both cases of the `?` or am I missing something?
[RaisesException] void update(GPUIndex32 slot, GPUBindingResource resource);
[RaisesException] GPUIndex32 insert(GPUBindingResource resource);
[RaisesException] void remove(GPUIndex32 slot);
Corentin WallezUse `undefined` not `void` for consistency with other WebGPU methods.
FrLooking at `gpu_buffer.idl` and `gpu_compute_pass_encoder.idl` we seem to use `void` consistently in Blink so keeping as is.
In that case, update `undefined destroy` to `void destroy` above
- Fr
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fr (Gerrit)
Fr voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fr
| Code-Review | +1 |
1 comment
GPUResourceTable* GPUDevice::createResourceTable(I've just realized we were missing this check:
> - if desc.size > 65536 throw a RangeError and return.
Related details
- Corentin Wallez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fr (Gerrit)
Fr added 1 comment![]( "Open in Gerrit")
[RuntimeEnabled=WebGPUExperimentalResourceTable] void setResourceTable(GPUResourceTable table);Corentin WallezProposal says `setResourceTable(GPUResourceTable? table)` so that the resource table can be unset by passing null to setResourceTable. Is that intended?
FrIt's a leftover from a previous version of the proposal that I forgot to fix. I guess we can keep like this and see later fixed to handle null in the implementation though.
Corentin WallezIt seems like you now check `nullptr` in `GPUComputePassEncoder::setResourceTable`. Why not in the IDL then?
The code checks it to call `SetResourceTable(nullptr)` which is actually supported in Dawn at the moment and unsets the resource table for the rest of the compute encoder, so it's handling both cases of the `?` or am I missing something?
Why not applying this change then?
```suggestion
[RuntimeEnabled=WebGPUExperimentalResourceTable] void setResourceTable(GPUResourceTable? table);
```
Corentin Wallez (Gerrit)
Corentin Wallez added 1 comment![]( "Open in Gerrit")
[RuntimeEnabled=WebGPUExperimentalResourceTable] void setResourceTable(GPUResourceTable table);Corentin WallezProposal says `setResourceTable(GPUResourceTable? table)` so that the resource table can be unset by passing null to setResourceTable. Is that intended?
FrIt's a leftover from a previous version of the proposal that I forgot to fix. I guess we can keep like this and see later fixed to handle null in the implementation though.
Corentin WallezIt seems like you now check `nullptr` in `GPUComputePassEncoder::setResourceTable`. Why not in the IDL then?
FrThe code checks it to call `SetResourceTable(nullptr)` which is actually supported in Dawn at the moment and unsets the resource table for the rest of the compute encoder, so it's handling both cases of the `?` or am I missing something?
Why not applying this change then?
```suggestion
[RuntimeEnabled=WebGPUExperimentalResourceTable] void setResourceTable(GPUResourceTable? table);
```
- Fr
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Corentin Wallez (Gerrit)
Corentin Wallez voted and added 2 comments![]( "Open in Gerrit")
Votes added by Corentin Wallez
| Commit-Queue | +1 |
2 comments
[RuntimeEnabled=WebGPUExperimentalResourceTable] void setResourceTable(GPUResourceTable table);Corentin WallezProposal says `setResourceTable(GPUResourceTable? table)` so that the resource table can be unset by passing null to setResourceTable. Is that intended?
FrIt's a leftover from a previous version of the proposal that I forgot to fix. I guess we can keep like this and see later fixed to handle null in the implementation though.
Corentin WallezIt seems like you now check `nullptr` in `GPUComputePassEncoder::setResourceTable`. Why not in the IDL then?
FrThe code checks it to call `SetResourceTable(nullptr)` which is actually supported in Dawn at the moment and unsets the resource table for the rest of the compute encoder, so it's handling both cases of the `?` or am I missing something?
Corentin WallezWhy not applying this change then?
```suggestion
[RuntimeEnabled=WebGPUExperimentalResourceTable] void setResourceTable(GPUResourceTable? table);
```
I clearly need to wake up still ^^
Done
I've just realized we were missing this check:
> - if desc.size > 65536 throw a RangeError and return.
Good catch! Done
Related details
- Fr
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Corentin Wallez (Gerrit)
Corentin Wallez voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Fr (Gerrit)
Fr voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fr
| Code-Review | +1 |
1 comment
FYI I think you'll need to global-interface-listing wpt tests as you're adding a new method in IDL
Related details
- Corentin Wallez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fr (Gerrit)
Fr voted Code-Review+1![]( "Open in Gerrit")
- Corentin Wallez
- Loko Kung
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Corentin Wallez (Gerrit)
Corentin Wallez voted Commit-Queue+2![]( "Open in Gerrit")
- Loko Kung
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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: Implement chromium-experimental-sampling-resource-table
- M android_webview/test/data/web_tests/webexposed/global-interface-listing-expected.txt
- M gpu/command_buffer/service/webgpu_decoder_impl.cc
- M third_party/blink/renderer/bindings/generated_in_modules.gni
- M third_party/blink/renderer/bindings/idl_in_modules.gni
- M third_party/blink/renderer/modules/webgpu/BUILD.gn
- M third_party/blink/renderer/modules/webgpu/dawn_enum_conversions.cc
- M third_party/blink/renderer/modules/webgpu/gpu_command_encoder.cc
- M third_party/blink/renderer/modules/webgpu/gpu_compute_pass_encoder.cc
- M third_party/blink/renderer/modules/webgpu/gpu_compute_pass_encoder.h
- M third_party/blink/renderer/modules/webgpu/gpu_compute_pass_encoder.idl
- M third_party/blink/renderer/modules/webgpu/gpu_device.cc
- M third_party/blink/renderer/modules/webgpu/gpu_device.h
- M third_party/blink/renderer/modules/webgpu/gpu_device.idl
- M third_party/blink/renderer/modules/webgpu/gpu_pipeline_layout.cc
- M third_party/blink/renderer/modules/webgpu/gpu_pipeline_layout_descriptor.idl
- M third_party/blink/renderer/modules/webgpu/gpu_render_pass_descriptor.idl
- A third_party/blink/renderer/modules/webgpu/gpu_resource_table.cc
- A third_party/blink/renderer/modules/webgpu/gpu_resource_table.h
- A third_party/blink/renderer/modules/webgpu/gpu_resource_table.idl
- M third_party/blink/renderer/modules/webgpu/gpu_supported_features.cc
- M third_party/blink/renderer/modules/webgpu/gpu_supported_features.idl
- M third_party/blink/web_tests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
- M third_party/blink/web_tests/webexposed/global-interface-listing-dedicated-worker-expected.txt
- M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
- M third_party/blink/web_tests/webexposed/global-interface-listing-shared-worker-expected.txt
Code-Review: +1 by Fr
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |