Re: [PATCH 5/7] d3drm: Fix device not assigning width and height after creation.

[Henri Verbeet]

On 24 June 2016 at 23:49, Aaryaman Vasishta  wrote:
> HRESULT d3drm_device_set_ddraw_device_d3d(struct d3drm_device *device, IDirect3DRM *d3drm, IDirect3D *d3d, IDirect3DDevice *d3d_device)
>  {
> -
>
> IDirectDrawSurface *surface;
> -
>
> IDirect3DDevice2 *d3d_device2;
> -
>
> DDSURFACEDESC desc;
> -
>
> HRESULT hr;
> -
>
> device->d3drm = d3drm;
>    IDirect3DRM_AddRef(d3drm);
>    device->device = d3d_device;
>    IDirect3DDevice_AddRef(d3d_device);
> -
>
> /* Fetch render target and get width/height from there */
> -
>
> if (FAILED(hr = IDirect3DDevice_QueryInterface(d3d_device, &IID_IDirectDrawSurface, (void **)&surface)))
> -
>
> {
> -
>    if (FAILED(hr = IDirect3DDevice_QueryInterface(d3d_device, &IID_IDirect3DDevice2, (void **)&d3d_device2)))
> -
>        return hr;
> -
>    hr = IDirect3DDevice2_GetRenderTarget(d3d_device2, &surface);
> -
>    IDirect3DDevice2_Release(d3d_device2);
> -
>    if (FAILED(hr))
> -
>        return hr;
> -
>
> }
> -
> -
>
> desc.dwSize = sizeof(desc);
> -
>
> if (FAILED(hr = IDirectDrawSurface_GetSurfaceDesc(surface, &desc)))
> -
>
> {
> -
>    IDirectDrawSurface_Release(surface);
> -
>    return hr;
> -
>
> }
> -
> -
>
> device->width = desc.dwWidth;
> -
>
> device->height = desc.dwHeight;
> -
>
> IDirectDrawSurface_Release(surface);
> -
>
> return IDirect3D_QueryInterface(d3d, &IID_IDirectDraw, (void **)&device->ddraw);
>
> }

This leaks some references (IDirect3DRM, IDirect3DDevice) on error
paths, are those on purpose?

[PATCH 5/7] d3drm: Fix device not assigning width and height after creation.

[Aaryaman Vasishta]

On Wed, Jun 29, 2016 at 7:21 PM, Henri Verbeet  wrote:
> On 24 June 2016 at 23:49, Aaryaman Vasishta 
> wrote:
> > HRESULT d3drm_device_set_ddraw_device_d3d(struct d3drm_device *device,
>
> IDirect3DRM *d3drm, IDirect3D *d3d, IDirect3DDevice *d3d_device)
> > {
> > -
> >
> > IDirectDrawSurface *surface;
> > -
> >
> > IDirect3DDevice2 *d3d_device2;
> > -
> >
> > DDSURFACEDESC desc;
> > -
> >
> > HRESULT hr;
> > -
> >
> > device->d3drm = d3drm;
> >    IDirect3DRM_AddRef(d3drm);
> >    device->device = d3d_device;
> >    IDirect3DDevice_AddRef(d3d_device);
> > -
> >
> > /* Fetch render target and get width/height from there */
> > -
> >
> > if (FAILED(hr = IDirect3DDevice_QueryInterface(d3d_device,
>
> &IID_IDirectDrawSurface, (void **)&surface)))
> > - {
> > -
> >    if (FAILED(hr = IDirect3DDevice_QueryInterface(d3d_device,
>
> &IID_IDirect3DDevice2, (void **)&d3d_device2)))
> > -
> >        return hr;
> > -
> >    hr = IDirect3DDevice2_GetRenderTarget(d3d_device2, &surface);
> > -
> >    IDirect3DDevice2_Release(d3d_device2);
> > -
> >    if (FAILED(hr))
> > -
> >        return hr;
> > - }
> > -
> > - desc.dwSize = sizeof(desc);
> > - if (FAILED(hr = IDirectDrawSurface_GetSurfaceDesc(surface, &desc)))
> > - {
> > -
> >    IDirectDrawSurface_Release(surface);
> > -
> >    return hr;
> > - }
> > -
> > - device->width = desc.dwWidth;
> > - device->height = desc.dwHeight;
> > - IDirectDrawSurface_Release(surface);
> > - return IDirect3D_QueryInterface(d3d, &IID_IDirectDraw, (void
>
> **)&device->ddraw);
> > }
>
> This leaks some references (IDirect3DRM, IDirect3DDevice) on error
> paths, are those on purpose?
>
> This is a helper function, so the caller would be handling the references.

