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