Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal for Handling Native/Webgpu Extensions and Expanding Limits #691

Closed
cwfitzgerald opened this issue Jun 1, 2020 · 3 comments
Closed
Labels
area: api Issues related to API surface type: enhancement New feature or request

Comments

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Jun 1, 2020

Introduction

As #690 implements the first wgpu native extension, it's important that there is a plan in place on how to implement additional extensions.

For the purpose of this discussion:

  • webgpu extension refers to an extension that is standardized in webgpu.
  • native extension refers to an extension to wgpu that is not part of the upstream webgpu standard. It may optionally be supported by other native webgpu implementations, such as dawn.
  • unsafe extension refers to an extension that could make running untrusted code unsafe, or cause UB. These would inherently be native extensions.

A couple considerations to keep in mind, especially regarding native extensions:

  • It should be hard for gecko/servo to accidentally use a native or unsafe extension. Their goal is to implement WebGPU, so it should be easy for them to disable all native extensions. Additionally, enabling an unsafe extension could result in a major security hole, which no one wants.
  • It should be very clear to wgpu-rs users that asking for a native extension is not portable to the web.
  • Enabling an unsafe extension should demand unsafe code to at turn it on, though the interface provided by the extension should also be unsafe if possible.

Proposal

With those considerations in mind, I propose the following way of implementing extensions.

NonExhaustive Member

There will be a struct in wgpu-types which is defined as

