Re: [PATCH 1/6] d3drm: Implement IDirect3DRMViewport*::Init. (v7)

[Stefan Dösinger]

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.