D3DRM Implementation patches upto CreateDeviceFromSurface

[Aaryaman Vasishta]

CreateDeviceFromD3D will be pushed as soon as patch 9 is finalised. The
main concern right now is the thunking of the Release methods of the device.

This concern comes up from the fact that the CreateDeviceFromSurface
methods from all 3 versions behave the same way except for one difference
in version 3, which I mention below.

When you create a device from IDirect3DRM3::CreateDeviceFromSurface and
release it, native will also destroy the depth surface attached to it (if
it created it internally), otherwise it'll let the application keep the
reference to the depth surface it created.

Other than the above peculiarity, the behaviour is identical.

I've attached all the commits which lead to the point of implementing
CreateDeviceFromSurface.

Please, review the patches as much as you'd like and if possible, provide
feedback and suggestions on any improvements that can be made, possible
comments required, any stupid mistakes I made etc. :)


Thank you!
Aaryaman

[Aaryaman Vasishta]

Keep in mind that patch 0001 and 0002 are already upstream, please take a
look at patches 0003 onward, sorry for not mentioning it earlier!

Jam

On Wed, Jul 8, 2015 at 11:28 PM, Aaryaman Vasishta <
jem456.vasishta(a)gmail.com> wrote:
> CreateDeviceFromD3D will be pushed as soon as patch 9 is finalised. The
> main concern right now is the thunking of the Release methods of the device.
>
> This concern comes up from the fact that the CreateDeviceFromSurface
> methods from all 3 versions behave the same way except for one difference
> in version 3, which I mention below.
>
> When you create a device from IDirect3DRM3::CreateDeviceFromSurface and
> release it, native will also destroy the depth surface attached to it (if
> it created it internally), otherwise it'll let the application keep the
> reference to the depth surface it created.
>
> Other than the above peculiarity, the behaviour is identical.
>
> I've attached all the commits which lead to the point of implementing
> CreateDeviceFromSurface.
>
> Please, review the patches as much as you'd like and if possible, provide
> feedback and suggestions on any improvements that can be made, possible
> comments required, any stupid mistakes I made etc. :)
>
> Thank you!
> Aaryaman

[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-----

[Aaryaman Vasishta]

On Thu, Jul 9, 2015 at 5:20 PM, Stefan Dösinger <stefandoesinger(a)gmail.com>
wrote:
> 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.

At the moment there doesn't seem to be a significant need, except for the
refcounting requirements.
> > +        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)

It's just for the DeleteAttachedSurface part. I'll remove it for now so
it's easier to thunk towards version 3.
> 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.

Since we're not gonna use DeleteAttachedSurface, we won't need to read the
refcount either. I agree there are some synchronization issues in this
case. A good idea for future reference would be to use flags to determine
which version the device was created on, to make it thread-safe and generic
enough for the release method to be used by all versions.
> 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.

Similar to set_primary_surface defined above init_device in the patches
(though we won't need the version parameter there anymore as I'll be using
the struct there.).


I've taken note of all the other suggestions and will incorporate them in
the next patchset. I do agree there's quite a bit of optimization left with
regards to the thunking and version checks.


Thanks!
Jam

[Aaryaman Vasishta]

Here's my current work on CreateDeviceFromClipper. If this is good enough,
the CreateDeviceFromSurface, after which CreateDeviceFromD3D
implementations will follow.

On Thu, Jul 9, 2015 at 8:04 PM, Aaryaman Vasishta <jem456.vasishta(a)gmail.com
> wrote:
> On Thu, Jul 9, 2015 at 5:20 PM, Stefan Dösinger <stefandoesinger(a)gmail.com
> > wrote:
> > 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.
>
> At the moment there doesn't seem to be a significant need, except for the
> refcounting requirements.
> > > +        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)
>
> It's just for the DeleteAttachedSurface part. I'll remove it for now so
> it's easier to thunk towards version 3.
> > 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.
>
> Since we're not gonna use DeleteAttachedSurface, we won't need to read the
> refcount either. I agree there are some synchronization issues in this
> case. A good idea for future reference would be to use flags to determine
> which version the device was created on, to make it thread-safe and generic
> enough for the release method to be used by all versions.
> > 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.
>
> Similar to set_primary_surface defined above init_device in the patches
> (though we won't need the version parameter there anymore as I'll be using
> the struct there.).
>
> I've taken note of all the other suggestions and will incorporate them in
> the next patchset. I do agree there's quite a bit of optimization left with
> regards to the thunking and version checks.
>
> Thanks!
> Jam

[Stefan Dösinger]

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

Hi,

I like the overall structure a lot more than the previous incarnation!

Here are a few more comments:
> +    hr = DirectDrawCreate(NULL, &ddraw, NULL);
> +    if (FAILED(hr))
> +        return hr;
> +
>      hr = Direct3DRMDevice_create(&object);
>      if (FAILED(hr))
>          return hr;
> You're leaking "ddraw" here. There are many more incomplete error handling paths in the patches.
> +HRESULT set_clipper_surface(struct d3drm_device *object, IDirectDraw *ddraw, IDirectDrawClipper *clipper, int width, int height,
> +            IDirectDrawSurface **surface) DECLSPEC_HIDDEN;
> I'm not entirely happy with this name, because it doesn't merely set any surfaces, it creates them. What do you think about d3drm_device_create_surfaces_from_clipper? Check for consistency wrt the "d3drm_device" part. E.g. the next function you could name "d3drm_device_init" or just "device_init".
> +HRESULT init_device(struct d3drm_device *device, UINT version, IDirect3DRM *d3drm, IDirectDraw *ddraw, IDirectDrawSurface *surface, BOOL z_surface, IDirect3D2 *d3d2,
> +            int width, int height)
> "BOOL z_surface" is unused right now. It won't be needed until you add CreateDeviceFromSurface version 3.

