D3DRM Implementation patches upto CreateDeviceFromSurface

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2015-07-08 um 19:58 schrieb Aaryaman Vasishta:

> +    hr = IDirect3DRMDevice3_QueryInterface(device3, &IID_IDirect3DRMDevice2, (void**)device);
> +    IDirect3DRMDevice3_Release(device3);
> +    if (FAILED(hr))
> +        return hr;
> +
> +    return D3DRM_OK;
Minor style point: The if check here is redundant, you can just use "return hr;" here.

> +
> +    hr = DirectDrawCreate(NULL, &ddraw, NULL);
> +    if (FAILED(hr))
> +        return hr;
> +
> +    hr = IDirectDrawClipper_GetHWnd(clipper, &window);
> +    if (FAILED(hr))
> +        return hr;
> +
> +    hr = IDirectDraw_SetCooperativeLevel(ddraw, window, DDSCL_NORMAL);
> +    if (FAILED(hr))
> +        return hr;
> ...
This leaks ddraw in a number of cases. same for device and surface later on.

> +    hr = Direct3DRMDevice_create(&IID_IDirect3DRMDevice3, (IUnknown **)device);
> +    if (FAILED(hr))
> +        return hr;
I recommend to add a forward declaration of struct d3drm_device to d3drm_private.h and make Direct3DRMDevice_create return a struct d3drm_device ** instead of an IUnknown. That way you can keep the actual structure in device.c and avoid the ugly void * + version hacks.

Have a look at how struct wined3d_device works in include/wine/wined3d.h and dlls/wined3d/wined3d_private.h for example.

Instead of calling IDirect3DRMDevice_Release() if the creation fails add a d3drm_device_destroy() helper function. If you need the different release behavior some day you can have the InterlockedDecrement() == 0 in each Release method and call d3drm_device_destroy.

> +    hr = IDirectDraw_CreateSurface(ddraw, &surface_desc, &surface, NULL);
I suggest to rename "surface" to "render_target".

> +void set_primary_surface(void **device, UINT version, IDirectDrawClipper *clipper, IDirectDrawSurface *primary_surface)
> +{
> +    struct d3drm_device *object;
> +
> ...
> +
> +    object->primary_surface = primary_surface;
> +    object->clipper = clipper;
> +    IDirectDrawClipper_AddRef(clipper);
> +}
Is there really a need to store (and AddRef) the clipper? The primary surface already keeps a reference to it, so you should be OK. It won't give you the increment by 2 that native has though, but I don't think that's important.

> +        hr = IDirectDraw_CreateSurface(ddraw, &surface_desc, &ds, NULL);
> +        if (FAILED(hr))
> +            return hr;
> +
> +        hr = IDirectDrawSurface_AddAttachedSurface(surface, ds);
> +        if (FAILED(hr))
> +        {
> +            IDirectDrawSurface_Release(ds);
> +            return hr;
> +        }
Similarly: Is there a need to store the depth stencil? You could just release your reference after attaching it to the render target, it will stay alive anyway. (If it's just for the DeleteAttachedSurface later: Ignore it)

Releasing ds right after AddAttachedSurface would simplify your Release() method a bit. Also keep in mind that you can always use GetAttachedSurface to get the depth stencil. That may be even more correct because the application can in theory change the depth stencil after creating the device.

(Possible problem: The device attaches another depth stencil, yours dies. Is this a problem? I'd say don't bother as long as no app does it. Offscreen rendering and depth buffer switching didn't exactly work well prior to d3d8, even though the API in theory supported it)

You have to keep ddraw alive because nothing (neither your surfaces nor your device) hold a reference to it. Same for the primary. I think it's also good to store a reference to the render target, I believe you need it to show the rendering results on the screen (The immediate mode device holds a ref too, but it's awkward to always query it from the device).

> +    if (device->ref == 1 && device->z_created_internally)
> +        IDirectDrawSurface_DeleteAttachedSurface(device->render_target, 0, device->depth_surface);
As mentioned on IRC this is not thread safe A lot can happen between your ref == 1 comparison and the actual InterlockedDecrement.

Wrt patch 6 (d3drm1_CreateDeviceFromClipper): It's a bit unfortunate that this is mostly a copypasted version of version 3, and the difference that actually matters is in init_device anyway. Maybe there is a nicer solution, e.g. a helper function for v1 and v3 with a version parameter.

> +    hr = Direct3DRMDevice_create(&IID_IDirect3DRMDevice, (IUnknown **)device);
> +    if (FAILED(hr))
> +        return hr;
> +
> +    desc.dwSize = sizeof(desc);
> +    hr = IDirectDrawSurface_GetSurfaceDesc(backbuffer, &desc);
> +    if (FAILED(hr))
> +    {
> +        IDirect3DRMDevice_Release(*device);
> +        return hr;
> +    }
You can reorder these calls, then you don't have to release *device.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJVnmARAAoJEN0/YqbEcdMwAbgP/2cVW21QIUvfyQegyrIuycLE
MTyYXHTKiNgjtTRbvik3DOkPMNkb/kRC08f6ozIkpDzsoZGU01MXPAEkzNEgnN4P
OJdCBaCSgt8G0yqel05WeWOsbdalxB4c10uuAsbyn5/T0nqYrYpLQr4Cxaqu8zUs
n/JPHDIHoVrrif9Up0/3yLGKykB+51uvCrDdEr3Jqs2GKCJSrph7cUCyne1TrQfG
AWY4qO7Q+lB4eyxEM+5vnnuBNfIzq4nN95131l2k0F3frhf3wKZKsmuYtljnbNeB
dK+fYaUFgPf9wquuM+3E3DysFJ7F7rIAu1mfgsgL6fqy41Zl+cWZZA2wA8tWx87a
JyM7SmP8TE94OMJi57/mksn6BAFr25h/P3gUxZjqhGZNzrqL2Fu4MvPLav9sxsFv
biaeUnwRuu6R4qVppm1nueMmJD0hOpJE8oS3WLrnVGfdUfTdE/iQ/9SpizMG8vqb
V5phn21gbKGGV11QVys4ZSV0UvoL3zQH49KLJhQjThyLN0jGwZWxveIHUAhf63v5
XHIetAn+huzlpP06CN/FHiRZ4mkxaP9PhdzeI/qbRBe8WZjja6mEiVXdfvBEU+zs
iOOs9lYu7SMt3BcusPUArjZLYyRdJI7yVO4mo5zOtpAiX9aC6P30+Obf2wXjhkLY
D5seFpSi4oe9wSl/CmUq
=0HT3
-----END PGP SIGNATURE-----