Re: [PATCH 2/6] d3drm: Implement IDirect3DRM*::CreateViewport. (v2 resend)

[Stefan Dösinger]

On 2016-07-19 19:08, Aaryaman Vasishta wrote:
> - if (FAILED(hr = IDirect3DRMDevice_QueryInterface(device, &IID_IDirect3DRMDevice3, (void **)&device3)))
> -
>    return hr;
> -
> - if (FAILED(hr = IDirect3DRMFrame_QueryInterface(camera, &IID_IDirect3DRMFrame3, (void **)&camera3)))
> - {
> -
>    IDirect3DRMDevice3_Release(device3);
> -
>    return hr;
> - }
> - IDirect3DRMDevice_Release(device);
> - IDirect3DRMFrame_Release(camera);

This releases references you don't own. Same in the V2 -> V3 thunk.
> - if (!device || !camera)
> -
>    return D3DRMERR_BADOBJECT;
> - if (!viewport)
> -
>    return D3DRMERR_BADVALUE;

These checks are redundant, Init() does that as well.
> - ok(ref4 == frame_ref, "Expected ref4 == frame_ref, got frame_ref = %u, ref4 = %u.\n", frame_ref, ref4);
> -
> - device_ref = get_refcount((IUnknown *)device1);
> - frame_ref = get_refcount((IUnknown *)frame);

The get_refcount here seems redundant because you've just shown that it is back to where it used to be.

[PATCH 2/6] d3drm: Implement IDirect3DRM*::CreateViewport. (v2 resend)

[Aaryaman Vasishta]

On Thu, Jul 28, 2016 at 12:21 AM, Stefan Dösinger <[e-mail]
> wrote:
> > - if (!device || !camera)
> > -
> >    return D3DRMERR_BADOBJECT;
> > - if (!viewport)
> > -
> >    return D3DRMERR_BADVALUE;
>
> These checks are redundant, Init() does that as well.

Agreed for version 3 of CreateViewport, but this this apply for version 1/2
as well? Since I'm dereferencing these interfaces there I thought the
checks were justified.
> > - ok(ref4 == frame_ref, "Expected ref4 == frame_ref, got frame_ref =
>
> %u, ref4 = %u.\n", frame_ref, ref4);
> > -
> > - device_ref = get_refcount((IUnknown *)device1);
> > - frame_ref = get_refcount((IUnknown *)frame);
>
> The get_refcount here seems redundant because you've just shown that it is
> back to where it used to be.

Right, these are definitely redundant.

Thanks for the review!

Cheers,
Aaryaman

[PATCH 2/6] d3drm: Implement IDirect3DRM*::CreateViewport. (v2 resend)

[Stefan Dösinger]

On 2016-07-27 20:27, Aaryaman Vasishta wrote:
> On Thu, Jul 28, 2016 at 12:21 AM, Stefan Dösinger
> <[e-mail] > wrote:
> > +    if (!device || !camera)
> > +        return D3DRMERR_BADOBJECT;
> > +    if (!viewport)
> > +        return D3DRMERR_BADVALUE;
> These checks are redundant, Init() does that as well.
>
> Agreed for version 3 of CreateViewport, but this this apply for version
> 1/2 as well? Since I'm dereferencing these interfaces there I thought
> the checks were justified.

No, in 1 and 2 you need them as you say. Otherwise you'll crash when you
try to call QI.