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

[Aaryaman Vasishta]

On Thu, Jul 28, 2016 at 12:13 AM, Stefan Dösinger <stefandoesinger at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20160728/e0611340/attachment.html>