<div dir="ltr"><div><div>Hi,<br></div></div><div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jun 12, 2016 at 8:17 PM, Stefan Dösinger <span dir="ltr"><<a href="mailto:stefandoesinger@gmail.com" target="_blank">stefandoesinger@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">-----BEGIN PGP SIGNED MESSAGE-----<br>
Hash: SHA256<br>
<br>
Hi,<br>
<br>
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?<br>
<br>
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.<br>
<span class=""><br></span></blockquote><div>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. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">
Am 2016-06-09 um 23:18 schrieb Aaryaman Vasishta:<br>
> +static HRESULT WINAPI d3drm_frame1_AddChild(IDirect3DRMFrame *iface, IDirect3DRMFrame *child)<br>
> +{<br>
> + struct d3drm_frame *frame = impl_from_IDirect3DRMFrame(iface);<br>
> + IDirect3DRMFrame3 *child3;<br>
> + HRESULT hr;<br>
> +<br>
> + TRACE("iface %p, child %p.\n", iface, child);<br>
> +<br>
> + if (!child)<br>
> + return D3DRMERR_BADOBJECT;<br>
> + hr = IDirect3DRMFrame_QueryInterface(child, &IID_IDirect3DRMFrame3, (void **)&child3);<br>
> + if (hr != S_OK)<br>
> + return D3DRMERR_BADOBJECT;<br>
> + IDirect3DRMFrame_Release(child);<br>
> +<br>
> + return IDirect3DRMFrame3_AddChild(&frame->IDirect3DRMFrame3_iface, child3);<br>
> +}<br>
</span>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? <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
If you stick to QI I think this should be more like this (error checking excluded):<br>
QI(child, IID_child3, &child3);<br>
hr = Frame3_AddChild(frame, child3);<br>
IDirect3DRMFrame3_Release(child3);<br>
<br>
d3drm_frame2_AddChild has the same problem, this is where your code seems to come from.<br></blockquote><div>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. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><br>
> + hr = IDirect3DRM_CreateFrame(d3drm1, NULL, &frame1);<br>
> + ok(SUCCEEDED(hr), "Cannot get IDirect3DRMFrame interface (hr = %#x).\n", hr);<br>
> +<br>
> + hr = IDirect3DRMFrame_QueryInterface(frame1, &IID_IDirect3DRMObject, (void **)&obj);<br>
> + ok(SUCCEEDED(hr), "Could not get IDirect3DRMObject interface (hr = %#x).\n", hr);<br>
> + ok(obj == (IDirect3DRMObject *)frame1, "got object pointer %p, expected %p", obj, frame1);<br>
> + IDirect3DRMObject_Release(obj);<br>
> + IDirect3DRMFrame_Release(frame1);<br>
</span>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.<br></blockquote><div>Agreed. I did write the vtable comparison tests last year (<a href="https://www.winehq.org/pipermail/wine-patches/2015-April/138466.html">https://www.winehq.org/pipermail/wine-patches/2015-April/138466.html</a>). I'll rewrite this one and send it over with the stubs.<br><br></div><div>Thanks for the review!<br></div><div><br></div><div>Cheers,<br></div><div>Aaryaman <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-----BEGIN PGP SIGNATURE-----<br>
Version: GnuPG v2<br>
<br>
iQIcBAEBCAAGBQJXXXYCAAoJEN0/YqbEcdMwFdcP/3D9Ldet3j2QS14fCOjsuamK<br>
bVzdTzr+WuYZlYAp6MUyiZUFCSy+tb8f8C1LO6wuxmuaJmgNzFXB4j+s7fUzH/PQ<br>
bljIsEOc0aDYRDrMLekxa86MaK25xfHFOsj7ixM72NZsCaRVsUpq9y993LNVRAEN<br>
cUazAn3SQWSQVXD97mZa1/4MjGExfKR6Sw932f5WOeUGoZRSAFUxZ1hlOKS7HJbp<br>
qIweKka1D8lz+DgJGpsVkSJqDUw4+uHhKlBPPcu5RDjgzzTefuk3k53I4ntasrY6<br>
611gv+xkmo32cfewPcte0b+ttA5MW/DXyTrXWAhffrZ+kFpB327HusbdD3Wa+O2S<br>
yOvbPBPTbWwTr1wGaIvD6QB2MSEsBQLLVnB73gMO8Ref0yY+Q4ujV/2IITyFjRzB<br>
lJz3821uSB+RFyfBKgS7KN81KBs35a7OsfQi7EwyuUQtwXY0NuEF/s48D7dHMJdO<br>
fLeRv928WqYY+hmXkj4gQZD8mmEzjEGyT1PqAum/EJcr9tOzEXWbhwlKrgL9f0k9<br>
g/u2WQh+OtwvAcueIWZaRgxNZZjsi6xi15Dnjwi7OSW5s19PCg5/g1qgyCA4nVCb<br>
jb9AHJX6HxhnGGGej5NTUw+uCW83y3ons9DV2s9lRprIFt58v5mb1tELO7TBFAGa<br>
VqA7RDllo5Zh3jOnYKyB<br>
=ll6/<br>
-----END PGP SIGNATURE-----<br>
</blockquote></div><br></div></div></div></div></div>