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  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.