-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hi,
Thanks for your persistence on this issue. I'll try to run those tests on some of my Windows machines to spot any card / driver dependent results. Some of your tests could be fragile, but I'll have a better opinion on that after trying them myself.
Overall, you could reduce the number of lines of code here by organizing the test data in a table. Try something like
static const struct { DWORD flags, width, height; HRESULT expect_hr; } test_data[] = { {DDSD_HEIGHT, 0, 128, DDERR_INVALIDPARAMS}, {DDSD_WIDTH, 128, 0, DDERR_INVALIDPARAMS}, {DDSD_WIDTH | HEIGHT, 128, 128, DD_OK}, {DDSD_WIDTH | HEIGHT, 0, 128, DDERR_INVALIDPARAMS}, {DDSD_WIDTH | HEIGHT, 128, 0, DDERR_INVALIDPARAMS}, etc. }
Some things, like the video memory size exhaustion checks and the primary surface size checks are hard to handle this way. You can keep them in separate code blocks.
Here are a few more issues, mostly minor style points:
- IDirectDrawSurface *dsurface = NULL; + IDirectDraw *lpDD;
Please don't use the hungarian notation. I recommend to use the names ddraw and surface.
- ok(ret == DDERR_INVALIDPARAMS, "Creating an offscreen plain
surface without a size info returned %08x (dsurface=%p)\n", ret, dsurface); This is a minor issue, but please add interpunctuation for consistency's sake. I don't think there's a reason to write dsurface here, other tests already establish that it should be NULL in case of an error.
- if(dsurface) + { + trace("Surface at %p\n",
dsurface); + IDirectDrawSurface_Release(dsurface); + dsurface = NULL; + }
The trace looks like it is a debugging leftover.
- /* Sanity check */ + memset(&desc, 0, sizeof(desc)); +
desc.dwSize = sizeof(desc); + desc.dwFlags = DDSD_CAPS | DDSD_HEIGHT | DDSD_WIDTH; + desc.ddsCaps.dwCaps |= DDSCAPS_OFFSCREENPLAIN; + desc.dwHeight = 128; + desc.dwWidth = 128; + ret = IDirectDraw_CreateSurface(lpDD, &desc, &dsurface, NULL);
This isn't really necessary, if surface creation fails here many other tests wouldn't work. If you use a test data table like described above I'd leave it in to make sure the test loop works properly.
- desc.dwHeight = 1; + desc.dwWidth = 0xffff; + ret =
IDirectDraw_CreateSurface(lpDD, &desc, &dsurface, NULL); + if(!(ddcaps.ddsCaps.dwCaps & DDSCAPS_VIDEOMEMORY)) + { + ok(ret == DDERR_INVALIDPARAMS, "Creating an offscreen plain
surface with width 0xffff and height 1 returned %08x\n", ret);
- } + else + { + todo_wine ok(ret == DD_OK,
"Creating an offscreen plain
surface with width 0xffff and height 1 returned %08x\n", ret);
- }
You can drop the curly brackets here, the existing code doesn't use them for single-line code blocks. (Well, mostly. It's not entirely consistent).
I don't really like the DDSCAPS_VIDEOMEMORY check. I'll see how my Windows boxes behave, then we'll know if there's any deeper relationship between software-only ddraw and a maximum surface size.
- ret = IDirectDraw_CreateSurface(lpDD, &desc, &dsurface,
NULL); + todo_wine ok(ret == DDERR_INVALIDPARAMS, "Creating an offscreen
plain surface with width 0x10000 and height 1 returned %08x\n", ret);
- if(dsurface) + { +
IDirectDrawSurface_Release(dsurface); + dsurface = NULL; + }
If all possible paths expect a failure from CreateSurface, there's no need to try to release the surface. Just leak it, the test fails anyway.
- ret = IDirectDraw_SetCooperativeLevel(lpDD, NULL,
DDSCL_NORMAL); + ok(ret == DD_OK, "SetCooperativeLevel failed with %08x\n", ret);
I don't think this is needed. Also, is there a reason why you're not passing the window to the first SetCooperativeLevel call?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hi Patrick,
I've run the tests on a few of my machines, and here are the results:
First up, a Windows 7 64 bit box with 16 GB ram and a 1024MB Radeon 5770:
ddraw7.c:5752: Test failed: Creating an offscreen plain surface with width 0xffff and height 0xffff returned 88760827
Looks like the AMD driver returns a different error code. I can't find anything about this error code in the headers or via Google - except for some registry cleaner spam.
ddraw7.c:5777: 776914944 bytes of video memory. ddraw7.c:5787: Test failed: surface flag DDSCAPS_SYSTEMMEMORY is not set. ddraw7.c:5788: Test failed: surface flag DDSCAPS_VIDEOMEMORY is set. ddraw7.c:5789: dwCaps 0x10004040, dwCaps2 0, width 3063, height 65535.
I've added osme extra lines as you can see. I've also increased the size for testing purposes. It looks like the AMD driver doesn't care about the video memory size here. I think the best is just to ignore this test until we find an application that depends on this behavior, which I think is unlikely. At very least this test needs an explicitly requested pixel format to make the width * height * 4 size calculation work out.
Although if you're interested in doing some extra work, porting test_vidmem_accounting() from dlls/d3d9/tests/device.c to d3d8 and ddraw might be a good idea. I guess I should have done so when I wrote that test, but at the time the main point was to separate d3d9 and d3d9ex behavior.
My 32 bit Windows Vista machine equipped with a Geforce 7400 responds like your test expects, except for this:
ddraw7.c:5686: Test failed: Creating an offscreen plain surface with width 0xffff and height 1 returned 80070057
Which is the same as your testbot VM codepath expects. I'll see if I find an explanation for this. Interestingly the height = 0xffff codepath works as expected.
My third test machine, an ancient Windows XP 32 bit box with 2 GB ram and a Radeon 9000 Mobility (dx8 era) passes the tests, except that the width = 0xffff and height = 0xffff test returns DDERR_INVALIDPIXELFORMAT instead of DDERR_OUTOFVIDEOMEMORY.
I think it's a good idea to explicitly request a specific format.
-
Stefan Dösinger