Re: [PATCH 5/7] d3drm: Fix device not assigning width and height after creation.
On 24 June 2016 at 23:49, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
HRESULT d3drm_device_set_ddraw_device_d3d(struct d3drm_device *device, IDirect3DRM *d3drm, IDirect3D *d3d, IDirect3DDevice *d3d_device) { + IDirectDrawSurface *surface; + IDirect3DDevice2 *d3d_device2; + DDSURFACEDESC desc; + HRESULT hr; + device->d3drm = d3drm; IDirect3DRM_AddRef(d3drm); device->device = d3d_device; IDirect3DDevice_AddRef(d3d_device);
+ /* Fetch render target and get width/height from there */ + if (FAILED(hr = IDirect3DDevice_QueryInterface(d3d_device, &IID_IDirectDrawSurface, (void **)&surface))) + { + if (FAILED(hr = IDirect3DDevice_QueryInterface(d3d_device, &IID_IDirect3DDevice2, (void **)&d3d_device2))) + return hr; + hr = IDirect3DDevice2_GetRenderTarget(d3d_device2, &surface); + IDirect3DDevice2_Release(d3d_device2); + if (FAILED(hr)) + return hr; + } + + desc.dwSize = sizeof(desc); + if (FAILED(hr = IDirectDrawSurface_GetSurfaceDesc(surface, &desc))) + { + IDirectDrawSurface_Release(surface); + return hr; + } + + device->width = desc.dwWidth; + device->height = desc.dwHeight; + IDirectDrawSurface_Release(surface); + return IDirect3D_QueryInterface(d3d, &IID_IDirectDraw, (void **)&device->ddraw); }
This leaks some references (IDirect3DRM, IDirect3DDevice) on error paths, are those on purpose?
On Wed, Jun 29, 2016 at 7:21 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 24 June 2016 at 23:49, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
HRESULT d3drm_device_set_ddraw_device_d3d(struct d3drm_device *device, IDirect3DRM *d3drm, IDirect3D *d3d, IDirect3DDevice *d3d_device) { + IDirectDrawSurface *surface; + IDirect3DDevice2 *d3d_device2; + DDSURFACEDESC desc; + HRESULT hr; + device->d3drm = d3drm; IDirect3DRM_AddRef(d3drm); device->device = d3d_device; IDirect3DDevice_AddRef(d3d_device);
+ /* Fetch render target and get width/height from there */ + if (FAILED(hr = IDirect3DDevice_QueryInterface(d3d_device, &IID_IDirectDrawSurface, (void **)&surface))) + { + if (FAILED(hr = IDirect3DDevice_QueryInterface(d3d_device, &IID_IDirect3DDevice2, (void **)&d3d_device2))) + return hr; + hr = IDirect3DDevice2_GetRenderTarget(d3d_device2, &surface); + IDirect3DDevice2_Release(d3d_device2); + if (FAILED(hr)) + return hr; + } + + desc.dwSize = sizeof(desc); + if (FAILED(hr = IDirectDrawSurface_GetSurfaceDesc(surface, &desc))) + { + IDirectDrawSurface_Release(surface); + return hr; + } + + device->width = desc.dwWidth; + device->height = desc.dwHeight; + IDirectDrawSurface_Release(surface); + return IDirect3D_QueryInterface(d3d, &IID_IDirectDraw, (void **)&device->ddraw); }
This leaks some references (IDirect3DRM, IDirect3DDevice) on error paths, are those on purpose?
This is a helper function, so the caller would be handling the references. See CreateDeviceFromD3D in device.c.
Cheers, Aaryaman
On 29 June 2016 at 16:13, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
This is a helper function, so the caller would be handling the references. See CreateDeviceFromD3D in device.c.
That construction makes it harder to verify correctness, so in general I'd recommend against that. Regardless, d3drm_device_destroy() takes care of the device reference, but doesn't help for the IDirect3DRM reference, since that's only released when device->ddraw is set.
On Wed, Jun 29, 2016 at 8:01 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 29 June 2016 at 16:13, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
This is a helper function, so the caller would be handling the references. See CreateDeviceFromD3D in device.c.
That construction makes it harder to verify correctness, so in general I'd recommend against that. Regardless, d3drm_device_destroy() takes care of the device reference, but doesn't help for the IDirect3DRM reference, since that's only released when device->ddraw is set.
device->ddraw is set while returning. What should be the better approach here? Cheers, Aaryaman
On 29 June 2016 at 17:54, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
On Wed, Jun 29, 2016 at 8:01 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 29 June 2016 at 16:13, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
This is a helper function, so the caller would be handling the references. See CreateDeviceFromD3D in device.c.
That construction makes it harder to verify correctness, so in general I'd recommend against that. Regardless, d3drm_device_destroy() takes care of the device reference, but doesn't help for the IDirect3DRM reference, since that's only released when device->ddraw is set.
device->ddraw is set while returning. What should be the better approach here?
You can probably fix the immediate issue just by storing the return value and releasing the reference if the QI failed. The larger issue is that code becomes much easier to follow when you can assume that functions are supposed to leave things (more or less) the way they found them on failure. (Unless explicitly indicated.) I.e., when d3drm_device_set_ddraw_device_d3d() releases all the references it created when it fails, and d3drm1_CreateDeviceFromD3D() can assume the device is in the same state as after calling d3drm_device_create(). Of course the even larger issue is that d3drm_device_set_ddraw_device_d3d() doesn't make a lot of sense. In a way similar to e.g. d3drm3_CreateTexture(), d3drm1_CreateDeviceFromD3D() should probably just call IDirect3DRMDevice::InitFromD3D().
On Thu, Jun 30, 2016 at 3:41 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
You can probably fix the immediate issue just by storing the return value and releasing the reference if the QI failed.
The larger issue is that code becomes much easier to follow when you can assume that functions are supposed to leave things (more or less) the way they found them on failure. (Unless explicitly indicated.) I.e., when d3drm_device_set_ddraw_device_d3d() releases all the references it created when it fails, and d3drm1_CreateDeviceFromD3D() can assume the device is in the same state as after calling d3drm_device_create().
So I could e.g. release the reference, to ddraw, d3drm and d3d_device and set them all to NULL, effectively putting them in the same state as it it weas before calling d3drm_device_set_ddraw_device_d3d(). Is this fine?
Of course the even larger issue is that d3drm_device_set_ddraw_device_d3d() doesn't make a lot of sense. In a way similar to e.g. d3drm3_CreateTexture(), d3drm1_CreateDeviceFromD3D() should probably just call IDirect3DRMDevice::InitFromD3D().
Agreed. Back then it wasn't clear how CreateObject and Init worked together, though after the texture and viewport implementations, it became increasingly apparent that Init works only via CreateObject. A series of patches to move the implementations to Init could be done later on. Thanks for the review! Cheers, Aaryaman
participants (2)
-
Aaryaman Vasishta -
Henri Verbeet