Re: [PATCH 2/4] d3d9/tests: Test that Direct3D9 doesn't modify the pixel format of the window it targets. (try 4)
On 17 February 2014 22:25, Ken Thomases ken@codeweavers.com wrote:
dlls/d3d9/tests/device.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+)
Unfortunately this fails here:
../../../tools/runtest -q -P wine -T ../../.. -M d3d9.dll -p d3d9_test.exe.so device && touch device.ok ... device.c:7547: Test succeeded X Error of failed request: BadMatch (invalid parameter attributes) Major opcode of failed request: 154 (GLX) Minor opcode of failed request: 11 (X_GLXSwapBuffers) Serial number of failed request: 4929 Current serial number in output stream: 4933 make: *** [device.ok] Error 1
On Feb 18, 2014, at 10:08 AM, Henri Verbeet wrote:
On 17 February 2014 22:25, Ken Thomases ken@codeweavers.com wrote:
dlls/d3d9/tests/device.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+)
Unfortunately this fails here:
../../../tools/runtest -q -P wine -T ../../.. -M d3d9.dll -p d3d9_test.exe.so device && touch device.ok ... device.c:7547: Test succeeded X Error of failed request: BadMatch (invalid parameter attributes) Major opcode of failed request: 154 (GLX) Minor opcode of failed request: 11 (X_GLXSwapBuffers) Serial number of failed request: 4929 Current serial number in output stream: 4933 make: *** [device.ok] Error 1
Is this with or without patch 1 applied?
-Ken
On 18 February 2014 23:14, Ken Thomases ken@codeweavers.com wrote:
On Feb 18, 2014, at 10:08 AM, Henri Verbeet wrote:
../../../tools/runtest -q -P wine -T ../../.. -M d3d9.dll -p d3d9_test.exe.so device && touch device.ok ... device.c:7547: Test succeeded X Error of failed request: BadMatch (invalid parameter attributes) Major opcode of failed request: 154 (GLX) Minor opcode of failed request: 11 (X_GLXSwapBuffers) Serial number of failed request: 4929 Current serial number in output stream: 4933 make: *** [device.ok] Error 1
Is this with or without patch 1 applied?
With. Without patch 1, I get test failures, but not the BadMatch error.
After thinking about it for a bit, I think the issue is essentially that context_acquire() depends on context_enter() to set context->restore_ctx when context_set_gl_context() needs to be called. If only the pixel format is different context_set_gl_context() will not be called.
On Feb 18, 2014, at 5:40 PM, Henri Verbeet wrote:
After thinking about it for a bit, I think the issue is essentially that context_acquire() depends on context_enter() to set context->restore_ctx when context_set_gl_context() needs to be called. If only the pixel format is different context_set_gl_context() will not be called.
OK, I can see how that would happen. However, my wined3d patch didn't change that part of the logic. I think my test just exposed a pre-existing bug. It doesn't show up without my wined3d patch applied because, in that case, wined3d fails to restore the window's pixel format and leaves it with a double-buffered pixel format. The two bugs cancel each other out and fixing one allowed the other to be revealed.
This fixes it, although it may be more crude than we'd like:
diff --git a/dlls/wined3d/context.c b/dlls/wined3d/context.c index 4679b25..3d11c16 100644 --- a/dlls/wined3d/context.c +++ b/dlls/wined3d/context.c @@ -3051,7 +3056,7 @@ struct wined3d_context *context_acquire(const struct wined3d_device *device, str if (!context_set_current(context)) ERR("Failed to activate the new context.\n"); } - else if (context->restore_ctx) + else if (context->restore_ctx || context->pixel_format != GetPixelFormat(wglGetCurrentDC())) { context_set_gl_context(context); }
-Ken
On Feb 18, 2014, at 7:53 PM, Ken Thomases wrote:
This fixes it, although it may be more crude than we'd like:
- else if (context->restore_ctx)
- else if (context->restore_ctx || context->pixel_format != GetPixelFormat(wglGetCurrentDC()))
On second thought, that would be unreliable. It depends on the app's DC still being valid, which it may not be.
We'd have to check using a DC for the window that would actually be used if the context were to be made current. (I think we can ignore the swapchain backup DC, since that's used when the pixel format can't be set on context->hdc and the condition will be true, which is what we want.) So:
else if (context->restore_ctx || context->pixel_format != GetPixelFormat(context->hdc))
I guess this may be less efficient than we'd want. Ideally, as little WGL state as possible would be checked other than when context->level transitions from 0 to 1. context_enter() could check it at that time. Rather than using restore_ctx being non-NULL as a proxy for whether the context needs to be set, we'd use a separate, explicit flag:
diff --git a/dlls/wined3d/context.c b/dlls/wined3d/context.c index 4679b25..a0d078c 100644 --- a/dlls/wined3d/context.c +++ b/dlls/wined3d/context.c @@ -1109,6 +1109,7 @@ void context_release(struct wined3d_context *context) context->restore_ctx = NULL; context->restore_dc = NULL; } + context->needs_set = 0; } }
@@ -1127,7 +1128,10 @@ static void context_enter(struct wined3d_context *context) current_gl, wglGetCurrentDC()); context->restore_ctx = current_gl; context->restore_dc = wglGetCurrentDC(); + context->needs_set = 1; } + else if (context->pixel_format != GetPixelFormat(context->hdc)) + context->needs_set = 1; } }
@@ -3051,7 +3055,7 @@ struct wined3d_context *context_acquire(const struct wined3d_device *device, str if (!context_set_current(context)) ERR("Failed to activate the new context.\n"); } - else if (context->restore_ctx) + else if (context->needs_set) { context_set_gl_context(context); } diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index 0a1511b..0ceb6f1 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -1085,7 +1085,8 @@ struct wined3d_context DWORD fixed_function_usage_map : 8; /* MAX_TEXTURES, 8 */ DWORD lowest_disabled_stage : 4; /* Max MAX_TEXTURES, 8 */ DWORD rebind_fbo : 1; - DWORD padding : 19; + DWORD needs_set : 1; + DWORD padding : 18; DWORD shader_update_mask; DWORD constant_update_mask; DWORD numbered_array_mask;
-Ken
On 19 February 2014 04:30, Ken Thomases ken@codeweavers.com wrote:
I guess this may be less efficient than we'd want. Ideally, as little WGL state as possible would be checked other than when context->level transitions from 0 to 1. context_enter() could check it at that time. Rather than using restore_ctx being non-NULL as a proxy for whether the context needs to be set, we'd use a separate, explicit flag:
diff --git a/dlls/wined3d/context.c b/dlls/wined3d/context.c index 4679b25..a0d078c 100644 --- a/dlls/wined3d/context.c +++ b/dlls/wined3d/context.c @@ -1109,6 +1109,7 @@ void context_release(struct wined3d_context *context) context->restore_ctx = NULL; context->restore_dc = NULL; }
context->needs_set = 0;}
@@ -1127,7 +1128,10 @@ static void context_enter(struct wined3d_context *context) current_gl, wglGetCurrentDC()); context->restore_ctx = current_gl; context->restore_dc = wglGetCurrentDC();
context->needs_set = 1; }else if (context->pixel_format != GetPixelFormat(context->hdc))context->needs_set = 1;}
@@ -3051,7 +3055,7 @@ struct wined3d_context *context_acquire(const struct wined3d_device *device, str if (!context_set_current(context)) ERR("Failed to activate the new context.\n"); }
- else if (context->restore_ctx)
- else if (context->needs_set) { context_set_gl_context(context); }
diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index 0a1511b..0ceb6f1 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -1085,7 +1085,8 @@ struct wined3d_context DWORD fixed_function_usage_map : 8; /* MAX_TEXTURES, 8 */ DWORD lowest_disabled_stage : 4; /* Max MAX_TEXTURES, 8 */ DWORD rebind_fbo : 1;
- DWORD padding : 19;
- DWORD needs_set : 1;
- DWORD padding : 18; DWORD shader_update_mask; DWORD constant_update_mask; DWORD numbered_array_mask;
I think that mostly works, but you'll probably want to clear needs_set in context_set_gl_context() instead of in context_release(). I think there's also an argument somewhere that context_update_window() should set needs_set instead of calling context_set_pixel_format() and context_set_gl_context().
On Feb 19, 2014, at 4:36 AM, Henri Verbeet wrote:
On 19 February 2014 04:30, Ken Thomases ken@codeweavers.com wrote:
@@ -3051,7 +3055,7 @@ struct wined3d_context *context_acquire(const struct wined3d_device *device, str if (!context_set_current(context)) ERR("Failed to activate the new context.\n"); }
- else if (context->restore_ctx)
- else if (context->needs_set) { context_set_gl_context(context); }
I think that mostly works, but you'll probably want to clear needs_set in context_set_gl_context() instead of in context_release().
Hmm. The old logic had context_acquire() doing context_set_gl_context() each time if context_enter() had set restore_ctx at the outmost level. I didn't want to change that because I wasn't confident it was superfluous.
I think there's also an argument somewhere that context_update_window() should set needs_set instead of calling context_set_pixel_format() and context_set_gl_context().
Yeah, it could do that. Actually, looking at the current code, context_acquire() calls context_update_window() before context_enter(). Couldn't that screw up context_enter()'s ability to a) notice that it needs to restore the context, or b) record the proper context to restore?
I have to say that my approach would be to record the state to restore at exactly the place where it is changed. So, the old WGL context and DC would be remembered at the call to wglMakeCurrent() or wglMakeContextCurrentARB(). In my pixel format patch, the old pixel format is remembered at the call to SetPixelFormat(). Etc. You'd check to make sure that something isn't already remembered, of course, so you don't overwrite the remembered app-set state with wined3d-set state.
My approach can be less efficient because you check the state too often. The current code uses the context->level transition from 0 to 1 to limit it. But we can use flags (something like "restore_ctx_valid", for example) to remember if we've checked since that transition and avoid checking again (even though restore_ctx itself may remain NULL).
-Ken
On Feb 19, 2014, at 12:25 PM, Ken Thomases wrote:
On Feb 19, 2014, at 4:36 AM, Henri Verbeet wrote:
I think there's also an argument somewhere that context_update_window() should set needs_set instead of calling context_set_pixel_format() and context_set_gl_context().
Hmm. The one issue is that, if the context_set_pixel_format() in context_update_window() fails, the context is marked as invalid and left that way. If we simply set needs_set instead, then context_acquire() will call context_set_gl_context(). That will try context_set_pixel_format() again but this time, if it fails, it will try the swapchain backup DC. I don't really know if that's desired. I assume the code is structured the way it is for a reason (i.e. a call to context_set_pixel_format() immediately before a call to context_set_gl_context(), which already begins with a call to the former).
-Ken
On 19 February 2014 20:06, Ken Thomases ken@codeweavers.com wrote:
Hmm. The one issue is that, if the context_set_pixel_format() in context_update_window() fails, the context is marked as invalid and left that way. If we simply set needs_set instead, then context_acquire() will call context_set_gl_context(). That will try context_set_pixel_format() again but this time, if it fails, it will try the swapchain backup DC. I don't really know if that's desired. I assume the code is structured the way it is for a reason (i.e. a call to context_set_pixel_format() immediately before a call to context_set_gl_context(), which already begins with a call to the former).
Trying the backup DC would be correct. I probably just forgot to get rid of the context_set_pixel_format() call in context_update_window() back when I made context_restore_gl_context() try to restore the pixel format as well. The backup DC code was added even later by Matteo.
On 19 February 2014 19:25, Ken Thomases ken@codeweavers.com wrote:
I think that mostly works, but you'll probably want to clear needs_set in context_set_gl_context() instead of in context_release().
Hmm. The old logic had context_acquire() doing context_set_gl_context() each time if context_enter() had set restore_ctx at the outmost level. I didn't want to change that because I wasn't confident it was superfluous.
If you only clear it in the outer context_release(), inner context_acquire() calls will call context_set_gl_context() without needing to.
I think there's also an argument somewhere that context_update_window() should set needs_set instead of calling context_set_pixel_format() and context_set_gl_context().
Yeah, it could do that. Actually, looking at the current code, context_acquire() calls context_update_window() before context_enter(). Couldn't that screw up context_enter()'s ability to a) notice that it needs to restore the context, or b) record the proper context to restore?
Yeah, I think so.
I have to say that my approach would be to record the state to restore at exactly the place where it is changed. So, the old WGL context and DC would be remembered at the call to wglMakeCurrent() or wglMakeContextCurrentARB(). In my pixel format patch, the old pixel format is remembered at the call to SetPixelFormat(). Etc.
It's conceptually not that different, context_acquire() / context_enter() is the wined3d equivalent of wglMakeCurrent(). The split between context_acquire() and context_enter() may have room for improvement. Also note that some parts are just left over from back when we still had ActivateContext().
On 19 February 2014 02:53, Ken Thomases ken@codeweavers.com wrote:
OK, I can see how that would happen. However, my wined3d patch didn't change that part of the logic. I think my test just exposed a pre-existing bug. It doesn't show up without my wined3d patch applied because, in that case, wined3d fails to restore the window's pixel format and leaves it with a double-buffered pixel format. The two bugs cancel each other out and fixing one allowed the other to be revealed.
With the current code, the pixel format only needs changing if the GL context also does, that's just an implication of setting/restoring both at the same time.
On Feb 19, 2014, at 4:36 AM, Henri Verbeet wrote:
On 19 February 2014 02:53, Ken Thomases ken@codeweavers.com wrote:
OK, I can see how that would happen. However, my wined3d patch didn't change that part of the logic. I think my test just exposed a pre-existing bug. It doesn't show up without my wined3d patch applied because, in that case, wined3d fails to restore the window's pixel format and leaves it with a double-buffered pixel format. The two bugs cancel each other out and fixing one allowed the other to be revealed.
With the current code, the pixel format only needs changing if the GL context also does, that's just an implication of setting/restoring both at the same time.
But wined3d doesn't always restore when returning to the app (because the app hadn't made a GL context current) and then doesn't set its context when the app calls back into it. But between those points, the app could set a pixel format on a window.
-Ken
On 19 February 2014 17:10, Ken Thomases ken@codeweavers.com wrote:
But wined3d doesn't always restore when returning to the app (because the app hadn't made a GL context current) and then doesn't set its context when the app calls back into it. But between those points, the app could set a pixel format on a window.
Well, no, because SetPixelFormat() can't set the pixel format after wined3d did. (Ignoring hypothetical applications that know about WGL_WINE_pixel_format_passthrough.)
-
Henri Verbeet -
Ken Thomases