Re: [PATCH 1/4] d3drm/tests: Add test for IDirect3DRM*::CreateDeviceFromClipper (try 2).

[Stefan Dösinger]

Hi,
> - hr = DirectDrawCreateClipper(0, &clipper, NULL);
> - ref = get_refcount((IUnknown *)clipper);

You're not reading ref anywhere as far as I can see. Did you forget to re-check the refcount after creating the device?

If the refcount is incremented by 2 or more: See if the clipper is assigned to the render target and / or depth stencil. This wouldn't really do anything, but it's Microsoft we're talking about here.
> - hr = IDirect3DRM_QueryInterface(d3drm1, &IID_IDirect3DRM2, (void **)&d3drm2);
> - ok(hr == D3DRM_OK, "Cannot get IDirect3DRM3 interface (hr = %x).\n", hr);

Minor mistake: IDirect3DRM2 vs IDirect3DRM3.
> - memcpy(&driver, &IID_IDirect3DRGBDevice, sizeof(GUID));

I think an assignment should do the job.
> - hr = IDirect3DRM2_CreateDeviceFromClipper(d3drm2, clipper, &driver, rc.right, rc.bottom, &device2);
> - ok(hr == D3DRM_OK, "Cannot get IDirect3DRMDevice2 interface (hr = %x).\n", hr);

I'd say "Cannot create IDirect3DRMDevice2 (hr = ...)" here. You're not retrieving an interface here.
> - /* Fetch immediate mode device in order to access render target */
> - hr = IDirect3DRMDevice2_GetDirect3DDevice2(device2, &d3ddevice2);
> - todo_wine ok(hr == D3DRM_OK, "Cannot get IDirect3DDevice2 interface (hr = %x).\n", hr);
> - if (FAILED(hr))
> - {
> -
>    IDirect3DRMDevice2_Release(device2);
> -
>    IDirect3DRM2_Release(d3drm2);
> -
>    IDirect3DRM_Release(d3drm1);
> -
>    IDirectDrawClipper_Release(clipper);
> -
>    return;
> - }

You can use the cleanup label here too if you initialize the interface pointers to NULL and add checks for NULL before calling release.
> - /* Check properties of primary and depth surfaces */
> - desc.dwSize = sizeof(desc);
> - hr = IDirectDrawSurface_GetSurfaceDesc(surface, &desc);
> - ok(hr == DD_OK, "Cannot get surface desc structure (hr = %x).\n", hr);
> - if (FAILED(hr))
> -
>    goto cleanup;

Minor nitpick: A "primary" surface represents the screen content. The correct term in in this case is "render target".  Note that a primary surface can be a render target in fullscreen mode.
> - ok((desc.dwWidth == rc.right) && (desc.dwHeight == rc.bottom), "Expected surface dimentions = %d, %d, got %d, %d.\n",
> -
>    rc.right, rc.bottom, desc.dwWidth, desc.dwHeight);
> - expected_caps = DDSCAPS_OFFSCREENPLAIN | DDSCAPS_3DDEVICE | DDSCAPS_SYSTEMMEMORY;
> - todo_wine ok(desc.ddsCaps.dwCaps == expected_caps, "Expected %x for caps, got %x.\n", expected_caps, desc.ddsCaps.dwCaps);

As discussed on IRC ignore DDSCAPS_SYSTEMMEMORY here. I think it happens because you use a RGB device, and I don't think it's something we should care about unless an application depends on this.

Check dwRGBBitCount of the render target. It should match the bit depth of the screen mode.
> - if (FAILED(hr = IDirectDraw_SetDisplayMode(ddraw, desc.dwWidth, desc.dwHeight, 8)))
> - {
> -
>    win_skip("Cannot set display mode to 8bpp (hr = %x), skipping tests.\n", hr);
> -
>    IDirectDrawSurface7_Release(surface7);
> -
>    IDirectDrawSurface_Release(ds);
> -
>    IDirectDrawSurface_Release(surface);
> -
>    IUnknown_Release(unknown);
> -
>    IDirectDraw_Release(ddraw);
> -
>    goto cleanup;
> - }

There's no need to try to set the display mode to 8. Also testing if changing the mode works is outside the scope of this test. What's interesting is that the pixel format of the primary surface of a new device after changing the mode.

Please also check the Z bit depth, and if the surface has stencil bits.

[PATCH 1/4] d3drm/tests: Add test for IDirect3DRM*::CreateDeviceFromClipper (try 2).

[Aaryaman Vasishta]

On Thu, Jun 11, 2015 at 9:34 PM, Stefan Dösinger 
wrote:
> Hi,
> > - hr = DirectDrawCreateClipper(0, &clipper, NULL);
> > - ref = get_refcount((IUnknown *)clipper);
>
> You're not reading ref anywhere as far as I can see. Did you forget to
> re-check the refcount after creating the device?
>
> If the refcount is incremented by 2 or more: See if the clipper is
> assigned to the render target and / or depth stencil. This wouldn't really
> do anything, but it's Microsoft we're talking about here.

That was a mistake from me. The refcount tests are in patch 4. I'll also
take a look at where the clipper is assigned.
> > - memcpy(&driver, &IID_IDirect3DRGBDevice, sizeof(GUID));
>
> I think an assignment should do the job.

That was just to maintain consistency with the existing tests, I could
still use an assignment if you want to.
> There's no need to try to set the display mode to 8. Also testing if
> changing the mode works is outside the scope of this test. What's
> interesting is that the pixel format of the primary surface of a new device
> after changing the mode.

By chaning the mode you mean the dimentions/bpp or both?

I will implement everything else you specified.

Thanks for the review! :)

[PATCH 1/4] d3drm/tests: Add test for IDirect3DRM*::CreateDeviceFromClipper (try 2).

[Stefan Dösinger]

Am 11.06.2015 um 18:40 schrieb Aaryaman Vasishta :
> > - memcpy(&driver, &IID_IDirect3DRGBDevice, sizeof(GUID));
>
> I think an assignment should do the job.
> That was just to maintain consistency with the existing tests, I could still use an assignment if you want to.

Yes, please use assignments. Existing bad code is no excuse to write new bad code :-) .
> There's no need to try to set the display mode to 8. Also testing if changing the mode works is outside the scope of this test. What's interesting is that the pixel format of the primary surface of a new device after changing the mode.
> By chaning the mode you mean the dimentions/bpp or both?

Well, you're changing the mode, and then checking if the mode change applied. That made sense for your experiments with Windows 8 because SetDisplayMode didn't behave as intended. It does not make sense for an upstream d3drm test. For d3drm just check how the render target format changes when the device is created when a different screen mode is active.

Stefan