D3DRM Implementation patches upto CreateDeviceFromSurface

[Stefan Dösinger]

Hi,

A few more suggestions, once they're implemented go ahead and send those patches to wine-patches :-) .

Patch 1:
> +    struct d3drm_device *object = NULL;
I don't think you need to initialize to NULL here. Looks good otherwise.

> +HRESULT init_device(struct d3drm_device *device, REFIID riid, IUnknown **out) DECLSPEC_HIDDEN;
You're not using this anywhere.

Patch 2 looks good

Patch 3, also applies to the others: I guess you have to set *device = NULL if the call fails. Please extend the tests accordingly. I recommend to also test what happens if you pass device = NULL. If this returns an error, you can do something like

if (!device)
    return ERROR;
*device = NULL;

if (!width || !height ...)
    return ERROR;

That way you have to bother about setting *device = NULL only once and not in every error case. If device = NULL always results in a crash you don't even need the if (!device) check first.

I also recommend to move the IDirect3D2_Release(d3d2); in patch 3 already, and not patch 5. The place where patch 5 puts it is better than the one where you add it initially.

Patch 4 looks good

Patch 5: Don't forget to set *device = NULL if the tests say you should do that. Also take care of the thunk :-)

Patch 6 looks good

Patch 7:
> +    if (!(desc.ddsCaps.dwCaps & DDSCAPS_3DDEVICE))
> +        return DDERR_INVALIDCAPS;
Do you need an explicit check for this? I imagine that IDirect3D2::CreateDevice or IDirectDrawSurface::QueryInterface(&IID_IDirect3DDevice) take care of this task for you.

> +    hr = IDirectDrawSurface_GetAttachedSurface(surface, &caps, &ds);
> +    if (SUCCEEDED(hr))
> +    {
> +        create_z_surface = FALSE;
> +        IDirectDrawSurface_Release(ds);
> +    }
> ...
>      if (FAILED(hr))
>      {
> -        IDirectDrawSurface_DeleteAttachedSurface(surface, 0, ds);
> +        if (ds)
> +            IDirectDrawSurface_DeleteAttachedSurface(surface, 0, ds);
>          return hr;
>      }
I think this can detach an application-attached depth surface. The GetAttachedSurface call sets ds != NULL, then something fails and you call DeleteAttachedSurface. I think the proper thing to do is to set ds = NULL in the if (SUCCEEDED(hr)) branch after the GetAttachedSurface call.

I also think you don't need the if (ds) check before calling DeleteAttachedSurface. Just let DeleteAttachedSurface(surface, 0, NULL) fail.

Patch 8 looks ok.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 842 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20150805/a773c438/attachment-0001.sig>