Since you're passing in a version parameter as I requested, you can now remove the "d3d2" parameter. The function can QI it from ddraw. That reduces duplicated code in the calling functions.
> +        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;
> +        }
> +
> +        if (version == 1)
> +            hr = IDirectDrawSurface_QueryInterface(surface, &IID_IDirect3DRGBDevice, (void **)&device1);
> +        else
> +            hr = IDirect3D2_CreateDevice(d3d2, &IID_IDirect3DRGBDevice, surface, &device2);
> +        if (FAILED(hr))
> +        {
> +            IDirectDrawSurface_DeleteAttachedSurface(surface, 0, ds);
> +            IDirectDrawSurface_Release(ds);
> +            return hr;
> +        }
> You are leaking ds here in the success case. You can release it after calling AddAttachedSurface.
> cref2 = get_refcount((IUnknown *)clipper);
> -    todo_wine ok(cref2 > cref1, "expected cref2 > cref1, got cref1 = %u , cref2 = %u.\n", cref1, cref2);
> +    ok(cref2 > cref1, "expected cref2 > cref1, got cref1 = %u , cref2 = %u.\n", cref1, cref2);

I'd expect cref2 to match the exact value Windows returns here (3). If that is indeed the case you can make this cfref2 == cref1 + 2 or even cref2 == 3.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJVtL6SAAoJEN0/YqbEcdMwGMsP/12R5+3oztVIOpd0MOvKJjUQ
Ys1wMU+1Ntpy3xrVw05s7UuZdLS4rAeaodCELYsV4tMKjWhSpQ0uzH4rbBlafDx1
Mfem1clZ+UEwoH2cvNJ6KwyUwLZTuTln3WuW3xTDyF2BIvu2jyjepWJ+p30nV2MB
C63nZAZUt0VruBQlYMy3xhRIOLJMRiTo+a7Zw6Ge8YXIDWCufYcU8iH7ROl+POSY
cZokIqwUaGOAn/PDXQfo+vhJUcLoHsaQBDBB+svc0CueFqNLlXExpE279jNgZaZz
wvpEcPpmibOSw1JBE6edipRs7t7UScLh3Tt+JUCqJiM+vQoBnZoSYk1wYTo604Oy
l5YI7Eeony6J8HSuQccC+XAog1emONiQlMU7Qb7zScNxXUCpZe7EUq7ExCGSBbdv
H/L+Qc9iWl/lqVabRI1KYixJcQpizlIp4k/Q6EXhwaBN25XrSdGyq+OHiM+E/ut3
ugQGYvWZtCf+xHV3N4zBJM/P5R7kBR5jBFola1TFXy4pxnAFWENn5Waz7XdUdqdq
p72J6VyV1rMeQ6uLMHj/3BCvlMW9Kr+6CP6UC8wltgn89H+xh6dJ7uiTBDuq2Y5b
7RrBCvS1/S9CMByXCEmugq0SPW3tv8criYz+FRFOP5s57tvnRSBHzfrhOOPlN0O/
cPAjhf9s9CuQRZPChhgX
=mIH2
-----END PGP SIGNATURE-----

[Aaryaman Vasishta]

On Sun, Jul 26, 2015 at 4:33 PM, Stefan Dösinger <stefandoesinger(a)gmail.com>
wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi,
>
> I like the overall structure a lot more than the previous incarnation!
>
> Here are a few more comments:
> > +    hr = DirectDrawCreate(NULL, &ddraw, NULL);
> > +    if (FAILED(hr))
> > +        return hr;
> > +
> >      hr = Direct3DRMDevice_create(&object);
> >      if (FAILED(hr))
> >          return hr;
> > You're leaking "ddraw" here. There are many more incomplete error handling
> > paths in the patches.

I'll post another incarnation with all the leaks I can possibly find fixed
:)
> > +HRESULT set_clipper_surface(struct d3drm_device *object, IDirectDraw
> > *ddraw, IDirectDrawClipper *clipper, int width, int height,
> > +            IDirectDrawSurface **surface) DECLSPEC_HIDDEN;
> > I'm not entirely happy with this name, because it doesn't merely set any
> > surfaces, it creates them. What do you think about
> > d3drm_device_create_surfaces_from_clipper? Check for consistency wrt the
> > "d3drm_device" part. E.g. the next function you could name
> > "d3drm_device_init" or just "device_init".

It took me a while to figure out a name, since a name like
d3drm_device_create_surfaces_from_clipper felt too long for me initially.
But if long function names are accepted then I don't mind putting that name
up.
> > +HRESULT init_device(struct d3drm_device *device, UINT version,
> > IDirect3DRM *d3drm, IDirectDraw *ddraw, IDirectDrawSurface *surface, BOOL
> > z_surface, IDirect3D2 *d3d2,
> > +            int width, int height)
> > "BOOL z_surface" is unused right now. It won't be needed until you add
> > CreateDeviceFromSurface version 3.

