[PATCH v4 0/2] MR2032: winex11: Allow replacing either context in wglShareLists.
The deleted test is a bad test: The behavior is different on different drivers, for example, the test fails on the Windows 10 and 11 testbot machines that have AMD video cards. Furthermore, MSDN does not say that the destination context "must not" have any display lists yet but rather that it "should not" have any.[1] The Khronos OpenGL Wiki similarly advises against calling wglShareLists after the source or destination context has at least one object, but if you do, it only says that "there is a chance that wglShareLists will fail", not that it will necessarily fail.[2] Since there's no clear "right" behavior here, we can adopt the more permissive behavior that some programs expect, as long as it doesn't corrupt the context. [1] https://learn.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-wglshar... [2] https://www.khronos.org/opengl/wiki/Platform_specifics:_Windows#wglShareList... Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=11436 -- v4: winex11: Allow replacing either context in wglShareLists. opengl32/tests: Make the wglShareLists tests comprehensive. https://gitlab.winehq.org/wine/wine/-/merge_requests/2032
From: Alex Henrie <alexhenrie24(a)gmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51311 --- dlls/opengl32/tests/opengl.c | 118 ++++++++++++++++++++++------------- 1 file changed, 74 insertions(+), 44 deletions(-) diff --git a/dlls/opengl32/tests/opengl.c b/dlls/opengl32/tests/opengl.c index 6979e00792e..e4a4ad424ed 100644 --- a/dlls/opengl32/tests/opengl.c +++ b/dlls/opengl32/tests/opengl.c @@ -541,54 +541,84 @@ static void test_setpixelformat(HDC winhdc) static void test_sharelists(HDC winhdc) { - HGLRC hglrc1, hglrc2, hglrc3; - BOOL res; - - hglrc1 = wglCreateContext(winhdc); - res = wglShareLists(0, 0); - ok(res == FALSE, "Sharing display lists for no contexts passed!\n"); + BOOL res, nvidia, amd, source_current, source_sharing, dest_current, dest_sharing; + HGLRC source, dest, other; - /* Test 1: Create a context and just share lists without doing anything special */ - hglrc2 = wglCreateContext(winhdc); - if(hglrc2) - { - res = wglShareLists(hglrc1, hglrc2); - ok(res, "Sharing of display lists failed\n"); - wglDeleteContext(hglrc2); - } + res = wglShareLists(NULL, NULL); + ok(!res, "Sharing display lists for no contexts passed!\n"); - /* Test 2: Share display lists with a 'destination' context which has been made current */ - hglrc2 = wglCreateContext(winhdc); - if(hglrc2) - { - res = wglMakeCurrent(winhdc, hglrc2); - ok(res, "Make current failed\n"); - res = wglShareLists(hglrc1, hglrc2); - todo_wine ok(res, "Sharing display lists with a destination context which has been made current failed\n"); - wglMakeCurrent(0, 0); - wglDeleteContext(hglrc2); - } + nvidia = !!strstr((const char*)glGetString(GL_VENDOR), "NVIDIA"); + amd = !!strstr((const char*)glGetString(GL_VENDOR), "AMD"); + if (!amd) amd = !!strstr((const char*)glGetString(GL_VENDOR), "ATI"); - /* Test 3: Share display lists with a context which already shares display lists with another context. - * According to MSDN the second parameter cannot share any display lists but some buggy drivers might allow it */ - hglrc3 = wglCreateContext(winhdc); - if(hglrc3) + for (source_current = FALSE; source_current <= TRUE; source_current++) { - res = wglShareLists(hglrc3, hglrc1); - ok(res == FALSE, "Sharing of display lists passed for a context which already shared lists before\n"); - wglDeleteContext(hglrc3); - } - - /* Test 4: Share display lists with a 'source' context which has been made current */ - hglrc2 = wglCreateContext(winhdc); - if(hglrc2) - { - res = wglMakeCurrent(winhdc, hglrc1); - ok(res, "Make current failed\n"); - res = wglShareLists(hglrc1, hglrc2); - ok(res, "Sharing display lists with a source context which has been made current failed\n"); - wglMakeCurrent(0, 0); - wglDeleteContext(hglrc2); + for (source_sharing = FALSE; source_sharing <= TRUE; source_sharing++) + { + for (dest_current = FALSE; dest_current <= TRUE; dest_current++) + { + for (dest_sharing = FALSE; dest_sharing <= TRUE; dest_sharing++) + { + winetest_push_context("source_current=%d source_sharing=%d dest_current=%d dest_sharing=%d", + source_current, source_sharing, dest_current, dest_sharing); + + source = wglCreateContext(winhdc); + ok(!!source, "Create source context failed\n"); + dest = wglCreateContext(winhdc); + ok(!!dest, "Create dest context failed\n"); + other = wglCreateContext(winhdc); + ok(!!other, "Create other context failed\n"); + + if (source_current) + { + res = wglMakeCurrent(winhdc, source); + ok(res, "Make source current failed\n"); + } + if (source_sharing) + { + res = wglShareLists(other, source); + todo_wine_if(source_current) + ok(res, "Sharing of display lists from other to source failed\n"); + } + if (dest_current) + { + res = wglMakeCurrent(winhdc, dest); + ok(res, "Make dest current failed\n"); + } + if (dest_sharing) + { + res = wglShareLists(other, dest); + todo_wine_if(dest_current) + ok(res, "Sharing of display lists from other to dest failed\n"); + } + + res = wglShareLists(source, dest); + todo_wine_if(dest_current || dest_sharing) + ok(res || broken(nvidia && !source_sharing && dest_sharing), + "Sharing of display lists from source to dest failed\n"); + + if (source_current || dest_current) + { + res = wglMakeCurrent(NULL, NULL); + ok(res, "Make none current failed\n"); + } + res = wglDeleteContext(source); + ok(res, "Delete source context failed\n"); + res = wglDeleteContext(dest); + ok(res, "Delete dest context failed\n"); + if (!strcmp(winetest_platform, "wine") || !amd || source_sharing || !dest_sharing) + { + /* If source_sharing=FALSE and dest_sharing=TRUE, wglShareLists succeeds on AMD, but + * sometimes wglDeleteContext crashes afterwards. On Wine, both functions should always + * succeed in this case. */ + res = wglDeleteContext(other); + ok(res, "Delete other context failed\n"); + } + + winetest_pop_context(); + } + } + } } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2032
From: Alex Henrie <alexhenrie24(a)gmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=11436 --- dlls/opengl32/tests/opengl.c | 5 ++--- dlls/winex11.drv/opengl.c | 41 +++++++++++++++++++----------------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/dlls/opengl32/tests/opengl.c b/dlls/opengl32/tests/opengl.c index e4a4ad424ed..3813ed17fb2 100644 --- a/dlls/opengl32/tests/opengl.c +++ b/dlls/opengl32/tests/opengl.c @@ -577,7 +577,6 @@ static void test_sharelists(HDC winhdc) if (source_sharing) { res = wglShareLists(other, source); - todo_wine_if(source_current) ok(res, "Sharing of display lists from other to source failed\n"); } if (dest_current) @@ -588,12 +587,12 @@ static void test_sharelists(HDC winhdc) if (dest_sharing) { res = wglShareLists(other, dest); - todo_wine_if(dest_current) + todo_wine_if(source_sharing && dest_current) ok(res, "Sharing of display lists from other to dest failed\n"); } res = wglShareLists(source, dest); - todo_wine_if(dest_current || dest_sharing) + todo_wine_if((source_current || source_sharing) && (dest_current || dest_sharing)) ok(res || broken(nvidia && !source_sharing && dest_sharing), "Sharing of display lists from source to dest failed\n"); diff --git a/dlls/winex11.drv/opengl.c b/dlls/winex11.drv/opengl.c index 565df8bb4c6..8c1ab90595a 100644 --- a/dlls/winex11.drv/opengl.c +++ b/dlls/winex11.drv/opengl.c @@ -1916,6 +1916,8 @@ done: */ static BOOL glxdrv_wglShareLists(struct wgl_context *org, struct wgl_context *dest) { + struct wgl_context *keep, *clobber; + TRACE("(%p, %p)\n", org, dest); /* Sharing of display lists works differently in GLX and WGL. In case of GLX it is done @@ -1926,34 +1928,35 @@ static BOOL glxdrv_wglShareLists(struct wgl_context *org, struct wgl_context *de * so there delaying context creation doesn't work. * * The new approach is to create a GLX context in wglCreateContext / wglCreateContextAttribsARB - * and when a program requests sharing we recreate the destination context if it hasn't been made - * current or when it hasn't shared display lists before. + * and when a program requests sharing we recreate the destination or source context if it + * hasn't been made current and it hasn't shared display lists before. */ - if(dest->has_been_current) + if (!dest->has_been_current && !dest->sharing) { - ERR("Could not share display lists because the destination context has already been current\n"); - return FALSE; + keep = org; + clobber = dest; } - else if(dest->sharing) + else if (!org->has_been_current && !org->sharing) { - ERR("Could not share display lists because the destination context has already shared lists\n"); - return FALSE; + keep = dest; + clobber = org; } else { - /* Re-create the GLX context and share display lists */ - pglXDestroyContext(gdi_display, dest->ctx); - dest->ctx = create_glxcontext(gdi_display, dest, org->ctx); - TRACE(" re-created context (%p) for Wine context %p (%s) sharing lists with ctx %p (%s)\n", - dest->ctx, dest, debugstr_fbconfig(dest->fmt->fbconfig), - org->ctx, debugstr_fbconfig( org->fmt->fbconfig)); - - org->sharing = TRUE; - dest->sharing = TRUE; - return TRUE; + ERR("Could not share display lists because both of the contexts have already been current or shared\n"); + return FALSE; } - return FALSE; + + pglXDestroyContext(gdi_display, clobber->ctx); + clobber->ctx = create_glxcontext(gdi_display, clobber, keep->ctx); + TRACE("re-created context (%p) for Wine context %p (%s) sharing lists with ctx %p (%s)\n", + clobber->ctx, clobber, debugstr_fbconfig(clobber->fmt->fbconfig), + keep->ctx, debugstr_fbconfig(keep->fmt->fbconfig)); + + org->sharing = TRUE; + dest->sharing = TRUE; + return TRUE; } static void wglFinish(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2032
@rbernon I just pushed the new tests and a revised fix that refactors the code as you suggested. I hope that it is clear now what exactly is being fixed. Would you be willing to review the two new patches? By the way, I was surprised to find that in the cases where wglShareLists succeeds on AMD and fails on NVIDIA, the AMD testbot crashes later in wglDestroyContext. The same tests pass without crashing on my own Windows 10 desktop that has an RX 5500 XT. My opinion remains unchanged that having Wine's wglShareLists succeed (and Wine's wglDestroyContext not crash) is the right thing to do. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2032#note_29425
In case anyone is wondering, the specific case that Pepakura needs to succeed is source_current=0 source_sharing=0 dest_current=1 dest_sharing=0. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2032#note_29426
Imho it looks good now, and we're clearly following the most common case (with only NVIDIA being broken). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2032#note_29543
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2032
participants (3)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Rémi Bernon