Re: [PATCH 05/12] d3drm: Implement IDirect3DRMDevice*::InitFromD3D.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Hi, Patches 1-4 look good to me, once the fixes you mentioned and we talked about on IRC are implemented. Am 2016-07-07 um 14:52 schrieb Aaryaman Vasishta:
struct d3drm_frame { + struct d3drm_object obj; This seems unrelated.
+ if (device->ddraw) + hr = D3DRMERR_BADOBJECT; I guess you're not returning here right away because you want to addref d3drm later. Wouldn't it be better to move those AddRefs in front in this case?
The QueryInterface / GetRenderTarget code below is unlikely to fail, if it does something is severely wrong (A d3d device without a render target???) and getting the reference leak right is the last of your worries.
device->width = desc.dwWidth; device->height = desc.dwHeight;
If I read the diff right this code will set width and height to 0 on an already initialized device.
static HRESULT WINAPI d3drm_device3_GetDirect3DDevice2(IDirect3DRMDevice3 *iface, IDirect3DDevice2 **d3d_device) { struct d3drm_device *device = impl_from_IDirect3DRMDevice3(iface); + HRESULT hr;
TRACE("iface %p, d3d_device %p.\n", iface, d3d_device);
- IDirect3DDevice_QueryInterface(device->device, &IID_IDirect3DDevice2, (void**)d3d_device); + if (FAILED(hr = IDirect3DDevice_QueryInterface(device->device, &IID_IDirect3DDevice2, (void**)d3d_device))) + hr = D3DRMERR_BADOBJECT;
- return D3DRM_OK; + return hr; } I think this change (like the device2_GetDirect3DDevice2 one) can be moved to a separate patch.
+ hr = IDirect3DRMDevice_InitFromD3D(device1, d3d1, d3ddevice1); + ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); + ref3 = get_refcount((IUnknown *)d3ddevice1); + ok(ref3 > device_ref2, "Expected ref3 > device_ref2, got ref3 = %u, device_ref2 = %u.\n", ref3, device_ref2); + ref3 = get_refcount((IUnknown *)d3d1); + ok(ref3 > d3d_ref2, "Expected ref3 > d3d_ref2, got ref3 = %u, d3d_ref2 = %u.\n", ref3, d3d_ref2); You forgot to test the reference of d3drm1.
+ hr = IDirect3DRMDevice_QueryInterface(device1, &IID_IDirect3DRMDevice2, (void **)&device2); + ok(SUCCEEDED(hr), "Cannot get IDirect3DRMDevice2 Interface (hr = %x).\n", hr); + hr = IDirect3DRMDevice2_GetDirect3DDevice2(device2, &d3ddevice2); + ok(SUCCEEDED(hr), "Expected hr == D3DRM_OK, got %#x.\n", hr); + ok(d3ddevice2 == NULL, "Expected d3ddevice2 == NULL, got %p.\n", d3ddevice2); + IDirect3DRMDevice2_Release(device2); You forgot d3ddevice2 = 0xdeadbeef here.
+ hr = IDirect3DRM2_CreateObject(d3drm2, &CLSID_CDirect3DRMDevice, NULL, &IID_IDirect3DRMDevice2, (void **)&device2); + ok(SUCCEEDED(hr), "Cannot get IDirect3DRMDevice2 interface (hr = %#x).\n", hr); + + hr = IDirectDraw_QueryInterface(ddraw1, &IID_IDirect3D, (void **)&d3d1); + ok(SUCCEEDED(hr), "Cannot get IDirect3D interface (hr = %x).\n", hr); + if (SUCCEEDED(hr = IDirect3DDevice2_QueryInterface(d3ddevice2, &IID_IDirect3DDevice, (void **)&d3ddevice1))) + { + hr = IDirect3DRMDevice2_InitFromD3D(device2, d3d1, d3ddevice1); + ok(hr == E_NOINTERFACE, "Expected hr == E_NOINTERFACE, got %#x.\n", hr); What happens if you QI IDirect3DRMDevice1 from IDirect3DRMDevice2 and call InitFromD3D on it? I would expect it to succeed because common decency suggests that CreateObject doesn't leave a history of which interface was requested and there's no CLSID_CDirect3DRMDevice2 or CLSID_CDirect3DRMDevice3. Still worth a check though I think.
Cheers, Stefan -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXgmxrAAoJEN0/YqbEcdMw9dQP/RbovFFjwIdo5ho2BcxWi1Sm 68U7n+sX+HAPmHPc2W/fOK1scbPaQz84e3cHj+dsFClCUKNLUpZ1eGdJDu1u9Fgh ZJVC9hgw8rzC+IDGr5vdmuzzOj65mI7KslJX9CfD9tnXigLtlvPf3me8jnhjL51b Eq8N1rStZTh805gDBEnmr/oqgQFkRoHKgs5lLIWq2inAfF3dq0BiThBGpC+eq19H YcihqOJWgSXrbXQYtTPjFtjp3jqqFa32+/ZQufCEAELcYUjM2gG23+O/rT5sa4d2 QuQOIhdUnTR9G6WnoZXgxKwxtJTvTu02GKlk5gaIkpWFJ2O+mPJuiBmylLoucQ6V BkYkhORuT2WYKainbTx3qNJaxBPSa5/y4HWTpQhzYH0LryFBLkL8MiVjO+pGk7QM 4wHTeL4ZXmjtyYOs6qfuxNIEJyfDs0hU5850lWdVD6MLaj3JwZYRLedCJ9TwV7Jb Qz3JQIK6YPOgTSqehIGTwzxgcbvjjiLP2zalEV4HOIqKeEkodcBxVS0QiXSDbuAj gLhbqwlRMshT5Dtt1Umi2KUoGljFjh1rbe42cyVxXcoa72D+g9LQUb4d0hUP/Ppt 6lNhfq+Vf2W+FmdB2Hv/WWof7vBV59r/TwjjExrN00yRGCLNLF3BaK8lzjeJprGO H6J9OAsscEyzMNpB+l0W =exYP -----END PGP SIGNATURE-----
On Sun, Jul 10, 2016 at 9:10 PM, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Hi,
Patches 1-4 look good to me, once the fixes you mentioned and we talked about on IRC are implemented.
Am 2016-07-07 um 14:52 schrieb Aaryaman Vasishta:
struct d3drm_frame { + struct d3drm_object obj; This seems unrelated.
Could be leftover from a rebase. Will remove it.
+ if (device->ddraw) + hr = D3DRMERR_BADOBJECT; I guess you're not returning here right away because you want to addref d3drm later. Wouldn't it be better to move those AddRefs in front in this case?
The QueryInterface / GetRenderTarget code below is unlikely to fail, if it does something is severely wrong (A d3d device without a render target???) and getting the reference leak right is the last of your worries.
This also brings me to another problem. What if init is called using a different d3d/device? I don't think it should re-set the new d3d/device on an existing object. I guess the double initialization path should be handled separately. I should probably add tests for this and change implementations accordingly.
device->width = desc.dwWidth; device->height = desc.dwHeight;
If I read the diff right this code will set width and height to 0 on an already initialized device.
Right, this should probably be only set in the case where the device is not already initialized.
static HRESULT WINAPI d3drm_device3_GetDirect3DDevice2(IDirect3DRMDevice3 *iface, IDirect3DDevice2 **d3d_device) { struct d3drm_device *device = impl_from_IDirect3DRMDevice3(iface); + HRESULT hr;
TRACE("iface %p, d3d_device %p.\n", iface, d3d_device);
- IDirect3DDevice_QueryInterface(device->device, &IID_IDirect3DDevice2, (void**)d3d_device); + if (FAILED(hr = IDirect3DDevice_QueryInterface(device->device, &IID_IDirect3DDevice2, (void**)d3d_device))) + hr = D3DRMERR_BADOBJECT;
- return D3DRM_OK; + return hr; } I think this change (like the device2_GetDirect3DDevice2 one) can be moved to a separate patch.
Right, this was noticed by me and I've split these changes into a separate patch (as discussed on IRC).
+ hr = IDirect3DRMDevice_InitFromD3D(device1, d3d1, d3ddevice1); + ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); + ref3 = get_refcount((IUnknown *)d3ddevice1); + ok(ref3 > device_ref2, "Expected ref3 > device_ref2, got ref3 = %u, device_ref2 = %u.\n", ref3, device_ref2); + ref3 = get_refcount((IUnknown *)d3d1); + ok(ref3 > d3d_ref2, "Expected ref3 > d3d_ref2, got ref3 = %u, d3d_ref2 = %u.\n", ref3, d3d_ref2); You forgot to test the reference of d3drm1.
+ hr = IDirect3DRMDevice_QueryInterface(device1, &IID_IDirect3DRMDevice2, (void **)&device2); + ok(SUCCEEDED(hr), "Cannot get IDirect3DRMDevice2 Interface (hr = %x).\n", hr); + hr = IDirect3DRMDevice2_GetDirect3DDevice2(device2, &d3ddevice2); + ok(SUCCEEDED(hr), "Expected hr == D3DRM_OK, got %#x.\n", hr); + ok(d3ddevice2 == NULL, "Expected d3ddevice2 == NULL, got %p.\n", d3ddevice2); + IDirect3DRMDevice2_Release(device2); You forgot d3ddevice2 = 0xdeadbeef here.
+ hr = IDirect3DRM2_CreateObject(d3drm2, &CLSID_CDirect3DRMDevice, NULL, &IID_IDirect3DRMDevice2, (void **)&device2); + ok(SUCCEEDED(hr), "Cannot get IDirect3DRMDevice2 interface (hr = %#x).\n", hr); + + hr = IDirectDraw_QueryInterface(ddraw1, &IID_IDirect3D, (void **)&d3d1); + ok(SUCCEEDED(hr), "Cannot get IDirect3D interface (hr = %x).\n", hr); + if (SUCCEEDED(hr = IDirect3DDevice2_QueryInterface(d3ddevice2, &IID_IDirect3DDevice, (void **)&d3ddevice1))) + { + hr = IDirect3DRMDevice2_InitFromD3D(device2, d3d1, d3ddevice1); + ok(hr == E_NOINTERFACE, "Expected hr == E_NOINTERFACE, got %#x.\n", hr); What happens if you QI IDirect3DRMDevice1 from IDirect3DRMDevice2 and call InitFromD3D on it? I would expect it to succeed because common decency suggests that CreateObject doesn't leave a history of which interface was requested and there's no CLSID_CDirect3DRMDevice2 or CLSID_CDirect3DRMDevice3. Still worth a check though I think.
True, it makes sense. These tests should be added in here as well. Will do the same for patch 6 too. I will do the rest of the changes and resend the patch, along with the fixes to similar problems to patch 6 as well. Thanks for the review! :) Cheers, Aaryaman
On Sun, Jul 10, 2016 at 9:10 PM, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
+ hr = IDirect3DRMDevice2_InitFromD3D(device2, d3d1, d3ddevice1); + ok(hr == E_NOINTERFACE, "Expected hr == E_NOINTERFACE, got %#x.\n", hr); What happens if you QI IDirect3DRMDevice1 from IDirect3DRMDevice2 and call InitFromD3D on it? I would expect it to succeed because common decency suggests that CreateObject doesn't leave a history of which interface was requested and there's no CLSID_CDirect3DRMDevice2 or CLSID_CDirect3DRMDevice3. Still worth a check though I think.
It still returns E_NOINTERFACE. This means that CreateObject does keep track of which interface was requested within the object after all. We could probably mimic this by using a version field within the struct, which can be looked up and InitFromD3D would be handled accordingly, similar to the version field in struct d3d_device. This could potentially be used in other functions with similar behavior on different versions as well. Cheers, Aaryaman
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Am 2016-07-14 um 00:33 schrieb Aaryaman Vasishta:
It still returns E_NOINTERFACE. This means that CreateObject does keep track of which interface was requested within the object after all. We could probably mimic this by using a version field within the struct, which can be looked up and InitFromD3D would be handled accordingly, similar to the version field in struct d3d_device. This could potentially be used in other functions with similar behavior on different versions as well. Don't worry too much about the InitFromD3D? corner case. Keep it in mind for debugging, but I doubt there's an application that tries to do this and depends on this thing to fail.
But yeah, you may need some sort of version tagging for future things at some point. Ddraw has something similar. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXh96HAAoJEN0/YqbEcdMwAicP/0sYEw43mK4ghm5H7uea/QHw 9Uv1PY2WGRPWoPPe4YXFK12ZD2E3gzSuTLPGhJruezqhOWB8Bxp0WPFX5q8IYTaf X30PjDOeRBvPh5ptHdRddXPDxGo89GhAFd+dz2lt3t5z5FdVWKyY7opjQhWS6nJF nNMbRC1di9zCuCyo/UGG/ILXxMPD9VAnH7Fahy4xWcRZFLAxJH8thPaiGbin7Nwi InWVqiUjG44BYff8roYXylu+rUIVBYdp8FHw5gowdNWAyDkMbGhHUtctlTqgPCM6 Irc+UOqmsxel2ccGlKHk1zDFjGbsNYkrLORCLeAtjaAhL+IaLgbrq+eu84DX7OQA XTO/PS0RVUwL6o+rsB6AG6Wsqq+dAHEDq0adSru4IqDrWlWBEAy6F9mYt1Qms8np Qa9J+1jo6fc7Eq35vcs0uW/Ymd+EH7qs4KHljJRsxMM/OzTOOmYIrJeBOmEo+oNq eGOGxNDa++gihpcNV7JzsfGj9Ar+vmfwyIRg8REGTisHzylIhlw22nRhmzr5k2rK mqI2M6KXlpG1lAQOSqpPKhdLLdPUwcWiRu7d+RDNadV3ypBlGgqjS6YY1mCUdynR yjK9Fabvo3RQV7bRC42rygIdYDvO8Bpj4cctejfD0Y0iuCw5VlJFbRXerjKNS3c6 Skm0afo5Ovb6y0rWLH+M =tnTR -----END PGP SIGNATURE-----
participants (2)
-
Aaryaman Vasishta -
Stefan Dösinger