Right, I'll remove this for now and re-add it in the
CreateDeviceFromSurface patches. Note that z_surface is still needed in
version 1 and 2 of CreateDeviceFromSurface in the cases where the surface
already has ds attached to it by the application (in which case native
wouldn't create its own ds internally, as seen by the tests).
> You are leaking ds here in the success case. You can release it after
> calling AddAttachedSurface.

I'm not sure why I allowed that leak to happen, but I believe it was
causing some refcounting/segfault issues if I released the ds there. I'll
get back with more details about this.



Jam

[Aaryaman Vasishta]

I've fixed whatever leaks I could find, and also attached implementations
of CreateDeviceFromSurface. Do take a look and if possible, provide
feedback on any leaks that I might have missed.


Jam

On Sun, Jul 26, 2015 at 5:34 PM, Aaryaman Vasishta <
jem456.vasishta(a)gmail.com> wrote:
> On Sun, Jul 26, 2015 at 4:33 PM, Stefan Dösinger <
> stefandoesinger(a)gmail.com> wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > Hi,
> >
> > I like the overall structure a lot more than the previous incarnation!
> >
> > Here are a few more comments:
> > > +    hr = DirectDrawCreate(NULL, &ddraw, NULL);
> > > +    if (FAILED(hr))
> > > +        return hr;
> > > +
> > >      hr = Direct3DRMDevice_create(&object);
> > >      if (FAILED(hr))
> > >          return hr;
> > > You're leaking "ddraw" here. There are many more incomplete error
> > > handling paths in the patches.
>
> I'll post another incarnation with all the leaks I can possibly find fixed
> :)
> > > +HRESULT set_clipper_surface(struct d3drm_device *object, IDirectDraw
> > > *ddraw, IDirectDrawClipper *clipper, int width, int height,
> > > +            IDirectDrawSurface **surface) DECLSPEC_HIDDEN;
> > > I'm not entirely happy with this name, because it doesn't merely set any
> > > surfaces, it creates them. What do you think about
> > > d3drm_device_create_surfaces_from_clipper? Check for consistency wrt the
> > > "d3drm_device" part. E.g. the next function you could name
> > > "d3drm_device_init" or just "device_init".
>
> It took me a while to figure out a name, since a name like
> d3drm_device_create_surfaces_from_clipper felt too long for me initially.
> But if long function names are accepted then I don't mind putting that name
> up.
> > > +HRESULT init_device(struct d3drm_device *device, UINT version,
> > > IDirect3DRM *d3drm, IDirectDraw *ddraw, IDirectDrawSurface *surface, BOOL
> > > z_surface, IDirect3D2 *d3d2,
> > > +            int width, int height)
> > > "BOOL z_surface" is unused right now. It won't be needed until you add
> > > CreateDeviceFromSurface version 3.
>
> Right, I'll remove this for now and re-add it in the
> CreateDeviceFromSurface patches. Note that z_surface is still needed in
> version 1 and 2 of CreateDeviceFromSurface in the cases where the surface
> already has ds attached to it by the application (in which case native
> wouldn't create its own ds internally, as seen by the tests).
> > You are leaking ds here in the success case. You can release it after
> > calling AddAttachedSurface.
>
> I'm not sure why I allowed that leak to happen, but I believe it was
> causing some refcounting/segfault issues if I released the ds there. I'll
> get back with more details about this.
>
> Jam

[Stefan Dösinger]

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

Hi,

- From patch 1:
> +    hr = init_device(object, 1, iface, ddraw, render_target, width, height);
> +    if (FAILED(hr))
> +        d3drm_device_destroy(object);
> +    else
> +        *device = IDirect3DRMDevice_from_impl(object);
> Aren't you leaking ddraw here? It's possible that d3drm_device_destroy destroys it because ddraw is stored in object, but the caller doesn't know this. I'd be better to explicitly destroy it.

What about render_target in this case? Clipper isn't a problem because you didn't create it - the application did.
> +HRESULT d3drm_device_create_surfaces_from_clipper(struct d3drm_device *object, IDirectDraw *ddraw, IDirectDrawClipper *clipper, int width, int height, IDirectDrawSurface **surface)
> ...
> +    hr = IDirectDraw_CreateSurface(ddraw, &surface_desc, &render_target, NULL);
> +    if (FAILED(hr))
> +        return hr;
> Here you have to release clipper (because you addref'ed it) and the primary surface.

You can avoid the clipper release by addrefing it after the CreateSurface call. AddRef never fails.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJVufURAAoJEN0/YqbEcdMwgCQP/06Ed03h8q0pbbk7IuVO+9aU
yVkglF4i+Wlw1JqM0qqIPeJg2pGd166qMdofiP29fks62Pp231A4kIvEcRGteBJf
stRWLLGyBsPGPWf+UB5SmT2Qi6rT48LQE9aDi82LvXlzHTA4itHhCtjxQgoAwFLk
hp7Wu6Fx6Rm6TtjijmBIpp2y3o42w+nTjdwGWFEgWSnwKk0oqHYDTHZbkvJE9h1K
vbriLQIO7yAE2/4C+mWmMiOlVHXksgDttUOFtG1dqb0Xg3mo/rERd/5HwHRntnKi
rVkc1W2ahagrmrckf72airTAyEwhO7RqAoqjW0zioHrBpU/xoUkxQuiMcrjyndGj
2yTeZ3Lrd4LRJhIKVJdCnzXhFLYEg2i0iUjAduh0UVERJw0ya6B43Rz4XgKjBXJ4
2mWK9NVkfSy+IZ5lpKZbUYQhNu+nO7BQtGm9CNjtHj13xt5KXy6MDaB4NiICOk2N
IMyymeuQl9TF+nWjw80qQohONYOwL7ty5e5oUSbiKdmlNeqGLNbZPhT5HH/NaqUP
WQ4qv+2br3n5p4IPFxmm54gvelcVdL39sC2/7bd9GUikvB2n0iESbtDNpZo1dvuM
R5v7tG+uvsz4jpiJ/xIW5eKy0naYHP/Ri1cXQHqbG5ogGbPY9EKXo59TQvuhfkGa
JES0dpGT/kJI2cW2bQoR
=DeiB
-----END PGP SIGNATURE-----

[Aaryaman Vasishta]

On Thu, Jul 30, 2015 at 3:27 PM, Stefan Dösinger <stefandoesinger(a)gmail.com>
wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi,
>
> - From patch 1:
> > +    hr = init_device(object, 1, iface, ddraw, render_target, width,
> > height);
> > +    if (FAILED(hr))
> > +        d3drm_device_destroy(object);
> > +    else
> > +        *device = IDirect3DRMDevice_from_impl(object);
> > Aren't you leaking ddraw here? It's possible that d3drm_device_destroy
> > destroys it because ddraw is stored in object, but the caller doesn't know
> > this. I'd be better to explicitly destroy it.
>
> What about render_target in this case? Clipper isn't a problem because you
> didn't create it - the application did.

d3drm_device_create_surfaces_from_clipper sets the clipper and primary
surface, and init_device sets ddraw and d3drm first before anything else,
so even if any of them fail d3drm_device_destroy will ensure they're
destroyed.
If I explicitly destroy ddraw, how should I handle destroying of ddraw in
d3drm_device_destroy (if it's not destroyed at that point)?




Jam

[Stefan Dösinger]

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

Am 2015-07-30 um 12:07 schrieb Aaryaman Vasishta:
> d3drm_device_create_surfaces_from_clipper sets the clipper and
> primary surface, and init_device sets ddraw and d3drm first before
> anything else, so even if any of them fail d3drm_device_destroy
> will ensure they're destroyed. If I explicitly destroy ddraw, how
> should I handle destroying of ddraw in d3drm_device_destroy (if
> it's not destroyed at that point)?
> The problem with the current way is that what init_device does if it
> fails is an implementation detail that the caller shouldn't know.
> Ideally nothing is ever changed when a function fails.

I see two options:

1) Make sure init_device doesn't set any objects in case of failure.

2) Always AddRef ddraw and render target in init_device and always
(including the success case) release them in the caller.

I prefer the second one because that way you have a clear pairing of
create/addref and release calls in both places.
(d3drm1_CreateDeviceFromClipper create + release, and init_device
Addref + d3drm_device_destroy Release).

I think I mentioned this before, but "init_device" and
"d3drm_device_destroy" are inconsistent names. I recommend to rename
"init_device" to "d3drm_device_init".

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

iQIcBAEBAgAGBQJVugbeAAoJEN0/YqbEcdMwIBkP/R4QwAu7FKYO67M2u+bf27sF
styuLfk1LjmzUg5e9bnWpKaZqdsVash8csXbskwkx1wIsP+Zf8hMG5rDP4pAdpA1
aF+BxjVc8Vzd4uY11E5Qqp8TMvJb2vbJSCHIx9uOL2T18fOxGQWZKzLtdWiYzWaq
r5BsjvJ7bvA08mTfbo6EjBydQrCdhTEWTNQg22AR6GcgNjX4sWwLyPmuRsUPciBS
k3TLnV2x67O56l2XSJHEXKE/dwaleOXUELVEOsUoGrfkNd64Ptd2EhE0sUZzu2us
MdyCohhmJyjjI9olA1DeJdRywa/0emmacHocveeaKILzraSxfZSQD7/ZX3u9UWBd
7ECeD77XBMXefrxA8/1Jxddmq8qi2UUwt3r3UnsozmpbIpb39X0Dsv1MxF2+KF1W
G5B5JfW+qB+jUv/cBT7ah/nah9l/WTSFRbxcKq9YY/dCve759LaDoC1rOQMevHuI
kgohAVhfFuJl0hHDMqe3u2hACp1VgaLcQMTNOe9z9PJ01fetF4bmcYNrOQq6Vvo0
ir7QeE1LohwjxGibfdbv6MqmhnGpAJVa0LeXyn4wEaCWkoQ/6Tv4kX/uoSEv2aZr
beV2+T5LmkgBcwHg6d3PzoZL2yBZeP94FyLsb8kq//P3u5J1VLYE0PLAs3zucvQQ
KSj7C89fLJMPYHielQKE
=CBf1
-----END PGP SIGNATURE-----

[Aaryaman Vasishta]

I've gone for the second option, I agree it makes things easier for the
caller to understand what's happening in terms of refcounting.
I've attached the patches changed from the ones sent previously. The rest
of the patches are the same as in the previous emails. Let me know of any
further changes required.


Thanks!
Jam

On Thu, Jul 30, 2015 at 4:43 PM, Stefan Dösinger <stefandoesinger(a)gmail.com>
wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Am 2015-07-30 um 12:07 schrieb Aaryaman Vasishta:
> > d3drm_device_create_surfaces_from_clipper sets the clipper and
> > primary surface, and init_device sets ddraw and d3drm first before
> > anything else, so even if any of them fail d3drm_device_destroy
> > will ensure they're destroyed. If I explicitly destroy ddraw, how
> > should I handle destroying of ddraw in d3drm_device_destroy (if
> > it's not destroyed at that point)?
> > The problem with the current way is that what init_device does if it
> > fails is an implementation detail that the caller shouldn't know.
> > Ideally nothing is ever changed when a function fails.
>
> I see two options:
>
> 1) Make sure init_device doesn't set any objects in case of failure.
>
> 2) Always AddRef ddraw and render target in init_device and always
> (including the success case) release them in the caller.
>
> I prefer the second one because that way you have a clear pairing of
> create/addref and release calls in both places.
> (d3drm1_CreateDeviceFromClipper create + release, and init_device
> Addref + d3drm_device_destroy Release).
>
> I think I mentioned this before, but "init_device" and
> "d3drm_device_destroy" are inconsistent names. I recommend to rename
> "init_device" to "d3drm_device_init".
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQIcBAEBAgAGBQJVugbeAAoJEN0/YqbEcdMwIBkP/R4QwAu7FKYO67M2u+bf27sF
> styuLfk1LjmzUg5e9bnWpKaZqdsVash8csXbskwkx1wIsP+Zf8hMG5rDP4pAdpA1
> aF+BxjVc8Vzd4uY11E5Qqp8TMvJb2vbJSCHIx9uOL2T18fOxGQWZKzLtdWiYzWaq
> r5BsjvJ7bvA08mTfbo6EjBydQrCdhTEWTNQg22AR6GcgNjX4sWwLyPmuRsUPciBS
> k3TLnV2x67O56l2XSJHEXKE/dwaleOXUELVEOsUoGrfkNd64Ptd2EhE0sUZzu2us
> MdyCohhmJyjjI9olA1DeJdRywa/0emmacHocveeaKILzraSxfZSQD7/ZX3u9UWBd
> 7ECeD77XBMXefrxA8/1Jxddmq8qi2UUwt3r3UnsozmpbIpb39X0Dsv1MxF2+KF1W
> G5B5JfW+qB+jUv/cBT7ah/nah9l/WTSFRbxcKq9YY/dCve759LaDoC1rOQMevHuI
> kgohAVhfFuJl0hHDMqe3u2hACp1VgaLcQMTNOe9z9PJ01fetF4bmcYNrOQq6Vvo0
> ir7QeE1LohwjxGibfdbv6MqmhnGpAJVa0LeXyn4wEaCWkoQ/6Tv4kX/uoSEv2aZr
> beV2+T5LmkgBcwHg6d3PzoZL2yBZeP94FyLsb8kq//P3u5J1VLYE0PLAs3zucvQQ
> KSj7C89fLJMPYHielQKE
> =CBf1
> -----END PGP SIGNATURE-----

[Stefan Dösinger]

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

Hi,

Looks better! There are a few minor issues (mostly not affecting functionality). As far as I can see the refcounting works right now, except for one leak.
> +HRESULT d3drm_device_init(struct d3drm_device *device, UINT version, IDirect3DRM *d3drm, IDirectDraw *ddraw, IDirectDrawSurface *surface,
> +            int width, int height)
> You shouldn't need the width and height parameters here. You can just query it from the render target and thus avoid some code duplication.

You can move the DDSCAPS_3DDEVICE check from d3drm1_CreateDeviceFromSurface into device_init as well, that way you don't need to fetch the surface description twice.
> +    hr = IDirect3DRMDevice3_QueryInterface(device3, &IID_IDirect3DRMDevice2, (void**)device);
> +    IDirect3DRMDevice3_Release(device3);
> +    if (FAILED(hr))
> +        return hr;
>
> return D3DRM_OK;
> You can replace the last 3 lines with "return hr;"

- From patch 3:
> if (FAILED(hr))
>      {
> -        IDirectDrawSurface_DeleteAttachedSurface(surface, 0, ds);
> +        IDirect3D2_Release(d3d2);
>          return hr;
>      }

Shouldn't you release d3d2 even in the success case whenever you have QI'd it from ddraw? You're leaking it otherwise. I'd say the best place to release it is right after the CreateDevice call.

"create_z_surface" is a better name than "z_surface" or "use_z_surface" (in patch 4). Technically patch 4 is a better place to add this parameter than patch 3 because in patch 3 it is always true. But I don't care too much about where you add it because it's needed after the patch series.
> -    todo_wine ok(hr == D3DRM_OK, "Cannot get IDirect3DDevice2 interface (hr = %x).\n", hr);
> +    ok(hr == D3DRM_OK, "Cannot get IDirect3DDevice2 interface (hr = %x).\n", hr);
>      if (FAILED(hr))
>          goto cleanup;
> You can now remove the if (FAILED(hr)).
> -    todo_wine ok(hr == DD_OK, "Cannot get attached depth surface (hr = %x).\n", hr);
> +    ok(hr == DD_OK, "Cannot get attached depth surface (hr = %x).\n", hr);
>      if (SUCCEEDED(hr))
>      {
> Same. There are a few more occurances of that in patch 4.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2

iQIcBAEBAgAGBQJVv0TXAAoJEN0/YqbEcdMw1kgP/Axc4DBFymQ/kU5guofGIBQF
Qoj3pRXHPYqCS4IdrghBqkCpK2dh8GUZN5zTw09EV7VWDJokhSJGq1wE34XylZZO
bFcZqdXZSZj1pjNB01OIUpAOqbcyQdDLVSqVq9V1zCGbkXlwjuK8qRmRVSL1/qX5
SwtLAsizkK7Ncz1n1z/Z/7xAokHLHdio36h225ocbOBkQ5CJsnd7h6m3dhGfAplN
N6suX/l1JdnBmyw2EYqlA88yaJA4BTzZPaYPQHBGa8p4Z3YkM7lK/XS7uPRQFzzU
V2ALMQVx/6pNeLeQSuF/nyNgiml1gO6x5Okw0cTMk9W4xdfY4+AL3DD1sGvvjkcS
vgxXVkS3PgKJUmvzLuR1DjJ6jWqXKrnOYIFE2zO8fq/VcURaUkTZhYrmJUlDvVQ0
QUAalmyfcAr6vz9SgqILfl59URyt2CI3QL6SABqhpxIq+zqS0mQJoWf9k5zfJHk9
3fbetDbVcaBS4Iy7MIgy711iTK18TqktF7tWeyJSuU5/bDrLKDsJ/i1AjLtO11sL
wG7bL74b5Txb0ctxYBQIxZWoLa1YvAnicdzQnv7zjaKi8qbtUzFlBDEJGFAZuyJx
N2Pa1OmDBCAgmyztIsxWjNa7BRMiF9p9lOT3rdrEDATbvIUbH68NVUPgaVmtTXln
VEh3hGOyVbVjvCuPvuvo
=7PE5
-----END PGP SIGNATURE-----

[Aaryaman Vasishta]

I agree with removing the width and height params and handling the checks
from inside device_init itself. I'll put it accordingly.

You can move the DDSCAPS_3DDEVICE check from d3drm1_CreateDeviceFromSurface
> into device_init as well, that way you don't need to fetch the surface
> description twice.

This could still lead to a redundant check for DDSCAPS_3DDEVICE in the case
of CreateDeviceFromClipper, as we're creating the surface internally so we
know that this flag will be set always. Is this overhead acceptable? If yes
then I have no problem in shifting the check there.

[Stefan Dösinger]

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

Am 2015-08-03 um 23:24 schrieb Aaryaman Vasishta:
> This could still lead to a redundant check for DDSCAPS_3DDEVICE in
>  the case of CreateDeviceFromClipper, as we're creating the surface
>  internally so we know that this flag will be set always. Is this 
> overhead acceptable? If yes then I have no problem in shifting the
>  check there.
> Yeah, I think that's fine. Better than duplicating the code in two places.

(And no, I don't think it's worth adding a parameter to skip the check)


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

iQIcBAEBAgAGBQJVv914AAoJEN0/YqbEcdMw/ZcP/iFB/y+65ok1hQwIleuqtrWQ
wDuTNJvibf7heqXCitoiaebb5QSB8HA5zfrnaFu+10XrzYNX7eCy7k2kXMi1GTGQ
a9i8pAfhcOrPbG9OLUL3g7whkj5A8obrwrXvplm5HwrT3aE0Hrg9xpobB7IrVtrZ
pke5/UWfy9G3ZjoLY+X+uSx5ANDT9DzAjnCqyDQq9zRl32ICB1u2pAlXuw3I0jA0
Popf3cRqY7RvKfz2G+I3nE37eKGCWV8M/uNn14DDDYgNwI48LfMTVajDN45KUkbZ
A/OkusMdzQ4uDI+QyxHk6pPq85zFzb6/vo1ucvf//XxS/5bJrofyoKUGuuraH1H5
l/Pyd956XyW9st457eIOW181OoyWUph8KWXSTUEo79z1BdnjP+UtC7P2bIn/V0MN
PBJeAFnSGrio5Iya8dg8XMg7De2yWP2pOyga+yl8KAkth4ynqE2/Mi5/VdYXJPR/
+hIEb6WE81CTrZSrfRsVCpSDNvYswtFncyk6MvqsObfddlKD7OU4c1FluWiZ7j6z
SjPrqOUmK1IfxhJyPnyjgGmjdDv10d7FhFmI7S8MDM1OcTSlSOPGYgx42STAH4ss
cIsIiizRRVAnuOBjGAL5NxQg5Bg07am1QHsju5XcjKOYc/oOF6xaUtoivOSTylxE
0SUk2FjWQMA7H03ScK5U
=tn/g
-----END PGP SIGNATURE-----

[Aaryaman Vasishta]

Okay, here's the updated patch with width and height removed from
device_init and quite a bit of useless NULL checks and redundant if's
removed (as they're no longer needed once the implementations are added)
I'll be following a similar approach while sending over
CreateDeviceFromD3D's implementation.

I've also removed some redundancy in device_init too, hopefully threre
shouldn't be any leaks this time.

On Tue, Aug 4, 2015 at 3:00 AM, Stefan Dösinger <stefandoesinger(a)gmail.com>
wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Am 2015-08-03 um 23:24 schrieb Aaryaman Vasishta:
> > This could still lead to a redundant check for DDSCAPS_3DDEVICE in
> >  the case of CreateDeviceFromClipper, as we're creating the surface
> >  internally so we know that this flag will be set always. Is this
> > overhead acceptable? If yes then I have no problem in shifting the
> >  check there.
> > Yeah, I think that's fine. Better than duplicating the code in two places.
>
> (And no, I don't think it's worth adding a parameter to skip the check)
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQIcBAEBAgAGBQJVv914AAoJEN0/YqbEcdMw/ZcP/iFB/y+65ok1hQwIleuqtrWQ
> wDuTNJvibf7heqXCitoiaebb5QSB8HA5zfrnaFu+10XrzYNX7eCy7k2kXMi1GTGQ
> a9i8pAfhcOrPbG9OLUL3g7whkj5A8obrwrXvplm5HwrT3aE0Hrg9xpobB7IrVtrZ
> pke5/UWfy9G3ZjoLY+X+uSx5ANDT9DzAjnCqyDQq9zRl32ICB1u2pAlXuw3I0jA0
> Popf3cRqY7RvKfz2G+I3nE37eKGCWV8M/uNn14DDDYgNwI48LfMTVajDN45KUkbZ
> A/OkusMdzQ4uDI+QyxHk6pPq85zFzb6/vo1ucvf//XxS/5bJrofyoKUGuuraH1H5
> l/Pyd956XyW9st457eIOW181OoyWUph8KWXSTUEo79z1BdnjP+UtC7P2bIn/V0MN
> PBJeAFnSGrio5Iya8dg8XMg7De2yWP2pOyga+yl8KAkth4ynqE2/Mi5/VdYXJPR/
> +hIEb6WE81CTrZSrfRsVCpSDNvYswtFncyk6MvqsObfddlKD7OU4c1FluWiZ7j6z
> SjPrqOUmK1IfxhJyPnyjgGmjdDv10d7FhFmI7S8MDM1OcTSlSOPGYgx42STAH4ss
> cIsIiizRRVAnuOBjGAL5NxQg5Bg07am1QHsju5XcjKOYc/oOF6xaUtoivOSTylxE
> 0SUk2FjWQMA7H03ScK5U
> =tn/g
> -----END PGP SIGNATURE-----

[Aaryaman Vasishta]

The patch numbers start from 6 because I haven't updated my origin yet.
Patches 0 - 5 have already been pushed upstream.

Cheers,
Jam
On Tue, Aug 4, 2015 at 10:01 PM, Aaryaman Vasishta <
jem456.vasishta(a)gmail.com> wrote:
> Okay, here's the updated patch with width and height removed from
> device_init and quite a bit of useless NULL checks and redundant if's
> removed (as they're no longer needed once the implementations are added)
> I'll be following a similar approach while sending over
> CreateDeviceFromD3D's implementation.
>
> I've also removed some redundancy in device_init too, hopefully threre
> shouldn't be any leaks this time.
>
> On Tue, Aug 4, 2015 at 3:00 AM, Stefan Dösinger <stefandoesinger(a)gmail.com
> > wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > Am 2015-08-03 um 23:24 schrieb Aaryaman Vasishta:
> > > This could still lead to a redundant check for DDSCAPS_3DDEVICE in
> > >  the case of CreateDeviceFromClipper, as we're creating the surface
> > >  internally so we know that this flag will be set always. Is this
> > > overhead acceptable? If yes then I have no problem in shifting the
> > >  check there.
> > > Yeah, I think that's fine. Better than duplicating the code in two places.
> >
> > (And no, I don't think it's worth adding a parameter to skip the check)
> >
> > -----BEGIN PGP SIGNATURE-----
> > Version: GnuPG v2
> >
> > iQIcBAEBAgAGBQJVv914AAoJEN0/YqbEcdMw/ZcP/iFB/y+65ok1hQwIleuqtrWQ
> > wDuTNJvibf7heqXCitoiaebb5QSB8HA5zfrnaFu+10XrzYNX7eCy7k2kXMi1GTGQ
> > a9i8pAfhcOrPbG9OLUL3g7whkj5A8obrwrXvplm5HwrT3aE0Hrg9xpobB7IrVtrZ
> > pke5/UWfy9G3ZjoLY+X+uSx5ANDT9DzAjnCqyDQq9zRl32ICB1u2pAlXuw3I0jA0
> > Popf3cRqY7RvKfz2G+I3nE37eKGCWV8M/uNn14DDDYgNwI48LfMTVajDN45KUkbZ
> > A/OkusMdzQ4uDI+QyxHk6pPq85zFzb6/vo1ucvf//XxS/5bJrofyoKUGuuraH1H5
> > l/Pyd956XyW9st457eIOW181OoyWUph8KWXSTUEo79z1BdnjP+UtC7P2bIn/V0MN
> > PBJeAFnSGrio5Iya8dg8XMg7De2yWP2pOyga+yl8KAkth4ynqE2/Mi5/VdYXJPR/
> > +hIEb6WE81CTrZSrfRsVCpSDNvYswtFncyk6MvqsObfddlKD7OU4c1FluWiZ7j6z
> > SjPrqOUmK1IfxhJyPnyjgGmjdDv10d7FhFmI7S8MDM1OcTSlSOPGYgx42STAH4ss
> > cIsIiizRRVAnuOBjGAL5NxQg5Bg07am1QHsju5XcjKOYc/oOF6xaUtoivOSTylxE
> > 0SUk2FjWQMA7H03ScK5U
> > =tn/g
> > -----END PGP SIGNATURE-----

[Aaryaman Vasishta]

As discussed on IRC, here's the patches rebased on top of the current
upstream's HEAD (as of 8/5/15).


Jam

On Tue, Aug 4, 2015 at 10:02 PM, Aaryaman Vasishta <
jem456.vasishta(a)gmail.com> wrote:
> The patch numbers start from 6 because I haven't updated my origin yet.
> Patches 0 - 5 have already been pushed upstream.
>
> Cheers,
> Jam
>
> On Tue, Aug 4, 2015 at 10:01 PM, Aaryaman Vasishta <
> jem456.vasishta(a)gmail.com> wrote:
> > Okay, here's the updated patch with width and height removed from
> > device_init and quite a bit of useless NULL checks and redundant if's
> > removed (as they're no longer needed once the implementations are added)
> > I'll be following a similar approach while sending over
> > CreateDeviceFromD3D's implementation.
> >
> > I've also removed some redundancy in device_init too, hopefully threre
> > shouldn't be any leaks this time.
> >
> > On Tue, Aug 4, 2015 at 3:00 AM, Stefan Dösinger <
> > stefandoesinger(a)gmail.com> wrote:
> > > -----BEGIN PGP SIGNED MESSAGE-----
> > > Hash: SHA1
> > >
> > > Am 2015-08-03 um 23:24 schrieb Aaryaman Vasishta:
> > > > This could still lead to a redundant check for DDSCAPS_3DDEVICE in
> > > >  the case of CreateDeviceFromClipper, as we're creating the surface
> > > >  internally so we know that this flag will be set always. Is this
> > > > overhead acceptable? If yes then I have no problem in shifting the
> > > >  check there.
> > > > Yeah, I think that's fine. Better than duplicating the code in two
> > > > places.
> > >
> > > (And no, I don't think it's worth adding a parameter to skip the check)
> > >
> > > -----BEGIN PGP SIGNATURE-----
> > > Version: GnuPG v2
> > >
> > > iQIcBAEBAgAGBQJVv914AAoJEN0/YqbEcdMw/ZcP/iFB/y+65ok1hQwIleuqtrWQ
> > > wDuTNJvibf7heqXCitoiaebb5QSB8HA5zfrnaFu+10XrzYNX7eCy7k2kXMi1GTGQ
> > > a9i8pAfhcOrPbG9OLUL3g7whkj5A8obrwrXvplm5HwrT3aE0Hrg9xpobB7IrVtrZ
> > > pke5/UWfy9G3ZjoLY+X+uSx5ANDT9DzAjnCqyDQq9zRl32ICB1u2pAlXuw3I0jA0
> > > Popf3cRqY7RvKfz2G+I3nE37eKGCWV8M/uNn14DDDYgNwI48LfMTVajDN45KUkbZ
> > > A/OkusMdzQ4uDI+QyxHk6pPq85zFzb6/vo1ucvf//XxS/5bJrofyoKUGuuraH1H5
> > > l/Pyd956XyW9st457eIOW181OoyWUph8KWXSTUEo79z1BdnjP+UtC7P2bIn/V0MN
> > > PBJeAFnSGrio5Iya8dg8XMg7De2yWP2pOyga+yl8KAkth4ynqE2/Mi5/VdYXJPR/
> > > +hIEb6WE81CTrZSrfRsVCpSDNvYswtFncyk6MvqsObfddlKD7OU4c1FluWiZ7j6z
> > > SjPrqOUmK1IfxhJyPnyjgGmjdDv10d7FhFmI7S8MDM1OcTSlSOPGYgx42STAH4ss
> > > cIsIiizRRVAnuOBjGAL5NxQg5Bg07am1QHsju5XcjKOYc/oOF6xaUtoivOSTylxE
> > > 0SUk2FjWQMA7H03ScK5U
> > > =tn/g
> > > -----END PGP SIGNATURE-----

[Aaryaman Vasishta]

Just to clarify for others: All the patches in the previous email are yet
to be pushed - I made a mistake in specifying which patches are already
upstream (where I said 0000-0005 are already upstream) two of those patches
are not yet upstream, which I've included in the patchset I just sent.

On Wed, Aug 5, 2015 at 1:19 AM, Aaryaman Vasishta <jem456.vasishta(a)gmail.com
> wrote:
> As discussed on IRC, here's the patches rebased on top of the current
> upstream's HEAD (as of 8/5/15).
>
> Jam
>
> On Tue, Aug 4, 2015 at 10:02 PM, Aaryaman Vasishta <
> jem456.vasishta(a)gmail.com> wrote:
> > The patch numbers start from 6 because I haven't updated my origin yet.
> > Patches 0 - 5 have already been pushed upstream.
> >
> > Cheers,
> > Jam
> >
> > On Tue, Aug 4, 2015 at 10:01 PM, Aaryaman Vasishta <
> > jem456.vasishta(a)gmail.com> wrote:
> > > Okay, here's the updated patch with width and height removed from
> > > device_init and quite a bit of useless NULL checks and redundant if's
> > > removed (as they're no longer needed once the implementations are added)
> > > I'll be following a similar approach while sending over
> > > CreateDeviceFromD3D's implementation.
> > >
> > > I've also removed some redundancy in device_init too, hopefully threre
> > > shouldn't be any leaks this time.
> > >
> > > On Tue, Aug 4, 2015 at 3:00 AM, Stefan Dösinger <
> > > stefandoesinger(a)gmail.com> wrote:
> > > > -----BEGIN PGP SIGNED MESSAGE-----
> > > > Hash: SHA1
> > > >
> > > > Am 2015-08-03 um 23:24 schrieb Aaryaman Vasishta:
> > > > > This could still lead to a redundant check for DDSCAPS_3DDEVICE in
> > > > >  the case of CreateDeviceFromClipper, as we're creating the surface
> > > > >  internally so we know that this flag will be set always. Is this
> > > > > overhead acceptable? If yes then I have no problem in shifting the
> > > > >  check there.
> > > > > Yeah, I think that's fine. Better than duplicating the code in two
> > > > > places.
> > > >
> > > > (And no, I don't think it's worth adding a parameter to skip the check)
> > > >
> > > > -----BEGIN PGP SIGNATURE-----
> > > > Version: GnuPG v2
> > > >
> > > > iQIcBAEBAgAGBQJVv914AAoJEN0/YqbEcdMw/ZcP/iFB/y+65ok1hQwIleuqtrWQ
> > > > wDuTNJvibf7heqXCitoiaebb5QSB8HA5zfrnaFu+10XrzYNX7eCy7k2kXMi1GTGQ
> > > > a9i8pAfhcOrPbG9OLUL3g7whkj5A8obrwrXvplm5HwrT3aE0Hrg9xpobB7IrVtrZ
> > > > pke5/UWfy9G3ZjoLY+X+uSx5ANDT9DzAjnCqyDQq9zRl32ICB1u2pAlXuw3I0jA0
> > > > Popf3cRqY7RvKfz2G+I3nE37eKGCWV8M/uNn14DDDYgNwI48LfMTVajDN45KUkbZ
> > > > A/OkusMdzQ4uDI+QyxHk6pPq85zFzb6/vo1ucvf//XxS/5bJrofyoKUGuuraH1H5
> > > > l/Pyd956XyW9st457eIOW181OoyWUph8KWXSTUEo79z1BdnjP+UtC7P2bIn/V0MN
> > > > PBJeAFnSGrio5Iya8dg8XMg7De2yWP2pOyga+yl8KAkth4ynqE2/Mi5/VdYXJPR/
> > > > +hIEb6WE81CTrZSrfRsVCpSDNvYswtFncyk6MvqsObfddlKD7OU4c1FluWiZ7j6z
> > > > SjPrqOUmK1IfxhJyPnyjgGmjdDv10d7FhFmI7S8MDM1OcTSlSOPGYgx42STAH4ss
> > > > cIsIiizRRVAnuOBjGAL5NxQg5Bg07am1QHsju5XcjKOYc/oOF6xaUtoivOSTylxE
> > > > 0SUk2FjWQMA7H03ScK5U
> > > > =tn/g
> > > > -----END PGP SIGNATURE-----

[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.

[Aaryaman Vasishta]

All the changes have been made, and the patches are being sent to
wine-devel as we speak.

On Wed, Aug 5, 2015 at 4:26 PM, Stefan Dösinger <stefandoesinger(a)gmail.com>
wrote:
> 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.

If I remove this check, wine will return me DDERR_CANNOTATTACHSURFACE in
the tests. That means it refuses to attach a depth surface to the render
target unless the render target itself contains DDSCAPS_3DDEVICE flag. Is
this the correct behavior for ddraw? If yes then I guess d3drm does check
this explicitly, as the tests reveal.


Thanks!
Jam