Re: [PATCH v2 3/4] ddraw/tests: Also test RT caps for software device for ddraw4.

[Henri Verbeet]

On Tue, 16 Feb 2021 at 12:10, Paul Gofman  wrote:
> - static const REFCLSID test_devices[] =
> - {
> -
>    &IID_IDirect3DHALDevice,
> -
>    &IID_IDirect3DRGBDevice,
> - };

I think typedefs in general, but pointer typedefs in particular are
usually best avoided, so I'd prefer this as "static const CLSID *
const test_devices[]". (Or "static const GUID * const test_devices[]";
ddraw considers these CLSIDs for CreateDevice(), but GUIDs for
LPD3DENUMDEVICESCALLBACK, and IIDs for QueryInterface().)
> @@ -6388,27 +6395,29 @@ static void test_rt_caps(void)
>          {
>              NULL,
>              DDSCAPS_OFFSCREENPLAIN | DDSCAPS_3DDEVICE | DDSCAPS_VIDEOMEMORY,
> -
>        DDSCAPS_OFFSCREENPLAIN | DDSCAPS_3DDEVICE | DDSCAPS_VIDEOMEMORY | DDSCAPS_LOCALVIDMEM,
> -
>        {DDSCAPS_OFFSCREENPLAIN | DDSCAPS_3DDEVICE | DDSCAPS_VIDEOMEMORY | DDSCAPS_LOCALVIDMEM,},
>        0,
>        0,
> -
>        D3D_OK,
> -
>        DD_OK,
>        D3D_OK,
>        D3D_OK,
>    },
>    {
>        NULL,
>        DDSCAPS_OFFSCREENPLAIN | DDSCAPS_3DDEVICE,
> -
>        DDSCAPS_OFFSCREENPLAIN | DDSCAPS_3DDEVICE | DDSCAPS_VIDEOMEMORY | DDSCAPS_LOCALVIDMEM,
> -
>        {DDSCAPS_OFFSCREENPLAIN | DDSCAPS_3DDEVICE | DDSCAPS_VIDEOMEMORY | DDSCAPS_LOCALVIDMEM,
> -
>                DDSCAPS_OFFSCREENPLAIN | DDSCAPS_3DDEVICE | DDSCAPS_SYSTEMMEMORY},
>        0,
>        0,
> -
>        D3D_OK,
> -
>        D3D_OK,
> -
>        D3D_OK,
> -
>        DD_OK,
> -
>        DD_OK,
> -
>        DD_OK,
>    },

D3D_OK and DD_OK are defined to the same value, of course, but since
you're changing them: As a principle IDirect3D* interfaces return D3D*
HRESULTs, so conceptually I think D3D_OK is correct here.
> -
>        if (surface_desc.ddsCaps.dwCaps & DDSCAPS_VIDEOMEMORY && hr == DDERR_NODIRECTDRAWHW)
> -
>        {
> -
>            skip("No 3d hardwate, skipping test %u, device_type %u.\n", i, device_type);

"hardware"
> -
>            continue;
> -
>        }
> -
>        ok(hr == DD_OK || (device_type && (surface_desc.ddsCaps.dwCaps & (DDSCAPS_VIDEOMEMORY | DDSCAPS_ZBUFFER))
> -
>                == (DDSCAPS_VIDEOMEMORY | DDSCAPS_ZBUFFER) && hr == DDERR_UNSUPPORTED)
> -
>                || broken(device_type && test_data[i].pf == &p8_fmt && hr == DDERR_INVALIDPIXELFORMAT),
> -
>                "Got unexpected hr %#x, test %u, device_type %u.\n", hr, i, device_type);
> -
>        if (FAILED(hr))
> -
>            continue;

Perhaps an "is_software_device_type()" would be clearer than testing
the index of the test_devices[] array.

Do you expect we'll have more RGB/software device tests in the future?
Perhaps it makes sense to handle this the same way we handle feature
levels in the d3d11 tests. I.e., passing the device type as a
parameter to the test function, and then simply invoking it multiple
times. One of the advantages would be that we could check whether a
particular device type is supported in a common caller.

As an aside, I'm not sure whether I mentioned this before or not, but
splitting ddraw test changes per ddraw version is probably one of the
few cases where splitting patches doesn't do much for making review
easier. If it's the same to you, feel free to combine these into a
single patch in the future.