Re: [PATCH] ddraw: add dsurface dimension tests, try 5

[Stefan Dösinger]

-----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 SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSrIlaAAoJEN0/YqbEcdMw48UP/R686XgOaYmgwxP23u5AUpM2
+zemL+zkTAmQdLXA0yrFTmXvQcEvJtd1j7Pbg32hCh1P4X27ygBO5L+XJ4oxfTwH
ymYYyIJsU96iTIDqojOlqzAvQ/Z/aibIfrfmviWx0LIClUymu5+VWYgYmONqbf5p
QAkVrAV12DRrQq1sJPXoqSZDaDvtsuj5hjuZbGdZBoP0vJygl+yZXgxdNsrfDh3l
TIRrYyETdWdCFjjxEbJyFeB6lrFweAHOel54cuvAlGjErlSczdj8oCMGoHHGtnNr
JOD5dK0LI1o9ngkbmIMSHfRo4Sk6Ug6476B9zNR/fETRXsBEPCVQfWoUIV3h0RI0
RjX5NMFRHANYRjXSQd8yXcpdyNWYONu0BhyBrMnHBNln+CpXyUJ8PM0nMptnVSKD
1fWzQfnQNnaklxYuHVvSuVctOPLmbf5PIHNSE333Xj0ddDA2uCtEzOkSH1aMStjV
TMZUa7ggY0wCjeLP1NKOY6jN7ExhANx8ItzXn4h8n9eWNMyqJXi9kpfiVGJA9/v8
/Xmi2nWivBBWUh8qq5nPUGnWcLgeKuvFTobGq3jUbCYEf3/T7sZz58xuFdAp/xUt
Yzg2ztvP9DUi8BuJuk51c0dtujgtIRMKkNCG/ISe7za2QhGloAGs0T/LWdUmrTxo
++uOj6zpdlFWCapAen6e
=13rk
-----END PGP SIGNATURE-----

[PATCH] ddraw: add dsurface dimension tests, try 5

[Stefan Dösinger]

-----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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSrNkLAAoJEN0/YqbEcdMwCikP/0Sa8DBLTDIOZn1Z68lrU65Z
NYiapS+6QUPGKqb6Lt/5J3rMzztjWMga/5bAXtC5l0H6lV85UsSNYn6+c9x8z33B
OrQUVxsYl7LBUjcFpUUAlgvaA6qolsXdlAhDVHiXMm3ilK2dUGKtOhRwgCElagbC
g1y9VwD9HK7B69bRFk2kaNJ6maSVipOEk92dmypJWMxdyHoy1jmEAVl1WclGT57x
ueaCc968Ny8mwtJyuXb84tzD6bUlvdkA45K90KoIxXWSbnw7YnnbepKcbFNUbhSF
T9fipCtxTIdCNtnuSr8WDMmc9r68/BdcFqcr2OcsOuRomoSshZyuUrYOfwB0RqZk
xGZt9QELI/7uwL3n5rQm1Cgj3U6CPefHZPJchBvvpJlbK2DaupBlYzOpslthZxt1
HvwggswvLR0IK5PU6d/nksogtOFsi7M+TWbalsuLDYNa4faEiXNR42Nal1a6n174
XgHKfup7gA+G5VLG3RmvnmlaHUhTqdP5SLMhjYPtqACESZ1VWGpie+xIGx64D64p
da1LtrAweydZ6aRD1pSpOurBc3ACJXVKGaio6WNKMHfFpgNRjM70k8+RVTrw0rwp
+BhXbEzKIHDS+fqJI7bHQMnPvWrUh1+L3ivk0HGDS4qF5hpknAxlE21Fzk8m6TcX
DoDJc0R+Fzp76PM775yl
=vQ0F
-----END PGP SIGNATURE-----