See CreateDeviceFromD3D in device.c.

Cheers,
Aaryaman

[PATCH 5/7] d3drm: Fix device not assigning width and height after creation.

[Henri Verbeet]

On 29 June 2016 at 16:13, Aaryaman Vasishta  wrote:
> This is a helper function, so the caller would be handling the references.
> See CreateDeviceFromD3D in device.c.

That construction makes it harder to verify correctness, so in general
I'd recommend against that. Regardless, d3drm_device_destroy() takes
care of the device reference, but doesn't help for the IDirect3DRM
reference, since that's only released when device->ddraw is set.

[PATCH 5/7] d3drm: Fix device not assigning width and height after creation.

[Aaryaman Vasishta]

On Wed, Jun 29, 2016 at 8:01 PM, Henri Verbeet  wrote:
> On 29 June 2016 at 16:13, Aaryaman Vasishta 
> wrote:
> > This is a helper function, so the caller would be handling the
>
> references.
> > See CreateDeviceFromD3D in device.c.
>
> That construction makes it harder to verify correctness, so in general
> I'd recommend against that. Regardless, d3drm_device_destroy() takes
> care of the device reference, but doesn't help for the IDirect3DRM
> reference, since that's only released when device->ddraw is set.

device->ddraw is set while returning. What should be the better approach
here?

Cheers,
Aaryaman

[PATCH 5/7] d3drm: Fix device not assigning width and height after creation.

[Henri Verbeet]

On 29 June 2016 at 17:54, Aaryaman Vasishta  wrote:
> On Wed, Jun 29, 2016 at 8:01 PM, Henri Verbeet  wrote:
> > On 29 June 2016 at 16:13, Aaryaman Vasishta 
> > wrote:
> > > This is a helper function, so the caller would be handling the
> > > references.
> > > See CreateDeviceFromD3D in device.c.
> >
> > That construction makes it harder to verify correctness, so in general
> > I'd recommend against that. Regardless, d3drm_device_destroy() takes
> > care of the device reference, but doesn't help for the IDirect3DRM
> > reference, since that's only released when device->ddraw is set.
>
> device->ddraw is set while returning. What should be the better approach
> here?

You can probably fix the immediate issue just by storing the return
value and releasing the reference if the QI failed.

The larger issue is that code becomes much easier to follow when you
can assume that functions are supposed to leave things (more or less)
the way they found them on failure. (Unless explicitly indicated.)
I.e., when d3drm_device_set_ddraw_device_d3d() releases all the
references it created when it fails, and d3drm1_CreateDeviceFromD3D()
can assume the device is in the same state as after calling
d3drm_device_create().

Of course the even larger issue is that
d3drm_device_set_ddraw_device_d3d() doesn't make a lot of sense. In a
way similar to e.g. d3drm3_CreateTexture(),
d3drm1_CreateDeviceFromD3D() should probably just call
IDirect3DRMDevice::InitFromD3D().

[PATCH 5/7] d3drm: Fix device not assigning width and height after creation.

[Aaryaman Vasishta]

On Thu, Jun 30, 2016 at 3:41 PM, Henri Verbeet  wrote:
> You can probably fix the immediate issue just by storing the return
> value and releasing the reference if the QI failed.
>
> The larger issue is that code becomes much easier to follow when you
> can assume that functions are supposed to leave things (more or less)
> the way they found them on failure. (Unless explicitly indicated.)
> I.e., when d3drm_device_set_ddraw_device_d3d() releases all the
> references it created when it fails, and d3drm1_CreateDeviceFromD3D()
> can assume the device is in the same state as after calling
> d3drm_device_create().

So I could e.g. release the reference, to ddraw, d3drm and d3d_device and
set them all to NULL, effectively putting them in the same state as it it
weas before calling d3drm_device_set_ddraw_device_d3d(). Is this fine?
> Of course the even larger issue is that
> d3drm_device_set_ddraw_device_d3d() doesn't make a lot of sense. In a
> way similar to e.g. d3drm3_CreateTexture(),
> d3drm1_CreateDeviceFromD3D() should probably just call
> IDirect3DRMDevice::InitFromD3D().

Agreed. Back then it wasn't clear how CreateObject and Init worked
together, though after the texture and viewport implementations, it became
increasingly apparent that Init works only via CreateObject. A series of
patches to move the implementations to Init could be done later on.
Thanks for the review!

Cheers,
Aaryaman