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

[Stefan Dösinger]

-----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.

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.

> +    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.

-----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-----