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.

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

[Aaryaman Vasishta]

On Thu, Jul 28, 2016 at 12:13 AM, Stefan Dösinger <[e-mail]
> 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

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

[Henri Verbeet]

On 27 July 2016 at 21:09, Aaryaman Vasishta  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.

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

[Aaryaman Vasishta]

On Thu, Jul 28, 2016 at 12:53 AM, Henri Verbeet  wrote:
> On 27 July 2016 at 21:09, Aaryaman Vasishta 
> 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