[PATCH 05/12] d3drm: Implement IDirect3DRMDevice*::InitFromD3D.

[Aaryaman Vasishta]

On Sun, Jul 10, 2016 at 9:10 PM, Stefan Dösinger <stefandoesinger at gmail.com>
wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> Hi,
>
> Patches 1-4 look good to me, once the fixes you mentioned and we talked
> about on IRC are implemented.
>
> Am 2016-07-07 um 14:52 schrieb Aaryaman Vasishta:
> >  struct d3drm_frame
> >  {
> > +    struct d3drm_object obj;
> This seems unrelated.
>
Could be leftover from a rebase. Will remove it.

>
> > +    if (device->ddraw)
> > +        hr = D3DRMERR_BADOBJECT;
> I guess you're not returning here right away because you want to addref
> d3drm later. Wouldn't it be better to move those AddRefs in front in this
> case?
>
> The QueryInterface / GetRenderTarget code below is unlikely to fail, if it
> does something is severely wrong (A d3d device without a render target???)
> and getting the reference leak right is the last of your worries.
>
This also brings me to another problem. What if init is called using a
different d3d/device? I don't think it should re-set the new d3d/device on
an existing object. I guess the double initialization path should be
handled separately.  I should probably add tests for this and change
implementations accordingly.

>
> >      device->width = desc.dwWidth;
> >      device->height = desc.dwHeight;
> If I read the diff right this code will set width and height to 0 on an
> already initialized device.
>
Right, this should probably be only set in the case where the device is not
already initialized.

>
> >  static HRESULT WINAPI
> d3drm_device3_GetDirect3DDevice2(IDirect3DRMDevice3 *iface,
> IDirect3DDevice2 **d3d_device)
> >  {
> >      struct d3drm_device *device = impl_from_IDirect3DRMDevice3(iface);
> > +    HRESULT hr;
> >
> >      TRACE("iface %p, d3d_device %p.\n", iface, d3d_device);
> >
> > -    IDirect3DDevice_QueryInterface(device->device,
> &IID_IDirect3DDevice2, (void**)d3d_device);
> > +    if (FAILED(hr = IDirect3DDevice_QueryInterface(device->device,
> &IID_IDirect3DDevice2, (void**)d3d_device)))
> > +        hr = D3DRMERR_BADOBJECT;
> >
> > -    return D3DRM_OK;
> > +    return hr;
> >  }
> I think this change (like the device2_GetDirect3DDevice2 one) can be moved
> to a separate patch.
>
Right, this was noticed by me and I've split these changes into a separate
patch (as discussed on IRC).

>
> > +    hr = IDirect3DRMDevice_InitFromD3D(device1, d3d1, d3ddevice1);
> > +    ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT,
> got %#x.\n", hr);
> > +    ref3 = get_refcount((IUnknown *)d3ddevice1);
> > +    ok(ref3 > device_ref2, "Expected ref3 > device_ref2, got ref3 = %u,
> device_ref2 = %u.\n", ref3, device_ref2);
> > +    ref3 = get_refcount((IUnknown *)d3d1);
> > +    ok(ref3 > d3d_ref2, "Expected ref3 > d3d_ref2, got ref3 = %u,
> d3d_ref2 = %u.\n", ref3, d3d_ref2);
> You forgot to test the reference of d3drm1.
>
> > +    hr = IDirect3DRMDevice_QueryInterface(device1,
> &IID_IDirect3DRMDevice2, (void **)&device2);
> > +    ok(SUCCEEDED(hr), "Cannot get IDirect3DRMDevice2 Interface (hr =
> %x).\n", hr);
> > +    hr = IDirect3DRMDevice2_GetDirect3DDevice2(device2, &d3ddevice2);
> > +    ok(SUCCEEDED(hr), "Expected hr == D3DRM_OK, got %#x.\n", hr);
> > +    ok(d3ddevice2 == NULL, "Expected d3ddevice2 == NULL, got %p.\n",
> d3ddevice2);
> > +    IDirect3DRMDevice2_Release(device2);
> You forgot d3ddevice2 = 0xdeadbeef here.
>
> > +    hr = IDirect3DRM2_CreateObject(d3drm2, &CLSID_CDirect3DRMDevice,
> NULL, &IID_IDirect3DRMDevice2, (void **)&device2);
> > +    ok(SUCCEEDED(hr), "Cannot get IDirect3DRMDevice2 interface (hr =
> %#x).\n", hr);
> > +
> > +    hr = IDirectDraw_QueryInterface(ddraw1, &IID_IDirect3D, (void
> **)&d3d1);
> > +    ok(SUCCEEDED(hr), "Cannot get IDirect3D interface (hr = %x).\n",
> hr);
> > +    if (SUCCEEDED(hr = IDirect3DDevice2_QueryInterface(d3ddevice2,
> &IID_IDirect3DDevice, (void **)&d3ddevice1)))
> > +    {
> > +        hr = IDirect3DRMDevice2_InitFromD3D(device2, d3d1, d3ddevice1);
> > +        ok(hr == E_NOINTERFACE, "Expected hr == E_NOINTERFACE, got
> %#x.\n", hr);
> What happens if you QI IDirect3DRMDevice1 from IDirect3DRMDevice2 and call
> InitFromD3D on it? I would expect it to succeed because common decency
> suggests that CreateObject doesn't leave a history of which interface was
> requested and there's no CLSID_CDirect3DRMDevice2 or
> CLSID_CDirect3DRMDevice3. Still worth a check though I think.
>
True, it makes sense. These tests should be added in here as well. Will do
the same for patch 6 too.
I will do the rest of the changes and resend the patch, along with the
fixes to similar problems to patch 6 as well.
Thanks for the review! :)

Cheers,
Aaryaman
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20160710/5a33028e/attachment-0001.html>