[PATCH] wined3d: Avoid crash in swapchain_gl_present() if context could not be acquired.
Signed-off-by: Paul Gofman gofmanp@gmail.com --- dlls/wined3d/swapchain.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/dlls/wined3d/swapchain.c b/dlls/wined3d/swapchain.c index 62bab65fcf..e4ffce8965 100644 --- a/dlls/wined3d/swapchain.c +++ b/dlls/wined3d/swapchain.c @@ -453,7 +453,12 @@ static void swapchain_gl_present(struct wined3d_swapchain *swapchain, struct wined3d_context *context; BOOL render_to_fbo;
- context = context_acquire(swapchain->device, swapchain->front_buffer, 0); + if (!(context = context_acquire(swapchain->device, swapchain->front_buffer, 0))) + { + WARN("Could not get context, skipping present.\n"); + return; + } + context_gl = wined3d_context_gl(context); if (!context_gl->valid) {
On Wed, 13 Nov 2019 at 18:45, Paul Gofman gofmanp@gmail.com wrote:
@@ -453,7 +453,12 @@ static void swapchain_gl_present(struct wined3d_swapchain *swapchain, struct wined3d_context *context; BOOL render_to_fbo;
- context = context_acquire(swapchain->device, swapchain->front_buffer, 0);
- if (!(context = context_acquire(swapchain->device, swapchain->front_buffer, 0)))
- {
WARN("Could not get context, skipping present.\n");return;- }
Can this legitimately happen?
On 11/13/19 18:35, Henri Verbeet wrote:
On Wed, 13 Nov 2019 at 18:45, Paul Gofman gofmanp@gmail.com wrote:
@@ -453,7 +453,12 @@ static void swapchain_gl_present(struct wined3d_swapchain *swapchain, struct wined3d_context *context; BOOL render_to_fbo;
- context = context_acquire(swapchain->device, swapchain->front_buffer, 0);
- if (!(context = context_acquire(swapchain->device, swapchain->front_buffer, 0)))
- {
WARN("Could not get context, skipping present.\n");return;- }
Can this legitimately happen?
That what happens with AION game, upon transition from login screen to 3D scene display. I did not unwind the sequence which leads to that, there is quite the time to pass loading to that point, and then it uses some mixture of d3d9 and d3d11 elements rendered to the same main window. Adding this check fixes the crash and it renders everything flawlessly after that. Somehow I though a lot of things can go wrong during acquiring or creating a new context for the thread, especially when the app is recreating Windows and devices. If nothing of that is supposed, I can try to detect what exactly is going on there, but that's going to take some time.
On Wed, 13 Nov 2019 at 19:16, Paul Gofman gofmanp@gmail.com wrote:
That what happens with AION game, upon transition from login screen to 3D scene display. I did not unwind the sequence which leads to that, there is quite the time to pass loading to that point, and then it uses some mixture of d3d9 and d3d11 elements rendered to the same main window. Adding this check fixes the crash and it renders everything flawlessly after that. Somehow I though a lot of things can go wrong during acquiring or creating a new context for the thread, especially when the app is recreating Windows and devices. If nothing of that is supposed, I can try to detect what exactly is going on there, but that's going to take some time.
There are certainly cases where creating a valid OpenGL context could legitimately fail, but for those cases I'd either expect it to fail earlier during swapchain creation, or to fallback to creating a "backup window" context.
On 11/13/19 19:03, Henri Verbeet wrote:
On Wed, 13 Nov 2019 at 19:16, Paul Gofman gofmanp@gmail.com wrote:
That what happens with AION game, upon transition from login screen to 3D scene display. I did not unwind the sequence which leads to that, there is quite the time to pass loading to that point, and then it uses some mixture of d3d9 and d3d11 elements rendered to the same main window. Adding this check fixes the crash and it renders everything flawlessly after that. Somehow I though a lot of things can go wrong during acquiring or creating a new context for the thread, especially when the app is recreating Windows and devices. If nothing of that is supposed, I can try to detect what exactly is going on there, but that's going to take some time.
There are certainly cases where creating a valid OpenGL context could legitimately fail, but for those cases I'd either expect it to fail earlier during swapchain creation, or to fallback to creating a "backup window" context.
I managed to get some more details on that. The NULL goes from wined3d_context_gl_acquire(), "Rendering onscreen" case. wined3d_context_gl_init() fails with 'Failed to set pixel format 27...'. This most likely happens because window is already destroyed. This window and device is likely not going to be used anymore, it only needs to survive the destruction without crashing the application.
On Wed, 13 Nov 2019 at 19:38, Paul Gofman gofmanp@gmail.com wrote:
On 11/13/19 19:03, Henri Verbeet wrote:
There are certainly cases where creating a valid OpenGL context could legitimately fail, but for those cases I'd either expect it to fail earlier during swapchain creation, or to fallback to creating a "backup window" context.
I managed to get some more details on that. The NULL goes from wined3d_context_gl_acquire(), "Rendering onscreen" case. wined3d_context_gl_init() fails with 'Failed to set pixel format 27...'. This most likely happens because window is already destroyed. This window and device is likely not going to be used anymore, it only needs to survive the destruction without crashing the application.
If the window is already destroyed, I'd expect the earlier GetDCEx() call to fail, in which case wined3d_context_gl_init() would try to create a backup context. We could potentially handle wined3d_context_gl_set_pixel_format() failures in the same way, but it would be good to know why that's happening first.
On 11/13/19 19:30, Henri Verbeet wrote:
On Wed, 13 Nov 2019 at 19:38, Paul Gofman gofmanp@gmail.com wrote:
On 11/13/19 19:03, Henri Verbeet wrote:
There are certainly cases where creating a valid OpenGL context could legitimately fail, but for those cases I'd either expect it to fail earlier during swapchain creation, or to fallback to creating a "backup window" context.
I managed to get some more details on that. The NULL goes from wined3d_context_gl_acquire(), "Rendering onscreen" case. wined3d_context_gl_init() fails with 'Failed to set pixel format 27...'. This most likely happens because window is already destroyed. This window and device is likely not going to be used anymore, it only needs to survive the destruction without crashing the application.
If the window is already destroyed, I'd expect the earlier GetDCEx() call to fail, in which case wined3d_context_gl_init() would try to create a backup context. We could potentially handle wined3d_context_gl_set_pixel_format() failures in the same way, but it would be good to know why that's happening first.
Yes, the window handle is actually valid, as well as dc. Yet wined3d_context_gl_set_pixel_format() called from wined3d_context_gl_init() fails. The failure comes from winex11.drv, create_gl_drawable() fails through create_client_window(), which in turn gets NULL get_win_data() and refuses to create the data here. This is some (WS_POPUP | WS_SYSMENU) window created with 1x1 size which never changes, and looks like it never get shown. It does not look like the application is serious about trying to display something with it, as nothing seems to be missing on the display, yet it create d3d11 device there (while the game is mostly d3d9), draws a few frames and gives up. Do you think that wined3d should try to work around any possible reason when display driver fails to create opengl visual or can't set pixel format for some other reason? If the context is not supposed to be NULL in swapchain present under normal circumnstances, maybe we put ERR() there for the time being?
On 11/13/19 21:16, Paul Gofman wrote:
Yes, the window handle is actually valid, as well as dc. Yet wined3d_context_gl_set_pixel_format() called from wined3d_context_gl_init() fails. The failure comes from winex11.drv, create_gl_drawable() fails through create_client_window(), which in turn gets NULL get_win_data() and refuses to create the data here. This is some (WS_POPUP | WS_SYSMENU) window created with 1x1 size which never changes, and looks like it never get shown. It does not look like the application is serious about trying to display something with it, as nothing seems to be missing on the display, yet it create d3d11 device there (while the game is mostly d3d9), draws a few frames and gives up. Do you think that wined3d should try to work around any possible reason when display driver fails to create opengl visual or can't set pixel format for some other reason? If the context is not supposed to be NULL in swapchain present under normal circumnstances, maybe we put ERR() there for the time being?
A bit of more details on this. The swapchain for which swapchain_gl_present() fails is not a main device swapchain. The main device swapchain is created for the other window and is fine. That failing swapchain is created from dxgi factory (dxgi_factory_CreateSwapChain).
The creation of the swapchain succeeds, wined3d does not try to create a GL context during that.
Then it gets swapchain backbuffer (dxgi d3d11_swapchain_GetBuffer) and renders something to this backbuffer as RTV.
Then it calls d3d11_swapchain_Present() for this swapchain, and here is when it tries to create GL context for the first time and fails in SetPixelFormat.
On Thu, 14 Nov 2019 at 15:47, Paul Gofman gofmanp@gmail.com wrote:
On 11/13/19 21:16, Paul Gofman wrote:
Yes, the window handle is actually valid, as well as dc. Yet wined3d_context_gl_set_pixel_format() called from wined3d_context_gl_init() fails. The failure comes from winex11.drv, create_gl_drawable() fails through create_client_window(), which in turn gets NULL get_win_data() and refuses to create the data here. This is some (WS_POPUP | WS_SYSMENU) window created with 1x1 size which never changes, and looks like it never get shown. It does not look like the application is serious about trying to display something with it, as nothing seems to be missing on the display, yet it create d3d11 device there (while the game is mostly d3d9), draws a few frames and gives up. Do you think that wined3d should try to work around any possible reason when display driver fails to create opengl visual or can't set pixel format for some other reason? If the context is not supposed to be NULL in swapchain present under normal circumnstances, maybe we put ERR() there for the time being?
A bit of more details on this. The swapchain for which swapchain_gl_present() fails is not a main device swapchain. The main device swapchain is created for the other window and is fine. That failing swapchain is created from dxgi factory (dxgi_factory_CreateSwapChain).
The creation of the swapchain succeeds, wined3d does not try to create a GL context during that.
Then it gets swapchain backbuffer (dxgi d3d11_swapchain_GetBuffer) and renders something to this backbuffer as RTV.
Then it calls d3d11_swapchain_Present() for this swapchain, and here is when it tries to create GL context for the first time and fails in SetPixelFormat.
I think that makes sense, although it's still not entirely clear to me why setting the pixel format fails. As for fixing the issue, wined3d_context_gl_init() should probably take the same approach as wined3d_context_gl_set_gl_context(), or perhaps things can be reordered in a way that wined3d_context_gl_init() can simply call wined3d_context_gl_set_gl_context().
On 11/14/19 17:41, Henri Verbeet wrote:
On Thu, 14 Nov 2019 at 15:47, Paul Gofman gofmanp@gmail.com wrote:
On 11/13/19 21:16, Paul Gofman wrote:
Yes, the window handle is actually valid, as well as dc. Yet wined3d_context_gl_set_pixel_format() called from wined3d_context_gl_init() fails. The failure comes from winex11.drv, create_gl_drawable() fails through create_client_window(), which in turn gets NULL get_win_data() and refuses to create the data here. This is some (WS_POPUP | WS_SYSMENU) window created with 1x1 size which never changes, and looks like it never get shown. It does not look like the application is serious about trying to display something with it, as nothing seems to be missing on the display, yet it create d3d11 device there (while the game is mostly d3d9), draws a few frames and gives up. Do you think that wined3d should try to work around any possible reason when display driver fails to create opengl visual or can't set pixel format for some other reason? If the context is not supposed to be NULL in swapchain present under normal circumnstances, maybe we put ERR() there for the time being?
A bit of more details on this. The swapchain for which swapchain_gl_present() fails is not a main device swapchain. The main device swapchain is created for the other window and is fine. That failing swapchain is created from dxgi factory (dxgi_factory_CreateSwapChain).
The creation of the swapchain succeeds, wined3d does not try to create a GL context during that.
Then it gets swapchain backbuffer (dxgi d3d11_swapchain_GetBuffer) and renders something to this backbuffer as RTV.
Then it calls d3d11_swapchain_Present() for this swapchain, and here is when it tries to create GL context for the first time and fails in SetPixelFormat.
I think that makes sense, although it's still not entirely clear to me why setting the pixel format fails. As for fixing the issue, wined3d_context_gl_init() should probably take the same approach as wined3d_context_gl_set_gl_context(), or perhaps things can be reordered in a way that wined3d_context_gl_init() can simply call wined3d_context_gl_set_gl_context().
But the application is trying to present a swapchain into the window it specified, the resource is onscreen (if we imagine that it is going to draw something in some other case with a visible window, as here the window is 1x1 and invisible). Does it make sense if we create another invisible window instead and present something to the drawable obtained from it?
On Thu, 14 Nov 2019 at 18:22, Paul Gofman gofmanp@gmail.com wrote:
On 11/14/19 17:41, Henri Verbeet wrote:
I think that makes sense, although it's still not entirely clear to me why setting the pixel format fails. As for fixing the issue, wined3d_context_gl_init() should probably take the same approach as wined3d_context_gl_set_gl_context(), or perhaps things can be reordered in a way that wined3d_context_gl_init() can simply call wined3d_context_gl_set_gl_context().
But the application is trying to present a swapchain into the window it specified, the resource is onscreen (if we imagine that it is going to draw something in some other case with a visible window, as here the window is 1x1 and invisible). Does it make sense if we create another invisible window instead and present something to the drawable obtained from it?
Somewhat, in the sense that it gives us the option of using e.g. (gdi32) BitBlt() to still get the contents into the target window, if we really needed to.
That isn't entirely the point though. Part of the issue is that the behaviour for creating a new context should be consistent with the behaviour for reusing an existing one. Another part is that for better or worse, the model we currently have is that provided OpenGL works at all, context_acquire() isn't supposed to fail. It may make sense to rethink that, but we have a lot more context_acquire() calls besides the one in swapchain_gl_present().
The background is that the target window can go away at any given point, and D3D is supposed to keep working when that happens. Modern versions of OpenGL/GLX can make a context current without a drawable, but originally the drawable was required. Provided FBOs are available current wined3d always renders offscreen and only needs an onscreen context for Present(), but in the past that was not the case either. (I.e., think Opengl 1.x or 2.x and "backbuffer" ORM.)
On 11/14/19 18:23, Henri Verbeet wrote:
But the application is trying to present a swapchain into the window it specified, the resource is onscreen (if we imagine that it is going to draw something in some other case with a visible window, as here the window is 1x1 and invisible). Does it make sense if we create another invisible window instead and present something to the drawable obtained from it?
Somewhat, in the sense that it gives us the option of using e.g. (gdi32) BitBlt() to still get the contents into the target window, if we really needed to.
That isn't entirely the point though. Part of the issue is that the behaviour for creating a new context should be consistent with the behaviour for reusing an existing one. Another part is that for better or worse, the model we currently have is that provided OpenGL works at all, context_acquire() isn't supposed to fail. It may make sense to rethink that, but we have a lot more context_acquire() calls besides the one in swapchain_gl_present().
The background is that the target window can go away at any given point, and D3D is supposed to keep working when that happens. Modern versions of OpenGL/GLX can make a context current without a drawable, but originally the drawable was required. Provided FBOs are available current wined3d always renders offscreen and only needs an onscreen context for Present(), but in the past that was not the case either. (I.e., think Opengl 1.x or 2.x and "backbuffer" ORM.)
Thanks for the explanation, I see now.
So I will make wined3d_context_gl_init() create backup context as well if it fails to set pixel format (and maybe as well if wglCreateContext() fails?).
On Thu, 14 Nov 2019 at 20:05, Paul Gofman gofmanp@gmail.com wrote:
On 11/14/19 18:23, Henri Verbeet wrote:
Somewhat, in the sense that it gives us the option of using e.g. (gdi32) BitBlt() to still get the contents into the target window, if we really needed to.
That isn't entirely the point though. Part of the issue is that the behaviour for creating a new context should be consistent with the behaviour for reusing an existing one. Another part is that for better or worse, the model we currently have is that provided OpenGL works at all, context_acquire() isn't supposed to fail. It may make sense to rethink that, but we have a lot more context_acquire() calls besides the one in swapchain_gl_present().
The background is that the target window can go away at any given point, and D3D is supposed to keep working when that happens. Modern versions of OpenGL/GLX can make a context current without a drawable, but originally the drawable was required. Provided FBOs are available current wined3d always renders offscreen and only needs an onscreen context for Present(), but in the past that was not the case either. (I.e., think Opengl 1.x or 2.x and "backbuffer" ORM.)
Thanks for the explanation, I see now.
So I will make wined3d_context_gl_init() create backup context as well if it fails to set pixel format (and maybe as well if wglCreateContext() fails?).
Right.
On 11/14/19 19:42, Henri Verbeet wrote:
On Thu, 14 Nov 2019 at 20:05, Paul Gofman gofmanp@gmail.com wrote:
On 11/14/19 18:23, Henri Verbeet wrote:
Somewhat, in the sense that it gives us the option of using e.g. (gdi32) BitBlt() to still get the contents into the target window, if we really needed to.
That isn't entirely the point though. Part of the issue is that the behaviour for creating a new context should be consistent with the behaviour for reusing an existing one. Another part is that for better or worse, the model we currently have is that provided OpenGL works at all, context_acquire() isn't supposed to fail. It may make sense to rethink that, but we have a lot more context_acquire() calls besides the one in swapchain_gl_present().
The background is that the target window can go away at any given point, and D3D is supposed to keep working when that happens. Modern versions of OpenGL/GLX can make a context current without a drawable, but originally the drawable was required. Provided FBOs are available current wined3d always renders offscreen and only needs an onscreen context for Present(), but in the past that was not the case either. (I.e., think Opengl 1.x or 2.x and "backbuffer" ORM.)
Thanks for the explanation, I see now.
So I will make wined3d_context_gl_init() create backup context as well if it fails to set pixel format (and maybe as well if wglCreateContext() fails?).
Right.
FWIW, I now figured out what exactly is wrong with that window. The window is created by a different process, and get_win_data() in winex11.drv/windows.c obviously can't find the window data which was set in another process to its own display. The attached patch works that around and SetPixelFormat() succeeds after that. This looks like something to be done in winex11.drv (but I realize that the rendering to other process windows might require much more than this). Yet I suppose it still worth handling the failures in gl context creation in wined3d_context_gl_init() as we discussed before.
-
Henri Verbeet -
Paul Gofman