Re: [PATCH 1/6] d3drm: Implement IDirect3DRMViewport*::Init. (v7)
On 2016-07-19 19:08, Aaryaman Vasishta wrote:
+ memset(&mat, 0, sizeof(mat)); + mat.dwSize = sizeof(mat); + mat.diffuse.r = RGBA_GETRED(color) / 255.0f; + mat.diffuse.g = RGBA_GETGREEN(color) / 255.0f; + mat.diffuse.b = RGBA_GETBLUE(color) / 255.0f; + mat.diffuse.a = RGBA_GETALPHA(color) / 255.0f; Doing this here seems questionable. What happens if the camera's background is changed after the viewport is created? You'll probably want to update the material property in viewport::clear(), or maybe in camera::SetSceneBackground.
+ if (material) + IDirect3DMaterial_Release(material); This will destroy the material, viewport::SetBackground doesn't addref it.
+ hr = IDirect3DRMViewport_Init(viewport, device1, frame, rc.left, rc.top, rc.right + 1, rc.bottom + 1); + ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); + hr = IDirect3DRMViewport_Init(viewport, device1, frame, rc.left, rc.top, rc.right + 1, rc.bottom); + ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); + hr = IDirect3DRMViewport_Init(viewport, device1, frame, rc.left, rc.top, rc.right + 1, rc.bottom + 1); + ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); The first and third test are identical.
+ IDirect3DRMDevice3_Release(device3); + IDirect3DRMDevice_Release(device1); + ref4 = get_refcount((IUnknown *)d3drm1); + todo_wine ok(ref4 > initial_ref1, "Expected ref4 > initial_ref1, got initial_ref1 = %u, ref4 = %u.\n", initial_ref1, ref4); The ref4 > inital_ref1 check isn't all too surprising because frame3 still holds a reference to its d3drm parent. Though it seems the leaked reference added by the second Viewport::Init call gets released somewhere. The test doesn't exactly show where. It is possible that releasing the device frees it, or releasing the frame does it.
On Thu, Jul 28, 2016 at 12:13 AM, Stefan Dösinger <stefandoesinger(a)gmail.com
wrote:
On 2016-07-19 19:08, Aaryaman Vasishta wrote:
+ memset(&mat, 0, sizeof(mat)); + mat.dwSize = sizeof(mat); + mat.diffuse.r = RGBA_GETRED(color) / 255.0f; + mat.diffuse.g = RGBA_GETGREEN(color) / 255.0f; + mat.diffuse.b = RGBA_GETBLUE(color) / 255.0f; + mat.diffuse.a = RGBA_GETALPHA(color) / 255.0f; Doing this here seems questionable. What happens if the camera's background is changed after the viewport is created? You'll probably want to update the material property in viewport::clear(), or maybe in camera::SetSceneBackground.
Right, that'll be added in the implementation of Clear. The camera frame itself doesn't contain a reference to the viewport so I doubt SetSceneBackground has any effect on it. (See the tests I wrote for Clear).
+ if (material) + IDirect3DMaterial_Release(material); This will destroy the material, viewport::SetBackground doesn't addref it.
Right, henri suggested the same. I'll move this outside the cleanup label.
+ IDirect3DRMDevice3_Release(device3); + IDirect3DRMDevice_Release(device1); + ref4 = get_refcount((IUnknown *)d3drm1); + todo_wine ok(ref4 > initial_ref1, "Expected ref4 > initial_ref1, got initial_ref1 = %u, ref4 = %u.\n", initial_ref1, ref4); The ref4 > inital_ref1 check isn't all too surprising because frame3 still holds a reference to its d3drm parent. Though it seems the leaked reference added by the second Viewport::Init call gets released somewhere. The test doesn't exactly show where. It is possible that releasing the device frees it, or releasing the frame does it.
The leaked references created by the second Init call are all released when the device is destroyed. This suggests that the device is using some sort of destroy callback and keeping track of all leaked references. See the ref4 == initial_ref1 test below it. I'll resend the patch with the fixes you mentioned. Thanks for the review! Cheers, Aaryaman
On 27 July 2016 at 21:09, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
On Thu, Jul 28, 2016 at 12:13 AM, Stefan Dösinger
+ if (material) + IDirect3DMaterial_Release(material); This will destroy the material, viewport::SetBackground doesn't addref it.
Right, henri suggested the same. I'll move this outside the cleanup label. I did say the sequence was odd, but it does work because "material" is set to NULL on successful viewport initialisation. On the other hand, I just noticed you'll also leak the material if IDirect3DViewport_SetBackground() fails.
On Thu, Jul 28, 2016 at 12:53 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 27 July 2016 at 21:09, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
On Thu, Jul 28, 2016 at 12:13 AM, Stefan Dösinger
+ if (material) + IDirect3DMaterial_Release(material); This will destroy the material, viewport::SetBackground doesn't addref it.
Right, henri suggested the same. I'll move this outside the cleanup label. I did say the sequence was odd, but it does work because "material" is set to NULL on successful viewport initialisation. On the other hand, I just noticed you'll also leak the material if IDirect3DViewport_SetBackground() fails.
Right, sorry to not mention about the sequence. I also noticed the leak while fixing the same. Cheers, Aaryaman
participants (3)
-
Aaryaman Vasishta -
Henri Verbeet -
Stefan Dösinger