Re: [PATCH 4/5] ddraw: Accept all 3 versions of D3DFINDDEVICERESULT
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Am 2015-10-04 um 19:52 schrieb Andrew D'Addesio:
+ /* Write ddHwDesc */ + desc_ptr = (BYTE*)&fdr->ddHwDesc; + memcpy(desc_ptr, &desc1, desc_size); + ((D3DDEVICEDESC*)desc_ptr)->dwSize = desc_size; + + /* Write ddSwDesc */ + desc_ptr += desc_size; + memcpy(desc_ptr, &desc1, desc_size); + ((D3DDEVICEDESC*)desc_ptr)->dwSize = desc_size; Is there a nicer way? I can't think of one to be honest, but this code feels somewhat unsatisfying.
Also I guess we're lucky that all versions of D3DDEVICEDESC have a DWORD-aligned size, so we don't have to worry about padding between them. Native ddraw doesn't support d3d in 64 bit. Ours does though, mostly out of laziness. Did you run the test in a 64 bit build? It might fail due to different struct alignment.
+ /* Invalid D3DDEVICEDESC sizes should fail */ + search.dwSize = sizeof(search); + search.dwFlags = 0; + clear_device_result(&result, (D3D1_DESC_SIZE-4), (D3D1_DESC_SIZE-4)); hr = IDirect3D_FindDevice(Direct3D1, &search, &result); - ok(hr == D3D_OK, - "Expected IDirect3D1::FindDevice to return D3D_OK, got 0x%08x\n", hr); + ok(hr == DDERR_INVALIDPARAMS, + "Expected IDirect3D1::FindDevice to return DDERR_INVALIDPARAMS, got 0x%08x\n", hr); What happens when you use something between D3D1_DESC_SIZE and D3D2_DESC_SIZE, or e.g. the largest size + 4?
I recommend to put the valid and invalid tests in a table together with an expect HRESULT. That way you don't have to duplicate the structure assignment code + FindDevice call and can just iterate over the table. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJWEiUOAAoJEN0/YqbEcdMwTCgP/AqlzZ9yyJktv2U+OTgxVGTU 9tj/ZbsKIVk7S0FQFDvyUaKCmP7CcYH+ARoJapn7it6peMdNKHB7zbAgsR7koWph hKECmx+xJ4w4Fjvkj159XYL28DokEHqDmZeaeghwldPbCcXTMO5BulJrooO5F1MI Q2/lwhr+pOO3EDOlLvvLntrOszuYJdy0CbWB34ORlV0PQfk/7Wwm+9rkDzVTDS4r xnpNAzEPaFHNrlhmXlaoFa6arsKTILcwhpmt6wzXTMhcyGs8NRcHA1F5lgIbnH3X m6q84FI5Law/epfKwJSyXTI1eDdWB0PKGqw+xEUv4T79q8oRVRe+W/Sjvq9bluEa fDYshHOP2uVRQ0kcObqSTx9iOWW9RtCdTdHxhloHDOOPIAJyxyoENOkYV/5V+QBT P3pKUG9qjNFODFxQvHb6rvbUisVUXGPihC1fbdAkoNYD4Mf2V5rZ+K88RgXQmofS +zbxziLAr6ItmeCw4/nAvt3/8HoQAtWkF7Bb6ZxhtViOzRraNkLvjqPtJ1HAmz8F TueLADA2bRXpGmkW+9bjLbQ0Z6aK2fqQHYy4Rmkt1LJwGSqrMz5ctq63XChZgA+G +c2OXfvcC+zJsVfLxFHmPjPv1eQ0CQ3gyssjRVGPXq9JAQNHqJxm4ldK5e/dJcTS VBxzcIjMMFVLld6a+dS5 =TBrC -----END PGP SIGNATURE-----
On 5 October 2015 at 09:21, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Am 2015-10-04 um 19:52 schrieb Andrew D'Addesio:
+ /* Write ddHwDesc */ + desc_ptr = (BYTE*)&fdr->ddHwDesc; + memcpy(desc_ptr, &desc1, desc_size); + ((D3DDEVICEDESC*)desc_ptr)->dwSize = desc_size; + + /* Write ddSwDesc */ + desc_ptr += desc_size; + memcpy(desc_ptr, &desc1, desc_size); + ((D3DDEVICEDESC*)desc_ptr)->dwSize = desc_size; Is there a nicer way? I can't think of one to be honest, but this code feels somewhat unsatisfying.
One way would be to just declare different versions of the structure for internal use (i.e., D3DFINDDEVICERESULTv1, etc.), and then pick the right one based on fdr->dwSize. That would probably simplify a couple of other things in the patch as well. There are also a couple of style issues in these patches. Please follow the style of the surrounding code, or that of wined3d when in doubt.
On 10/05/2015 03:57 AM, Henri Verbeet wrote:
On 5 October 2015 at 09:21, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Am 2015-10-04 um 19:52 schrieb Andrew D'Addesio:
+ /* Write ddHwDesc */ + desc_ptr = (BYTE*)&fdr->ddHwDesc; + memcpy(desc_ptr, &desc1, desc_size); + ((D3DDEVICEDESC*)desc_ptr)->dwSize = desc_size; + + /* Write ddSwDesc */ + desc_ptr += desc_size; + memcpy(desc_ptr, &desc1, desc_size); + ((D3DDEVICEDESC*)desc_ptr)->dwSize = desc_size; Is there a nicer way? I can't think of one to be honest, but this code feels somewhat unsatisfying.
One way would be to just declare different versions of the structure for internal use (i.e., D3DFINDDEVICERESULTv1, etc.), and then pick the right one based on fdr->dwSize. That would probably simplify a couple of other things in the patch as well.
In that case, we could do:
if (ver == 1) { D3DFINDDEVICERESULTv1 *fdrv1 = (D3DFINDDEVICERESULTv1 *)fdr; D3DDEVICEDESCv1 *desc1v1 = (D3DDEVICEDESCv1 *)&desc1; fdrv1->ddHwDesc = *desc1v1; fdrv1->ddSwDesc = *desc1v1; } else if (ver == 2) { /* ... */
On 10/05/2015 03:57 AM, Henri Verbeet wrote:
There are also a couple of style issues in these patches. Please follow the style of the surrounding code, or that of wined3d when in doubt.
Sorry, I was so busy checking for code correctness that I forgot about indentation. I think that's mainly what you're referring to. For example, this:
hel_desc.dwFlags = D3DDD_COLORMODEL | D3DDD_DEVCAPS | D3DDD_TRANSFORMCAPS | D3DDD_LIGHTINGCAPS | D3DDD_BCLIPPING;
should become:
hel_desc.dwFlags = D3DDD_COLORMODEL | D3DDD_DEVCAPS | D3DDD_TRANSFORMCAPS | D3DDD_LIGHTINGCAPS | D3DDD_BCLIPPING;
and similarly:
ok(device > ctx->prev_device, "D3D%d %s enumerated after %s\n", ver, expected_name, device_list[ctx->prev_device].device_name);
should become:
ok(device > ctx->prev_device, "D3D%d %s enumerated after %s\n", ver, expected_name, device_list[ctx->prev_device].device_name);
And I think the other big problem was that I did "enum {" instead of placing the { on its own line. Is there anything else you had in mind that I should know about? Thanks, Andrew
On 5 October 2015 at 23:24, Andrew D'Addesio <andrew(a)fatbag.net> wrote:
ok(device > ctx->prev_device, "D3D%d %s enumerated after %s\n", ver, expected_name, device_list[ctx->prev_device].device_name);
should become:
ok(device > ctx->prev_device, "D3D%d %s enumerated after %s\n", ver, expected_name, device_list[ctx->prev_device].device_name);
That should actually just get double indentation as well.
And I think the other big problem was that I did "enum {" instead of placing the { on its own line.
Yeah.
Is there anything else you had in mind that I should know about?
You're missing spaces around operators in a couple of places, like e.g. "2*D3D1_DESC_SIZE".
On 10/05/2015 02:21 AM, Stefan Dösinger wrote:
Is there a nicer way? I can't think of one to be honest, but this code feels somewhat unsatisfying.
Well it makes pretty logically clear statements. If it makes you happier I could change
desc_ptr += desc_size;
to
desc_ptr = ((BYTE*)&fdr->ddHwDesc) + desc_size;
so that the two blocks can be swapped freely. On 10/05/2015 02:21 AM, Stefan Dösinger wrote:
Also I guess we're lucky that all versions of D3DDEVICEDESC have a DWORD-aligned size, so we don't have to worry about padding between them.
Native ddraw doesn't support d3d in 64 bit. Ours does though, mostly out of laziness. Did you run the test in a 64 bit build? It might fail due to different struct alignment.
I defined D3D*_DESC_SIZE like such:
/* Size of D3DDEVICEDESC in Direct3D 1-3 */ enum { D3D1_DESC_SIZE = FIELD_OFFSET(D3DDEVICEDESC, dwMinTextureWidth), /* 172 */ D3D2_DESC_SIZE = FIELD_OFFSET(D3DDEVICEDESC, dwMaxTextureRepeat), /* 204 */ D3D3_DESC_SIZE = sizeof(D3DDEVICEDESC) /* 252 */ };
where dwMinTextureWidth is the first field not in D3D1 and dwMaxTextureRepeat is the first field not in D3D2. This should work regardless of struct padding. The primary concern is whether the comments (172, 204, 252) are still correct in 64-bit. Here is a test which shows that they're still correct:
#include <windows.h> #include <d3d.h> #include <stdio.h> enum { D3D1_DESC_SIZE = FIELD_OFFSET(D3DDEVICEDESC, dwMinTextureWidth), D3D2_DESC_SIZE = FIELD_OFFSET(D3DDEVICEDESC, dwMaxTextureRepeat), D3D3_DESC_SIZE = sizeof(D3DDEVICEDESC) }; int main() { printf("D3D1_DESC_SIZE: %u\n", D3D1_DESC_SIZE); printf("D3D2_DESC_SIZE: %u\n", D3D2_DESC_SIZE); printf("D3D3_DESC_SIZE: %u\n", D3D3_DESC_SIZE); return 0; }
My results are:
C:\sizetest>"C:\mingw32\bin\gcc.exe" -m32 -Wall -Wextra -ansi -pedantic -o sizetest-32.exe sizetest.c
C:\sizetest>sizetest-32.exe D3D1_DESC_SIZE: 172 D3D2_DESC_SIZE: 204 D3D3_DESC_SIZE: 252
C:\sizetest>"C:\mingw64\bin\gcc.exe" -m64 -Wall -Wextra -ansi -pedantic -o sizetest-64.exe sizetest.c
C:\sizetest>sizetest-64.exe D3D1_DESC_SIZE: 172 D3D2_DESC_SIZE: 204 D3D3_DESC_SIZE: 252
C:\sizetest>
On 10/05/2015 02:21 AM, Stefan Dösinger wrote:
What happens when you use something between D3D1_DESC_SIZE and D3D2_DESC_SIZE, or e.g. the largest size + 4?
I can add those two cases if you want. I can also do what test_get_caps1 (in tests/d3d.c) currently does and test all sizes from 0 to 1023, like such:
/* Test all matching sizes (0, 0), (1, 1), ..., (1023, 1023). */ for (i = 0; i < 1024; i++) { search.dwSize = sizeof(search); search.dwFlags = 0; clear_device_result(&result, i, i); hr = IDirect3D_FindDevice(Direct3D1, &search, &result);
if (i == D3D1_DESC_SIZE || i == D3D2_DESC_SIZE || i == D3D3_DESC_SIZE) { ok(hr == D3D_OK, "FindDevice with size %u returned hr 0x%08X,
expected D3D_OK.\n", i, hr);
} else { ok(hr == DDERR_INVALIDPARAMS, "FindDevice with size %u returned hr 0x%08X, expected
DDERR_INVALIDPARAMS.\n", i, hr);
} }
In this loop, I wouldn't really test all size-pairs (0, 0), (0, 1), ..., (0, 1023), (1, 0), (1, 1), ... (1023, 1023); I would only test the matching pairs (0, 0), (1, 1), ..., (1023, 1023).
I recommend to put the valid and invalid tests in a table together with an expect HRESULT. That way you don't have to duplicate the structure assignment code + FindDevice call and can just iterate over the table.
With the for loop we won't need a table. But if we don't use the for-loop, then sure, I can add a table. Regards, Andrew
On Windows, FindDevice is only able to find those devices that EnumDevices lists for that Direct3D version. Here's the test program I wrote showing the result of FindDevice for each device: https://pastebin.mozilla.org/8849627
* IDirect3D: * IDirect3DRampDevice: 0x00000000 * IDirect3DRGBDevice: 0x00000000 * IDirect3DHALDevice: 0x00000000 * IDirect3DMMXDevice: 0x887600FF * IDirect3DRefDevice: 0x887600FF * IDirect3DNullDevice: 0x887600FF * IDirect3DTnLHalDevice: 0x887600FF * IDirect3D2: * IDirect3DRampDevice: 0x00000000 * IDirect3DRGBDevice: 0x00000000 * IDirect3DHALDevice: 0x00000000 * IDirect3DMMXDevice: 0x00000000 * IDirect3DRefDevice: 0x887600FF * IDirect3DNullDevice: 0x887600FF * IDirect3DTnLHalDevice: 0x887600FF * IDirect3D3: * IDirect3DRampDevice: 0x887600FF * IDirect3DRGBDevice: 0x00000000 * IDirect3DHALDevice: 0x00000000 * IDirect3DMMXDevice: 0x887600FF * IDirect3DRefDevice: 0x887600FF * IDirect3DNullDevice: 0x887600FF * IDirect3DTnLHalDevice: 0x887600FF
(where 0x887600FF means DDERR_NOTFOUND.) This holds for every VM tested by WineTestBot (Windows 2000 through Windows 10). Are we interested in replicating this behavior? Currently, the FindDevice test in tests/d3d.c (which uses Direct3D1) explicitly checks that the MMX, Ref, TnlHal, and Null devices are not found. But since our implementation of CreateDevice (unlike Windows) already accepts any device regardless of the Direct3D version, it seems like we might as well report it through FindDevice. I guess there is one catch: the current comments for D3D7EnumTest say that some games (Delta Force LW and C&C TFD) break if EnumDevices enumerates the T&L HAL device but not the ordinary HAL device. There might be a similar problem for FindDevice. It might be worth simply adding a small check for that.
participants (3)
-
Andrew D'Addesio -
Henri Verbeet -
Stefan Dösinger