After spamming wine-patches with d3d tests, I tried to implement the probed behaviour. Attached are two patches, which makes all d3d8 refcount test pass. They are small and ugly. The problems are:
The implicit surfaces now gets released if the refcount is -1. Somehow we have to force releasing d3d8 sufaces from wined3d. No idea how.
I added a flag to mark the implicit surfaces. It's redundant, because wined3d knows the implicit surfaces. The other idea is calling GetRenderTarget and friends in Surface_(AddRef/Release). Besides the problem of an endless recursion, it would be quite an overhead.
Another problem is, that IWineD3DDeviceImpl_Uninit3D calls via GetParent AddRef/Release on the d3d8 surface. This causes the device to be released again(=>endless recursion). I "solved" it with ignoring an addref from 0 in the d3d8 Device, which causes the release to be -1 => no wined3d device releasing. Works but ugly.
Ideas, suggestions?
From 1a73ec5958e2e4c7fd076184c9e1f1cf7427d936 Mon Sep 17 00:00:00 2001
From: Markus Amsler markus.amsler@oribi.org Date: Mon, 13 Nov 2006 22:29:12 +0100 Subject: [PATCH] d3d8: Force implicit surfaces to handle their own refcount. To: wine-patches wine-patches@winehq.org
--- dlls/d3d8/d3d8_private.h | 3 +++ dlls/d3d8/directx.c | 2 ++ dlls/d3d8/surface.c | 4 ++-- dlls/d3d8/tests/device.c | 6 +++--- 4 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/dlls/d3d8/d3d8_private.h b/dlls/d3d8/d3d8_private.h index b32033f..9ca9989 100644 --- a/dlls/d3d8/d3d8_private.h +++ b/dlls/d3d8/d3d8_private.h @@ -246,6 +246,9 @@ struct IDirect3DSurface8Impl
/* Parent reference */ LPDIRECT3DDEVICE8 parentDevice; + + /* Flags an implicit surface */ + BOOL implicit; };
/* ------------------ */ diff --git a/dlls/d3d8/directx.c b/dlls/d3d8/directx.c index 42ae40d..b51181e 100644 --- a/dlls/d3d8/directx.c +++ b/dlls/d3d8/directx.c @@ -202,6 +202,7 @@ HRESULT WINAPI D3D8CB_CreateRenderTarget *ppSurface = d3dSurface->wineD3DSurface; IUnknown_Release(d3dSurface->parentDevice); d3dSurface->parentDevice = NULL; + d3dSurface->implicit = TRUE; } else { *ppSurface = NULL; } @@ -281,6 +282,7 @@ HRESULT WINAPI D3D8CB_CreateDepthStencil *ppSurface = d3dSurface->wineD3DSurface; IUnknown_Release(d3dSurface->parentDevice); d3dSurface->parentDevice = NULL; + d3dSurface->implicit = TRUE; } return res; } diff --git a/dlls/d3d8/surface.c b/dlls/d3d8/surface.c index 8ab312e..83265e9 100644 --- a/dlls/d3d8/surface.c +++ b/dlls/d3d8/surface.c @@ -46,7 +46,7 @@ static ULONG WINAPI IDirect3DSurface8Imp
TRACE("(%p)\n", This);
- IWineD3DSurface_GetContainerParent(This->wineD3DSurface, &containerParent); + if (!This->implicit) IWineD3DSurface_GetContainerParent(This->wineD3DSurface, &containerParent); if (containerParent) { /* Forward to the containerParent */ TRACE("(%p) : Forwarding to %p\n", This, containerParent); @@ -65,7 +65,7 @@ static ULONG WINAPI IDirect3DSurface8Imp
TRACE("(%p)\n", This);
- IWineD3DSurface_GetContainerParent(This->wineD3DSurface, &containerParent); + if (!This->implicit) IWineD3DSurface_GetContainerParent(This->wineD3DSurface, &containerParent); if (containerParent) { /* Forward to the containerParent */ TRACE("(%p) : Forwarding to %p\n", This, containerParent); diff --git a/dlls/d3d8/tests/device.c b/dlls/d3d8/tests/device.c index 06f8aab..86e10af 100644 --- a/dlls/d3d8/tests/device.c +++ b/dlls/d3d8/tests/device.c @@ -429,17 +429,17 @@ static void test_refcount(void) /* check implicit back buffer */ hr = IDirect3DSwapChain8_GetBackBuffer(pSwapChain, 0, 0, &pBackBuffer); todo_wine CHECK_CALL( hr, "GetBackBuffer", pDevice, ++refcount); - todo_wine CHECK_REFCOUNT( pSwapChain, 1); + CHECK_REFCOUNT( pSwapChain, 1); if(pBackBuffer) { todo_wine CHECK_CONTAINER( Surface, pBackBuffer, IID_IDirect3DDevice8, pDevice, S_OK); todo_wine CHECK_REFCOUNT( pBackBuffer, 1);
todo_wine CHECK_ADDREF_REFCOUNT(pBackBuffer, 2); - todo_wine CHECK_REFCOUNT(pSwapChain, 1); + CHECK_REFCOUNT(pSwapChain, 1); todo_wine CHECK_REFCOUNT(pDevice, refcount); todo_wine CHECK_RELEASE_REFCOUNT(pBackBuffer, 1); - todo_wine CHECK_REFCOUNT(pSwapChain, 1); + CHECK_REFCOUNT(pSwapChain, 1); todo_wine CHECK_REFCOUNT(pDevice, refcount);
todo_wine CHECK_RELEASE_REFCOUNT( pBackBuffer, 0);
On 14/11/06, Markus Amsler markus.amsler@oribi.org wrote:
After spamming wine-patches with d3d tests, I tried to implement the probed behaviour. Attached are two patches, which makes all d3d8 refcount test pass. They are small and ugly. The problems are:
The implicit surfaces now gets released if the refcount is -1. Somehow we have to force releasing d3d8 sufaces from wined3d. No idea how.
You could probably either force the refcount to 0 (ie, something like "while(IWineD3DSurface_Release(surface));"), or add an explicit destructor to the d3d8 implementation. That's not too pretty either, but I suppose it's better than releasing on -1.
I added a flag to mark the implicit surfaces. It's redundant, because wined3d knows the implicit surfaces. The other idea is calling GetRenderTarget and friends in Surface_(AddRef/Release). Besides the problem of an endless recursion, it would be quite an overhead.
Shouldn't we just check if the surface's container is the same as the surface's device?
Totally unrelated to this, are you ever on IRC?
H. Verbeet wrote:
On 14/11/06, Markus Amsler markus.amsler@oribi.org wrote:
The implicit surfaces now gets released if the refcount is -1. Somehow we have to force releasing d3d8 sufaces from wined3d. No idea how.
You could probably either force the refcount to 0 (ie, something like "while(IWineD3DSurface_Release(surface));"),
I'm not sure what you mean. IWineD3DSurface_Release won't release the d3d8 surface. Besides, the surface shouldn't be released with refcount==0, only on device destruction.
or add an explicit destructor to the d3d8 implementation. That's not too pretty either, but I suppose it's better than releasing on -1.
This would mean a wine specific d3d8 interface extension. I'm not sure whether that's acceptable. Perhaps we could misuse an existing function (e.g QueryInterface with a magic refiid, to destroy the surface, or set/get maigc private data). Or handle implicit surface destruction in IDirect3DDevice8_Release and not in IWineD3DDevice_Uninit3D. There we could use our private d3d8 destruct function. Looks for me like the way to go.
I added a flag to mark the implicit surfaces. It's redundant, because wined3d knows the implicit surfaces. The other idea is calling GetRenderTarget and friends in Surface_(AddRef/Release). Besides the problem of an endless recursion, it would be quite an overhead.
Shouldn't we just check if the surface's container is the same as the surface's device?
For d3d8 this would be a nice solution. But in d3d9 the implicit RenderTarget's container is the swapchain. I think we should handle d3d8 and d3d9 the same way.
Totally unrelated to this, are you ever on IRC?
Not yet, but I could. Is it easier to discuss such stuff on IRC?
On 14/11/06, Markus Amsler markus.amsler@oribi.org wrote:
H. Verbeet wrote:
On 14/11/06, Markus Amsler markus.amsler@oribi.org wrote:
The implicit surfaces now gets released if the refcount is -1. Somehow we have to force releasing d3d8 sufaces from wined3d. No idea how.
You could probably either force the refcount to 0 (ie, something like "while(IWineD3DSurface_Release(surface));"),
I'm not sure what you mean. IWineD3DSurface_Release won't release the d3d8 surface. Besides, the surface shouldn't be released with refcount==0, only on device destruction.
Nevermind, you're right, d3d8 releases wined3d, not the other way around. You can get to the wined3d surface's parent (ie, the d3d8 surface) from inside wined3d though.
or add an explicit destructor to the d3d8 implementation. That's not too pretty either, but I suppose it's better than releasing on -1.
This would mean a wine specific d3d8 interface extension. I'm not sure whether that's acceptable.
Well, it wouldn't get added to the public interface, but you could eg. pass it to the wined3d surface constructor.
Perhaps we could misuse an existing function (e.g QueryInterface with a magic refiid, to destroy the surface, or set/get maigc private data).
That would probably be worse.
Or handle implicit surface destruction in IDirect3DDevice8_Release and not in IWineD3DDevice_Uninit3D. There we could use our private d3d8 destruct function. Looks for me like the way to go.
That's probably the cleanest option.
I added a flag to mark the implicit surfaces. It's redundant, because wined3d knows the implicit surfaces. The other idea is calling GetRenderTarget and friends in Surface_(AddRef/Release). Besides the problem of an endless recursion, it would be quite an overhead.
Shouldn't we just check if the surface's container is the same as the surface's device?
For d3d8 this would be a nice solution. But in d3d9 the implicit RenderTarget's container is the swapchain. I think we should handle d3d8 and d3d9 the same way.
Isn't it always the swapchain in wined3d? Anyway, that's not going to work then.
Totally unrelated to this, are you ever on IRC?
Not yet, but I could. Is it easier to discuss such stuff on IRC?
Sometimes. Most people doing d3d stuff are on there.
Hi, Maybe its the best way not to construct the implicit surfaces in d3d8 at device creation time, only its wined3d counterpart(with parent = NULL). When GetBackBuffer or whatever is called, the wined3d backbuffer is returned, and a d3d8 surface created for it. The wined3d surface gets the d3d8/9 surface assigned to it, so future GetBackBuffers find it.
When the implicit surface is destroyed(aka refcount falls to 0) destroy it(without destroying the wined3d surface) and set the wined3d surface's parent to NULL.
When the device is destroyed wined3d destroys the implicit surface's parent ( while(IUnknonw_Release(parent)); ), and then the wined3d surface(which has parent == NULL then). This should never happen I think because when the implicit surface is created the device is addrefed. So in Uninit3D we can assume that the implicit d3d8 surface has been destroyed already.
Alternatively we can keep things as they are, and add some resource::isImplicit field to d3d8 / 9. Set this when the implicit things are created, and init their refcount to 0. In release, do not destroy resources with the isImplicit bit set. When the device is destroyed set isImplicit to 0 and force the refcount to 1, then release as usual.
It should be possible to test which approach windows uses: Write a test case which calls GetBackBuffer, store the address, release the object. After allocating quite a bit of memory call GetBackBuffer again and see if the returned pointer is the same.
We need a proper test case for such refcounting hacks, but then it shouldn't be a problem to get a clone of Microsofts refcounting hacks into wine :-) (I cloned some ddraw refcounting hacks)
And once the wined3d refcount scheme is updated, I have to update ddraw.
-
H. Verbeet -
Markus Amsler -
Stefan Dösinger