-
Notifications
You must be signed in to change notification settings - Fork 1.1k
winit-wayland: add linux dmabuf listener infra #4319
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
base: master
Are you sure you want to change the base?
Conversation
This extension trait allows general-purpose applications to query the preferred GPU device so that they do not engage the dedicated GPU unnecessarily. Without such a hint, applications are likely to choose the first available device which will always be the dedicated GPU which is undesirable on mobile devices. Similarly, on desktop dual-GPU systems where the integrated GPU is otherwise unused, it is preferred for all applications to use the dedicated GPU.
| if let Some(event_loop) = event_loop.as_wayland_event_loop() { | ||
| event_loop.listen_linux_dmabuf::<App>(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can not really avoid that, but I guess it's fine. That's how at least what I remember from previous attempts with dynamically extending winit's functionality like that, so we don't bind globals, etc, that are no always needed and we don't plumb into WindowEvent, etc, etc.
winit-wayland/src/lib.rs
Outdated
| pub use self::window::Window; | ||
|
|
||
| /// Listen to Linux Direct Rendering Manager (DRM) events. | ||
| pub trait LinuxDrm: ApplicationHandler + 'static { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bound is the crux of this api and makes this a no go as far as I can tell.
You can do this instead:
trait ApplicationHandler {
type ExtensionEvent: ExtensionEvent;
fn extension_event(&mut self, Self::ExtensionEvent);
}
trait ExtensionEvent: 'static {
fn builder<T: 'static>() -> Option<fn(T) -> Self>;
} With a proc macro to derive the trait for enums. This gets rid of the registration function and will devirtualize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I did it a bit wrong, you basically need a static only on register thing.
The goal is to not glue anything into ApplicationHandler, so you can extend it without patching winit-core, so you can wire your custom events.
For official(common) stuff the completely different (without registration) approach will be used, like the macos_handler we have on ApplicationHandler right now, but more broadly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've adjusted and now it kind of similar to what you've proposed? The static is only present on registration, similar to how your builder requires it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The static is only present on registration,
It has the same problem as before. It means that you can no longer use a non-'static App.
similar to how your builder requires it?
My solution only requires the extension events to be 'static.
The goal is to not glue anything into ApplicationHandler, so you can extend it without patching winit-core
There is no event-specific code in my suggestion. Once it exists it can be extended without ever touching winit-core again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is how my suggestion would be used in user code:
#[derive(winit::ExtensionEvent)]
enum ExtraEvents {
DrmDevice(winit_wayland::DrmDeviceEvent),
}
impl ApplicationHandler for App {
type ExtensionEvent = ExtraEvents;
} There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the problem is App<'a> and pump_events? Since with the rest of the api, they kind of demand owned access(at least there was proposal around to fully consume state), but pump_events indeed doesn't consume, and lifetime won't allow it work, since you can not use Any in the context of non 'static.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a hard time understanding what a static App would be that is a non-go
If you think that requiring ApplicationHandler to be 'static is not a problem, then you should add the 'static bound to ApplicationHandler. Otherwise, if you already have a non-'static App, there will be a huge burden to opting into the new event.
I'd assume that you'd need some From method implemented on the said type to be built from a specific backend concrete type? And that From is implemented or derived from the user?
Recall what I wrote above:
trait ExtensionEvent: 'static {
fn builder<T: 'static>() -> Option<fn(T) -> Self>;
}The builder function returns the From impl. If it returns Some, then the user is interested in that event type and backend can construct the user defined type by invoking it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then you should add the 'static bound to ApplicationHandler
That's fair point.
I do believe that 'static is probably the way it should be given that we have platforms that simply can not be anything other than 'static, like ios in the first place, where you call to -> ! function to make event loop run.
The builder function returns the From impl. If it returns Some, then the user is interested in that event type and backend can construct the user defined type by invoking it.
Yeah, I've asked to confirm that I understood correctly how it will work. And in case you want to return from the extension_event you either use &mut and rely on that or you return another associated type, like ExtensionReply.
While I hate with my option that you need 'static and register it explicitly, it'll be way more clear when actually dealing with the event and in case you have a return it won't bleed through out all the backends like it'll (unless you use &mut and write reply to event itself, but that just gets not good).
Would like to hear from you, which one would you personally prefer, considering that we have backends that can only be 'static and that we'll have cases with returning values from the user handler to communicate back with event loop.
If the events were one sided for its majority, I'd prefer your approach (or if the lifetime requirement was strong), but given the return type stuff, I tend to think that what I suggested (if fixing the bound) is probably not a bad option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think not requiring 'static is valuable especially for the pump_events case but I could probably live with 'static.
If you want to go with 'static, then you can also ApplicationHandler: Any and use trait upcasting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just with pump_events I feel like you can have your data right in the state, and then just move it out, it's not like you execute anything along side or need to clone.
If you want to go with 'static, then you can also ApplicationHandler: Any and use trait upcasting.
Yeah, I understand and trait upcasting is already used through out.
cc @mahkoh that's how I was thinking on adding that. Not sure if
queryis needed, since event is likely preferable?