707: Implement Bounds Checking on Buffers to Buffer and Texture Copies r=kvark a=cwfitzgerald
**Connections**
Closes#362, closes#554
This addresses issues found by @gretchenfrage when accidentally writing off the end of a texture causing a DEVICE_LOST error.
**Description**
Adds bounds checks to write_texture and write_buffer, and copy_texture_to_texture and friends. The bounds checking functions themselves follow guidance from the webgpu standard.
This doesn't make wgpu 100% to all the checks required by the standard, but takes care of the ones related to bounds and buffer overruns.
**Testing**
I tested this against all the examples, including intentionally making mistakes in texture copies, with this successfully catching those errors.
**Review Notes**
This code should be picked through fairly closely, as it's very likely there's some typos due to the close-but-not-quite repetition that these tests require. There's a bunch of LOC, but they are fairly boring and should be easy to understand.
I have tried to give assert messages that are as descriptive as possible, but if there is a message that could be changed to be more clear, let me know.
One thing that could change is the location of the functions. I left them where I originally wrote them, before I realized that the functions on queue also needed bounds checking, but they could have a better home elsewhere, maybe even their own file.
Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
704: Pipeline layout validation r=cwfitzgerald a=kvark
**Connections**
Implements a solid part of #269
Starts converting the function to return results, related to #638
cc @GabrielMajeri
**Description**
This change matches shader bindings against the pipeline layout. It's *mostly* complete, minus some bugs and not handling the `storage_texture_format` properly.
The risk here is that Naga reflection may have bugs, or our validation may have bugs, and we don't want to break the user content while this is in flux. So the PR introduces an internal `WGPU_SHADER_VALIDATION` environment variable. Switching it to "0" skips Naga shader parsing completely and allows the users to unsafely use the API.
Another aspect of the PR is that some of the functions now return `Result`. The way I see us proceeding is that any errors that we don't expect users to handle should result in panics when `wgpu` is used natively (i.e. not from a browser). These panics would happen in the "direct" backend of wgpu-rs (as well as in wgpu-native), but the `Result` would not be exposed to wgpu-rs, so that it matches the Web behavior.
At the same time, browser implementations (Gecko and Servo) will check the result on their GPU process and implement the WebGPU error model accordingly. This means `wgpu-core` can be super Rusty and safe.
**Testing**
Running on wgpu-rs examples. Most of them fail to get parsed by Naga, but `boids` succeeds and passes validation 🎉
Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
667: Add asserts to validate that a resource exists when used r=kvark a=DavidPeicho
Hi,
As discussed in #610, this is mostly for C/C++ users. Some questions:
* Would it be possible to display the ID of the resource that failed? I don't know what you think about implementing the `Display` trait on the `Id` struct?
* Do you see other places needing for checks?
Thanks!
Co-authored-by: David Peicho <david.peicho@gmail.com>
668: Return failures to the user in swap_chain_get_next_texture, rather than transparently reconfiguring. r=kvark a=AlphaModder
TODO:
- [x] Change `Global::swap_chain_get_next_texture` in `wgpu-core`.
- [x] Update `wgpu_swap_chain_get_next_texture` in `wgpu-native`. (https://github.com/gfx-rs/wgpu-native/pull/32)
- [x] Wrap `SwapChainOutput`/`SwapChainStatus` in a nice enum in `wgpu-rs`. (https://github.com/gfx-rs/wgpu-rs/pull/323)
- [ ] Update `wgpu_bindings` (?)
Co-authored-by: AlphaModder <quasiflux@gmail.com>
* "Use the whole buffer" is !0, not 0
Fixes#654
Applies to BufferBinding, set_vertex_buffer, set_index_buffer
* Add BufferSize type alias
* Make BufferSize a transparent type
Add a custom serialization "buddy" type
Use BufferSize::WHOLE instead of crate::WHOLE_SIZE
* Move SerBufferSize into device::trace mod
Co-authored-by: Paul Kernfeld <paulkernfeld@gmail.com>
- Clean up after the pending writes on destroy.
- Fix temporary buffer creation.
- Fix internal thread initialization by the command allocator.
- Clean up player event_loop usage.
645: Add a loom test for RefCount r=kvark a=paulkernfeld
A first effort at gfx-rs/wgpu-rs#96
loom testing is gated behind cfg(loom)
Co-authored-by: Paul Kernfeld <paulkernfeld@gmail.com>
615: Use General allocator at all times for now r=grovesNL a=kvark
In all user-managed resources, we don't have control of the lifetime. Since we don't know when it's released, we can't use any more specific allocator kind than `General`.
Previously, we assumed that buffers created for MAP_READ+COPY_SRC, for example, were one-time buffers created to copy data. However, there appear to be cases where they were used to fill data in once, and then persistently used as a copy source destination.
In the future, one WebGPU data transfer story is settled, we'll be able to use `Linear` kind again for all internally managed uploads. I.e. writeBuffer, writeTexture, and createBuffeMappedOnlyAtStart.
Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
591: Warn when binding a buffer that is still mapped r=kvark a=almarklein
Fixes#510
I'd like to make more contributions wrt validation. Though I'm quite new to Rust, so let's start small :)
From the discussion in #510 I concluded that in this case the buffer has actually been dropped, does the error message make sense like this?
Do the messages logged with `log::warn`, end up in the same logging system as the layer validation messages? So a solution to get those messages into Python (for wgpu-py) would work for both kinds of validation messages?
Co-authored-by: Almar Klein <almar.klein@gmail.com>
582: Track pipeline layout lifetime r=grovesNL a=kvark
Fixes#580
This one is interesting: it's not instantly destroyed when dropped, like bind group layouts. And it's not tracked for GPU usage like the resources. It's somewhat in-between. We have the following classes of tracking now:
1. no tracking, destroyed on drop. Applies to: bind group layouts, adapters
2. only CPU-side tracking. They go to "suspected" list on drop of self or anything that can point to them. Applies to: pipeline layouts
3. full GPU tracking. They go to "suspected" list on drop, then get associated with active submission before destruction. Applies to: buffers, textures, views, bind groups, pipelines
Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
581: Fix buffer unmap warning r=kvark a=kvark
It's more consistent if neither map_buffer or unmap_buffer care about the status.
Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
570: Improve error message when bind group and bind group layout have different number of entries r=kvark a=HalfVoxel
Co-authored-by: Aron Granberg <aron.granberg@gmail.com>
When multiple "replace" style transitions are happening,
we weren't properly retaining the "first" state, which
is required for proper stitching of command buffers.
This logic is fixed and fortified with a new set of
"change" and "merge" tests in the track module.
We were improperly detecting if a swapchain image has already
been used by a command buffer. In this case, we need to assume
that it's already in the PRESENT state.
553: Fix order of maintenace between submission tracking and buffer mapping. r=kvark a=kvark
Fixes https://github.com/gfx-rs/wgpu-rs/issues/237
Logic needs to have the following order:
1. we first wait for the device to finish
2. then we move some tracked resources from per-submission lists into the "ready to destroy" or "ready to map" lists.
3. then we handle mapping, which goes through "ready to map" lists
4. then we destroy everything for realz that needs to be
That order got broken with #547
Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
550: Add check for bound pipeline r=kvark a=kunalmohan
fix#456
Validate that a pipeline is bound before issuing any draw/dispatch call.
Co-authored-by: Kunal Mohan <kunalmohan99@gmail.com>
540: Add serialization to more types r=kvark a=HeroesGrave
With these changes, pretty much everything in wgpu-types can be serialized with the exception of BufferDescriptor, CommandEncoderDescriptor, and TextureDescriptor which contain a `*const c_char`.
Options for dealing with those:
- Leave these types as not de/serializable
- Skip when serializing, deserialize as nullptr
- Serialize as a string, deserialize as nullptr
AFAICT there's not really a way to allow a full roundtrip for these fields because nul-terminated strings don't play nicely with serde. Maybe it could serialize as a byte array?
Co-authored-by: HeroesGrave <heroesgrave@gmail.com>
514: Move some types into shared wgpu-types crate r=kvark a=grovesNL
As we discussed a while ago, we need to be able to share some types between wgpu-core/wgpu-native/wgpu-remote/wgpu-rs.
The problem is that we want to avoid a dependency on wgpu-core and wgpu-native when building [wgpu-rs for the wasm32-unknown-unknown target](https://github.com/gfx-rs/wgpu-rs/issues/101). We can avoid this by moving all shared types into a separate crate which is exposed on all targets.
Let me know if we should use some other approach or organize the types somehow. This isn't complete yet, but it might be easier to integrate this over several PRs instead of diverging my branch too far.
Co-authored-by: Joshua Groves <josh@joshgroves.com>
480: Implement `enumerate_adapters`. r=kvark a=daxpedda
This is take two of #478, it's a much smaller change with less abstraction.
- implement `internal_enumerate_adapters`, which refactors `instance.{BACKEND}.enumerate_adapters()` out of `pick_adapter`
- implement `enumerate_adapters`, which just returns a vector of all GPUs in the form of `AdapterId`
Co-authored-by: daxpedda <daxpedda@gmail.com>
468: Switch value of BufferUsage's INDIRECT and STORAGE_READ fields r=kvark a=almarklein
Closes#467. This makes the value of `INDIRECT` equal to it's value in the IDL spec. (`STORAGE_READ` is not present in the IDL spec.)
I changed the value in `resource.rs` and then ran `make ffi/wgpu.h`.
Co-authored-by: Almar Klein <almar.klein@gmail.com>
448: Fix framebuffers not always being cleaned up if invalid r=kvark a=LaylConway
This changes framebuffers to be cleaned up if only one view is cleaned up, instead of if all views are cleaned up. This is necessary because currently, at least for me, swapchain images will have a different view every frame. This means that if other views continue to exist between frames, the resulting framebuffers will never be cleaned up.
Fixes#447
Co-authored-by: Layl <2385329-layl@users.noreply.gitlab.com>
PowerPreference::Default will now prefer discrete GPUs
when on AC power and will prefer integrated GPUs while
on battery power (i.e. the battery is discharging).
418: Hotfix deadlocks on resource cleanup r=kvark a=LaylConway
This resolves resource cleanup causing deadlocks with `maintain`, because of `Device::pending` being locked in reverse order.
I'm calling this a "hotfix" because I don't think this is a scaleable approach to resolve how the issue happened in the first place, but it alleviates current deadlocking isssues.
I'm not quite sure how `Token` works, so I don't know if I used that right here.
This fixes#417
Co-authored-by: Layl <2385329-layl@users.noreply.gitlab.com>
412: Another big tracking refactor r=later a=kvark
Fixes#409
Note: there is way more code that had to be thrown out in the process of this PR than there is in the PR itself.
Also fixes [this comment](04e17b3f4f/wgpu-core/src/track/texture.rs (L29)):
> //TODO: make this less awkward!
## Logic
Instead of having a run-time per-operation control over what state is being used for stitching (with `Stitch` enum), we are now deriving this automatically on a per-subresource level. Whenever we are detecting a transition for a sub-resource, and we know there wasn't any "first" state associated with it in the current tracker, we are saving this "first" state. Unlike previous code, where it was confusing what of `Unit` fields (`init`, `last`) are valid, now `first` is `Option<>`, so we know we should be using it if it's there.
This allows us to have this hybrid tracking state of a render pass, where all resources are immutable (and their usage is extended), except for the output attachments. This, together with a whole lot of refactoring, gets us #409.
I'm actually quite happy with the tracking code now. It's finally taking shape we aren't afraid to tell others about :)
Note: this was tested on wgpu-rs examples and vange-rs.
Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
416: Makefile package command and travis github releases r=kvark a=Korijn
Closes#414
Approach mostly mimicked from [gfx-portability](https://github.com/gfx-rs/portability) as recommended to me by @kvark. I needed a number of commits to iron out some kinks for the Windows builds, so I would recommend using squash merge when accepting this PR.
Note that the Windows rust=nightly build fails, but it's also broken on master, so it's unrelated to the changes in this PR.
/cc @almarklein
# Questions
- [x] Would it make sense to also regenerate the `ffi/wgpu.h` and `wgpu-remote.h` header files and include them in the zip archive?
# Todo for maintainers
- [x] Configure encrypted `api_key`
As in [gfx-portability](https://github.com/gfx-rs/portability/blob/master/.travis.yml#L61) you will need to create an API key and commit it to the `.travis.yml` file. I've checked "allow edits from maintainers" so you should be able to commit to this branch directly. You may want to reference the [Travis instructions](https://docs.travis-ci.com/user/deployment/releases/#authenticating-with-an-oauth-token) as well.
- [ ] Tag revisions
Previous versions (`v0.1` - `v0.4`) have not been tagged on the master branch, you will want to do so retroactively. Also, when releasing in the future, make sure to tag the commit before pushing. Alternatively, you can schedule a travis build manually after applying the tag retroactively.
420: Make Origin3d::{x,y,z} all have type u32 r=kvark a=fintelia
Fixes#419
424: swapchain creation: check if selected present mode is supported r=kvark a=Veykril
Implements a simple check as talked about in #350 to fallback to `FIFO` should the selected present mode not be available on the system.
Co-authored-by: Korijn van Golen <k.vangolen@clinicalgraphics.com>
Co-authored-by: Korijn van Golen <korijn@gmail.com>
Co-authored-by: Jonathan Behrens <fintelia@gmail.com>
Co-authored-by: Veykril <lukastw97@gmail.com>
To make later bind group layout compatibility checks simpler (and
cheaper), deduplicate them on creation. If two bind group layouts with
same descriptors are requested, only one is created and the same id is
returned for both.
387: Stop dynamic offset iteration when the bind groups stop r=grovesNL a=kvark
Fixes#386
I imagine the code is not performance-optimal and could be improved. This PR focuses on correctness.
Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
