Re: [PATCH 1/3] d3drm: Add stubs for IDirect3DRMFrame interface. (v2)
-----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-----
Hi, On Sun, Jun 12, 2016 at 8:17 PM, Stefan Dösinger <stefandoesinger(a)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-----
On Sun, Jun 12, 2016 at 8:45 PM, Aaryaman Vasishta < jem456.vasishta(a)gmail.com> wrote
+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.
I think the reasoning behind that approach could be that all versions share the same refcount, so it really doesn't matter which version is released, as long as the recount is maintained. Also the refcount tests for AddChild are present in test_Frame, so the leak is intentional.
Cheers, Aaryaman
I'll be adding a separate field in struct qi_test called vtable_iid, and using this, comparing whether or not the vtables are equal within test_qi. I'll be sending a patch which will make all current qi tests consistent with the new table layout. Cheers, Aaryaman
participants (2)
-
Aaryaman Vasishta -
Stefan Dösinger