Allow optional `RawDisplayHandle` in `InstanceDescriptor`

[MarijnS95]

Connections Closes #7969 Fixes https://github.com/gfx-rs/wgpu/issues/5505 Fixes https://github.com/gfx-rs/wgpu/issues/3176 Fixes https://github.com/gfx-rs/wgpu/discussions/7965 Fixes #7665

Description This allows platforms like GLES with EGL context backend to properly initialize such that their EGLDisplay and EGLContext are compatible with an incoming Surface, which is otherwise impossible if not for hotpatching the context (and loosing all resources created on it) when a surface comes in and that compositor connection becomes known.

This hotpatching is known to break quite a few systems and initialization ordering (i.e. the alternative is forcing users to create surfaces before querying adapters, which isn't feasible on platform like Android), and is better avoided or at least made possible by using the (Raw)DisplayHandle abstraction provided by raw_display_handle in the way it's intended to be used (additionally, removing it from the surface would be a sensible decision). This was suggested long ago in https://github.com/gfx-rs/wgpu/issues/6211#issuecomment-2367769172.

There may however be cases where users want to initialize multiple Instances or perhaps Adapters for independent RawDisplayHandles, and certain backends may be designed specifically to cope with this (i.e. Vulkan).

Note that this PR does nothing to address many other soundness problems inside the EGL backend.

Testing Using a Wayland compositor with:

WGPU_BACKEND=gl cargo r -p wgpu-examples -- shadow

Squash or Rebase? rebase

Checklist

• [x] Run cargo fmt.
• [ ] Run taplo format.
• [ ] Run cargo clippy --tests. If applicable, add:
  • [ ] --target wasm32-unknown-unknown
• [ ] Run cargo xtask test to run tests.
• [ ] If this contains user-facing changes, add a CHANGELOG.md entry.

[alextechcc]

Wow this cleans up a ton of code. Wonderful!

Have you thought about putting the display connection into GlBackendOptions? https://github.com/gfx-rs/wgpu/blob/trunk/wgpu-types/src/instance.rs#L321

Obviously it's already been mentioned that the vulkan backend could benefit from this, though that means we could also chuck the parameter in that struct too if we're being selective. Just thinking of minimizing the blast radius of these changes. As it stands, I think anyone who constructs an InstanceDescriptor has the "breaking change" with this PR that they need to specify something for display. Not sure how breaking changes are handled with wgpu.

I haven't had much time to work on this for a while, but I'm planning to try this with bevy at some point.

EDIT: I got bevy working with this PR. I'm not sure if I'm stepping on any toes here but bevy has its winit crate depend on the crate doing wgpu stuff, so it feels pretty wonky - but I can move around the types such that I have access to the display handle immediately and pass it to the instance descriptor. It seems there's a good amount of thread safety related stuff I have to read through, and if I want to make GL a viable option again I probably should make this a little less annoying: https://github.com/bevyengine/bevy/issues/18932 and this https://github.com/bevyengine/bevy/issues/13115

[cwfitzgerald]

the alternative is forcing users to create surfaces before querying adapters, which isn't feasible on platform like Android

What isn't feasible about this on Android? I am definitely leaning towards the "must provide a surface to request an adapter" side than this as it fits more cleanly into the initialization flow and allows new, separate, windows to be handled without making a whole new instance.

[MarijnS95]

NativeWindow (Java Surface) and e.g. a VkSwapchainKHR/VkSurfaceKHR created from these disappear at runtime, commonly when an app becomes invisible. They also arrive "relatively late" after application startup, requiring users to delay all their adapter and graphics setup until a surface arrives, if wgpu pushes ahead with that requirement (just to provide functional legacy EGL support). At the same time, they might/must now likely assume that any future surface they receive from Android after such a ""suspend -> resume"" cycle is compatible with the Instance/Adapter queried previously.

---

and allows new, separate, windows to be handled without making a whole new instance.

Note that RawWindowHandle != RawDisplayHandle, those are two completely unrelated concepts. This change/PR definitely doesn't require new instances to be created when new windows are opened (unless the user created a new connection to a Wayland compositor, or to a different compositor).

[alextechcc]

NativeWindow (Java Surface) and e.g. a VkSwapchainKHR/VkSurfaceKHR created from these disappear at runtime, commonly when an app becomes invisible. They also arrive "relatively late" after application startup, requiring users to delay all their adapter and graphics setup until a surface arrives

Obviously this could be worked around but it seems that Bevy is in a similar situation. At least currently, there's built in GPU preprocessing steps (mipmap generation) that want to happen long before the window exists, and obviously due to the current behavior likely apps will rely on this phase before the window exists.

[cwfitzgerald]

Note that RawWindowHandle != RawDisplayHandle, those are two completely unrelated concepts.

OH! Yes indeed. I find the term "display" a bit confusing, but seeing that it's available from the event loop makes me much more amenable to this! Then I think this is perfectly reasonable and we should go ahead with it.

[MarijnS95]

Okay @cwfitzgerald, what would be your preferred way to land this? Should I rebase it, pull it out of draft and close its relative/alternative, #7969?

It seems some silly conflicts have been introduced to egl.rs 😰

[cwfitzgerald]

@MarijnS95 Yup, that sounds good to me, I'll self assign this, hopefully I can get to it in the next week once it's ready.

[babakkarimib]

@cwfitzgerald regarding #7973, just wanted to mention that hello-compute does not create any surfaces. I hope this is relevant.

[cwfitzgerald]

@MarijnS95 Hey just following up on this. I'd love to land this if this as it's better than the current situation.

More generally though, as I look through the code and see how much needs to be copied from glutin, I'm really convinced we should just migrate to glutin. None of the regular maintainers of wgpu know how any of the native gl context creation works, and having all the gl context creation in one focused place in the ecosystem is a big win, we shouldn't be duplicating that effort.

I'll file an issue about it tomorrow, but would you be willing to either take on, or point me in the right direction wrt the glutin migration? I'm slammed but I know you are too.

[MarijnS95]

@cwfitzgerald I would like to see this merged too, now that merge conflicts surface every once in a while that could have been avoided.

---

For glutin, as proposed elsewhere, I'd just rm the entire EGL backend and implement it from scratch using glutin, that is something I can look into. Do you want to see this happen first, in parallel or on top of this PR as it may currently seem blocked on the // TODO: Copypaste glutin... Or use glutin... comment? Though the only thing commented out or unhandled in that match loop is native ANGLE.

---

Separately, how do you feel about finally removing trait WindowHandle and all the existing hackery that forces RawDisplayHandle + RawWindowHandle to "live together", now that this PR provides RawDisplayHandle in the Instance? If I factor that part with removal of WindowHandle out into a separate PR? And/or is there already a tracking issue for this? We have users that find it incredibly painful to deal with the raw-window-handle traits in a "platform agnostic" manner because wgpu decided to glue them together in a wrapper trait (even though raw-window-handle has clear reasons to treat them separately...), see for example https://github.com/rust-windowing/softbuffer/issues/298#issuecomment-3617254907.

However, I can also think of niche use-cases (not EGL, but Vulkan should support this) where one GPU could "technically" present to multiple "compositor connections" (RawDisplayHandles) at once, which make it inconvenient to tie an Instance to a particular RawDisplayHandle (again, excluding EGL).

Maybe I would just separate them, with RawDisplayHandle in an Option to fall back to the (also optional) Instance::display added by this PR?

[cwfitzgerald]

For glutin, as proposed elsewhere, I'd just rm the entire EGL backend and implement it from scratch using glutin, that is something I can look into. Do you want to see this happen first, in parallel or on top of this PR

I'm quite down to just go ahead on the glutin rewrite, I'm very uninterested in continuing to maintain GL context creation. With the release being relatively close:

• If this PR doesn't regress anything and having it ready isn't too much work, lets land this as it gives us the RawDisplayHandle which we will need for glutin.
• If this PR is going to be a pain in the ass to bringup to snuff, feel free to change it to only have the RawDisplayHandle change and we can land.
• Then you've got my all clear to rip out EGL and write a glutin GL context creation backend.

Whichever you decide, unmark this as draft when you're ready for me to review it.

Separately, how do you feel about finally removing trait WindowHandle and all the existing hackery that forces RawDisplayHandle + RawWindowHandle to "live together", now that this PR provides RawDisplayHandle in the Instance? If I factor that part with removal of WindowHandle out into a separate PR?

I think we should do it. Properly modeling our requirements is very important. Yeah that should be a separate PR. Up to you which side of the glutin rewrite it lands on.

However, I can also think of niche use-cases (not EGL, but Vulkan should support this) where one GPU could "technically" present to multiple "compositor connections" (RawDisplayHandles) at once, which make it inconvenient to tie an Instance to a particular RawDisplayHandle (again, excluding EGL).

Lets start with the common case, then we can figure out if there's some other api shape that would make that use case possible. This is a very edge case and getting "standard" gl creation working is much more important.

Maybe I would just separate them, with RawDisplayHandle in an Option to fall back to the (also optional) Instance::display added by this PR?

That could work but would be a bit confusing - we don't have to decide on this shape now. For now Instance::display + Surface having window is totally fine.

[mahkoh]

You forgot to make Instance::new unsafe.

[MarijnS95]

Whichever you decide, unmark this as draft when you're ready for me to review it.

The missing Angle portion is now implemented again (though untested). I feel like this is ready for an early view by maintainers though not nearly ready for "undrafting" which signifies I'd be okay with merging it. There are still a bunch of (small) questions remaining that I don't think I get to answer myself; perhaps you can read through it otherwise I'll summarize them here.

If this PR doesn't regress anything and having it ready isn't too much work, lets land this as it gives us the RawDisplayHandle which we will need for glutin.

There are some rough/weird edges, but nothing significant that requires migrating to glutin first; indeed having this PR as a base (API-wise) is most useful before rewriting with glutin.

Before I forget, glutin will also replace WGL and hopefully become the core for all GL(ES) context management.

---

Lets start with the common case, then we can figure out if there's some other api shape that would make that use case possible. This is a very edge case and getting "standard" gl creation working is much more important.

The main thing is that wgpu already receives the RawDisplayHandle with the Window. First removing that (assuming the base case should be that people pass it into Instance, if only to satisfy EGL) and later adding it if someone wants that sort of flexibility on e.g. Vulkan seems counterproductive.

[cwfitzgerald]

The missing Angle portion is now implemented again (though untested). I feel like this is ready for an early view by maintainers though not nearly ready for "undrafting" which signifies I'd be okay with merging it. There are still a bunch of (small) questions remaining that I don't think I get to answer myself; perhaps you can read through it otherwise I'll summarize them here.

Alright, I'll take a look tonight. If there's any questions outside of what is in the PR, feel free to ask them here.

There are some rough/weird edges, but nothing significant that requires migrating to glutin first; indeed having this PR as a base (API-wise) is most useful before rewriting with glutin.

Sounds good.

Before I forget, glutin will also replace WGL and hopefully become the core for all GL(ES) context management.

Music to my ears

The main thing is that wgpu already receives the RawDisplayHandle with the Window. First removing that (assuming the base case should be that people pass it into Instance, if only to satisfy EGL) and later adding it if someone wants that sort of flexibility on e.g. Vulkan seems counterproductive.

Alright, if you have an idea of what the API would look like, then you can go ahead, I was mainly trying to get roadblocks and places for bikeshedding out of the way.

[MarijnS95]

The main thing is that wgpu already receives the RawDisplayHandle with the Window. First removing that (assuming the base case should be that people pass it into Instance, if only to satisfy EGL) and later adding it if someone wants that sort of flexibility on e.g. Vulkan seems counterproductive.

Alright, if you have an idea of what the API would look like, then you can go ahead, I was mainly trying to get roadblocks and places for bikeshedding out of the way.

This would be really ugly, I briefly pushed https://github.com/gfx-rs/wgpu/commit/5b79f7d8a into this PR to start on it but it's basically turning the RawDisplayHandle in create_surface() into an Option to fall back to InstanceDescriptor::display (and alternatively validate that they're identical - but that's not valid on Vulkan).

[MarijnS95]

I think we should have wgt::InstanceDescriptor have a Box<dyn HasDisplayHandle + Send + Sync + 'static>.

Note that this structure in -types and -hal both require Clone and Debug too. While OwnedDisplayHandle provides it, because it's not an auto-type it's not allowed in dyn HasDisplayHandle + Clone + Debug...

Defining a trait: OurHack: HasDisplayHandle + Clone + Debug is not implementable by downstream crates, as both OurHack and winit::event_loop::OwnedDisplayHandle are foreign.

[cwfitzgerald]

Defining a trait: OurHack: HasDisplayHandle + Clone + Debug is not implementable by downstream crates, as both OurHack and winit::event_loop::OwnedDisplayHandle are foreign.

Wouldn't this be handled by a blanket to make a trait alias

trait: OurHack: HasDisplayHandle + Clone + Debug {}

impl<T: HasDisplayHandle + Clone + Debug> OurHack for T {}

Edit: Clone is not dyn-safe. I think we should remove the impl of Clone.

[MarijnS95]

Edit: Clone is not dyn-safe. I think we should remove the impl of Clone.

That is correct. Currently only one web workaround seems to require it to be clone, all other consumers take a borrow.

But for the intended consumption of winit's "cheaply copyable" OwnedDisplayHandle to keep it alive when only passed a "borrow" via &wgt::InstanceDescriptor, it still needs to be Clone.

[cwfitzgerald]

But for the intended consumption of winit's "cheaply copyable" OwnedDisplayHandle to keep it alive when only passed a "borrow" via &wgt::InstanceDescriptor, it still needs to be Clone.

Bleh we should just remove the & from InstanceDescriptor - we need to take ownership now.

[cwfitzgerald]

This might fix https://github.com/gfx-rs/wgpu/issues/2384 too?

[MarijnS95]

Bleh we should just remove the & from InstanceDescriptor - we need to take ownership now.

Don't you like it when a struct requires cloning/moving owned objects into it... Only to take that struct by borrow and subsequently clone all contents again?

[MarijnS95]

This might fix #2384 too?

Unsure, if I read that issue correctly it seems to revolve around creating wgpu::Surfaces for multiple backends on the same underlying Android Surface or Windows HWND. That is not something this PR aims to be solving.

[cwfitzgerald]

No rush or anything, just wanted to stay on the same page that I'm currently waiting on another iteration before I review again.

[MarijnS95]

Alright, if you have an idea of what the API would look like, then you can go ahead, I was mainly trying to get roadblocks and places for bikeshedding out of the way.

Got that initial idea going at https://github.com/MarijnS95/wgpu/compare/window-handle-without-display, based on top of this PR.

[MarijnS95]

CI keeps failing on:

error: unused import: `num_traits::float::FloatCore`
  --> naga/src/back/pipeline_constants.rs:22:5
   |
22 | use num_traits::float::FloatCore as _;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unused_imports)]`

Which sits behind cfg(no_std). Other PRs and trunk are fine, did I break something with the feature selection? After all raw-window-handle is now no longer an optional dependency, though it's no_std by default (and has no default features) unless wgpu/std is enabled which forwards to raw-window-handle/std.