Re: [PATCH 3/6] ddraw: Keep a ddraw object reference in d3d7 vertex buffers.

[Henri Verbeet]

On 7 February 2017 at 22:08, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
> +static void test_vb_refcount(void)
> +{
> +    ULONG prev_d3d_refcount, prev_device_refcount;
> +    ULONG cur_d3d_refcount, cur_device_refcount;
> +    D3DVERTEXBUFFERDESC vb_desc;
> +    IDirect3DVertexBuffer *vb;
> +    IDirect3DDevice3 *device;
> +    IDirect3D3 *d3d;
> +    HWND window;
> +    HRESULT hr;
> +
> +    window = CreateWindowA("static", "d3d3_test", WS_OVERLAPPEDWINDOW,
> +            0, 0, 640, 480, 0, 0, 0, 0);
> +    if (!(device = create_device(window, DDSCL_NORMAL)))
> +    {
> +        skip("Failed to create a 3D device, skipping test.\n");
> +        DestroyWindow(window);
> +        return;
> +    }
> +
> +    hr = IDirect3DDevice3_GetDirect3D(device, &d3d);
> +    ok(SUCCEEDED(hr), "Failed to get Direct3D3 interface, hr %#x.\n", hr);
> +
> +    IDirect3D3_AddRef(d3d);
> +    prev_d3d_refcount = IDirect3D3_Release(d3d);
> Why not get_refcount()?
> +    hr = IDirect3D3_CreateVertexBuffer(d3d, &vb_desc, &vb, 0, NULL);
> +    ok(SUCCEEDED(hr), "Failed to create vertex buffer, hr %#x.\n", hr);
> +
> +    IDirect3D3_AddRef(d3d);
> +    cur_d3d_refcount = IDirect3D3_Release(d3d);
> +    IDirect3DDevice3_AddRef(device);
> +    cur_device_refcount = IDirect3DDevice3_Release(device);
> +    ok(cur_d3d_refcount == prev_d3d_refcount, "D3D object refcount changed from %u to %u.\n",
> +            prev_d3d_refcount, cur_d3d_refcount);
> +    ok(cur_device_refcount == prev_device_refcount, "D3D device refcount changed from %u to %u.\n",
> +            prev_device_refcount, cur_device_refcount);
> +
> +    IDirect3DVertexBuffer_Release(vb);
> It might not be bad to check the reference count after release as well.
> diff --git a/dlls/ddraw/vertexbuffer.c b/dlls/ddraw/vertexbuffer.c
> index 797afe2..7914805e 100644
> --- a/dlls/ddraw/vertexbuffer.c
> +++ b/dlls/ddraw/vertexbuffer.c
> @@ -126,16 +126,26 @@ static ULONG WINAPI d3d_vertex_buffer1_AddRef(IDirect3DVertexBuffer *iface)
>      return d3d_vertex_buffer7_AddRef(&buffer->IDirect3DVertexBuffer7_iface);
>  }
>
> +static void d3d_vertex_buffer_destroy(struct d3d_vertex_buffer *buffer)
> +{
> +    struct wined3d_buffer *vb = NULL;
> +    UINT offset, stride;
> +
> +    /* D3D7 vertex buffers don't stay bound in the device, they are passed
> +     * as a parameter to DrawPrimitiveVB. DrawPrimitiveVB sets them as the
> +     * stream source in wined3d and they should get unset there before
> +     * they are destroyed. */
> +    wined3d_mutex_lock();
> +    wined3d_device_get_stream_source(buffer->ddraw->wined3d_device,
> +            0, &vb, &offset, &stride);
> +    if (vb == buffer->wined3d_buffer)
> +        wined3d_device_set_stream_source(buffer->ddraw->wined3d_device, 0, NULL, 0, 0);
> +
> +    wined3d_vertex_declaration_decref(buffer->wined3d_declaration);
> +    wined3d_buffer_decref(buffer->wined3d_buffer);
> +    wined3d_mutex_unlock();
> +}
> We'd typically call this kind of function d3d_vertex_buffer_cleanup(),
> the difference being that _destroy() deallocates the object itself,
> while _cleanup() only releases references, allocations, etc. inside
> the object.
> static ULONG WINAPI d3d_vertex_buffer1_Release(IDirect3DVertexBuffer *iface)
>  {
>      struct d3d_vertex_buffer *buffer = impl_from_IDirect3DVertexBuffer(iface);
> +    ULONG ref = InterlockedDecrement(&buffer->ref);
>
> -    TRACE("iface %p.\n", iface);
> +    TRACE("%p decreasing refcount to %u.\n", buffer, ref);
>
> -    return d3d_vertex_buffer7_Release(&buffer->IDirect3DVertexBuffer7_iface);
> +    if (!ref)
> +    {
> +        d3d_vertex_buffer_destroy(buffer);
> +
> +        HeapFree(GetProcessHeap(), 0, buffer);
> +    }
> +    return ref;
>  }
> Does this imply that if you query an IDirect3DVertexBuffer interface
> from an IDirect3DVertexBuffer7, and that ends up being the last
> reference to be released, the ddraw reference should be released? (And
> in the reverse case, that you'd release a reference you don't have.)
> For surfaces and palettes, we use the "ifaceToRelease" mechanism for
> this kind of thing.