[PATCH 1/3] d3drm: Add stubs for IDirect3DRMFrame interface. (v2)

[Aaryaman Vasishta]

Hi,

On Sun, Jun 12, 2016 at 8:17 PM, Stefan Dösinger <stefandoesinger at gmail.com>
wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> Hi,
>
> On the big picture this code is a copypaste of the frame2 -> frame3
> thunks. There's nothing wrong with that on its own, but unfortunately the
> frame2 -> frame3 thunks have a number of issues. In particular some thunks
> are not trivial, e.g. AddVisual, which accepts a IDirect3DRMVisual in one
> version and an IUnknown in another. Or GetChildren - which interface is
> added in to the Children array?
>
> I think we had that discussion with other interfaces in the past -
> implement thunks for everything or implement thunks for the implemented
> frame3 functions only and make the others return an error. Judging from the
> existing code we decided on the latter option for IDirect3DRM* . I would
> suggest to make all frame1 functions return E_NOTIMPL in the first patch
> (except IUnknown), then implement the thunks that have sufficient existing
> tests (adding tests for version 1 in the same patches) and then add the
> rest of the thunks as you implement the version 3 function. That way you
> won't be blocked by discussions about thunks that aren't relevant to you
> yet.
>
> Right, makes sense to make them all return E_NOTIMPL and implement the
ones with proper tests. This patch is mostly a copy-paste and refactor,
since most of the common functions are the same.

> Am 2016-06-09 um 23:18 schrieb Aaryaman Vasishta:
> > +static HRESULT WINAPI d3drm_frame1_AddChild(IDirect3DRMFrame *iface,
> IDirect3DRMFrame *child)
> > +{
> > +    struct d3drm_frame *frame = impl_from_IDirect3DRMFrame(iface);
> > +    IDirect3DRMFrame3 *child3;
> > +    HRESULT hr;
> > +
> > +    TRACE("iface %p, child %p.\n", iface, child);
> > +
> > +    if (!child)
> > +        return D3DRMERR_BADOBJECT;
> > +    hr = IDirect3DRMFrame_QueryInterface(child, &IID_IDirect3DRMFrame3,
> (void **)&child3);
> > +    if (hr != S_OK)
> > +        return D3DRMERR_BADOBJECT;
> > +    IDirect3DRMFrame_Release(child);
> > +
> > +    return IDirect3DRMFrame3_AddChild(&frame->IDirect3DRMFrame3_iface,
> child3);
> > +}
> This doesn't seem right. Right now you're releasing a reference you don't
> own (child's frame1) and leak one (child's frame3). Is there a reason why
> you're using QueryInterface instead of accessing
> child->IDirect3DRMFrame3_iface?
>

> If you stick to QI I think this should be more like this (error checking
> excluded):
> QI(child, IID_child3, &child3);
> hr = Frame3_AddChild(frame, child3);
> IDirect3DRMFrame3_Release(child3);
>
> d3drm_frame2_AddChild has the same problem, this is where your code seems
> to come from.
>
I didn't write this (copied from version 3), though your approach does make
sense. I could fix this in a separate patch if needed.

>
> > +    hr = IDirect3DRM_CreateFrame(d3drm1, NULL, &frame1);
> > +    ok(SUCCEEDED(hr), "Cannot get IDirect3DRMFrame interface (hr =
> %#x).\n", hr);
> > +
> > +    hr = IDirect3DRMFrame_QueryInterface(frame1,
> &IID_IDirect3DRMObject, (void **)&obj);
> > +    ok(SUCCEEDED(hr), "Could not get IDirect3DRMObject interface (hr =
> %#x).\n", hr);
> > +    ok(obj == (IDirect3DRMObject *)frame1, "got object pointer %p,
> expected %p", obj, frame1);
> > +    IDirect3DRMObject_Release(obj);
> > +    IDirect3DRMFrame_Release(frame1);
> What's missing here is a test that frame1 != frame2 != frame3 and frame1
> == visual and frame1 == IUnknown (I guess that one will be false). You
> could also consider making this and patch use the same kind of table as
> e.g. the IDirect3DRM tests. That would keep the tests for various
> interfaces similar to each other. On the other hand because there's only
> one refcount (thanks to aggregation support) I am fine with a smaller test
> for refcounts.
>
Agreed. I did write the vtable comparison tests last year (
https://www.winehq.org/pipermail/wine-patches/2015-April/138466.html). I'll
rewrite this one and send it over with the stubs.

Thanks for the review!

Cheers,
Aaryaman

>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQIcBAEBCAAGBQJXXXYCAAoJEN0/YqbEcdMwFdcP/3D9Ldet3j2QS14fCOjsuamK
> bVzdTzr+WuYZlYAp6MUyiZUFCSy+tb8f8C1LO6wuxmuaJmgNzFXB4j+s7fUzH/PQ
> bljIsEOc0aDYRDrMLekxa86MaK25xfHFOsj7ixM72NZsCaRVsUpq9y993LNVRAEN
> cUazAn3SQWSQVXD97mZa1/4MjGExfKR6Sw932f5WOeUGoZRSAFUxZ1hlOKS7HJbp
> qIweKka1D8lz+DgJGpsVkSJqDUw4+uHhKlBPPcu5RDjgzzTefuk3k53I4ntasrY6
> 611gv+xkmo32cfewPcte0b+ttA5MW/DXyTrXWAhffrZ+kFpB327HusbdD3Wa+O2S
> yOvbPBPTbWwTr1wGaIvD6QB2MSEsBQLLVnB73gMO8Ref0yY+Q4ujV/2IITyFjRzB
> lJz3821uSB+RFyfBKgS7KN81KBs35a7OsfQi7EwyuUQtwXY0NuEF/s48D7dHMJdO
> fLeRv928WqYY+hmXkj4gQZD8mmEzjEGyT1PqAum/EJcr9tOzEXWbhwlKrgL9f0k9
> g/u2WQh+OtwvAcueIWZaRgxNZZjsi6xi15Dnjwi7OSW5s19PCg5/g1qgyCA4nVCb
> jb9AHJX6HxhnGGGej5NTUw+uCW83y3ons9DV2s9lRprIFt58v5mb1tELO7TBFAGa
> VqA7RDllo5Zh3jOnYKyB
> =ll6/
> -----END PGP SIGNATURE-----
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20160612/cfb9758b/attachment-0001.html>