Re: [PATCH 1/1] ddraw: Return the primary legacy ddraw device last.
There are a few things I am wondering about and where the test can be extended: The existing test
/* Test with valid callback parameter and count the number of primary devices */ callbackCount = 0; ret = pDirectDrawEnumerateExA(test_count_callbackExA, &callbackCount, 0); ok(ret == DD_OK, "Expected DD_OK, got %d\n", ret); ok(callbackCount == 1, "Expected 1 primary device, got %d\n", callbackCount);
Combined with your change that suggests that without any flag only the NULL guid is enumerated. If so I think that's worth an explicit check. Is the NULL guid enumerated when DDENUM_DETACHEDSECONDARYDEVICES and/or DDENUM_NONDISPLAYDEVICES are set? Regarding the specific changes you make:
dlls/ddraw/tests/ddrawmodes.c | 35 +++++++++++++++++++++++++++++++++++ I'd prefer that this test be moved to ddraw1.c
+ static char DriverDescription[] = "DirectDraw HAL"; + static char DriverName[] = "display"; Please do not use CamelCase or lpFoo in d3d-related code.
Somewhat related, but that can be a separate patch: d3d3_EnumDevices() mentions that some games modify the strings. In your code (and the current code), such modifications would persist for the runtime of the application. It would be interesting to add a test for this.
+static BOOL WINAPI test_last_callbackExA(GUID *lpGUID, char *lpDriverDescription, + char *lpDriverName, void *lpContext, HMONITOR hm) You should be able to combine the callbacks. Just declare a structure and put a counter in it.
+{ + GUID **context_guid = (GUID **)lpContext; + static GUID last_guid; I guess this works, but it is somewhat ugly. If you use a struct you can put a GUID and a BOOL guid_was_null in it. Or a GUID and a GUID *.
Please extend the test to DirectDrawEnumerateA. MSDN suggests it is equivalent to DirectDrawEnumerateExA(DDENUM_NONDISPLAYDEVICES), but I have my doubts here. Comparing the strings of the non-NULL device against "DirectDraw HAL" and "display" (in the Ex and non-Ex case) would also be a good idea. Am 13.10.2014 um 21:16 schrieb Erich E. Hoover <erich.e.hoover(a)gmail.com>:
Fixes bug #37307. There are a couple of other bugs that are preventing Urban Assault from working, but this fixes the problem with returning the interfaces in the wrong order. <0001-ddraw-Return-the-primary-legacy-ddraw-device-last.patch>
On Tue, Oct 14, 2014 at 2:32 AM, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
There are a few things I am wondering about and where the test can be extended:
The existing test
/* Test with valid callback parameter and count the number of primary devices */ callbackCount = 0; ret = pDirectDrawEnumerateExA(test_count_callbackExA, &callbackCount, 0); ok(ret == DD_OK, "Expected DD_OK, got %d\n", ret); ok(callbackCount == 1, "Expected 1 primary device, got %d\n", callbackCount);
Combined with your change that suggests that without any flag only the NULL guid is enumerated. If so I think that's worth an explicit check.
That's correct, for once this also matches the documentation ;) That was the point of that test, what kind of check were you thinking?
Is the NULL guid enumerated when DDENUM_DETACHEDSECONDARYDEVICES and/or DDENUM_NONDISPLAYDEVICES are set?
Yes, the NULL/primary device is always enumerated (unless the callback requests an early stop).
... Please do not use CamelCase or lpFoo in d3d-related code.
Sorry, I thought that you wanted the variable names maintained based on some of the other patches I've done here - I'll fix that.
Somewhat related, but that can be a separate patch: d3d3_EnumDevices() mentions that some games modify the strings. In your code (and the current code), such modifications would persist for the runtime of the application. It would be interesting to add a test for this.
Taking a look at that code it looks like we're "protecting" such applications from having a problem by copying the memory. I can easily add a separate patch for exploring this, but are we really interested in protecting ourselves from such an app here when we've (to my knowledge) never run into one? The primary device callback code has been in place for a long time...
+static BOOL WINAPI test_last_callbackExA(GUID *lpGUID, char *lpDriverDescription, + char *lpDriverName, void *lpContext, HMONITOR hm) You should be able to combine the callbacks. Just declare a structure and put a counter in it.
Will do.
+{ + GUID **context_guid = (GUID **)lpContext; + static GUID last_guid; I guess this works, but it is somewhat ugly. If you use a struct you can put a GUID and a BOOL guid_was_null in it. Or a GUID and a GUID *.
Can do.
Please extend the test to DirectDrawEnumerateA. MSDN suggests it is equivalent to DirectDrawEnumerateExA(DDENUM_NONDISPLAYDEVICES), but I have my doubts here. Comparing the strings of the non-NULL device against "DirectDraw HAL" and "display" (in the Ex and non-Ex case) would also be a good idea.
This sounds like something that should be done separately, as we don't currently support the DDENUM_NONDISPLAYDEVICES flag. None of the testbots actually return "DirectDraw HAL"/"display", nor do my tests on a Windows 7 machine. I'd hesitate to change what we return here to what Windows returns now (Primary Display Driver), since that's likely to break QuickTime (according to the comment). Best, Erich
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2014-10-14 18:22, schrieb Erich E. Hoover:
On Tue, Oct 14, 2014 at 2:32 AM, Stefan Dösinger
Combined with your change that suggests that without any flag only the NULL guid is enumerated. If so I think that's worth an explicit check.
That's correct, for once this also matches the documentation ;) That was the point of that test, what kind of check were you thinking? Checking guid == NULL in addition to callbackCount == 1.
Is the NULL guid enumerated when DDENUM_DETACHEDSECONDARYDEVICES and/or DDENUM_NONDISPLAYDEVICES are set?
Yes, the NULL/primary device is always enumerated (unless the callback requests an early stop). Please add a test for that as well. You don't have to check any detached or non-display devices, just check that the last GUID * is NULL with those flags as well.
Sorry, I thought that you wanted the variable names maintained based on some of the other patches I've done here - I'll fix that. Yeah, the existing code isn't consistent, that doesn't make things easier. The best for what new/modified d3d code should look like is dlls/wined3d/cs.c.
Taking a look at that code it looks like we're "protecting" such applications from having a problem by copying the memory. driver_desc and driver_name are static. They will be initialized only once, but yes, we wouldn't get a segfault.
I can easily add a separate patch for exploring this, but are we really interested in protecting ourselves from such an app here when we've (to my knowledge) never run into one? The primary device callback code has been in place for a long time... I don't have a strong opinion on this, but we already have a game that modifies the strings passed by another callback function, I think that's reason enough to check the Windows behavior for this callback function as well. Don't bother too much about it though, addressing the regression is more important than satisfying my curiosity.
Please extend the test to DirectDrawEnumerateA. MSDN suggests it is equivalent to DirectDrawEnumerateExA(DDENUM_NONDISPLAYDEVICES), but I have my doubts here. Comparing the strings of the non-NULL device against "DirectDraw HAL" and "display" (in the Ex and non-Ex case) would also be a good idea.
This sounds like something that should be done separately, as we don't currently support the DDENUM_NONDISPLAYDEVICES flag. I'm not worried too much about the DDENUM_NONDISPLAYDEVICES part. What needs to be verified is that the last GUID enumerated by DirectDrawEnumerateA is NULL like in DirectDrawEnumerateExA.
None of the testbots actually return "DirectDraw HAL"/"display", nor do my tests on a Windows 7 machine. Ok. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJUPZhpAAoJEN0/YqbEcdMwyvAP/i+1vAyzZ716l+mA4h8Jbl85 oS8FPWLder258+KEp+ZC0YUcwo1UuAx6ISX4eIWn0/P6hOy1aRYbFSbZWivb9XwA jcdaCX0CWQVQe1983bqEEnrXDHITb0lBtcpLDhCzmOc4aeBUSpkW9rPOGdaez24f y5Ne6RIU5eHW/bkhKx/b1jvOxFdpe4SBVkivkwrooL2xXyI6pxPQ93dYBO/dvnH7 4B0VfH7QQcBWwDXVTnII9DZbAcMrCzdyOONMeTOJo2xiCaU/cy7A6RjdxUwKUK1g NqBBXKm1STrud/hycXmgq+bQycJMyZy4SY2EYxjcK32CRKL/eiHRyJUKupQRBXx+ uF+NEyt+xo2g0bhwL3mArv6aVVK/k+ZkOJb0J6wZ0TTDIWBHHNVYY9mgOV1PbyYu PAFeFQSswIbkk5srui23jWPnYfq2AfiNK9To9tCnxL93HB8IhLCtIuQp4BEs/io3 cFet3myXRZF5nVdLoDnADdi3dQzP8d6hXFvIasePnmg9sMZjYSKmcylrfo1p60ME Kh1RUbI7m7XR9GnE+nxyRWuNFJ3uSiyd/Sj1isoDXhUgvIPUHrHmKw2PzDkXMFe2 RYTLJGXSpoGL5SwFCxkVqWkP8FNtnMq0KwmV9jSMypU1wwc4QZOHQdGQFNTdYnWf uJtFZrIGhLP/PI7SbIae =ibRe -----END PGP SIGNATURE-----
participants (2)
-
Erich E. Hoover -
Stefan Dösinger