-----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
On Sun, Jul 10, 2016 at 9:10 PM, Stefan Dösinger stefandoesinger@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@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.
-
Aaryaman Vasishta -
Stefan Dösinger