feat(core): Expose descriptor validation separately from create_texture - #9979
Conversation
6e19a1a to
6bb782a
Compare
sagudev
left a comment
sagudev
left a comment
There was a problem hiding this comment.
Yes please! In servo we used dummy texture for this rn: https://github.com/servo/servo/blob/86465d626e3404147ef80e9df548602ee1eb06ec/components/webgpu/wgpu_thread.rs#L502
| self: &Arc<Self>, | ||
| desc: &resource::TextureDescriptor, | ||
| ) -> Result<Arc<Texture>, resource::CreateTextureError> { | ||
| self.check_is_valid()?; |
There was a problem hiding this comment.
Why is this not done in validate_texture_descriptor_inner?
There was a problem hiding this comment.
I considered it, but decided that the device not being valid is independent of validating the texture descriptor. (I didn't see any use of the device by validate_texture_descriptor other than immutable capability lookup.) Although I also don't see check_is_valid in create_texture_from_hal_inner, we should probably add that. And I'm not opposed to adding the device validity check to the descriptor validation.
There was a problem hiding this comment.
We would need check_is_valid in validate_texture_descriptor for servo and I think this also makes more sense conceptually as device.validate_texture_descriptor actually validates tex desc against device.
There was a problem hiding this comment.
I would tend to agree with @sagudev, because the validation asks, "If I create a texture at this point in time, is this valid?" If the device is invalid, then the answer is, "Definitely." forever onward.
If device validity were intermittent, I could see an argument for keeping these separate, but a device never becomes valid again, so it doesn't seem harmful to report that issue earlier than when creation may actually happen.
Although I also don't see
check_is_validincreate_texture_from_hal_inner, we should probably add that. And I'm not opposed to adding the device validity check to the descriptor validation.
Yes please!
There was a problem hiding this comment.
LGTM, modulo resolution @sagudev's point about device validity (which I'm trusting you to resolve before merging).
|
|
||
| - Support the `wasm64-unknown-unknown` target for the web backend. Building for wasm64 requires a nightly toolchain with `-Z build-std=std,panic_abort`. By @nickbabcock in [#9836](https://github.com/gfx-rs/wgpu/pull/9836). | ||
| - `wgpu-core` now exposes a `validate_device_descriptor` function that validates a device descriptor as `request_device` would. This may be useful in conjunction with `create_device_from_hal`. By @andyleiserson in [#9967](https://github.com/gfx-rs/wgpu/pull/9967). | ||
| - `wgpu-core` now exposes `validate_device_descriptor` and `validate_texture_descriptor` functions that perform the same descriptor validation the corresponding resource creation APIs would, without actually creating a resource. This may be useful in conjunction with hal raw APIs. By @andyleiserson in [#9967](https://github.com/gfx-rs/wgpu/pull/9967) and [#9979](https://github.com/gfx-rs/wgpu/pull/9979). |
There was a problem hiding this comment.
thought: We intend to expand this list to other resources, too, right? Sounds like a multi-line list would be easier to review than an inline comma-separated list in prose going forward.
ErichDonGubler
added
area: validation
- Move from `create_texture_inner` to `validate_texture_descriptor_inner` - Add check in `create_texture_from_hal_inner`
12a9fd6 to
0670ad2
Compare
3 participants
Similar to #9967, but for textures this time.
Testing
Simple refactor.
Squash or Rebase? Squash
Checklist
CHANGELOG.mdentries for the user-facing effects of this change are present.