Re: [PATCH 1/2] d3drm: Implement IDirect3DRMFrame*::GetScene.

[Stefan Dösinger]

On 2016-08-06 16:29, Aaryaman Vasishta wrote:
> - while (!done)
> - {
> -
>    if (FAILED(hr = IDirect3DRMFrame3_GetParent(frame, &parent_frame)))
> -
>        return hr;
> -
>    if (parent_frame)
> -
>    {
> -
>        frame = parent_frame;
> -
>        IDirect3DRMFrame3_Release(parent_frame);
> -
>    }
> -
>    else
> -
>        done = TRUE;
> - }

What happens if GetScene is called on the root frame? Does it return itself or NULL or an error?

You can eliminate the done variable from this loop by replacing it with a break:

for (;;)
{
    if (FAILED(hr = IDirect3DRMFrame3_GetParent(frame, &parent_frame)))
        return hr;
    if (parent_frame)
    {
        frame = parent_frame;
        IDirect3DRMFrame3_Release(parent_frame);
    }
    else
        break;
}

I originally thought about a do {} while(parent_frame); kind of construct but that doesn't gain you much because you need the if check anyway to release parent_frame.

It's not nice to call GetParent in the next iteration after you release the frame, but so far I haven't come up with an implementation that avoids that and isn't terribly ugly. I'll keep thinking.

[PATCH 1/2] d3drm: Implement IDirect3DRMFrame*::GetScene.

[Aaryaman Vasishta]

Hi,

On Sun, Aug 7, 2016 at 3:42 AM, Stefan Dösinger 
wrote:
> On 2016-08-06 16:29, Aaryaman Vasishta wrote:
> > - while (!done)
> > - {
> > -
> >    if (FAILED(hr = IDirect3DRMFrame3_GetParent(frame,
>
> &parent_frame)))
> > -
> >        return hr;
> > -
> >    if (parent_frame)
> > -
> >    {
> > -
> >        frame = parent_frame;
> > -
> >        IDirect3DRMFrame3_Release(parent_frame);
> > -
> >    }
> > -
> >    else
> > -
> >        done = TRUE;
> > - }
>
> What happens if GetScene is called on the root frame? Does it return
> itself or NULL or an error?

It returns itself. I'll add the test in the resend.
> You can eliminate the done variable from this loop by replacing it with a
> break:
>
> for (;;)
> {
>     if (FAILED(hr = IDirect3DRMFrame3_GetParent(frame, &parent_frame)))
>         return hr;
>     if (parent_frame)
>     {
>         frame = parent_frame;
>         IDirect3DRMFrame3_Release(parent_frame);
>     }
>     else
>         break;
> }
>
> I originally thought about a do {} while(parent_frame); kind of construct
> but that doesn't gain you much because you need the if check anyway to
> release parent_frame.
>
> It's not nice to call GetParent in the next iteration after you release
> the frame, but so far I haven't come up with an implementation that avoids
> that and isn't terribly ugly. I'll keep thinking.

Makes sense. You're right about the way I'm releasing parent_frame being a
bit odd when you look at it. But going in terms of refcounts (and the fact
that all frame versions share the same refcount) there shouldn't be an
issue for now.

Thanks for the review :)

Cheers,
Aaryaman