Hi,
This email brings together several threads of conversation that have been spread across different bug reports. Sorry for the length.
The problem: WineD3D is based on WGL (a.k.a. opengl32). As a consequence, it conflicts with application use of WGL in a number of ways. It's also limited by WGL, which doesn't allow for all of the behaviors of DirectDraw and Direct3D.
https://bugs.winehq.org/show_bug.cgi?id=28869 WineD3D needs to make its own GL context current temporarily and then restore the previous GL context, but WGL requires a DC to restore it and the DC may no longer be valid.
https://bugs.winehq.org/show_bug.cgi?id=29301 WineD3D changes the window style of the device window when it should not. It needs to do that in order to render to the full screen where WGL only allows full-featured rendering to a window.
https://bugs.winehq.org/show_bug.cgi?id=2082 Again, WineD3D is rendering to a window when it wants to render full-screen. In this case, the affected apps create an owned window that sits in front of the device window, obscuring it.
http://bugs.winehq.org/show_bug.cgi?id=29934 WineD3D sets a pixel format on the application window. This conflicts with a later attempt by the app to set a different pixel format on the window via WGL.
I think the solution to these problems is for WineD3D to have an alternative path to set up its GL context. Henri and I have been discussing using Wine-specific extensions to WGL to do this.
There's already the extension WGL_WINE_pixel_format_passthrough which provides the wglSetPixelFormatWINE() function which is just like GDI's SetPixelFormat() and WGL's wglSetPixelFormat() except it allows for changing the pixel format after it's been set once. WineD3D needs to change it even though WGL doesn't allow that.
In some of the bug reports listed above, I've suggested adding new extensions or modifying WGL_WINE_pixel_format_passthrough. We probably need a more comprehensive approach.
WGL exposes various pieces of state to the application. Ideally, WineD3D would not alter any of that state since such alterations are visible to the app and Direct3D doesn't do that on Windows. I'm not sure that's achievable but we should at least minimize such visible state changes to the extent we can.
WGL state:
* The DC pixel format. For window DCs (which I believe are the only ones of real interest), this is actually a property of the window, not the DC. Changed by SetPixelFormat(), wglSetPixelFormat(); exposed by GetPixelFormat(), wglGetPixelFormat().
* The current GL context for the thread. Changed by wglMakeCurrent(), wglMakeContextCurrentARB(); exposed by wglGetCurrentContext().
* The DC for the current GL context. Changed by wglMakeCurrent(), wglMakeContextCurrentARB(); exposed by wglGetCurrentDC(), wglGetCurrentReadDCARB().
The most important things to achieve are a) to not "consume" the only opportunity the app has to set the window's pixel format, and b) to restore the app's current GL context (and implicitly its DC) when returning control to it. I don't think it's necessary to avoid WineD3D ever changing WGL's notion of the current context. Certainly, while a WineD3D GL context is current, GL functions like glClear() will operate on WineD3D's context. If the app thinks its context is current and calls GL functions, its context better really be current. So, WineD3D really needs to restore the app's context before returning control (and it generally does, I think). If it's going to do that, then wglGetCurrentContext() will take care of itself. Same for wglGetCurrentDC().
Here's my proposal:
Extension WGL_WINE_surface:
HANDLE wglCreateSurfaceWINE(HDC hdc, int pixel_format); // if hdc is GetDC(<some normal window>), a window surface is created // if hdc is GetDC(0), GetDC(GetDesktopWindow()), or CreateDC("display", …), a full-screen surface is created. // once the surface is created, it no longer refers to hdc
void wglDestroySurfaceWINE(HANDLE surface);
HDC wglGetSurfaceDCWINE(HANDLE surface); // Returns a DC created for the surface. This is not the DC passed in to wglCreateSurfaceWINE().
void wglReleaseSurfaceDCWINE(HANDLE surface, HDC hdc);
This is somewhat modeled on WGL_ARB_pbuffer. http://www.opengl.org/registry/specs/ARB/wgl_pbuffer.txt
A surface represents a "place" where GL can render, a potential target for a context. It can correspond to a Win32 window or it can be a full-screen surface. A full-screen surface will likely be a platform window (X11 or Mac) managed by the graphics driver but won't be a Win32 window. It should not interfere with mouse events or focus.
A surface has a pixel format, similar to a window. Even if the surface was created from a window DC, its pixel format is independent of the window's. Since a surface is created with a pixel format, it shouldn't be necessary to set one with SetPixelFormat() on a surface DC. If it helps keep WineD3D's code simpler, we could allow it. I'm not sure if it will be necessary to support changing the pixel format after it's been set, using wglSetPixelFormatWINE(), or if the surface should just be destroyed and recreated as necessary. Doing the latter may eventually allow the removal of wglSetPixelFormatWINE().
The lifetime of a surface controls whatever resources the graphics driver has to manage for it. For a normal window, this may include a drawable. For a full-screen surface, this would include any full-screen platform window it might have created.
WineD3D would obtain a DC from a surface using wglGetSurfaceDCWINE() just as it now obtains one for a window using GetDC(). There can be multiple DCs for a given surface. In particular, a DC can only be used on a single thread, so multiple threads would each have to get their own DC. The different lifetimes of surfaces vs. surface DCs is why this proposal differs from my approach in bug 2082, where I proposed an extension which just added full-screen DCs without a corresponding full-screen surface object.
Destroying a surface while it still has extant surface DCs results in undefined behavior.
Once WineD3D has a surface DC, it should be able to work with it as it currently does with window DCs. wglMakeCurrent() with a surface DC will associate the context with the surface drawable. For a surface created from a window, this would work as it does now in the graphics drivers with the exception of how the drawable is looked up from the DC. For a full-screen surface, this would target whatever full-screen drawable the graphics driver provides (possibly a platform window).
There's a wrinkle having to do with the pixel format. User32 and the wineserver use the fact that a window has a pixel format to compute the "surface region" for the top-level window. This is a different "surface" than the concept central to this proposal. This "surface" is used for client-side GDI rendering using the DIB engine. Maintaining a surface region is necessary to prevent the contents of the window surface buffer from being blitted on top of any 3D rendering done to the window. To keep that functioning, the graphics drivers will still have to inform user32 that 3D rendering is being done to the window. It can do that using the existing mechanism, but that's not currently reversible because it's tied to the pixel format and that's a permanent property of a window. We'd like it to be reversible, since WineD3D can stop targeting a window. I'm thinking of using a count in user32.
Extension WGL_WINE_make_current:
BOOL wglMakeContextCurrentWINE(HGLRC hglrc);
The standard WGL function wglMakeCurrent() combines two operations. It sets the a GL context's drawable to the provided DC and it makes the GL context current for the thread. This new function, wglMakeContextCurrentWINE(), would only do the latter. It would make a GL context current for the thread without changing which DC it's associated with. Both the X11 and Mac drivers already maintain the necessary state to do that. They don't need to have the DC passed in again. (If a GL context is associated with separate draw and read DCs using wglMakeContextCurrentARB(), then this function will leave it associated with both DCs.)
This allows WineD3D to faithfully restore the app's previously-current WGL context. This addresses bug 28869.
I considered some other designs but I don't think they work well. I looked for a way to do without special DCs. We could directly specify a pixel format when creating a context rather than getting it from the window or surface via the DC. We could make a context current directly on a surface rather than a surface DC. Unfortunately, too much of WGL depends on having a DC. For example, ChoosePixelFormat() and wglSwapBuffers().
What do folks think? Any feedback would be appreciated.
-Ken
On 29 January 2014 01:47, Ken Thomases ken@codeweavers.com wrote:
The most important things to achieve are a) to not "consume" the only opportunity the app has to set the window's pixel format, and b) to restore the app's current GL context (and implicitly its DC) when returning control to it. I don't think it's necessary to avoid WineD3D ever changing WGL's notion of the current context. Certainly, while a WineD3D GL context is current, GL functions like glClear() will operate on WineD3D's context. If the app thinks its context is current and calls GL functions, its context better really be current. So, WineD3D really needs to restore the app's context before returning control (and it generally does, I think). If it's going to do that, then wglGetCurrentContext() will take care of itself. Same for wglGetCurrentDC().
There may be performance reasons though. The wglMakeCurrent() required to restore the previous GL context is pretty expensive, so applications that have a GL context current while calling into wined3d take a considerable performance hit. I think I've only ever seen 1 or 2 applications that were affected that though, so perhaps we don't care enough.
There's a wrinkle having to do with the pixel format. User32 and the wineserver use the fact that a window has a pixel format to compute the "surface region" for the top-level window. This is a different "surface" than the concept central to this proposal. This "surface" is used for client-side GDI rendering using the DIB engine. Maintaining a surface region is necessary to prevent the contents of the window surface buffer from being blitted on top of any 3D rendering done to the window. To keep that functioning, the graphics drivers will still have to inform user32 that 3D rendering is being done to the window. It can do that using the existing mechanism, but that's not currently reversible because it's tied to the pixel format and that's a permanent property of a window. We'd like it to be reversible, since WineD3D can stop targeting a window. I'm thinking of using a count in user32.
Potentially related, http://bugs.winehq.org/show_bug.cgi?id=15232. I'm not entirely sure on all the details around WS_CLIPCHILDREN, particularly in fullscreen mode, but it's probably something else to keep in mind.
Extension WGL_WINE_make_current:
BOOL wglMakeContextCurrentWINE(HGLRC hglrc);
The standard WGL function wglMakeCurrent() combines two operations. It sets the a GL context's drawable to the provided DC and it makes the GL context current for the thread. This new function, wglMakeContextCurrentWINE(), would only do the latter. It would make a GL context current for the thread without changing which DC it's associated with. Both the X11 and Mac drivers already maintain the necessary state to do that. They don't need to have the DC passed in again. (If a GL context is associated with separate draw and read DCs using wglMakeContextCurrentARB(), then this function will leave it associated with both DCs.)
This allows WineD3D to faithfully restore the app's previously-current WGL context. This addresses bug 28869.
Maybe. Note that some of the quirk detection code renders to this context. I think all rendering there goes to FBOs, so in that regard it's probably safe, but it does feel a bit fragile. For regular wined3d rendering we do of course need a drawable, although this may work out in practice.
What do folks think? Any feedback would be appreciated.
I think WGL_WINE_surface would work from wined3d's point of view, at the very least interface wise. There are probably some details to work out about how it should interact with gdi32 rendering and things like the window moves, minimization, etc. I don't have as good of an overview of how it would work out for winex11. I'm not so sure about WGL_WINE_make_current, but it may be enough in practice.
On 01/29/2014 03:10 AM, Henri Verbeet wrote:
There may be performance reasons though. The wglMakeCurrent() required to restore the previous GL context is pretty expensive, so applications that have a GL context current while calling into wined3d take a considerable performance hit. I think I've only ever seen 1 or 2 applications that were affected that though, so perhaps we don't care enough.
The multi-threaded command stream patches would fix this issue, wouldn't it? In that case, WineD3D has a GL context on its own thread to handle the D3D calls, leaving the app threads more or less alone. Of course, it'll be a while before they get into Wine proper.
On 29 January 2014 19:04, Chris Robinson chris.kcat@gmail.com wrote:
The multi-threaded command stream patches would fix this issue, wouldn't it? In that case, WineD3D has a GL context on its own thread to handle the D3D calls, leaving the app threads more or less alone. Of course, it'll be a while before they get into Wine proper.
It could, but only if we decide to enable CSMT even on uniprocessor systems, and only if we decide to do all GL calls in the worker thread, including e.g. resource maps and updates.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-01-29 19:11, schrieb Henri Verbeet:
On 29 January 2014 19:04, Chris Robinson chris.kcat@gmail.com wrote:
The multi-threaded command stream patches would fix this issue, wouldn't it? In that case, WineD3D has a GL context on its own thread to handle the D3D calls, leaving the app threads more or less alone. Of course, it'll be a while before they get into Wine proper.
It could, but only if we decide to enable CSMT even on uniprocessor systems, and only if we decide to do all GL calls in the worker thread, including e.g. resource maps and updates.
I agree with Henri on this. We should not rely on the CSMT patches to take care of this. Using it on uniprocessor systems is going to hurt performance. It'd also be very useful to keep the single threaded command stream fully operational (including the optional StrictDrawOrdering) for debugging purposes.
On Jan 29, 2014, at 5:10 AM, Henri Verbeet wrote:
On 29 January 2014 01:47, Ken Thomases ken@codeweavers.com wrote:
The most important things to achieve are a) to not "consume" the only opportunity the app has to set the window's pixel format, and b) to restore the app's current GL context (and implicitly its DC) when returning control to it. I don't think it's necessary to avoid WineD3D ever changing WGL's notion of the current context. Certainly, while a WineD3D GL context is current, GL functions like glClear() will operate on WineD3D's context. If the app thinks its context is current and calls GL functions, its context better really be current. So, WineD3D really needs to restore the app's context before returning control (and it generally does, I think). If it's going to do that, then wglGetCurrentContext() will take care of itself. Same for wglGetCurrentDC().
There may be performance reasons though. The wglMakeCurrent() required to restore the previous GL context is pretty expensive, so applications that have a GL context current while calling into wined3d take a considerable performance hit. I think I've only ever seen 1 or 2 applications that were affected that though, so perhaps we don't care enough.
My point is that the current GL context is exposed through WGL but it also affects the behavior of GL functions (of course). So, while we might be able to hide the change of the current GL context from wglGetCurrentContext(), that doesn't do us any good because we need GL functions to target the right context. There's just no getting around switching the current GL context to WineD3D's when WineD3D needs to call GL functions and then back to the app's when control is returned to it. (Except Chris's point of doing all GL on a different thread.)
Do you know what makes wglMakeCurrent() expensive? For X11, we'll still have to use glXMakeCurrent(), which sets the context drawable, even for my proposed wglMakeContextCurrentWINE(). Maybe it's the setting of the drawable that's expensive. For the Mac, we can just make the context current without setting the drawable again. That might be less expensive, although maybe there's a lot of thread state inside the GL engine that has to be swapped in and out. (I would hope that that would all be stored in the context and switching contexts would be just a matter of swapping a pointer somewhere.)
Extension WGL_WINE_make_current:
BOOL wglMakeContextCurrentWINE(HGLRC hglrc);
The standard WGL function wglMakeCurrent() combines two operations. It sets the a GL context's drawable to the provided DC and it makes the GL context current for the thread. This new function, wglMakeContextCurrentWINE(), would only do the latter. It would make a GL context current for the thread without changing which DC it's associated with. Both the X11 and Mac drivers already maintain the necessary state to do that. They don't need to have the DC passed in again. (If a GL context is associated with separate draw and read DCs using wglMakeContextCurrentARB(), then this function will leave it associated with both DCs.)
This allows WineD3D to faithfully restore the app's previously-current WGL context. This addresses bug 28869.
Maybe. Note that some of the quirk detection code renders to this context. I think all rendering there goes to FBOs, so in that regard it's probably safe, but it does feel a bit fragile. For regular wined3d rendering we do of course need a drawable, although this may work out in practice.
I'm not sure I understand. "quirk detection code renders to this context" – to which context do you refer?
The point of this extension is just for restoring the app's GL context. It would not necessarily be used to make a WineD3D context current, it would only be used to restore the app's WGL context to be current. It doesn't take a DC so that this works even for the case where the original DC was destroyed/released.
(It could be used to make a WineD3D context current a second time, if you don't want to change the draw DC. But to associate the draw DC with the context the first time, you'd still use wglMakeCurrent().)
I think WGL_WINE_surface would work from wined3d's point of view, at the very least interface wise. There are probably some details to work out about how it should interact with gdi32 rendering and things like the window moves, minimization, etc. I don't have as good of an overview of how it would work out for winex11. I'm not so sure about WGL_WINE_make_current, but it may be enough in practice.
OK, thanks. I guess my next step will be to put together a patch to implement this all. I may do it quick-and-dirty at first just to prove the concept.
I _think_ that window moves, etc. should be handled roughly as they are already. For a surface created from a window, things should work almost exactly as they currently do. The main difference would be where the pixel format is stored. With a window DC – how things currently work – it's conceptually stored as a property of the window. With a surface DC, it would be stored as a property of the surface.
The other thing needing an update would be sync_gl_drawable(). That's for window moves and resizes. Currently, it looks up the single drawable for a given HWND. With surfaces, it would have to iterate over the surfaces for that window, as well.
-Ken
On 29 January 2014 22:34, Ken Thomases ken@codeweavers.com wrote:
My point is that the current GL context is exposed through WGL but it also affects the behavior of GL functions (of course). So, while we might be able to hide the change of the current GL context from wglGetCurrentContext(), that doesn't do us any good because we need GL functions to target the right context. There's just no getting around switching the current GL context to WineD3D's when WineD3D needs to call GL functions and then back to the app's when control is returned to it. (Except Chris's point of doing all GL on a different thread.)
Do you know what makes wglMakeCurrent() expensive? For X11, we'll still have to use glXMakeCurrent(), which sets the context drawable, even for my proposed wglMakeContextCurrentWINE(). Maybe it's the setting of the drawable that's expensive. For the Mac, we can just make the context current without setting the drawable again. That might be less expensive, although maybe there's a lot of thread state inside the GL engine that has to be swapped in and out. (I would hope that that would all be stored in the context and switching contexts would be just a matter of swapping a pointer somewhere.)
I'm not sure. I have the impression most of the time is inside the driver instead of e.g. winex11 though.
Maybe. Note that some of the quirk detection code renders to this context. I think all rendering there goes to FBOs, so in that regard it's probably safe, but it does feel a bit fragile. For regular wined3d rendering we do of course need a drawable, although this may work out in practice.
I'm not sure I understand. "quirk detection code renders to this context" – to which context do you refer?
The one created by wined3d_caps_gl_ctx_create(). Quirk detection code like match_broken_arb_fog() uses that context for rendering. (But only ever renders to a FBO.)
The point of this extension is just for restoring the app's GL context. It would not necessarily be used to make a WineD3D context current, it would only be used to restore the app's WGL context to be current. It doesn't take a DC so that this works even for the case where the original DC was destroyed/released.
Right. I was under the impression it would be used by wined3d_caps_gl_ctx_create() to avoid changing the current WGL DC. If it's only for restoring the application's GL context, I'm not sure how that's effectively much different from just making the application's GL context current on wined3d's DC? (I'd have to look into the details again, but IIRC that wasn't enough for bug 28869, even as a hack.)
On Thu, Jan 30, 2014 at 4:34 AM, Henri Verbeet hverbeet@gmail.com wrote:
On 29 January 2014 22:34, Ken Thomases ken@codeweavers.com wrote:
My point is that the current GL context is exposed through WGL but it
also affects the behavior of GL functions (of course). So, while we might be able to hide the change of the current GL context from wglGetCurrentContext(), that doesn't do us any good because we need GL functions to target the right context. There's just no getting around switching the current GL context to WineD3D's when WineD3D needs to call GL functions and then back to the app's when control is returned to it. (Except Chris's point of doing all GL on a different thread.)
Do you know what makes wglMakeCurrent() expensive? For X11, we'll still
have to use glXMakeCurrent(), which sets the context drawable, even for my proposed wglMakeContextCurrentWINE(). Maybe it's the setting of the drawable that's expensive. For the Mac, we can just make the context current without setting the drawable again. That might be less expensive, although maybe there's a lot of thread state inside the GL engine that has to be swapped in and out. (I would hope that that would all be stored in the context and switching contexts would be just a matter of swapping a pointer somewhere.)
I'm not sure. I have the impression most of the time is inside the driver instead of e.g. winex11 though.
At some I had a similar use case regarding hiding contexts more at the SwapBuffers level for individual frames. The initial implementation used context switching which worked, but Nvidia heavily discouraged us from using it. At least their implementation (on Fermi/Kepler at the time) performed a pipeline flush every context switch (I guess at glFlush or worse), which makes glXMakeCurrent a very heavy call.
The other option as Chris said is moving GL to a different thread. Choosing between two evils, I would probably more lean that way.
I have had a quick look at the tickets, but are all the issues ddraw related or are there other known issues for newer d3d versions for which this also helps?
We may also want to take into account future situations in which WineD3D and GL may have conflicts. Maybe at some point we want to handle some of the OpenGL / Direct3D (very Nvidia specifc) interop extensions or the OpenCL versions. At least for D3D/GL interop there are probably funny situations, though we may not care.
On Jan 30, 2014, at 6:34 AM, Henri Verbeet wrote:
On 29 January 2014 22:34, Ken Thomases ken@codeweavers.com wrote:
The point of this extension is just for restoring the app's GL context. It would not necessarily be used to make a WineD3D context current, it would only be used to restore the app's WGL context to be current. It doesn't take a DC so that this works even for the case where the original DC was destroyed/released.
Right. I was under the impression it would be used by wined3d_caps_gl_ctx_create() to avoid changing the current WGL DC. If it's only for restoring the application's GL context, I'm not sure how that's effectively much different from just making the application's GL context current on wined3d's DC? (I'd have to look into the details again, but IIRC that wasn't enough for bug 28869, even as a hack.)
Well, it would be different because it wouldn't change the app's GL context to point at a potentially-different drawable. Which, by the way, is actually happening in bug 28869. The app does GL against a top-level window but uses ddraw/wined3d on a child window of that window. So, your hack wouldn't work because it would make the app's GL context current on a window that's destroyed soon after.
I was investigating that bug further and discovered that it doesn't actually have anything to do with the DC having been deleted. There's a place in wined3d where it clears the GL context and doesn't save the information necessary to restore the app's GL context. So, it just leaves the app with no current GL context so its GL calls all do nothing. Removing that one call fixes that bug.
That bug was the main motivation for this extension. Still, the general principle of being able to restore the GL context even after its DC was destroyed remains.
In experimenting with implementing the extension and making wined3d use it, I discovered a complication. At the same point where wined3d attempts to restore the app's GL context, it also tries to restore the pixel format on the app's DC. On the one hand, my extension doesn't help with that which undermines its rationale since the DC is still required. On the other hand, that's the wrong thing for wined3d to try to do there! Wined3d has set the pixel format on its target window (through a DC it obtained itself). In many cases, the app will be targeting the same window with its GL context, but not always (as we've seen). So, wined3d should be trying to restore the pixel format of the window it modified, not the window the app's GL context may be targeting.
This is a separate bug that should be fixed. Probably, wined3d should keep track of the window on which it set the pixel format and set that window's pixel format back before returning control. (It can use any DC for that window to do that, even one it obtains just for that purpose.)
-Ken
On 5 February 2014 20:20, Ken Thomases ken@codeweavers.com wrote:
Right. I was under the impression it would be used by wined3d_caps_gl_ctx_create() to avoid changing the current WGL DC. If it's only for restoring the application's GL context, I'm not sure how that's effectively much different from just making the application's GL context current on wined3d's DC? (I'd have to look into the details again, but IIRC that wasn't enough for bug 28869, even as a hack.)
Well, it would be different because it wouldn't change the app's GL context to point at a potentially-different drawable. Which, by the way, is actually happening in bug 28869. The app does GL against a top-level window but uses ddraw/wined3d on a child window of that window. So, your hack wouldn't work because it would make the app's GL context current on a window that's destroyed soon after.
Actually, what confused me there was thinking of the current DC as thread state as opposed to context state. So yeah, wglMakeContextCurrentWINE() should work. It does have me wondering slightly if wglMakeCurrent() shouldn't behave like that anyway if the DC it gets passes is the same as the one the context was last current with, but perhaps not enough to go find out.
In experimenting with implementing the extension and making wined3d use it, I discovered a complication. At the same point where wined3d attempts to restore the app's GL context, it also tries to restore the pixel format on the app's DC. On the one hand, my extension doesn't help with that which undermines its rationale since the DC is still required. On the other hand, that's the wrong thing for wined3d to try to do there! Wined3d has set the pixel format on its target window (through a DC it obtained itself). In many cases, the app will be targeting the same window with its GL context, but not always (as we've seen). So, wined3d should be trying to restore the pixel format of the window it modified, not the window the app's GL context may be targeting.
It may not be enough in some cases, and redundant in others, but I don't think it's wrong as such. I.e., if the wined3d GL DC and the application's GL DC aren't the same, restoring the pixel format on the application's GL DC is redundant because we didn't touch it, but it should never restore the wrong pixel format. There are two cases where it wouldn't be enough: - The application has set a pixel format on the wined3d DC before, is not currently using GL on that DC, but will do so (again) in the future. I think this case can be fixed, although it's not entirely trivial because of e.g. context_update_window(), and IMO a bit of a rare case. - The application has never set a pixel format on the wined3d DC, but will try to do so in the future. I don't think we can fix this with the current WGL_WINE_pixel_format_passthrough implementation. As soon as wined3d sets a pixel format, it becomes impossible for the application to do so in the future. This is essentially bug 29934.
In theory this entire issue should go away with wglGetSurfaceDCWINE() though, because in that case we'd never have to restore the pixel format at all, provided there's no way the application can get at the same DC wined3d is using.
On Feb 6, 2014, at 4:19 AM, Henri Verbeet wrote:
… It does have me wondering slightly if wglMakeCurrent() shouldn't behave like that anyway if the DC it gets passes is the same as the one the context was last current with, but perhaps not enough to go find out.
I don't think it can work that way. DC handle values get recycled pretty aggressively. Just because the handle value is the same between point A and point B doesn't mean it references the same window.
On 5 February 2014 20:20, Ken Thomases ken@codeweavers.com wrote:
In experimenting with implementing the extension and making wined3d use it, I discovered a complication. At the same point where wined3d attempts to restore the app's GL context, it also tries to restore the pixel format on the app's DC. On the one hand, my extension doesn't help with that which undermines its rationale since the DC is still required. On the other hand, that's the wrong thing for wined3d to try to do there! Wined3d has set the pixel format on its target window (through a DC it obtained itself). In many cases, the app will be targeting the same window with its GL context, but not always (as we've seen). So, wined3d should be trying to restore the pixel format of the window it modified, not the window the app's GL context may be targeting.
It may not be enough in some cases, and redundant in others, but I don't think it's wrong as such. I.e., if the wined3d GL DC and the application's GL DC aren't the same, restoring the pixel format on the application's GL DC is redundant because we didn't touch it, but it should never restore the wrong pixel format.
Agreed, it won't restore the wrong pixel format.
There are two cases where it wouldn't be enough:
- The application has set a pixel format on the wined3d DC before,
is not currently using GL on that DC, but will do so (again) in the future. I think this case can be fixed, although it's not entirely trivial because of e.g. context_update_window(), and IMO a bit of a rare case.
I think I have a patch for this. It may not be ideal because it tracks the window to restore the pixel format on separately from the context window. (In some cases, e.g. swapchain backup DC, the context window is not what the context DC references.) I'll send it along in a bit. I suppose I need to add a test case, too.
Anyway, I believe it's necessary to decouple restoring the pixel format from restoring the GL context, anyway, in order to use wglMakeContextCurrentWINE().
- The application has never set a pixel format on the wined3d DC,
but will try to do so in the future. I don't think we can fix this with the current WGL_WINE_pixel_format_passthrough implementation. As soon as wined3d sets a pixel format, it becomes impossible for the application to do so in the future. This is essentially bug 29934.
Right. I don't have a solution for that with current APIs but WGL_WINE_surface will sidestep it.
In theory this entire issue should go away with wglGetSurfaceDCWINE() though, because in that case we'd never have to restore the pixel format at all, provided there's no way the application can get at the same DC wined3d is using.
Yeah, but, as I said, I think I need to solve the current case anyway, to the degree it can be solved.
-Ken
I have an implementation of WGL_WINE_surface for the Mac driver and modified wined3d to use it. I still have to implement it for the X11 driver. I'm sending it along here to let folks see where it's going and review/critique my approach.
I also need to update my description from my original proposal, since things didn't quite work how I planned. See below.
The attached patches apply on top of the patches I just sent to wine-patches (patches 103407 through 103414).
On Jan 28, 2014, at 6:47 PM, Ken Thomases wrote:
WGL exposes various pieces of state to the application. Ideally, WineD3D would not alter any of that state since such alterations are visible to the app and Direct3D doesn't do that on Windows. …
- The DC pixel format. …
- The current GL context for the thread. …
- The DC for the current GL context. …
I realized that another piece of WGL state is the swap interval. That is set on the window of the current context, not on the context as such. So, when the context is targeting a surface, the swap interval will instead be set on that surface rather than the window.
Extension WGL_WINE_surface:
HANDLE wglCreateSurfaceWINE(HDC hdc, int pixel_format);
This no longer takes a pixel format argument. Wined3d doesn't have a pixel format chosen at the point where a surface should be created. Instead, one just calls SetPixelFormat() or wglSetPixelFormatWINE() on a surface DC as is currently done for window DCs. Similarly, GetPixelFormat() works on a surface DC. (It shouldn't end up that wglSetPixelFormatWINE() is ever actually called on a surface DC since we won't need to change it after it's first set, but it works.)
Also, instead of using the generic HANDLE type, the extension defines the HSURFACEWINE type.
Destroying a surface while it still has extant surface DCs results in undefined behavior.
This was untenable given the constraints that wined3d operates under. The surface is destroyed when the swapchain's window is changed, but there may still be contexts targeting the surface DC. Those may be current on another thread, so there's no way to clear them and release the DC synchronously. So, I've made the internal object underlying a surface handle reference counted. The handle itself holds one reference, which is released when the handle is destroyed. Each DC obtained by wglGetSurfaceDCWINE() holds an additional reference, which is released during wglReleaseSurfaceDCWINE().
-Ken
(Resending with attachments compressed since my previous message doesn't seem to have made it through. Apologies if anybody receives this twice.)
I have an implementation of WGL_WINE_surface for the Mac driver and modified wined3d to use it. I still have to implement it for the X11 driver. I'm sending it along here to let folks see where it's going and review/critique my approach.
I also need to update my description from my original proposal, since things didn't quite work how I planned. See below.
The attached patches apply on top of the patches I just sent to wine-patches (patches 103407 through 103414).
On Jan 28, 2014, at 6:47 PM, Ken Thomases wrote:
WGL exposes various pieces of state to the application. Ideally, WineD3D would not alter any of that state since such alterations are visible to the app and Direct3D doesn't do that on Windows. …
- The DC pixel format. …
- The current GL context for the thread. …
- The DC for the current GL context. …
I realized that another piece of WGL state is the swap interval. That is set on the window of the current context, not on the context as such. So, when the context is targeting a surface, the swap interval will instead be set on that surface rather than the window.
Extension WGL_WINE_surface:
HANDLE wglCreateSurfaceWINE(HDC hdc, int pixel_format);
This no longer takes a pixel format argument. Wined3d doesn't have a pixel format chosen at the point where a surface should be created. Instead, one just calls SetPixelFormat() or wglSetPixelFormatWINE() on a surface DC as is currently done for window DCs. Similarly, GetPixelFormat() works on a surface DC. (It shouldn't end up that wglSetPixelFormatWINE() is ever actually called on a surface DC since we won't need to change it after it's first set, but it works.)
Also, instead of using the generic HANDLE type, the extension defines the HSURFACEWINE type.
Destroying a surface while it still has extant surface DCs results in undefined behavior.
This was untenable given the constraints that wined3d operates under. The surface is destroyed when the swapchain's window is changed, but there may still be contexts targeting the surface DC. Those may be current on another thread, so there's no way to clear them and release the DC synchronously. So, I've made the internal object underlying a surface handle reference counted. The handle itself holds one reference, which is released when the handle is destroyed. Each DC obtained by wglGetSurfaceDCWINE() holds an additional reference, which is released during wglReleaseSurfaceDCWINE().
-Ken
(Resending with a download link instead of an attachment. The previous attempt got dropped due to its size. Apologies if anybody gets duplicates.)
Hi,
I now have a nearly complete implementation of the WGL_WINE_surface extension for both the X11 and Mac drivers. A tarball of patches is available from http://www.codeweavers.com/xfer/ken/wgl_wine_surface/wgl_surface.tar.bz2. You'll have to enter an access key, "wgl_wine_surface" (without the quotes).
While implementing support for full-screen surfaces in the X11 driver, it became clear that I needed a reference to the wined3d device window or, more generally, the window for which the surface is a substitute. This is necessary to manage focus since the full-screen window has to have X focus but the Win32 focus needs to remain with the device window.
Because of that, the function wglCreateSurfaceWINE() now takes a proxy window parameter. This feels like an ugly kludge but I didn't find a cleaner alternative. I'd be happy to discuss the X windowing issues that I feel lead to this requirement with anybody who may have better ideas.
There are some minor rough edges to the implementation. In particular, in any desktop environment view of all windows (e.g. Exposé or Mission Control on OS X, the Alt-Tab switcher on Linux, etc.) both the device window and the window for the full-screen surface will appear. The device window is likely to be blank because the rendering is happening in the full-screen surface window. On the other hand, the full-screen surface window will not have a title or an icon, while the device window will. There's a bit of split personality there, which may be confusing.
If the user attempts to switch to the device window, the full-screen window should take focus, instead. So, the user should get the desired result and the problem should be merely cosmetic. I have not had the opportunity to test this on Linux. It's possible that some window managers will fight my attempt to redirect focus like that.
I believe I can hide the device window and also copy its title and icon to the full-screen window, which will resolve this. (The device window will only be hidden on the X or Cocoa side. From a Win32 perspective, it will still be visible.)
Other than those rough edges, the main thing holding me back from just submitting these patches is the lack of testing. I would greatly appreciate if folks would give these patches a test and report any issues they introduce.
Things to test, specifically:
* Of course, that they don't break Direct3D games in general. Things that used to work still work.
* They should fix regressions introduced by my commit 4c4552c5, such as bugs 35718, 35975, and 35950.
* They shouldn't have much performance impact other than those regressions. There may be a slight performance improvement since they allow wined3d to skip some housekeeping.
* Full-screen games are actually full-screen, appearing in front of other apps, docks, panels, menus, taskbars, etc.
* But you can switch to another app as well/easily as you could without these patches. Full-screen games don't take over your whole display such that you can't escape without drastic measures.
* After switching away, you can switch back successfully.
* Switching, especially repeated switching, among full-screen resolutions and between full-screen and windowed modes works.
* Mouse input works as expected. In particular, clicks are not offset from where the cursor appears to be, especially after switching resolutions or between windowed and full-screen modes.
* Keyboard input works as expected. The use of input methods such as for Chinese, Japanese, or Korean especially needs testing.
* With the X11 driver, games behave properly in virtual desktop mode, in both windowed and full-screen modes.
* In windowed mode, resizing the window works properly.
If you suspect the patches have introduced a bug, please be sure to double-check that the bug doesn't exist in unpatched Wine. Once you're sure, please report it as a reply to this thread or a reply directly to me.
Thanks very much, Ken
On 21 April 2014 17:49, Ken Thomases ken@codeweavers.com wrote:
While implementing support for full-screen surfaces in the X11 driver, it became clear that I needed a reference to the wined3d device window or, more generally, the window for which the surface is a substitute. This is necessary to manage focus since the full-screen window has to have X focus but the Win32 focus needs to remain with the device window.
Because of that, the function wglCreateSurfaceWINE() now takes a proxy window parameter. This feels like an ugly kludge but I didn't find a cleaner alternative. I'd be happy to discuss the X windowing issues that I feel lead to this requirement with anybody who may have better ideas.
I'll admit to only having looked over the implementation very briefly so far, but are we quite sure we need a separate X window for this?
On Apr 21, 2014, at 12:42 PM, Henri Verbeet wrote:
On 21 April 2014 17:49, Ken Thomases ken@codeweavers.com wrote:
While implementing support for full-screen surfaces in the X11 driver, it became clear that I needed a reference to the wined3d device window or, more generally, the window for which the surface is a substitute. This is necessary to manage focus since the full-screen window has to have X focus but the Win32 focus needs to remain with the device window.
Because of that, the function wglCreateSurfaceWINE() now takes a proxy window parameter. This feels like an ugly kludge but I didn't find a cleaner alternative. I'd be happy to discuss the X windowing issues that I feel lead to this requirement with anybody who may have better ideas.
I'll admit to only having looked over the implementation very briefly so far, but are we quite sure we need a separate X window for this?
Good question. It's not a slam dunk that we do.
We would at least need a separate X window as a child of the device window's whole_window. This much would be just like how it works without my patches or for window surfaces with my patches. We need that separate X window because any given surface may have a different pixel format, and thus X visual, from other surfaces or the Win32 window using normal WGL. Likewise, the swap interval is a property of the drawable, so surfaces and the window should have separate drawables from each other.
After that, we'd need to alter the properties of the existing X window to decouple it from the Win32 properties so it can be a full-screen window without the undesirable changes to its Win32 styles, etc. This might require even more invasive hooks into all of the existing window and event functions. Or it might be cleaner. (Certainly, it would be cleaner in terms of focus management.)
It's worth investigating. I hadn't considered this approach. I'll take a look.
Thanks, Ken
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hi,
I tested this a little bit sooner than expected, thanks to a regression in Starcraft and Age of Empires 2. (Looks like bugs 35950 and 35655).
The good news is that your patches fix the regressions in those games.
Am 2014-04-21 17:49, schrieb Ken Thomases:
- Full-screen games are actually full-screen, appearing in front
of other apps, docks, panels, menus, taskbars, etc.
Kde 4.11.5 here. The fullscreen apps indeed appear fullscreen. But once Starcraft was shifted down and only showing the top ~50 pixels at the bottom of the screen.
Inside a virtual desktop Age of Empires 2 seemed to snap into place after a resolution change. It first rendered at the bottom of the screen and after ~0.5 seconds the rendering was correct.
- But you can switch to another app as well/easily as you could
without these patches. Full-screen games don't take over your whole display such that you can't escape without drastic measures.
Alt-Tab does not work. The game loses focus and stops redrawing, but my other apps don't show up. Switching to a different virtual desktop works. I don't know if this worked in the past with those two games, probably not. I'll retest with plain Wine, as far as the regressions allow.
- After switching away, you can switch back successfully.
I have to select the Wine window. If I select the game window, Wine's rendering window does not take over.
- Mouse input works as expected. In particular, clicks are not
offset from where the cursor appears to be, especially after switching resolutions or between windowed and full-screen modes.
No issues noticed there, but both games draw their own mouse cursor.
I've also tested Warhammer 40k: Dawn of War, it has the same focus loss / regain behavior as the ddraw games.
Hi,
On Apr 23, 2014, at 4:13 PM, Stefan Dösinger wrote:
I tested this a little bit sooner than expected, thanks to a regression in Starcraft and Age of Empires 2. (Looks like bugs 35950 and 35655).
Thanks for testing!
The good news is that your patches fix the regressions in those games.
Nice.
Am 2014-04-21 17:49, schrieb Ken Thomases:
- Full-screen games are actually full-screen, appearing in front
of other apps, docks, panels, menus, taskbars, etc.
Kde 4.11.5 here. The fullscreen apps indeed appear fullscreen. But once Starcraft was shifted down and only showing the top ~50 pixels at the bottom of the screen.
Strange. The only positioning of the full-screen windows that the code ever does is moving it to get_primary_rect() converted from Win32 virtual screen coordinates to root coordinates.
Inside a virtual desktop Age of Empires 2 seemed to snap into place after a resolution change. It first rendered at the bottom of the screen and after ~0.5 seconds the rendering was correct.
Yes, the code watches for root window configuration changes and repositions itself when that happens. It tries to keep itself filling the screen. The delay was probably because the app didn't return to processing events for a bit. I think I can improve that by hooking into X11DRV_resize_desktop().
When it's managed (not in a virtual desktop window), it also sets _NET_WM_STATE_FULLSCREEN which should lead the window manager to keep it in position.
- But you can switch to another app as well/easily as you could
without these patches. Full-screen games don't take over your whole display such that you can't escape without drastic measures.
Alt-Tab does not work. The game loses focus and stops redrawing, but my other apps don't show up. Switching to a different virtual desktop works. I don't know if this worked in the past with those two games, probably not. I'll retest with plain Wine, as far as the regressions allow.
My understanding is that most window managers only keep _NET_WM_STATE_FULLSCREEN windows over everything else as long as they have focus. Did the desktop environment's panels, docks, taskbars, etc. show up when the game lost focus?
The problem may be that I also set _NET_WM_STATE_ABOVE. That's what happens for a WS_EX_TOPMOST window, which wined3d used to use prior to my patches, so I emulated that.
- After switching away, you can switch back successfully.
I have to select the Wine window. If I select the game window, Wine's rendering window does not take over.
I have an additional patch that may fix that. Please try applying http://www.codeweavers.com/xfer/ken/wgl_wine_surface/wgl_surface_fixes_and_diag.patch on top of the other patches.
The main thing it does for this issue is set WM_HINTS.window_group to be the device window. That should convince KDE to allow it to "steal" focus from the device window. (I think what you were encountering is KDE's focus stealing prevention feature.)
I've also reordered setting focus vs. raising the window. And I'm trying a _NET_ACTIVATE_WINDOW request on top of all that.
It also adds a bunch of additional logging for diagnosing other issues.
-Ken
Hi Ken
Your patchset causes X11 errors with the Battle.net client. 100% reproducible, and also crashes within explorer.exe with +synchronous.
I thought it was something else at first due to a bad revert and filed the details here: https://bugs.winehq.org/show_bug.cgi?id=36141
You can download the Battle.net Client here: https://us.battle.net/account/download/ J. Leclanche
On Thu, Apr 24, 2014 at 4:29 PM, Ken Thomases ken@codeweavers.com wrote:
Hi,
On Apr 23, 2014, at 4:13 PM, Stefan Dösinger wrote:
I tested this a little bit sooner than expected, thanks to a regression in Starcraft and Age of Empires 2. (Looks like bugs 35950 and 35655).
Thanks for testing!
The good news is that your patches fix the regressions in those games.
Nice.
Am 2014-04-21 17:49, schrieb Ken Thomases:
- Full-screen games are actually full-screen, appearing in front
of other apps, docks, panels, menus, taskbars, etc.
Kde 4.11.5 here. The fullscreen apps indeed appear fullscreen. But once Starcraft was shifted down and only showing the top ~50 pixels at the bottom of the screen.
Strange. The only positioning of the full-screen windows that the code ever does is moving it to get_primary_rect() converted from Win32 virtual screen coordinates to root coordinates.
Inside a virtual desktop Age of Empires 2 seemed to snap into place after a resolution change. It first rendered at the bottom of the screen and after ~0.5 seconds the rendering was correct.
Yes, the code watches for root window configuration changes and repositions itself when that happens. It tries to keep itself filling the screen. The delay was probably because the app didn't return to processing events for a bit. I think I can improve that by hooking into X11DRV_resize_desktop().
When it's managed (not in a virtual desktop window), it also sets _NET_WM_STATE_FULLSCREEN which should lead the window manager to keep it in position.
- But you can switch to another app as well/easily as you could
without these patches. Full-screen games don't take over your whole display such that you can't escape without drastic measures.
Alt-Tab does not work. The game loses focus and stops redrawing, but my other apps don't show up. Switching to a different virtual desktop works. I don't know if this worked in the past with those two games, probably not. I'll retest with plain Wine, as far as the regressions allow.
My understanding is that most window managers only keep _NET_WM_STATE_FULLSCREEN windows over everything else as long as they have focus. Did the desktop environment's panels, docks, taskbars, etc. show up when the game lost focus?
The problem may be that I also set _NET_WM_STATE_ABOVE. That's what happens for a WS_EX_TOPMOST window, which wined3d used to use prior to my patches, so I emulated that.
- After switching away, you can switch back successfully.
I have to select the Wine window. If I select the game window, Wine's rendering window does not take over.
I have an additional patch that may fix that. Please try applying http://www.codeweavers.com/xfer/ken/wgl_wine_surface/wgl_surface_fixes_and_diag.patch on top of the other patches.
The main thing it does for this issue is set WM_HINTS.window_group to be the device window. That should convince KDE to allow it to "steal" focus from the device window. (I think what you were encountering is KDE's focus stealing prevention feature.)
I've also reordered setting focus vs. raising the window. And I'm trying a _NET_ACTIVATE_WINDOW request on top of all that.
It also adds a bunch of additional logging for diagnosing other issues.
-Ken
Hi,
On Apr 28, 2014, at 6:53 AM, Jerome Leclanche wrote:
Your patchset causes X11 errors with the Battle.net client. 100% reproducible, and also crashes within explorer.exe with +synchronous.
I thought it was something else at first due to a bad revert and filed the details here: https://bugs.winehq.org/show_bug.cgi?id=36141
Thanks. Somebody else already reported something like that in private email and I have a fix already. I've uploaded that fix here http://www.codeweavers.com/xfer/ken/wgl_wine_surface/wgl_surface_fix_list_moving.patch if you want to test. It applies on top of the original patch set plus the additional patch I linked in my previous email on this thread. I hope to have a revised consolidated patch set soon, too.
-Ken
On Mon, Apr 28, 2014 at 12:27 PM, Ken Thomases ken@codeweavers.com wrote:
... Thanks. Somebody else already reported something like that in private email and I have a fix already. I've uploaded that fix here http://www.codeweavers.com/xfer/ken/wgl_wine_surface/wgl_surface_fix_list_moving.patch if you want to test. It applies on top of the original patch set plus the additional patch I linked in my previous email on this thread. I hope to have a revised consolidated patch set soon, too.
That links shows an access forbidden patch requesting an unlock code.
-Ken
Bruno
On Apr 28, 2014, at 11:25 AM, Bruno Jesus wrote:
On Mon, Apr 28, 2014 at 12:27 PM, Ken Thomases ken@codeweavers.com wrote:
... Thanks. Somebody else already reported something like that in private email and I have a fix already. I've uploaded that fix here http://www.codeweavers.com/xfer/ken/wgl_wine_surface/wgl_surface_fix_list_moving.patch if you want to test. It applies on top of the original patch set plus the additional patch I linked in my previous email on this thread. I hope to have a revised consolidated patch set soon, too.
That links shows an access forbidden patch requesting an unlock code.
Sorry, yes. The access code was mentioned in the email where I first linked to my patch set. It is "wgl_wine_surface". I'll try to remember to mention that each time I post such a link.
-Ken
-
Bruno Jesus -
Chris Robinson -
Henri Verbeet -
Jerome Leclanche -
Ken Thomases -
Roderick Colenbrander -
Stefan Dösinger