ddraw: Keep track of interface attached by AddAttachedSurface and detach correct interface when parent is released. (try 3)
Octavian Voicu
octavian.voicu at gmail.com
Tue Oct 11 14:59:21 CDT 2011
--
try 3:
- fix test failure by moving error check from *_DeleteAttachedSurface to
ddraw_surface_delete_attached_surface, after another check (thanks Henri).
try 2:
- use IUnknown_Release(attached_iface) instead of manual dispatch,
as suggested by Henri;
- change type of attached_iface from void* to IUnknown* to reflect
fact that it's a pointer to an interface;
- add test to show that attaching a surface only attaches a specific
interface and return error if DeleteAttachedSurface is called with
another interface than the attached one (instead of detaching the
surface anyway).
Fixes a bug that causes a stencil buffer leak in the game The Longest Journey.
Problem can be seen in the console output log attached to bug #11819.
This patch doesn't fix the original bug, but now game exits cleanly.
The root of the problem is that when releasing a surface, its attached
surfaces are detached using IDirectDraw7_DeleteAttachedSurface, regardless
of what interface was originally attached.
Here are the relevant lines from my own log, before the patch is applied:
trace:ddraw:ddraw_create_surface ddraw 0x1861e8, surface_desc 0x32ecf0, surface 0x32edc8, level 0.
trace:ddraw:ddraw_create_surface (0x1861e8) Requesting surface desc :
trace:ddraw:DDRAW_dump_members - DDSD_CAPS : DDSCAPS_3DDEVICE DDSCAPS_VIDEOMEMORY DDSCAPS_ZBUFFER
trace:ddraw:DDRAW_dump_members - DDSD_HEIGHT : 480
trace:ddraw:DDRAW_dump_members - DDSD_WIDTH : 640
trace:ddraw:DDRAW_dump_members - DDSD_PIXELFORMAT : ( DDPF_ZBUFFER , Z bits : 16)
...
trace:d3d_surface:wined3d_surface_create Created surface 0x1bf6f0.
...
trace:ddraw:ddraw_create_surface Created surface 0x1bf5e0.
trace:ddraw:ddraw_surface1_AddAttachedSurface iface 0x1bc9f8, attachment 0x1bf5f0.
trace:ddraw:ddraw_surface3_AddAttachedSurface iface 0x1bc9f0, attachment 0x1bf5e8.
...
trace:ddraw:ddraw_surface_wined3d_object_destroyed surface 0x1bc9e8.
trace:ddraw:ddraw_surface7_DeleteAttachedSurface iface 0x1bc9e8, flags 0, attachment 0x1bf5e0.
trace:ddraw:ddraw_surface_delete_attached_surface surface 0x1bc9e8, attachment 0x1bf5e0.
trace:ddraw:ddraw_surface7_Release iface 0x1bf5e0 decreasing refcount to 4294967295.
...
trace:d3d:wined3d_device_uninit_3d Releasing depth/stencil buffer 0x1bf6f0.
trace:d3d_surface:wined3d_surface_decref Surface 0x1bf6f0, container (nil) of type 0.
trace:d3d_surface:wined3d_surface_decref 0x1bf6f0 decreasing refcount to 1.
err:d3d:wined3d_device_uninit_3d Something is still holding a reference to depth/stencil buffer 0x1bf6f0.
...
fixme:d3d:wined3d_device_decref Device released with resources still bound, acceptable but unexpected.
fixme:d3d:wined3d_device_decref Leftover resource 0x1bf6f0 with type WINED3DRTYPE_SURFACE (0x1).
---
dlls/ddraw/ddraw_private.h | 1 +
dlls/ddraw/surface.c | 81 +++++++++++++++----------------------------
dlls/ddraw/tests/refcount.c | 12 +++++-
3 files changed, 39 insertions(+), 55 deletions(-)
diff --git a/dlls/ddraw/ddraw_private.h b/dlls/ddraw/ddraw_private.h
index e75acc0..4fb6fc4 100644
--- a/dlls/ddraw/ddraw_private.h
+++ b/dlls/ddraw/ddraw_private.h
@@ -165,6 +165,7 @@ struct IDirectDrawSurfaceImpl
/* This implementation handles attaching surfaces to other surfaces */
IDirectDrawSurfaceImpl *next_attached;
IDirectDrawSurfaceImpl *first_attached;
+ IUnknown *attached_iface;
/* Complex surfaces are organized in a tree, although the tree is degenerated to a list in most cases.
* In mipmap and primary surfaces each level has only one attachment, which is the next surface level.
diff --git a/dlls/ddraw/surface.c b/dlls/ddraw/surface.c
index 6140ee0..24d0497 100644
--- a/dlls/ddraw/surface.c
+++ b/dlls/ddraw/surface.c
@@ -1350,7 +1350,7 @@ static HRESULT WINAPI ddraw_surface1_Blt(IDirectDrawSurface *iface, RECT *dst_re
*
* Attaches a surface to another surface. How the surface attachments work
* is not totally understood yet, and this method is prone to problems.
- * he surface that is attached is AddRef-ed.
+ * The surface that is attached is AddRef-ed.
*
* Tests with complex surfaces suggest that the surface attachments form a
* tree, but no method to test this has been found yet.
@@ -1449,6 +1449,7 @@ static HRESULT WINAPI ddraw_surface7_AddAttachedSurface(IDirectDrawSurface7 *ifa
return hr;
}
ddraw_surface7_AddRef(attachment);
+ attachment_impl->attached_iface = (IUnknown *)attachment;
return hr;
}
@@ -1468,6 +1469,7 @@ static HRESULT WINAPI ddraw_surface4_AddAttachedSurface(IDirectDrawSurface4 *ifa
}
ddraw_surface4_AddRef(attachment);
ddraw_surface7_Release(&attachment_impl->IDirectDrawSurface7_iface);
+ attachment_impl->attached_iface = (IUnknown *)attachment;
return hr;
}
static HRESULT WINAPI ddraw_surface3_AddAttachedSurface(IDirectDrawSurface3 *iface, IDirectDrawSurface3 *attachment)
@@ -1512,6 +1514,7 @@ static HRESULT WINAPI ddraw_surface3_AddAttachedSurface(IDirectDrawSurface3 *ifa
return hr;
}
ddraw_surface3_AddRef(attachment);
+ attachment_impl->attached_iface = (IUnknown *)attachment;
return hr;
}
@@ -1531,6 +1534,7 @@ static HRESULT WINAPI ddraw_surface2_AddAttachedSurface(IDirectDrawSurface2 *ifa
}
ddraw_surface2_AddRef(attachment);
ddraw_surface3_Release(&attachment_impl->IDirectDrawSurface3_iface);
+ attachment_impl->attached_iface = (IUnknown *)attachment;
return hr;
}
@@ -1550,6 +1554,7 @@ static HRESULT WINAPI ddraw_surface1_AddAttachedSurface(IDirectDrawSurface *ifac
}
ddraw_surface1_AddRef(attachment);
ddraw_surface3_Release(&attachment_impl->IDirectDrawSurface3_iface);
+ attachment_impl->attached_iface = (IUnknown *)attachment;
return hr;
}
@@ -1569,11 +1574,11 @@ static HRESULT WINAPI ddraw_surface1_AddAttachedSurface(IDirectDrawSurface *ifac
*
*****************************************************************************/
static HRESULT ddraw_surface_delete_attached_surface(IDirectDrawSurfaceImpl *This,
- IDirectDrawSurfaceImpl *Surf)
+ IDirectDrawSurfaceImpl *Surf, IUnknown *detach_iface)
{
IDirectDrawSurfaceImpl *Prev = This;
- TRACE("surface %p, attachment %p.\n", This, Surf);
+ TRACE("surface %p, attachment %p, detach_iface %p.\n", This, Surf, detach_iface);
EnterCriticalSection(&ddraw_cs);
if (!Surf || (Surf->first_attached != This) || (Surf == This) )
@@ -1582,6 +1587,13 @@ static HRESULT ddraw_surface_delete_attached_surface(IDirectDrawSurfaceImpl *Thi
return DDERR_CANNOTDETACHSURFACE;
}
+ if (Surf->attached_iface != detach_iface)
+ {
+ WARN("Surf->attach_iface %p != detach_iface %p.\n", Surf->attached_iface, detach_iface);
+ LeaveCriticalSection(&ddraw_cs);
+ return DDERR_SURFACENOTATTACHED;
+ }
+
/* Remove MIPMAPSUBLEVEL if this seemed to be one */
if (This->surface_desc.ddsCaps.dwCaps &
Surf->surface_desc.ddsCaps.dwCaps & DDSCAPS_MIPMAP)
@@ -1612,6 +1624,8 @@ static HRESULT ddraw_surface_delete_attached_surface(IDirectDrawSurfaceImpl *Thi
IDirect3DDeviceImpl_UpdateDepthStencil(This->ddraw->d3ddevice);
}
LeaveCriticalSection(&ddraw_cs);
+ IUnknown_Release(Surf->attached_iface);
+ Surf->attached_iface = NULL;
return DD_OK;
}
@@ -1620,17 +1634,10 @@ static HRESULT WINAPI ddraw_surface7_DeleteAttachedSurface(IDirectDrawSurface7 *
{
IDirectDrawSurfaceImpl *This = impl_from_IDirectDrawSurface7(iface);
IDirectDrawSurfaceImpl *attachment_impl = unsafe_impl_from_IDirectDrawSurface7(attachment);
- HRESULT hr;
TRACE("iface %p, flags %#x, attachment %p.\n", iface, flags, attachment);
- hr = ddraw_surface_delete_attached_surface(This, attachment_impl);
- if (FAILED(hr))
- {
- return hr;
- }
- ddraw_surface7_Release(attachment);
- return hr;
+ return ddraw_surface_delete_attached_surface(This, attachment_impl, (IUnknown *)attachment);
}
static HRESULT WINAPI ddraw_surface4_DeleteAttachedSurface(IDirectDrawSurface4 *iface,
@@ -1638,17 +1645,10 @@ static HRESULT WINAPI ddraw_surface4_DeleteAttachedSurface(IDirectDrawSurface4 *
{
IDirectDrawSurfaceImpl *This = impl_from_IDirectDrawSurface4(iface);
IDirectDrawSurfaceImpl *attachment_impl = unsafe_impl_from_IDirectDrawSurface4(attachment);
- HRESULT hr;
TRACE("iface %p, flags %#x, attachment %p.\n", iface, flags, attachment);
- hr = ddraw_surface_delete_attached_surface(This, attachment_impl);
- if (FAILED(hr))
- {
- return hr;
- }
- ddraw_surface4_Release(attachment);
- return hr;
+ return ddraw_surface_delete_attached_surface(This, attachment_impl, (IUnknown *)attachment);
}
static HRESULT WINAPI ddraw_surface3_DeleteAttachedSurface(IDirectDrawSurface3 *iface,
@@ -1656,16 +1656,10 @@ static HRESULT WINAPI ddraw_surface3_DeleteAttachedSurface(IDirectDrawSurface3 *
{
IDirectDrawSurfaceImpl *This = impl_from_IDirectDrawSurface3(iface);
IDirectDrawSurfaceImpl *attachment_impl = unsafe_impl_from_IDirectDrawSurface3(attachment);
- HRESULT hr;
+
TRACE("iface %p, flags %#x, attachment %p.\n", iface, flags, attachment);
- hr = ddraw_surface_delete_attached_surface(This, attachment_impl);
- if (FAILED(hr))
- {
- return hr;
- }
- ddraw_surface3_Release(attachment);
- return hr;
+ return ddraw_surface_delete_attached_surface(This, attachment_impl, (IUnknown *)attachment);
}
static HRESULT WINAPI ddraw_surface2_DeleteAttachedSurface(IDirectDrawSurface2 *iface,
@@ -1673,16 +1667,10 @@ static HRESULT WINAPI ddraw_surface2_DeleteAttachedSurface(IDirectDrawSurface2 *
{
IDirectDrawSurfaceImpl *This = impl_from_IDirectDrawSurface2(iface);
IDirectDrawSurfaceImpl *attachment_impl = unsafe_impl_from_IDirectDrawSurface2(attachment);
- HRESULT hr;
+
TRACE("iface %p, flags %#x, attachment %p.\n", iface, flags, attachment);
- hr = ddraw_surface_delete_attached_surface(This, attachment_impl);
- if (FAILED(hr))
- {
- return hr;
- }
- ddraw_surface2_Release(attachment);
- return hr;
+ return ddraw_surface_delete_attached_surface(This, attachment_impl, (IUnknown *)attachment);
}
static HRESULT WINAPI ddraw_surface1_DeleteAttachedSurface(IDirectDrawSurface *iface,
@@ -1690,16 +1678,10 @@ static HRESULT WINAPI ddraw_surface1_DeleteAttachedSurface(IDirectDrawSurface *i
{
IDirectDrawSurfaceImpl *This = impl_from_IDirectDrawSurface(iface);
IDirectDrawSurfaceImpl *attachment_impl = unsafe_impl_from_IDirectDrawSurface(attachment);
- HRESULT hr;
+
TRACE("iface %p, flags %#x, attachment %p.\n", iface, flags, attachment);
- hr = ddraw_surface_delete_attached_surface(This, attachment_impl);
- if (FAILED(hr))
- {
- return hr;
- }
- ddraw_surface1_Release(attachment);
- return hr;
+ return ddraw_surface_delete_attached_surface(This, attachment_impl, (IUnknown *)attachment);
}
/*****************************************************************************
@@ -5100,9 +5082,6 @@ static void STDMETHODCALLTYPE ddraw_surface_wined3d_object_destroyed(void *paren
/* Check for attached surfaces and detach them. */
if (surface->first_attached != surface)
{
- IDirectDrawSurface7 *root = &surface->first_attached->IDirectDrawSurface7_iface;
- IDirectDrawSurface7 *detach = &surface->IDirectDrawSurface7_iface;
-
/* Well, this shouldn't happen: The surface being attached is
* referenced in AddAttachedSurface(), so it shouldn't be released
* until DeleteAttachedSurface() is called, because the refcount is
@@ -5111,18 +5090,14 @@ static void STDMETHODCALLTYPE ddraw_surface_wined3d_object_destroyed(void *paren
WARN("Surface is still attached to surface %p.\n", surface->first_attached);
/* The refcount will drop to -1 here */
- if (FAILED(IDirectDrawSurface7_DeleteAttachedSurface(root, 0, detach)))
+ if (FAILED(ddraw_surface_delete_attached_surface(surface->first_attached, surface, surface->attached_iface)))
ERR("DeleteAttachedSurface failed.\n");
}
while (surface->next_attached)
- {
- IDirectDrawSurface7 *root = &surface->IDirectDrawSurface7_iface;
- IDirectDrawSurface7 *detach = &surface->next_attached->IDirectDrawSurface7_iface;
-
- if (FAILED(IDirectDrawSurface7_DeleteAttachedSurface(root, 0, detach)))
+ if (FAILED(ddraw_surface_delete_attached_surface(surface,
+ surface->next_attached, surface->next_attached->attached_iface)))
ERR("DeleteAttachedSurface failed.\n");
- }
/* Having a texture handle set implies that the device still exists. */
if (surface->Handle)
diff --git a/dlls/ddraw/tests/refcount.c b/dlls/ddraw/tests/refcount.c
index 736ce8d..cf94986 100644
--- a/dlls/ddraw/tests/refcount.c
+++ b/dlls/ddraw/tests/refcount.c
@@ -216,6 +216,14 @@ static void test_ddraw_objects(void)
{
ref = getRefcount( (IUnknown *) stencil);
ok(ref == 2, "Got refcount %d, expected 2\n", ref);
+ hr = IDirectDrawSurface7_QueryInterface(surface, &IID_IDirectDrawSurface, (void **) &surface1);
+ ok(hr == DD_OK, "IDirectDrawSurface7_QueryInterface returned %08x\n", hr);
+ hr = IDirectDrawSurface7_QueryInterface(stencil, &IID_IDirectDrawSurface, (void **) &stencil1);
+ ok(hr == DD_OK, "IDirectDrawSurface7_QueryInterface returned %08x\n", hr);
+ hr = IDirectDrawSurface_DeleteAttachedSurface(surface1, 0, stencil1);
+ ok(hr == DDERR_SURFACENOTATTACHED, "DeleteAttachedSurface returned %08x\n", hr);
+ if (stencil1 != NULL) IDirectDrawSurface_Release(stencil1);
+ if (surface1 != NULL) IDirectDrawSurface_Release(surface1);
hr = IDirectDrawSurface7_DeleteAttachedSurface(surface, 0, stencil);
ok(hr == DD_OK, "DeleteAttachedSurface returned %08x\n", hr);
ref = getRefcount( (IUnknown *) stencil);
@@ -270,9 +278,9 @@ static void test_ddraw_objects(void)
ref = IDirectDrawSurface_Release(surface1);
ok(!ref, "Got refcount %d, expected 0\n", ref);
ref = getRefcount( (IUnknown *) stencil1);
- todo_wine ok(ref == 1, "Got refcount %d, expected 1\n", ref);
+ ok(ref == 1, "Got refcount %d, expected 1\n", ref);
ref = IDirectDrawSurface_Release(stencil1);
- todo_wine ok(!ref, "Got refcount %d, expected 0\n", ref);
+ ok(!ref, "Got refcount %d, expected 0\n", ref);
}
else
IDirectDrawSurface_Release(surface1);
--
1.7.4.1
More information about the wine-patches
mailing list