[PATCH 0/1] MR132: opengl32: Don't prioritize low bit depth formats with non-matching stencil.
Fixes ClaDun X2 missing rendering parts on AMD. The GL game uses stencil and requests pixel format with depth 16, stencil 8. On Wine / AMD that ends up with 16x0 format as there is no 16x8 advertised (16x0 only) and depth takes absolute priority. On the same Windows machine I see both 16x0 and 16x8 formats, but when 16x8 is requested 24x8 is returned (and 16x0 if 16x0 is requested). That currently works under Wine / Nvidia because there is no 16 bit depth formats advertised at all and it ends up with 24x8 without this patch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/132
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/opengl32/tests/opengl.c | 21 +++++++++++++++++++++ dlls/opengl32/wgl.c | 16 +++++++++++++--- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/dlls/opengl32/tests/opengl.c b/dlls/opengl32/tests/opengl.c index 8b0e70ccf9c..0a37223ab80 100644 --- a/dlls/opengl32/tests/opengl.c +++ b/dlls/opengl32/tests/opengl.c @@ -342,6 +342,27 @@ static void test_choosepixelformat(void) pfd.cDepthBits = 0; pfd.cStencilBits = 0; pfd.dwFlags &= ~PFD_DEPTH_DONTCARE; + + pfd.cDepthBits = 16; + ok( test_pfd(&pfd, &ret_fmt), "depth 16 failed.\n" ); + ok( ret_fmt.cDepthBits >= 16, "Got unexpected cDepthBits %u.\n", ret_fmt.cDepthBits ); + pfd.cDepthBits = 0; + + pfd.cDepthBits = 16; + pfd.cStencilBits = 8; + ok( test_pfd(&pfd, &ret_fmt), "depth 16, stencil 8 failed.\n" ); + ok( ret_fmt.cDepthBits >= 16, "Got unexpected cDepthBits %u.\n", ret_fmt.cDepthBits ); + ok( ret_fmt.cStencilBits == 8, "Got unexpected cStencilBits %u.\n", ret_fmt.cStencilBits ); + pfd.cDepthBits = 0; + pfd.cStencilBits = 0; + + pfd.cDepthBits = 8; + pfd.cStencilBits = 8; + ok( test_pfd(&pfd, &ret_fmt), "depth 8, stencil 8 failed.\n" ); + ok( ret_fmt.cDepthBits >= 16, "Got unexpected cDepthBits %u.\n", ret_fmt.cDepthBits ); + ok( ret_fmt.cStencilBits == 8, "Got unexpected cStencilBits %u.\n", ret_fmt.cStencilBits ); + pfd.cDepthBits = 0; + pfd.cStencilBits = 0; } static void WINAPI gl_debug_message_callback(GLenum source, GLenum type, GLuint id, GLenum severity, diff --git a/dlls/opengl32/wgl.c b/dlls/opengl32/wgl.c index bd7f7d22e98..a2845a08ca8 100644 --- a/dlls/opengl32/wgl.c +++ b/dlls/opengl32/wgl.c @@ -454,6 +454,7 @@ INT WINAPI wglChoosePixelFormat(HDC hdc, const PIXELFORMATDESCRIPTOR* ppfd) PIXELFORMATDESCRIPTOR format, best; int i, count, best_format; int bestDBuffer = -1, bestStereo = -1; + BYTE cDepthBits; TRACE_(wgl)( "%p %p: size %u version %u flags %u type %u color %u %u,%u,%u,%u " "accum %u depth %u stencil %u aux %u\n", @@ -464,6 +465,15 @@ INT WINAPI wglChoosePixelFormat(HDC hdc, const PIXELFORMATDESCRIPTOR* ppfd) count = wglDescribePixelFormat( hdc, 0, 0, NULL ); if (!count) return 0; + cDepthBits = ppfd->cDepthBits; + if (ppfd->dwFlags & PFD_DEPTH_DONTCARE) cDepthBits = 0; + else if (ppfd->cStencilBits && cDepthBits <= 16) + { + /* Even if, e. g., depth 16, stencil 8 is available Window / AMD may return 24x8 (and not 16x0). + * Adjust to 24 as 24x8 is universally available and we won't end up without stencil. */ + cDepthBits = 24; + } + best_format = 0; best.dwFlags = 0; best.cAlphaBits = -1; @@ -564,10 +574,10 @@ INT WINAPI wglChoosePixelFormat(HDC hdc, const PIXELFORMATDESCRIPTOR* ppfd) continue; } } - if (ppfd->cDepthBits && !(ppfd->dwFlags & PFD_DEPTH_DONTCARE)) + if (cDepthBits) { - if (((ppfd->cDepthBits > best.cDepthBits) && (format.cDepthBits > best.cDepthBits)) || - ((format.cDepthBits >= ppfd->cDepthBits) && (format.cDepthBits < best.cDepthBits))) + if (((cDepthBits > best.cDepthBits) && (format.cDepthBits > best.cDepthBits)) || + ((format.cDepthBits >= cDepthBits) && (format.cDepthBits < best.cDepthBits))) goto found; if (best.cDepthBits != format.cDepthBits) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/132
Matteo Bruni (@Mystral) commented about dlls/opengl32/tests/opengl.c:
+ + pfd.cDepthBits = 16; + pfd.cStencilBits = 8; + ok( test_pfd(&pfd, &ret_fmt), "depth 16, stencil 8 failed.\n" ); + ok( ret_fmt.cDepthBits >= 16, "Got unexpected cDepthBits %u.\n", ret_fmt.cDepthBits ); + ok( ret_fmt.cStencilBits == 8, "Got unexpected cStencilBits %u.\n", ret_fmt.cStencilBits ); + pfd.cDepthBits = 0; + pfd.cStencilBits = 0; + + pfd.cDepthBits = 8; + pfd.cStencilBits = 8; + ok( test_pfd(&pfd, &ret_fmt), "depth 8, stencil 8 failed.\n" ); + ok( ret_fmt.cDepthBits >= 16, "Got unexpected cDepthBits %u.\n", ret_fmt.cDepthBits ); + ok( ret_fmt.cStencilBits == 8, "Got unexpected cStencilBits %u.\n", ret_fmt.cStencilBits ); + pfd.cDepthBits = 0; + pfd.cStencilBits = 0; I have tweaked / extended the tests a little further, see [gl-depth-stencil.txt](/uploads/4ae59fc98d8067d7babb524984574df7/gl-depth-stencil.txt). I only tested that on Nvidia for now, curious if they also pass with AMD with those changes.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/132#note_2293
Matteo Bruni (@Mystral) commented about dlls/opengl32/wgl.c:
count = wglDescribePixelFormat( hdc, 0, 0, NULL ); if (!count) return 0;
+ cDepthBits = ppfd->cDepthBits; + if (ppfd->dwFlags & PFD_DEPTH_DONTCARE) cDepthBits = 0; + else if (ppfd->cStencilBits && cDepthBits <= 16) + { + /* Even if, e. g., depth 16, stencil 8 is available Window / AMD may return 24x8 (and not 16x0). + * Adjust to 24 as 24x8 is universally available and we won't end up without stencil. */ + cDepthBits = 24; + } +
So, as I mentioned to you privately before, this seems very ad-hoc i.e. I'd expect ChoosePixelFormat() generally, or at least usually, to return a pixel format with stencil bits when the PFD explicitly asks for it. I'd like to see more evidence that's not the case and an improved test (or comment, if it can't be properly shown in a Wine test) to back it up. Also nitpick, typo "Window". -- https://gitlab.winehq.org/wine/wine/-/merge_requests/132#note_2294
On Fri Jun 17 19:11:37 2022 +0000, Matteo Bruni wrote:
I have tweaked / extended the tests a little further, see [gl-depth-stencil.txt](/uploads/4ae59fc98d8067d7babb524984574df7/gl-depth-stencil.txt). I only tested that on Nvidia for now, curious if they also pass with AMD with those changes. I run the test on my AMD / Windows and added some traces / additional tests. I am attaching diff to the test (which includes your patch as well) and the output from AMD / Windows.
It seems that unfortunately 32/8 tests is a bit inconclusive here as somehow the output pixel format is 32 / 8 (honestly not sure what that means but that's what I see here; see no test failure on line 382 and trace output from line 381). The rests of tests suggest that it prefers 24 bit whenever in doubt, see, e. g., 8x8 test, trace at line 364: it could choose 16x8 but preferred 24x8. From what I see it seems that the pattern is whenever stencil is requested it returns depths >= 24. That's what my current patch is doing. If we prioritize stencil formats when requested over depth match that will probably look more straightforward logic-wise but that would break all those tests if we'd tighten them to what is actually returned on AMD. Also, if specific games depending on the stencil choice are concerned, if we plainly prioritize stencil presence they will get 16 bit depth on Wine while getting 24 on Windows and that difference may matter (even if not break the thing completely like returning no stencil format). What do you think? [diff.txt](/uploads/4355976f66b6decb44d43c5b126e5479/diff.txt) [output.txt](/uploads/13bfe7a03d34c1303f04612c9d809e0e/output.txt) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/132#note_2371
On Mon Jun 20 16:08:35 2022 +0000, Paul Gofman wrote:
I run the test on my AMD / Windows and added some traces / additional tests. I am attaching diff to the test (which includes your patch as well) and the output from AMD / Windows. It seems that unfortunately 32/8 tests is a bit inconclusive here as somehow the output pixel format is 32 / 8 (honestly not sure what that means but that's what I see here; see no test failure on line 382 and trace output from line 381). The rests of tests suggest that it prefers 24 bit whenever in doubt, see, e. g., 8x8 test, trace at line 364: it could choose 16x8 but preferred 24x8. From what I see it seems that the pattern is whenever stencil is requested it returns depths >= 24. That's what my current patch is doing. If we prioritize stencil formats when requested over depth match that will probably look more straightforward logic-wise but that would break all those tests if we'd tighten them to what is actually returned on AMD. Also, if specific games depending on the stencil choice are concerned, if we plainly prioritize stencil presence they will get 16 bit depth on Wine while getting 24 on Windows and that difference may matter (even if not break the thing completely like returning no stencil format). What do you think? [diff.txt](/uploads/4355976f66b6decb44d43c5b126e5479/diff.txt) [output.txt](/uploads/13bfe7a03d34c1303f04612c9d809e0e/output.txt) It seems pretty clear and reasonable to me actually. It looks like ChoosePixelFormat() returns a pixel format with stencil bits when requested so. Matching depth bits comes at a lower priority.
As it turns out, the only actual pixel format with stencil bits that's supported everywhere is D24S8. D32S8 is apparently supported on Windows AMD, but not on Windows Nvidia. FWIW Linux AMD doesn't return any D32 visual / fbconfig at all. I'm attaching some more changes on top of yours: [choosepixelformat.txt](/uploads/b892cfcd3cb9fb13f8eee3ab81b2ddfb/choosepixelformat.txt). Basically I swapped the two blocks for stencil and depth in wglChoosePixelFormat(), making sure we take care of stencil before checking depth. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/132#note_2409
On Mon Jun 20 22:00:21 2022 +0000, Matteo Bruni wrote:
It seems pretty clear and reasonable to me actually. It looks like ChoosePixelFormat() returns a pixel format with stencil bits when requested so. Matching depth bits comes at a lower priority. As it turns out, the only actual pixel format with stencil bits that's supported everywhere is D24S8. D32S8 is apparently supported on Windows AMD, but not on Windows Nvidia. FWIW Linux AMD doesn't return any D32 visual / fbconfig at all. I'm attaching some more changes on top of yours: [choosepixelformat.txt](/uploads/b892cfcd3cb9fb13f8eee3ab81b2ddfb/choosepixelformat.txt). Basically I swapped the two blocks for stencil and depth in wglChoosePixelFormat(), making sure we take care of stencil before checking depth. Also adding the output of the tests on Windows Nvidia after my patch.
[output_nvidia.txt](/uploads/99fd7148e6a77bfea79713e71aa0a03b/output_nvidia.txt) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/132#note_2411
participants (3)
-
Matteo Bruni (＠Mystral) -
Paul Gofman
-
Paul Gofman (＠gofman)