#[doc(hidden)]
pub struct NonExhaustive(#[doc(hidden)] pub ());

There will be a note on the struct saying that manual construction of this type is not protected under our semver rules, and if you construct it, your code may break at any point. This type will not be reexported from wgpu-rs, so most users cannot use it.

Types declared in this proposal as NonExhaustive shall have a public member of this type to prevent direct construction without a partial construction + default.

struct SomeStruct {
   // members
   pub _nonexhaustive: NonExhaustive,
}

This is because #[non_exhaustive] doesn't allow users to construct structs manually, even partially. Additionally, due to the desugaring of partial construction (rust-lang/rust#63538) private fields are not allowed to be constructed either. No matter the method, it still leaves us between a rock and a hard place, as this type needs to be fully constructable from wgpu-core, and rust doesn't consider wgpu-core any different than any other user of wgpu-types.

Extensions Struct

The extensions struct will be a single bitfield that contains all the extensions that are in webgpu or supported by wgpu. Native extensions will always report false on the web, and unsupported webgpu extensions will always return false.

bitflags::bitflags! {
    pub struct Extensions: u64 {
        const ANISOTROPIC_FILTERING = 0x0000_0001_0000_0000;
        const WEBGPU_EXTENSIONS = 0x0000_0000_FFFF_FFFF;
        const NATIVE_EXTENSIONS = 0xFFFF_FFFF_0000_0000;
    }
}

Unsafe extensions

Unsafe extensions be normal members of the Extensions bitflags. When you create a device, there will be an extra argument to device creation which must be provided to enable the use of unsafe extensions. The type would look like this:

#[derive(Default)]
struct UnsafeExtensions { allow_unsafe: bool }
impl UnsafeExtensions {
    pub fn disable() -> Self { Self { allow_unsafe: false } }
    pub unsafe fn enable() -> Self { Self { allow_unsafe: true } }
}

If any unsafe extensions are requested, allow_unsafe must be true.

We follow the nomicon's definition of rust's safety. You must not be able to invoke UB from safe rust. There must be unsafe in order to invoke it. Additionally, the UB should not occur too far from the use of unsafe, if at all possible

Limits Struct

The limits struct is NonExhaustive.

Extensions and limits can interact in a couple of ways:

Extension adds limit

If an extension adds a limit, it will be added to the Limits struct as is. If the limit has a sane value for "not enabled" the limit will be that value when the extension is disabled. For example, anisotropic_filtering could have a associated limit of max_sampler_anisotropy. If the extension is disabled, it will be 1, representing no anisotropy. If the limit has no sane default value, it will be an Option<T> and the limit will be None if the extension is disabled.

Extension turns webgpu restriction into limit

Webgpu is (necessarily) more restrictive than the underlying apis as it needs to run on all apis. In wgpu, we can expose the underlying api/hardware's restriction directly. This situation is most easily explained with an example.

In webgpu, there is a requirement that all offsets into buffers are 256 byte aligned. gfx-hal exposes the actual alignment requirement of the underlying api/hardware. A native extension could be designed: relaxed_uniform_buffer_offset_alignment which adds a new limit uniform_buffer_offset_alignment. If the extension is requested, and granted, the limit value will be the value of the underlying hardware. If the extension is not requested or not supported (web), the limit will be the value of the current webgpu restriction.

This streamlines both implementation and usage. These limits will often be hardcoded into both wgpu and user code as constants. In both cases, that constant will be replaced with reading the value of the limit. Because the limit defaults to the current webgpu restriction and gecko/servo will not enable native extensions, this will ensure webgpu validation remains correct. Additionally, user code will be optimal and portable across both native and web.

Extension removes limit

I am including this for completeness, but I honestly can't think of a situation where this would actually happen.

Descriptor Structs

All descriptors that are possibly extendable are NonExhaustive.

As discussed in #689, any additional data needed for an extension will be directly added to the wgpu-types struct, likely as an Option<T>. Setting the value to anything other than None requires the extension be enabled.

In wgpu-native, each extension gets its own extension struct, chained together with pnext pointers. This is to be consistent with how webgpu extensions will have their own structs.

Member functions

Extensions might add member functions to various components. These will always exist, but have an assert validating that the extension is enabled on call.

Thoughts?

This is my idea for how extensions should be implemented. Details are of course, open for modification to best serve users/implementation.

I'd love to hear feedback from both maintainers and users :)

Edit: Updated with the results of a discussion on matrix with @kvark

Edit: Updated 2020-06-03

@cwfitzgerald cwfitzgerald added the type: enhancement New feature or request label Jun 1, 2020
bors bot added a commit that referenced this issue Jun 1, 2020
690: Implement Anisotropic Filtering Extension and Expose Extension Interface r=kvark a=cwfitzgerald

**Connections**

Resolves #568.

**Description**

This PR solves two tightly related problems:
 - Adds the ability to create a sampler with anisotropic filtering
 - Adds the webgpu interface for getting the limits/extensions on adapters and devies.

**Testing**

I have tested this against my codebase which uses it. Adding an example in wgpu-rs on how to use extensions/limits would be good, especially as we have native/webgpu extensions, but can come as a future enhancement, once we feel the extension situation has stabilized a bit.

**TODO**

- [x] Allow wgpu-rs to call limit/extension interface (gfx-rs/wgpu-rs#338)
- [ ] Determine how wgpu-native is going to adopt this functionality.
- [ ] After discussing #691, what changes need to be made. 

Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
bors bot added a commit that referenced this issue Jun 1, 2020
690: Implement Anisotropic Filtering Extension and Expose Extension Interface r=kvark a=cwfitzgerald

**Connections**

Resolves #568.

**Description**

This PR solves two tightly related problems:
 - Adds the ability to create a sampler with anisotropic filtering
 - Adds the webgpu interface for getting the limits/extensions on adapters and devies.

**Testing**

I have tested this against my codebase which uses it. Adding an example in wgpu-rs on how to use extensions/limits would be good, especially as we have native/webgpu extensions, but can come as a future enhancement, once we feel the extension situation has stabilized a bit.

**TODO**

- [x] Allow wgpu-rs to call limit/extension interface (gfx-rs/wgpu-rs#338)
- [ ] Determine how wgpu-native is going to adopt this functionality.
- [ ] After discussing #691, what changes need to be made. 

Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
bors bot added a commit that referenced this issue Jun 3, 2020
696: Rustification of Extensions and SamplerDescriptor r=kvark a=cwfitzgerald

**Connections**

This begins the rustificaiton of `wgpu-types` as discussed in #689, starting with `Extensions` and `SamplerDescriptor`.

#691 and the resulting discussion was used to advise the shape of the extension struct.

**Description**

The PR should be fairly straight forward. As discussed, I have left extensions as a bitflag until the webgpu-native issue can be opened and discussed regarding allocation of extensions.

The most controversial part of this will be the `AnisotropyLevel` enum. I created it for two reasons, one that matters, one that doesn't:
 - It means that, with the exception of enabling the aniso extension (and lod_bias), the sampler is correct by construction, which is helpful to both beginners and experts. It also better exposes the range of valid values and means less panics in user code.
 - It saves a byte in the `Option<u8>` 😄 

I definitely think that, if at all possible, we should have explicitly typed enums for "numbers" which have a limited amount of valid values (so _not_ lod bias, though that may be better expressed, idk) to make it very explicit which values can be accepted. This also feels more "rusty" and reduces the amount of "magicness" in the interface.

Ofc, I'm not married to the idea.

**Testing**

Follow up PR into wgpu-rs will include conversion of the examples.

**TODO**

- [x] wgpu-native pr rolling up this PR and #690 (gfx-rs/wgpu-native#34)
- [x] wgpu-rs pr updating examples with the new structures (gfx-rs/wgpu-rs#345)

Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
bors bot added a commit that referenced this issue Jun 3, 2020
696: Rustification of Extensions and SamplerDescriptor r=kvark a=cwfitzgerald

**Connections**

This begins the rustificaiton of `wgpu-types` as discussed in #689, starting with `Extensions` and `SamplerDescriptor`.

#691 and the resulting discussion was used to advise the shape of the extension struct.

**Description**

The PR should be fairly straight forward. As discussed, I have left extensions as a bitflag until the webgpu-native issue can be opened and discussed regarding allocation of extensions.

The most controversial part of this will be the `AnisotropyLevel` enum. I created it for two reasons, one that matters, one that doesn't:
 - It means that, with the exception of enabling the aniso extension (and lod_bias), the sampler is correct by construction, which is helpful to both beginners and experts. It also better exposes the range of valid values and means less panics in user code.
 - It saves a byte in the `Option<u8>` 😄 

I definitely think that, if at all possible, we should have explicitly typed enums for "numbers" which have a limited amount of valid values (so _not_ lod bias, though that may be better expressed, idk) to make it very explicit which values can be accepted. This also feels more "rusty" and reduces the amount of "magicness" in the interface.

Ofc, I'm not married to the idea.

**Testing**

Follow up PR into wgpu-rs will include conversion of the examples.

**TODO**

- [x] wgpu-native pr rolling up this PR and #690 (gfx-rs/wgpu-native#34)
- [x] wgpu-rs pr updating examples with the new structures (gfx-rs/wgpu-rs#345)

Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
cwfitzgerald added a commit to cwfitzgerald/wgpu that referenced this issue Jun 5, 2020
cwfitzgerald added a commit to cwfitzgerald/wgpu that referenced this issue Jun 5, 2020
cwfitzgerald added a commit to cwfitzgerald/wgpu that referenced this issue Jun 5, 2020
@cwfitzgerald cwfitzgerald added the area: api Issues related to API surface label Jun 6, 2020
cwfitzgerald added a commit to cwfitzgerald/wgpu that referenced this issue Jun 6, 2020
cwfitzgerald added a commit to cwfitzgerald/wgpu that referenced this issue Jun 6, 2020
bors bot added a commit that referenced this issue Jun 6, 2020
703: Implement Extensions Interface r=kvark a=cwfitzgerald

## Description

This is the first step of implementing #691. This implements the described changes to all the core structs and to `SamplerDescriptor`. I intend this PR to be an example of what the plan looks like. If we think this works, we can roll out the rest of the changes either in bulk or as we need them. 

The rolling out of this is also tied to the rolling out of #689. Hopefully the macro work done in gfx-rs/wgpu-native#34 will make this easier.

## Questions

One outstanding question I had is what the default values for lod_min/max should be. As of right now I just derive default, which puts them at zero, which is probably a reasonable default, though most of the examples use -100, 100 which is normally what you use when doing mipmapping.

## TODO:

- [ ] Discuss if this meets our needs
- [x] Implement this in wgpu-rs (gfx-rs/wgpu-rs#350)
- [ ] Implement this in wgpu-native

Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
@kvark
Copy link
Member

kvark commented Jul 1, 2020

@zakarumych raised a concern about unsafe extensions. Opting into them with unsafe only at initialization makes it difficult to reason about safety down the road, i.e. anything can be unsafe, implicitly. This is bending the concept of unsafe in Rust a bit too much, and is not sound. For example, if there is a middleware layer, and the user app gets access to, say, command buffers, it has no means to discover that some operations on those are unsafe.

I think the reasoning makes sense. We should remove the UnsafeFeatures mechanics and instead let them add unsafe methods to wgpu-core.

@cwfitzgerald
Copy link
Member Author

Fwiw, my understanding of the plan was to do both UnsafeFeatures + unsafe interface for unsafe extensions. This provides a check to make sure that unsafe things can't just be freely called without some change in extension init. If you'd still rather not do the first part, that's still perfectly fine. I agree that unsafe needs to be as local as possible both for soundness and for reasonable debugging.

bors bot added a commit that referenced this issue Jul 7, 2020
764: Remove UnsafeFeatures as we decided the top level guard is not useful r=cwfitzgerald a=kvark

**Connections**
Reverts part of #691 about unsafe extensions

**Description**
Top-level unsafe is not sound. We still need unsafety close to the spot where it's triggered.

**Testing**
untested

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
@cwfitzgerald
Copy link
Member Author

I think that we have implemented enough extensions at this point that we can call this implemented :)

kvark pushed a commit to kvark/wgpu that referenced this issue Jun 3, 2021
693: Expose get_swap_chain_preferred_format r=kvark a=manugildev

Fixes gfx-rs#691

This functionality was committed in gfx-rs/wgpu@6f1d614 but never exposed to `wgpu-rs`.

Problem: Examples made use of `wgpu::TextureFormat::Bgra8UnormSrgb` but that's unreliable and according to the [API](https://gpuweb.github.io/gpuweb/#dom-gpucanvascontext-getswapchainpreferredformat) this method is the way of properly obtaining an optimal texture format for the `SwapChain` based on the platform.

- [x]  Expose `get_swap_chain_preferred_format()` on Device
- [x]  Use `TextureFormat::Bgra8Unorm` for [web.rs](https://github.com/gfx-rs/wgpu-rs/blob/e00ef5d5842a1fcfb46bb531789b1c340bdee3ab/src/backend/web.rs#L970)
- [x]  Update [framework.rs](https://github.com/gfx-rs/wgpu-rs/blob/e00ef5d5842a1fcfb46bb531789b1c340bdee3ab/examples/framework.rs#L227) and examples to call this new method

Co-authored-by: Manuel Gil <manugildev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Issues related to API surface type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants