[PATCH v3 0/1] 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 -- v3: winex11: Allow replacing either context in wglShareLists. https://gitlab.winehq.org/wine/wine/-/merge_requests/2032
From: Alex Henrie <alexhenrie24(a)gmail.com> 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 --- dlls/opengl32/tests/opengl.c | 14 ++---------- dlls/winex11.drv/opengl.c | 41 +++++++++++++++++++----------------- 2 files changed, 24 insertions(+), 31 deletions(-) diff --git a/dlls/opengl32/tests/opengl.c b/dlls/opengl32/tests/opengl.c index 6979e00792e..d48b08825af 100644 --- a/dlls/opengl32/tests/opengl.c +++ b/dlls/opengl32/tests/opengl.c @@ -541,7 +541,7 @@ static void test_setpixelformat(HDC winhdc) static void test_sharelists(HDC winhdc) { - HGLRC hglrc1, hglrc2, hglrc3; + HGLRC hglrc1, hglrc2; BOOL res; hglrc1 = wglCreateContext(winhdc); @@ -569,17 +569,7 @@ static void test_sharelists(HDC winhdc) wglDeleteContext(hglrc2); } - /* 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) - { - 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 */ + /* Test 3: Share display lists with a 'source' context which has been made current */ hglrc2 = wglCreateContext(winhdc); if(hglrc2) { diff --git a/dlls/winex11.drv/opengl.c b/dlls/winex11.drv/opengl.c index 565df8bb4c6..2ef107859f9 100644 --- a/dlls/winex11.drv/opengl.c +++ b/dlls/winex11.drv/opengl.c @@ -1926,34 +1926,37 @@ 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; - } - else if(dest->sharing) - { - ERR("Could not share display lists because the destination context has already shared lists\n"); - return FALSE; - } - else - { - /* Re-create the GLX context and share display lists */ + /* Re-create the destination 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; } - return FALSE; + else if (!org->has_been_current && !org->sharing) + { + /* Re-create the source GLX context and share display lists */ + pglXDestroyContext(gdi_display, org->ctx); + org->ctx = create_glxcontext(gdi_display, org, dest->ctx); + TRACE(" re-created context (%p) for Wine context %p (%s) sharing lists with ctx %p (%s)\n", + org->ctx, org, debugstr_fbconfig(org->fmt->fbconfig), + dest->ctx, debugstr_fbconfig(dest->fmt->fbconfig)); + } + else + { + ERR("Could not share display lists because both of the contexts have already been current or shared\n"); + return FALSE; + } + + org->sharing = TRUE; + dest->sharing = TRUE; + return TRUE; } static void wglFinish(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2032
Rémi Bernon (@rbernon) commented about dlls/winex11.drv/opengl.c:
} - return FALSE; + else if (!org->has_been_current && !org->sharing) + { + /* Re-create the source GLX context and share display lists */ + pglXDestroyContext(gdi_display, org->ctx); + org->ctx = create_glxcontext(gdi_display, org, dest->ctx); + TRACE(" re-created context (%p) for Wine context %p (%s) sharing lists with ctx %p (%s)\n", + org->ctx, org, debugstr_fbconfig(org->fmt->fbconfig), + dest->ctx, debugstr_fbconfig(dest->fmt->fbconfig)); + } + else + { + ERR("Could not share display lists because both of the contexts have already been current or shared\n"); + return FALSE; + }
I have no idea what this change is about, and the only comment I can probably make about it is that it seems to be alright (from what I can read and tell), and that this could be factored out, first choosing which context is best to destroy, and then re-creating it or failing to. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2032#note_29036
On Wed Apr 5 17:27:10 2023 +0000, Rémi Bernon wrote:
I have no idea what this change is about, and the only comment I can probably make about it is that it seems to be alright (from what I can read and tell), and that this could be factored out, first choosing which context is best to destroy, and then re-creating it or failing to. Thanks for the feedback. Who is the right person to review this code? I thought either Stefan or you would be familiar with the problem, and I'm at a loss as to who else to ask.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2032#note_29037
On Wed Apr 5 17:33:04 2023 +0000, Alex Henrie wrote:
Thanks for the feedback. Who is the right person to review this code? I thought either Stefan or you would be familiar with the problem, and I'm at a loss as to who else to ask. I don't know, and at some point it's maybe just a matter of whether this seems to make sense, and testing it. The test indeed passes on AMD, so maybe it's fine?
At the same time you say that you tried on an NVidia machine and the test passes -it doesn't allow sharing with a destination context that's been used? or do you mean it succeeds sharing and fails the `ok`?- how can the application also work then? If this test is supposed to be reversed, maybe instead of removing it you should reverse its expectations, adding a broken result for all the cases where it fails, and adding a comment about what we chose as the "right" case to make it more explicit. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2032#note_29046
The test indeed passes on AMD, so maybe it's fine?
The test fails on AMD, which is one reason why I want to remove it. See [Bug 51311](https://bugs.winehq.org/show_bug.cgi?id=51311).
At the same time you say that you tried on an NVidia machine and the test passes -it doesn't allow sharing with a destination context that's been used?
NVIDIA does not allow sharing to a context that is already sharing with another context, so the test passes on NVIDIA.
how can the application also work then?
The application depends on a different but related behavior, which Wine does not test: Whether a context can share to a context that is not _sharing_ yet, but has been made _current_. The application could be fixed without touching the test or changing the behavior of sharing to a context that is already sharing. Doing it that way would be as simple as changing `else if (!org->has_been_current && !org->sharing)` to `else if (!org->has_been_current && !org->sharing && !dest->sharing)`. However, it would look weird to treat a context that is already sharing differently from a context that has already been made current, and there doesn't appear to be any good reason for implementing that restriction in the first place, seeing as the test for it fails on AMD, and the comment above the test implies that it was written to test a strict interpretation of the WGL spec rather than testing what applications actually expect in the real world.
If this test is supposed to be reversed, maybe instead of removing it you should reverse its expectations, adding a broken result for all the cases where it fails, and adding a comment about what we chose as the "right" case to make it more explicit.
That's not a bad idea. Maybe it _would_ be better to just declare the AMD behavior for a context that is already sharing to be the "correct" behavior and add a second test that declares the "correct" behavior for a context that has already been made current. That way we would at least have tests that show that Wine is behaving as intended. If I make the changes to the tests, would you be willing to review/approve this patch, or do I still need to look for someone else? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2032#note_29111
The application depends on a different but related behavior, which Wine does not test: Whether a context can share to a context that is not *sharing* yet, but has been made *current*.
I think that's "Test 2" isn't it? If you mean the source context, then it's "Test 4", although yeah it also tests with a context which has already shared lists (though its peer has been destroyed since then, so maybe that has some importance). Imho you should probably clean these tests up, testing only one aspect at a time (and then the combined aspects), and add more tests if they don't cover this use case. Then, see if the fix implementation still fits, or adjust it. If some tests pass only under some specific driver, we can decide which behavior to chose, but at the same time it also has to mean that the application doesn't run on this hardware. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2032#note_29119
On Thu Apr 6 06:54:41 2023 +0000, Rémi Bernon wrote:
The application depends on a different but related behavior, which Wine does not test: Whether a context can share to a context that is not *sharing* yet, but has been made *current*. I think that's "Test 2" isn't it? If you mean the source context, then it's "Test 4", although yeah it also tests with a context which has already shared lists (though its peer has been destroyed since then, so maybe that has some importance). Imho you should probably clean these tests up, testing only one aspect at a time (and then the combined aspects), and add more tests if they don't cover this use case. Then, see if the fix implementation still fits, or adjust it. If some tests pass only under some specific driver, we can decide which behavior to chose, but at the same time it also has to mean that the application doesn't run on this hardware. You're right, "Test 2" covers sharing to a context that has been made current. I overlooked it because that test still fails even with my fix because it's sharing from a context that is already sharing.
Based on your feedback, I think we need to test all 16 combinations of the 4 relevant booleans: source current, source sharing, destination current, and destination sharing. The tests will show that if we want to keep Wine's implementation simple, we will have to implement the AMD behavior in cases where AMD differs from Nvidia. And unless we find an application that depends on Nvidia's behavior, simplicity wins, right? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2032#note_29423
participants (3)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Rémi Bernon