Re: [1/3] winex11: Select for XRandR screen change events and implement an event handler.
Sam Edwards <cfsworks(a)gmail.com> writes:
This patch adds an event handler to xrandr.c to process RRScreenChangeNotify events, so that when non-Wine applications change the display resolution, Wine can find out about it.
Two main discussion points:
- Previously, there was nothing pulling events out of gdi_display's queue, thus necessitating the need for a new event handler. I opted to put it in xrandr.c since this handler only manages XRandR events, but it might be cleaner to move it over to event.c.
- Are we okay with having to call the event handler, or would this work better in a continuously-running background thread instead?
It's okay only because you are not actually calling it ;-) But no, you can't select for events on the gdi display; actually I don't think you want to do it this way at all. Handling external resizes should be done in the desktop process, and most likely involves the wineserver too. It certainly won't be an easy task. -- Alexandre Julliard julliard(a)winehq.org
On May 9, 2013, at 1:36 PM, Alexandre Julliard wrote:
Handling external resizes should be done in the desktop process, and most likely involves the wineserver too. It certainly won't be an easy task.
Hmm. Then that probably means I messed it up in the Mac driver, since what I did didn't seem so hard (and didn't involve the wineserver). ;) -Ken
On 05/09/2013 12:36 PM, Alexandre Julliard wrote:
It's okay only because you are not actually calling it ;-) Yeah, I don't call it until patch 2 in the series. This patch just introduces it without calling it, which does cause a warning. (I hope this doesn't violate the "atomic patches" rule.)
But no, you can't select for events on the gdi display; actually I don't think you want to do it this way at all. So gdi_display should not receive events? I'm guessing this has something to do with the fact that Wine opens one Xlib Display per thread, so the XRandR resize events need to be pulled down on one of the thread-local X connections, rather than the master gdi_display. (I'm still not sure why Wine does it this way. Are certain versions of Xlib not thread-safe?)
Handling external resizes should be done in the desktop process, and most likely involves the wineserver too. The event handler does call X11DRV_resize_desktop, which messages the desktop process with the new size. But, I can see how it would be cleaner to put the event checker in the same process as well, since that way the XRandR resize doesn't have to get processed once for every process on the system.
How would this involve the wineserver, though? It looks like we already have the infrastructure for synchronizing screen_width and screen_height changes from one process to another. At any rate, it makes sense to keep winex11 and winemac in sync. If Ken's resize handling isn't proper, we should decide how to proceed now, so that both can be fixed in the same way.
It certainly won't be an easy task.
That's fine; this is the last issue that I need to resolve before Wine is pretty much "bug free" for me, so I'm more than happy to put in the extra effort. :)
Sam Edwards <cfsworks(a)gmail.com> writes:
On 05/09/2013 12:36 PM, Alexandre Julliard wrote:
It's okay only because you are not actually calling it ;-) Yeah, I don't call it until patch 2 in the series. This patch just introduces it without calling it, which does cause a warning. (I hope this doesn't violate the "atomic patches" rule.)
It does, you can't add dead code (or warnings for that matter).
But no, you can't select for events on the gdi display; actually I don't think you want to do it this way at all. So gdi_display should not receive events? I'm guessing this has something to do with the fact that Wine opens one Xlib Display per thread, so the XRandR resize events need to be pulled down on one of the thread-local X connections, rather than the master gdi_display. (I'm still not sure why Wine does it this way. Are certain versions of Xlib not thread-safe?)
Most window events need to be handled in the thread that owns the window. Pulling them from a global connection in a different thread would only cause extra context switches and potential deadlocks.
Handling external resizes should be done in the desktop process, and most likely involves the wineserver too. The event handler does call X11DRV_resize_desktop, which messages the desktop process with the new size. But, I can see how it would be cleaner to put the event checker in the same process as well, since that way the XRandR resize doesn't have to get processed once for every process on the system.
How would this involve the wineserver, though? It looks like we already have the infrastructure for synchronizing screen_width and screen_height changes from one process to another.
Not really, only the process that triggered the change gets properly updated at the moment. The Mac driver tries to do it by broadcasting a message, but that doesn't update processes that currently don't own a window. There are also race conditions, and various issues with window surfaces and DCE regions. Resizing the desktop is a tricky problem. As far as XRandr is concerned, at this point you could probably simply cache the mode and don't bother to update except when the app triggers a change, since that's what we do for the rest of the desktop parameters anyway. -- Alexandre Julliard julliard(a)winehq.org
On 05/10/2013 09:11 AM, Alexandre Julliard wrote:
Sam Edwards <cfsworks(a)gmail.com> writes:
Yeah, I don't call it until patch 2 in the series. This patch just introduces it without calling it, which does cause a warning. (I hope this doesn't violate the "atomic patches" rule.) It does, you can't add dead code (or warnings for that matter). So, if I were to give this patch a try 2, I'd have to call the event handler somewhere (probably right after selecting for the resize event)? I'll keep that in mind for the future. :)
But no, you can't select for events on the gdi display; actually I don't think you want to do it this way at all. So gdi_display should not receive events? I'm guessing this has something to do with the fact that Wine opens one Xlib Display per thread, so the XRandR resize events need to be pulled down on one of the thread-local X connections, rather than the master gdi_display. (I'm still not sure why Wine does it this way. Are certain versions of Xlib not thread-safe?) Most window events need to be handled in the thread that owns the window. Pulling them from a global connection in a different thread would only cause extra context switches and potential deadlocks. This is kind of a special case because RRScreenChangeNotify is a root window event, so there's no "thread that owns the window" there (unless Wine has some internal representation for the X11 root window, and a thread that owns that representation, that I'm not aware of).
Handling external resizes should be done in the desktop process, and most likely involves the wineserver too. The event handler does call X11DRV_resize_desktop, which messages the desktop process with the new size. But, I can see how it would be cleaner to put the event checker in the same process as well, since that way the XRandR resize doesn't have to get processed once for every process on the system.
How would this involve the wineserver, though? It looks like we already have the infrastructure for synchronizing screen_width and screen_height changes from one process to another. Not really, only the process that triggered the change gets properly updated at the moment. The Mac driver tries to do it by broadcasting a message, but that doesn't update processes that currently don't own a window. There are also race conditions, and various issues with window surfaces and DCE regions. Resizing the desktop is a tricky problem. Aha; this is why the wineserver is necessary: So that applications that haven't created any windows can receive the update.
As far as XRandr is concerned, at this point you could probably simply cache the mode and don't bother to update except when the app triggers a change, since that's what we do for the rest of the desktop parameters anyway.
Thanks for the suggestion. That sounds like it has a much smaller impact, so I think I'll use that approach instead. Until then, I'll withdraw this patch. Thanks, Sam
participants (3)
-
Alexandre Julliard -
Ken Thomases -
Sam Edwards