Re: [PATCH 1/2] d3drm: Store animated frame pointer in animation object

[Henri Verbeet]

On 27 June 2017 at 10:17, Nikolay Sivov  wrote:
> static HRESULT WINAPI d3drm_animation1_SetFrame(IDirect3DRMAnimation *iface, IDirect3DRMFrame *frame)
>  {
> - FIXME("iface %p, frame %p.\n", iface, frame);
> - struct d3drm_animation *animation = impl_from_IDirect3DRMAnimation(iface);
> - HRESULT hr = D3DRM_OK;
> - return E_NOTIMPL;
> - TRACE("iface %p, frame %p.\n", iface, frame);
> -
> - if (frame)
> - {
> -
>    hr = IDirect3DRMFrame_QueryInterface(frame, &IID_IDirect3DRMFrame3, (void **)&animation->frame);
> -
>    if (SUCCEEDED(hr))
> -
>        IDirect3DRMFrame3_Release(animation->frame);
> - }
> - else
> -
>    animation->frame = NULL;
> -
> - return hr;
>
> }

There's nothing necessarily wrong with this, but I feel the following
alternative is worth pointing out:

struct d3drm_frame *f = unsafe_impl_from_IDirect3DRMFrame(frame);
    ...
    animation->frame = f ? &f->IDirect3DRMFrame3_iface : NULL;

return D3DRM_OK;

or even

...
    return d3drm_animation2_SetFrame(&animation->IDirect3DRMAnimation2_iface,
            f ? &f->IDirect3DRMFrame3_iface : NULL);

which is a pattern that's used in ddraw for this kind of thing.

[PATCH 1/2] d3drm: Store animated frame pointer in animation object

[Nikolay Sivov]

On 27.06.2017 15:43, Henri Verbeet wrote:
> On 27 June 2017 at 10:17, Nikolay Sivov  wrote:
> > static HRESULT WINAPI d3drm_animation1_SetFrame(IDirect3DRMAnimation *iface, IDirect3DRMFrame *frame)
> >  {
> > - FIXME("iface %p, frame %p.\n", iface, frame);
> > - struct d3drm_animation *animation = impl_from_IDirect3DRMAnimation(iface);
> > - HRESULT hr = D3DRM_OK;
> > - return E_NOTIMPL;
> > - TRACE("iface %p, frame %p.\n", iface, frame);
> > -
> > - if (frame)
> > - {
> > -
> >    hr = IDirect3DRMFrame_QueryInterface(frame, &IID_IDirect3DRMFrame3, (void **)&animation->frame);
> > -
> >    if (SUCCEEDED(hr))
> > -
> >        IDirect3DRMFrame3_Release(animation->frame);
> > - }
> > - else
> > -
> >    animation->frame = NULL;
> > -
> > - return hr;
> >
> > }
>
> There's nothing necessarily wrong with this, but I feel the following
> alternative is worth pointing out:
> struct d3drm_frame *f = unsafe_impl_from_IDirect3DRMFrame(frame);
> ...
> animation->frame = f ? &f->IDirect3DRMFrame3_iface : NULL;
>
> return D3DRM_OK;
>
> or even
> ...
> return d3drm_animation2_SetFrame(&animation->IDirect3DRMAnimation2_iface,
>         f ? &f->IDirect3DRMFrame3_iface : NULL);
>
> which is a pattern that's used in ddraw for this kind of thing.

That makes sense, I usually avoid unsafe_* stuff when public methods are
enough. I'll resend as is, to send updated patch 2/2, but will probably
patch it later in a way you suggested.