<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jul 10, 2016 at 9:10 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">-----BEGIN PGP SIGNED MESSAGE-----<br>
Hash: SHA256<br>
<br>
Hi,<br>
<br>
Patches 1-4 look good to me, once the fixes you mentioned and we talked about on IRC are implemented.<br>
<span><br>
Am 2016-07-07 um 14:52 schrieb Aaryaman Vasishta:<br>
> struct d3drm_frame<br>
> {<br>
> + struct d3drm_object obj;<br>
</span>This seems unrelated.<br></blockquote><div>Could be leftover from a rebase. Will remove it. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
> + if (device->ddraw)<br>
> + hr = D3DRMERR_BADOBJECT;<br>
</span>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?<br>
<br>
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.<br></blockquote><div>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.<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
> device->width = desc.dwWidth;<br>
> device->height = desc.dwHeight;<br>
</span>If I read the diff right this code will set width and height to 0 on an already initialized device.<br></blockquote><div>Right, this should probably be only set in the case where the device is not already initialized.<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
> static HRESULT WINAPI d3drm_device3_GetDirect3DDevice2(IDirect3DRMDevice3 *iface, IDirect3DDevice2 **d3d_device)<br>
> {<br>
> struct d3drm_device *device = impl_from_IDirect3DRMDevice3(iface);<br>
> + HRESULT hr;<br>
><br>
> TRACE("iface %p, d3d_device %p.\n", iface, d3d_device);<br>
><br>
> - IDirect3DDevice_QueryInterface(device->device, &IID_IDirect3DDevice2, (void**)d3d_device);<br>
> + if (FAILED(hr = IDirect3DDevice_QueryInterface(device->device, &IID_IDirect3DDevice2, (void**)d3d_device)))<br>
> + hr = D3DRMERR_BADOBJECT;<br>
><br>
> - return D3DRM_OK;<br>
> + return hr;<br>
> }<br>
</span>I think this change (like the device2_GetDirect3DDevice2 one) can be moved to a separate patch.<br></blockquote><div>Right, this was noticed by me and I've split these changes into a separate patch (as discussed on IRC).<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
> + hr = IDirect3DRMDevice_InitFromD3D(device1, d3d1, d3ddevice1);<br>
> + ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr);<br>
> + ref3 = get_refcount((IUnknown *)d3ddevice1);<br>
> + ok(ref3 > device_ref2, "Expected ref3 > device_ref2, got ref3 = %u, device_ref2 = %u.\n", ref3, device_ref2);<br>
> + ref3 = get_refcount((IUnknown *)d3d1);<br>
> + ok(ref3 > d3d_ref2, "Expected ref3 > d3d_ref2, got ref3 = %u, d3d_ref2 = %u.\n", ref3, d3d_ref2);<br>
</span>You forgot to test the reference of d3drm1.<br>
<span><br>
> + hr = IDirect3DRMDevice_QueryInterface(device1, &IID_IDirect3DRMDevice2, (void **)&device2);<br>
> + ok(SUCCEEDED(hr), "Cannot get IDirect3DRMDevice2 Interface (hr = %x).\n", hr);<br>
> + hr = IDirect3DRMDevice2_GetDirect3DDevice2(device2, &d3ddevice2);<br>
> + ok(SUCCEEDED(hr), "Expected hr == D3DRM_OK, got %#x.\n", hr);<br>
> + ok(d3ddevice2 == NULL, "Expected d3ddevice2 == NULL, got %p.\n", d3ddevice2);<br>
> + IDirect3DRMDevice2_Release(device2);<br>
</span>You forgot d3ddevice2 = 0xdeadbeef here.<br>
<span><br>
> + hr = IDirect3DRM2_CreateObject(d3drm2, &CLSID_CDirect3DRMDevice, NULL, &IID_IDirect3DRMDevice2, (void **)&device2);<br>
> + ok(SUCCEEDED(hr), "Cannot get IDirect3DRMDevice2 interface (hr = %#x).\n", hr);<br>
> +<br>
> + hr = IDirectDraw_QueryInterface(ddraw1, &IID_IDirect3D, (void **)&d3d1);<br>
> + ok(SUCCEEDED(hr), "Cannot get IDirect3D interface (hr = %x).\n", hr);<br>
> + if (SUCCEEDED(hr = IDirect3DDevice2_QueryInterface(d3ddevice2, &IID_IDirect3DDevice, (void **)&d3ddevice1)))<br>
> + {<br>
> + hr = IDirect3DRMDevice2_InitFromD3D(device2, d3d1, d3ddevice1);<br>
> + ok(hr == E_NOINTERFACE, "Expected hr == E_NOINTERFACE, got %#x.\n", hr);<br>
</span>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. <br></blockquote><div>True, it makes sense. These tests should be added in here as well. Will do the same for patch 6 too.<br></div><div>I will do the rest of the changes and resend the patch, along with the fixes to similar problems to patch 6 as well.<br></div><div>Thanks for the review! :)<br></div><div><br></div><div>Cheers,<br></div><div>Aaryaman<br></div></div></div></div>