On Thu, Jul 28, 2016 at 12:13 AM, Stefan Dösinger <stefandoesinger@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