Re: [PATCH 2/3] d3drm: Implement IDirect3DRMFrame*::GetScene. (v2)

[Henri Verbeet]

On 9 August 2016 at 03:16, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
> static HRESULT WINAPI d3drm_frame3_GetScene(IDirect3DRMFrame3 *iface, IDirect3DRMFrame3 **scene)
>  {
> -    FIXME("iface %p, scene %p stub!\n", iface, scene);
> +    HRESULT hr;
> +    IDirect3DRMFrame3 *frame = iface, *parent_frame = NULL;
>
> -    return E_NOTIMPL;
> +    TRACE("iface %p, scene %p.\n", iface, scene);
> +
> +    if (!scene)
> +        return D3DRMERR_BADVALUE;
> +
> +    for (;;)
> +    {
> +        if (FAILED(hr = IDirect3DRMFrame3_GetParent(frame, &parent_frame)))
> +            return hr;
> +        if (parent_frame)
> +        {
> +            frame = parent_frame;
> +            IDirect3DRMFrame3_Release(parent_frame);
> +        }
> +        else
> +            break;
> +    }
> You could just do

if (!parent_frame)
        break;

More importantly though, as Stefan already mentioned, the reference
counting is weird. I think the code would be much nicer if you just
used impl_from_IDirect3DRMFrame3() and then followed the frame->parent
pointers.

[PATCH 2/3] d3drm: Implement IDirect3DRMFrame*::GetScene. (v2)

[Aaryaman Vasishta]

On Tue, Aug 9, 2016 at 9:23 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
> On 9 August 2016 at 03:16, Aaryaman Vasishta <jem456.vasishta(a)gmail.com>
> wrote:
> > static HRESULT WINAPI d3drm_frame3_GetScene(IDirect3DRMFrame3 *iface,
> > IDirect3DRMFrame3 **scene)
> >  {
> > -    FIXME("iface %p, scene %p stub!\n", iface, scene);
> > +    HRESULT hr;
> > +    IDirect3DRMFrame3 *frame = iface, *parent_frame = NULL;
> >
> > -    return E_NOTIMPL;
> > +    TRACE("iface %p, scene %p.\n", iface, scene);
> > +
> > +    if (!scene)
> > +        return D3DRMERR_BADVALUE;
> > +
> > +    for (;;)
> > +    {
> > +        if (FAILED(hr = IDirect3DRMFrame3_GetParent(frame,
> > &parent_frame)))
> > +            return hr;
> > +        if (parent_frame)
> > +        {
> > +            frame = parent_frame;
> > +            IDirect3DRMFrame3_Release(parent_frame);
> > +        }
> > +        else
> > +            break;
> > +    }
> > You could just do
>
> if (!parent_frame)
>         break;
>
> More importantly though, as Stefan already mentioned, the reference
> counting is weird. I think the code would be much nicer if you just
> used impl_from_IDirect3DRMFrame3() and then followed the frame->parent
> pointers.

Right, since we have internal access to the struct, why not take advantage
of it? Using the approach you described is just like traversing through a
linked list, simple and easier to read.

Cheers,
Aaryaman