I've run into a somewhat annoying problem with d3d9/wined3d recently. It appears that on d3d9 on Windows a surface either forwards its AddRef / Release calls to its container if it has one, or just shares the refcount directly. Either way, it comes down to the same thing. Calling AddRef on the surface increases the texture's refcount and calling AddRef on the texture increases the surface's refcount. Same thing for Release.
As a consequence, on Windows, it's perfectly legal to do something like this:
/* Create a texture */ IDirect3DDevice9_CreateTexture() /* Get a surface. Adds a reference to the surface/texture combination */ IDirect3DTexture9_GetSurfaceLevel() /* Release the texture. Note that the texture isn't freed yet, because we still hold a reference to the surface */ IDirect3DTexture9_Release() <Do some stuff with the surface, perhaps call GetContainer, etc> /* Release the surface. The texture and all of it's surfaces will now be released. */ IDirect3DSurface9_Release()
In wine, the call to IDirect3DTexture9_Release() frees the texture. (It also calls Release on it's surfaces, but since the surface had an extra reference from the call to GetSurfaceLevel the surface is still ok). Calling any function on the surface that needs access to the container will now have undefines results, most likely a crash.
Naively, I tried to fix this by having the surface keep a reference to it's container. That keeps the code above from crashing, but only because it creates a circular reference between the surface and the container. Oops.
Fixing the problem for wined3d is probably not even that hard. Just separate the Release code from the Cleanup code, then have WineD3DSurface forward its AddRef / Release calls to the container, if it has one. The container can then call the Cleanup code for its surfaces when it gets deleted.
However, there is another problem, related to the way d3d9 (and ddraw/d3d7/d3d8, when their respective rewrites are finished) wraps wined3d. After fixing the problem for wined3d as above, the relation between IDirect3DSurface9 and IDirect3DTexture9 in wine would look roughly like this:
|-------------------| |-------------------| | IDirect3DSurface9 | | IDirect3DTexture9 | |-------------------| |-------------------| ^ ^ | | |----------------| |----------------| | WineD3DSurface |<--------->| WineD3DTexture | |----------------| |----------------|
WineD3DSurface and WineD3DTexture "share" reference counts, IDirect3DSurface9 keeps a reference to WineD3DSurface, and IDirect3DTexture keeps a reference to WineD3DTexture. WineD3DSurface and WineD3DTexture don't keep references to their parents (and can't, without creating circular references).
So releasing the texture before the surface would still not work, since the d3d9 texture still has only 1 reference, the one it got when it was created. That means that in the call to IDirect3DTexture9_Release the d3d9 texture gets freed, while the wined3d texture doesn't. When we do something with the d3d9 surface that needs the container, the wined3d surface correctly returns its container, but when d3d9 then tries to get the parent of that container it still dies.
One way to solve this would be to have wined3d handle the reference counting for d3d7/8/9 as well, so that their AddRef/Release calls just call AddRef/Release on the wined3d object they wrap. That would also mean we need to pass a pointer to the d3d7/8/9 object's cleanup function. (Releasing a d3d9 surface to 0 should cause a d3d9 texture to be freed). Note that for creating a texture, currently a similair construction is used, in that IWineD3DDeviceImpl_CreateTexture gets passed a pointer to a d3d9 surface create function, in order to create the texture's surfaces.
Obviously doing all that would be quite a change in the way d3d is setup in wine, and would also have some implications for the d3d7 and d3d8 rewrites. I'm wondering if this solution would be acceptable, and whether perhaps there are other, better solutions. Anyone care to comment?
One way to solve this would be to have wined3d handle the reference counting for d3d7/8/9 as well, so that their AddRef/Release calls just call AddRef/Release on the wined3d object they wrap. That would also mean we need to pass a pointer to the d3d7/8/9 object's cleanup function. (Releasing a d3d9 surface to 0 should cause a d3d9 texture to be freed). Note that for creating a texture, currently a similair construction is used, in that IWineD3DDeviceImpl_CreateTexture gets passed a pointer to a d3d9 surface create function, in order to create the texture's surfaces.
I have had some reference counting problems with ddraw too, but they were more simple(Mainly, that needed WineD3D objects had no ddraw counterpart, or that WineD3D destroys the ddraw parents, although they were still needed).
DDraw doesn't use containers, instead it has surface attachments and complex surfaces, with some reference counting difficulties. My solution was to handle that things in ddraw.
Could it be that d3d9 surfaces and textures are the same objects on windows? What happens if you QueryInterface the surface for the texture GUID?
Obviously doing all that would be quite a change in the way d3d is setup in wine, and would also have some implications for the d3d7 and d3d8 rewrites. I'm wondering if this solution would be acceptable, and whether perhaps there are other, better solutions. Anyone care to comment?
It might be interesting if there are more such refcount connections. For textures, my parent/child connections are:
DDraw surface 1 -> WineD3DTexture IParent -> WineD3DSurface 1 DDraw surface 2 -> WineD3DSurface 2 etc
Directdraw textures are the same objects as directdraw surfaces, so they share the refcount.
First, I create a ddraw surface, and create a WineD3DTexture for it. The surface is the child for texture. WineD3D calls my surface creation callback, which creates a WineD3DSurface for the ddraw surface on the first call, and a Parent object for the WineD3Dsurface. The IParent objects only purpose is to release it's child when it's destroyed. For other levels, I create a new ddraw surface, which is the parent of the new WineD3D surface, and attach it to the first ddraw surface. The attachment list is managed in ddraw.
When the app releases the compound, it (hopefully) calls the release method of the first surface. This releases the WineD3D texture, which causes a release to the IParent object and the further ddraw surfaces. The parent and the ddraw surfaces release the Wined3d surfaces. (That code works for 1 level textures, I haven't yet tried an app which uses multiple levels, or I silently broke that functionality)
My suggestion is to keep d3dX specific refcounting in ddraw / d3d8 / d3d9, and use the WineD3D refcounts for wined3d internal things. A quick guessis to seperate the D3D9 release code from the cleanup code, instead of doing this in WineD3D.
What happens if a texture has 2 surface levels? A GetSurfaceLevel for the first level AddRefs both the texture and the surface. A GetSurfaceLevel for the secound addrefs the texture and the 2nd surface, but what happens to the first one? A Release() of the first surface Releases() the texture, what happens to the secound surface?
Stefan
On 12/02/06, Stefan Dösinger stefandoesinger@gmx.at wrote:
Could it be that d3d9 surfaces and textures are the same objects on windows? What happens if you QueryInterface the surface for the texture GUID?
It returns 0x80004002 (E_NOINTERFACE). Same thing for the reverse.
When the app releases the compound, it (hopefully) calls the release method of the first surface.
At least for d3d9, it's not guaranteed which surface / texture gets released last. Could be the texture, could be the first surface, could be any of the other surfaces.
My suggestion is to keep d3dX specific refcounting in ddraw / d3d8 / d3d9, and use the WineD3D refcounts for wined3d internal things. A quick guessis to seperate the D3D9 release code from the cleanup code, instead of doing this in WineD3D.
With the solution mentioned in my previous post, d3d7/8/9 objects would essentially not be real COM objects on their own, they would just wrap the relevant wined3d AddRef/Release functions, and not have refcounts of their own. Wined3d would be responsible for it's own refcounting, and cleaning up the d3d7/8/9 "object" as part of it's own cleanup. Separating the d3d9 cleanup and release code would obviously be part of that. Note that if we separate the cleanup and release code we can't just call the cleanup code from wined3d, because we don't know the type of the parent, all we have is an IUnknown pointer. Which is why we would have to pass a pointer to that function to wined3d during object creation.
What happens if a texture has 2 surface levels? A GetSurfaceLevel for the first level AddRefs both the texture and the surface. A GetSurfaceLevel for the secound addrefs the texture and the 2nd surface, but what happens to the first one? A Release() of the first surface Releases() the texture, what happens to the secound surface?
The reference count appears to be shared between the texture and all of its surfaces. This is the output of a small test program I wrote:
texture_refs.c:67:Calling CreateTexture texture_refs.c:69:texture @ 001DB220 texture_refs.c:71:texture refs: 1 texture_refs.c:73:Calling GetSurfaceLevel (0) texture_refs.c:75:surface 0 @ 001DB420 texture_refs.c:78:texture refs: 2 texture_refs.c:79:surface 0 refs: 2 texture_refs.c:82:QueryInterface for surface on texture returned 0x80004002, ptr 00000000 texture_refs.c:84:QueryInterface for texture on surface 0 returned 0x80004002, ptr 00000000 texture_refs.c:86:Calling GetSurfaceLevel (1) texture_refs.c:88:surface 1 @ 001DB540 texture_refs.c:92:texture refs: 3 texture_refs.c:93:surface 0 refs: 3 texture_refs.c:94:surface 1 refs: 3 texture_refs.c:96:Calling Release on texture texture_refs.c:101:texture refs: 2 texture_refs.c:102:surface 0 refs: 2 texture_refs.c:103:surface 1 refs: 2 texture_refs.c:105:Calling Release on surface 1 texture_refs.c:110:texture refs: 1 texture_refs.c:111:surface 0 refs: 1 texture_refs.c:112:surface 1 refs: 1 texture_refs.c:114:Calling AddRef on surface 0 texture_refs.c:119:texture refs: 2 texture_refs.c:120:surface 0 refs: 2 texture_refs.c:121:surface 1 refs: 2
The refcounts in that program are retrieved like this: static int get_refcount(IUnknown *object) { IUnknown_AddRef(object); return IUnknown_Release(object); }
With the solution mentioned in my previous post, d3d7/8/9 objects would essentially not be real COM objects on their own, they would just wrap the relevant wined3d AddRef/Release functions, and not have refcounts of their own. Wined3d would be responsible for it's own refcounting, and cleaning up the d3d7/8/9 "object" as part of it's own cleanup. Separating the d3d9 cleanup and release code would obviously be part of that. Note that if we separate the cleanup and release code we can't just call the cleanup code from wined3d, because we don't know the type of the parent, all we have is an IUnknown pointer. Which is why we would have to pass a pointer to that function to wined3d during object creation.
I don't think that this will work for ddraw. The ddraw object connections are a little different from D3D9 / WineD3D. For example, in ddraw, the app creates a DirectDraw interface. This interface doesn't have any surfaces on it's own, the primary surfaces are created by the app. When a Direct3DDevice is created, the surfaces already exist. In D3D9, creating the Direct3DDevice automatically creates the surfaces. There's no Texture object in ddraw too. I don't think that a direct DDraw <-> WineD3D refcount mapping can work.
What happens if a texture has 2 surface levels? A GetSurfaceLevel for the first level AddRefs both the texture and the surface. A GetSurfaceLevel for the secound addrefs the texture and the 2nd surface, but what happens to the first one? A Release() of the first surface Releases() the texture, what happens to the secound surface?
The reference count appears to be shared between the texture and all of its surfaces. This is the output of a small test program I wrote:
texture_refs.c:67:Calling CreateTexture texture_refs.c:69:texture @ 001DB220 texture_refs.c:71:texture refs: 1 texture_refs.c:73:Calling GetSurfaceLevel (0) texture_refs.c:75:surface 0 @ 001DB420 texture_refs.c:78:texture refs: 2 texture_refs.c:79:surface 0 refs: 2 texture_refs.c:82:QueryInterface for surface on texture returned 0x80004002, ptr 00000000 texture_refs.c:84:QueryInterface for texture on surface 0 returned 0x80004002, ptr 00000000 texture_refs.c:86:Calling GetSurfaceLevel (1) texture_refs.c:88:surface 1 @ 001DB540 texture_refs.c:92:texture refs: 3 texture_refs.c:93:surface 0 refs: 3 texture_refs.c:94:surface 1 refs: 3 texture_refs.c:96:Calling Release on texture texture_refs.c:101:texture refs: 2 texture_refs.c:102:surface 0 refs: 2 texture_refs.c:103:surface 1 refs: 2 texture_refs.c:105:Calling Release on surface 1 texture_refs.c:110:texture refs: 1 texture_refs.c:111:surface 0 refs: 1 texture_refs.c:112:surface 1 refs: 1 texture_refs.c:114:Calling AddRef on surface 0 texture_refs.c:119:texture refs: 2 texture_refs.c:120:surface 0 refs: 2 texture_refs.c:121:surface 1 refs: 2
Seems to be really one refcount. I'd suggest to create a addref_override and release_ovveride member for Direct3D9 Surfaces (and Direct3D8), and set them when a texture is created, and unset them when the texture is destroyed and before the surfaces are released the last time(so destroying the surface works normally with IDirect3DSurface9::Release).
As a sidenote, such overrides bring some problems. The current ddraw implementation is full of them. The callbacks and the existance of 3 different IDirectDraw(Main, user, hal) implementations and 5 different IDirectDrawSurface(Main, user, hal, dib, zbuffer) implementations made it quite difficult to understand the code. I don't think that I've fully understood it by now.
I'll have a look about the consequences of a WineD3DTexture <-> WineD3DSurface refcount connection for ddraw. I think it's not hard to cope with that case, but I can't promise right now. Can you check if there are more refcount connections?
I think that I should line out the changes to WineD3D I am planning: I'm going to add some methods, like IWineD3DSurface::Blt, IWineD3DSurface::BltFast, ..., this doesn't make any difference for D3D9. I split up the WineD3DDevice initalisation code into the CreateDevice part, which doesn't initialize OpenGL, and a IWineD3DDevice::Init3D to create a swapchain and the render targets. As a counterpart, there's a IWineD3DDevice::Uninit3D, which destroys the swapchain, but not the rendertargets, and the IWineD3DDevice::Release method, which frees the rest. The necessary changes to D3D9 are a call to IWineD3DDevice::Init3D during creation, and IWineD3DDevice::Uninit3D when the d3d9 device is destroyed. The d3d9 device has to release it's rendertarget surfaces on it's own too.
For 2D operation without OpenGL, I've created a secound surface implementation, IWineX11Surface(well, bad name). It replaces a few methods(Blt, Lock, Unlock, Flip, ...), but uses the same management code. CreateSurface takes another parameter which determines the surface type. To avoid a too complex code, there's no way to change a X11 surface into a 3D surface and vice versa. If for some reason a switch is necessary, ddraw releases all WineD3D surfaces and re-creates them. The advantage is that if are 2D app is run with GL surfaces, DDraw operation is hardware accellerated. This can be configured by a setting in winecfg in future.
The ugly part is the screen setup. Right now, this is done in the swapchain code with GL, and I've made a switch in IWineD3DDevice::GetDisplayMode and the new IWineD3DDevice::SetDisplayMode to have a different setup code without a swapchain. As a dublicate of IWineD3DDevice::Present, there's a IWineD3DSurface::Flip method, which calls Present for GL surfaces, or handles a flip by it's own(X11 surfaces).
DDraw-specific management in handled in ddraw directly. This concernes complex surfaces(~containers in d3d9), surface attachments and the (IMO horrible) DDraw structures. The code in WineD3D works with WineD3D or D3D9 types(except of the DDBLTFX function). I hope that I can avoid including the ddraw headers in WineD3D. My first patches will add some WineD3D types which don't exist in d3d7 to avoid using D3D9 types in ddraw.
Hi, I've thought over this: If you want to connect the IWineD3DTexture and IWineD3DSurface refcounts only, then there's no problem for me. If you want to connect the Texture's and Surface's Parents refcount, then you have do the same for the rendertargets and the swapchain(I guess that Windows does that too), and I can use that connection to manage complex ddraw surfaces.
Stefan
On 12/02/06, Stefan Dösinger stefandoesinger@gmx.at wrote:
Hi, I've thought over this: If you want to connect the IWineD3DTexture and IWineD3DSurface refcounts only, then there's no problem for me. If you want to connect the Texture's and Surface's Parents refcount,
Are you talking about IDirect3DSurface9 <=> IDirect3DTexture9 or do you mean IDirect3DSurface9 <=> IWineD3DSurface?
then you have do the same for the rendertargets and the swapchain(I guess that Windows does that too), and I can use that connection to manage complex ddraw surfaces.
I haven't verified it, but the MSDN documentation for GetContainer appears to imply that it does. http://msdn.microsoft.com/library/default.asp?url=/library/en-us/directx9_c/... Unfortunately my knowledge of ddraw and d3d7 is somewhat limited, so it's a bit hard for me to oversee all the implications those changes would have for ddraw / d3d7. The choice for Surface and Texture as an example is somewhat arbitrary I guess, but probably the one for which it matters the most. (As in, is most likely to break stuff if it isn't implemented correctly). In principle it would change for any object that has a container or uses similar construction. Perhaps it would even make sense to forward AddRef and Release calls for all d3d7/8/9 objects that directly wrap a wined3d object.
For reference, the code for Release & CleanUp would look something like this for the different objects:
/* WineD3D Surface */ ULONG WINAPI IWineD3DSurfaceImpl_Release(IWineD3DSurface *iface) { IWineD3DSurfaceImpl *This = (IWineD3DSurfaceImpl *)iface; if (This->container) { return IWineD3DBase_Release(This->container); } else { /* Handle our own refcounting */ } }
void WINAPI IWineD3DSurfaceImpl_CleanUp(IWineD3DSurface *iface) { IWineD3DSurfaceImpl *This = (IWineD3DSurfaceImpl *)iface;
/* Do some cleaning for ourselves */
This->parentCleanUp(This->parent); }
/* WineD3D Texture */ ULONG WINAPI IWineD3DTextureImpl_Release(IWineD3DTexture *iface) { IWineD3DTextureImpl *This = (IWineD3DTextureImpl *)iface; ULONG ref = InterlockedDecrement(&This->resource.ref); if (!ref) { IWineD3DTexture_CleanUp(iface); }
return ref; }
void WINAPI IWineD3DTextureImpl_CleanUp(IWineD3DTexture *iface) { IWineD3DTextureImpl *This = (IWineD3DTextureImpl *)iface; int i; for (i = 0; i < This->baseTexture.levels; ++i) { IWineD3DSurface_Cleanup(This->surfaces[i]); }
/* Do some cleaning for ourselves */
This->parentCleanUp(This->parent); }
/* IDirect3DSurface9 */ ULONG WINAPI IDirect3DSurface9Impl_Release(IDirect3DSurface9 *iface) { IDirect3DSurface9Impl *This = (IDirect3DSurface9Impl *)iface; return IWineD3DSurface_Release(This->wineD3DSurface); }
void WINAPI IDirect3DSurface9Impl_CleanUp(IDirect3DSurface9 *iface) { IDirect3DSurface9Impl *This = (IDirect3DSurface9Impl *)iface; /* Cleanup Some stuff */ }
/* IDirect3DTexture9 */ ULONG WINAPI IDirect3DTexture9Impl_Release(IDirect3DTexture9 *iface) { IDirect3DTexture9Impl *This = (IDirect3DTexture9Impl *)iface; return IWineD3DTexture_Release(This->wineD3DTexture); }
void WINAPI IDirect3DTexture9Impl_CleanUp(IDirect3DTexture9 *iface) { IDirect3DTexture9Impl *This = (IDirect3DTexture9Impl *)iface; /* Cleanup Some stuff */ }
AddRef would follow a similar pattern, although somewhat simpler since it doesn't have to handle cleaning up the object.
Hi,
Are you talking about IDirect3DSurface9 <=> IDirect3DTexture9 or do you mean IDirect3DSurface9 <=> IWineD3DSurface?
I meant IWineD3DSurface <=> IWineD3DTexture and Parent(IWineD3DSurface) <=> Parent(IWineD3DTexture). The first case has no consequences for ddraw, if the secound one is forced by WineD3D this would cause a connection between IDirect3DSurface9 <=> IDirect3DTexture9 and between {Direct Draw texture root surface} <=> {Other ddraw texture surfaces}
Unfortunately my knowledge of ddraw and d3d7 is somewhat limited, so it's a bit hard for me to oversee all the implications those changes would have for ddraw / d3d7. The choice for Surface and Texture as an example is somewhat arbitrary I guess, but probably the one for which it matters the most. (As in, is most likely to break stuff if it isn't implemented correctly). In principle it would change for any object that has a container or uses similar construction. Perhaps it would even make sense to forward AddRef and Release calls for all d3d7/8/9 objects that directly wrap a wined3d object.
For reference, the code for Release & CleanUp would look something like this for the different objects:
I could deal with such a Release / cleanup difference in ddraw, but I don't like the idea. I think it creates a too complex connection between WineD3D and the d3d libs. My d3d9 knowledge is limited too, but what if you do something like this: (sort of pseude-code only)
IDirect3DSurface9::AddRef { if(This->container /* Or get the container from WineD3D */) { return IUnknown_AddRef(This->container); } else { ULONG ref = This->ref++; etc } }
IDirect3DSurface9::Releae { if(This->container /* Or get the container from WineD3D */) { return IUnknown_Release(This->container); } else { ULONG ref = This->ref--;
if(ref == 0) { /*Destroy code*/ IWineD3DSurface_Release(This->wineD3DSurface); } return ref; } }
IDirect3DTexure9::AddRef { /* normal addref, nothing special */ }
IDirect3DTexure9::Release { ULONG ref = This->ref--;
if(ref == 0) { for(all surfaces in this container) { surfaceImpl->container = NULL; /* Or place this call in WineD3DTexture::Releaase*/
IWineD3DTexture_Release(This->wineD3DTexture); /* IWineD3DTexture_Release will call the release method * of the IDirect3D9Surface(because it releases the parent), * run the surface's release code(not the container, because * it's set to NULL and destroy the surface, because it's own * refcount is 1 from creation, and we never changed it. * The D3D9 surface release code releases the wineD3DSurface, * and everthing is cleaned. */
HeapFree(GetProcessHeap, 0, This); } } }
The important part is to set the surface's container when they are created, before the first AddRef() or Release() is called, and to set the container to NULL when the container is destroyed and before the surfaces are released. You can eighter track the containers in d3d9, or use wineD3D to get the container.
Could this work?
Stefan
Hi
IDirect3DTexure9::Release { ULONG ref = This->ref--;
if(ref == 0) { for(all surfaces in this container) { surfaceImpl->container = NULL; /* Or place this call in WineD3DTexture::Releaase*/
IWineD3DTexture_Release(This->wineD3DTexture); /* IWineD3DTexture_Release will call the release method * of the IDirect3D9Surface(because it releases the parent), * run the surface's release code(not the container, because * it's set to NULL and destroy the surface, because it's own * refcount is 1 from creation, and we never changed it. * The D3D9 surface release code releases the wineD3DSurface, * and everthing is cleaned. */
HeapFree(GetProcessHeap, 0, This); } } }
Oops, little problem here: That should be
IDirect3DTexure9::Release { ULONG ref = This->ref--; if(ref == 0) { for(all surfaces in this container) { surfaceImpl->container = NULL; /* Don't free the objects in this loop of course ;) */ }
/* Or place this call in WineD3DTexture::Releaase*/ IWineD3DTexture_Release(This->wineD3DTexture);
/* IWineD3DTexture_Release will call the release method * of the IDirect3D9Surface(because it releases the parent), * run the surface's release code(not the container, because * it's set to NULL and destroy the surface, because it's own * refcount is 1 from creation, and we never changed it. * The D3D9 surface release code releases the wineD3DSurface, * and everthing is cleaned. */ HeapFree(GetProcessHeap, 0, This); } }
On 12/02/06, Stefan Dösinger stefandoesinger@gmx.at wrote:
I meant IWineD3DSurface <=> IWineD3DTexture and Parent(IWineD3DSurface) <=> Parent(IWineD3DTexture). The first case has no consequences for ddraw, if the secound one is forced by WineD3D this would cause a connection between IDirect3DSurface9 <=> IDirect3DTexture9 and between {Direct Draw texture root surface} <=> {Other ddraw texture surfaces}
Connecting the parents' refcounts wasn't really the idea, at least not directly. I think it's probably a good idea to have as little logic in the wrappers as possible. However, they are of course indirectly connected through wined3d; The connection between IDirect3DSurface9 and IDirect3DTexture9 is the reason for doing it in the first place.
For reference, the code for Release & CleanUp would look something like this for the different objects:
I could deal with such a Release / cleanup difference in ddraw, but I don't like the idea. I think it creates a too complex connection between WineD3D and the d3d libs.
I'm not sure that connection is much more complex than the way it currently is. Right now the d3d7/8/9 wrapper objects are responsible for cleaning up their wined3d object, while with that change the wined3d object would become responsible for deleting the d3d7/8/9 objects.
The important part is to set the surface's container when they are created, before the first AddRef() or Release() is called, and to set the container to NULL when the container is destroyed and before the surfaces are released. You can eighter track the containers in d3d9, or use wineD3D to get the container.
Could this work?
It probably would. However, it's not all that different from the other option. The main difference seems to be that you don't separate the CleanUp and Release code, but use the parent in combination with a reference count that's guaranteed to be one as some sort of flag to decide whether to do the entire release code or just the cleanup part. That, and in order the use the container in that way you either have to store it somewhere in d3d7/8/9 or retrieve it on every call to Release. I'm not sure if that reduces the complexity all that much. Regardless of whether separating the Release and CleanUp code would make things a lot more complex, I do think it's somewhat cleaner. Also, note that most (all?) wined3d objects already have a CleanUp function.
Connecting the parents' refcounts wasn't really the idea, at least not directly. I think it's probably a good idea to have as little logic in the wrappers as possible. However, they are of course indirectly connected through wined3d; The connection between IDirect3DSurface9 and IDirect3DTexture9 is the reason for doing it in the first place.
Right, I can't imagine how a IWineD3DTexture <=> IWineD3DSurface connection can help you, except if you use that connection to build the Direct3D9Texture <=> Direct3D9Surface connection.
I'm not sure that connection is much more complex than the way it currently is. Right now the d3d7/8/9 wrapper objects are responsible for cleaning up their wined3d object, while with that change the wined3d object would become responsible for deleting the d3d7/8/9 objects.
Err, the WineD3D objects clean themselves, it's the duty of the d3d7/8/9 objects the release them if they are not needed any more. If the WineD3D refcount falls to 0 then(the general case), the WineD3D object will clean itself. If the object is in use somewhere internally in WineD3D, it's destroyed as soon WineD3D releases that refcount.
The "exception" is when WineD3D asked d3d7/8/9 to create objects via callbacks(e.g. the callback during texture creation), then wineD3D releases the parents of the objects it created. The parents take care for cleaning up, and they release their WineD3D child.
It probably would. However, it's not all that different from the other option. The main difference seems to be that you don't separate the CleanUp and Release code, but use the parent in combination with a reference count that's guaranteed to be one as some sort of flag to decide whether to do the entire release code or just the cleanup part.
I personally think that we should stay withhin the possibilities that COM gives us, instead of adding cross-library destroy callbacks. Why? Read the current ddraw surface creation code and the surface release code, and follow the callbacks between the different ddraw / surface types.
That, and in order the use the container in that way you either have to store it somewhere in d3d7/8/9 or retrieve it on every call to Release. I'm not sure if that reduces the complexity all that much. Regardless of whether separating the Release and CleanUp code would make things a lot more complex, I do think it's somewhat cleaner. Also, note that most (all?) wined3d objects already have a CleanUp function.
Well, at last it's up to Alexandre to decide which way to take.
I also think that seperated release / cleanup code makes it easy to do sloopy and incorrect refcounting. The ddraw code is full of bad surface reference counting, which is cascaded by a bad reference counting in ddraw, which works for most games, but breaks e.g. Empire Earth.
I'm not sure that connection is much more complex than the way it currently is. Right now the d3d7/8/9 wrapper objects are responsible for cleaning up their wined3d object, while with that change the wined3d object would become responsible for deleting the d3d7/8/9 objects.
Err, the WineD3D objects clean themselves, it's the duty of the d3d7/8/9 objects the release them if they are not needed any more. If the WineD3D refcount falls to 0 then(the general case), the WineD3D object will clean itself. If the object is in use somewhere internally in WineD3D, it's destroyed as soon WineD3D releases that refcount.
I should have said "releasing", of course :)
It probably would. However, it's not all that different from the other option. The main difference seems to be that you don't separate the CleanUp and Release code, but use the parent in combination with a reference count that's guaranteed to be one as some sort of flag to decide whether to do the entire release code or just the cleanup part.
I personally think that we should stay withhin the possibilities that COM gives us, instead of adding cross-library destroy callbacks. Why? Read the current ddraw surface creation code and the surface release code, and follow the callbacks between the different ddraw / surface types.
That, and in order the use the container in that way you either have to store it somewhere in d3d7/8/9 or retrieve it on every call to Release. I'm not sure if that reduces the complexity all that much. Regardless of whether separating the Release and CleanUp code would make things a lot more complex, I do think it's somewhat cleaner. Also, note that most (all?) wined3d objects already have a CleanUp function.
Well, at last it's up to Alexandre to decide which way to take.
I also think that seperated release / cleanup code makes it easy to do sloopy and incorrect refcounting. The ddraw code is full of bad surface reference counting, which is cascaded by a bad reference counting in ddraw, which works for most games, but breaks e.g. Empire Earth.
Well, I agree it's not a great construction. I'm just not sure if using the container in that way makes things much better. It would mean having to retrieve the surface container in each call to IDirect3D9Surface_Release. You can't easily store it in d3d9 since when the wined3d surface's container changes the d3d9 container would have to change as well, but you can't call much of anything in d3d9 without adding another callback. Also, you're now duplicating the relation between a surface and it's container in d3d9, while I don't think d3d9 should know that much about the specific implementation in wined3d. That doesn't mean I couldn't live with going with that solution, just that I'm not sure what the advantage would be.
Hi,
Well, I agree it's not a great construction. I'm just not sure if using the container in that way makes things much better. It would mean having to retrieve the surface container in each call to IDirect3D9Surface_Release.
Do you think it might be a performance problem?
You can't easily store it in d3d9 since when the wined3d surface's container changes the d3d9 container would have to change as well, but you can't call much of anything in d3d9 without adding another callback.
Can containers change? I thought that the container is set at creation time and can't change?
Also, you're now duplicating the relation between a surface and it's container in d3d9, while I don't think d3d9 should know that much about the specific implementation in wined3d. That doesn't mean I couldn't live with going with that solution, just that I'm not sure what the advantage would be.
I think that we can't avoid keeping certain management things in d3d7/8/9 because of the differences between the versions.
I've had a look at your proposal again, and I could handle it in ddraw. The only problem is that I have to be able to release(and destroy) the wined3dSurface without having the ddraw surface destroyed. This happens when I switch the surface implementation, or when I lose the primary surface due to a screen resolution change(Restoring the surface re-creates the WineD3DSurface in this case). The best solution for this would be to allow the cleanup CB to be NULL.
My opinion is to use the container in the d3d9 addref and release function, as it uses only methods that COM and WineD3D already have, and both d3d9 surfaces and wined3d surfaces are valid COM objects(they have their own refcount) and we avoid callbacks. Furthermore the d3d9 and wined3d objects are clearly seperated.
What's the opinion of the other directx people? Lionel? Oliver?
On 13/02/06, Stefan Dösinger stefandoesinger@gmx.at wrote:
Hi,
Well, I agree it's not a great construction. I'm just not sure if using the container in that way makes things much better. It would mean having to retrieve the surface container in each call to IDirect3D9Surface_Release.
Do you think it might be a performance problem?
I'm not sure, probably not that much, but it depends on whether the AddRef / Release functions are called a lot. If we use GetContainer, it would make a call to QueryInterface. If the surface hasn't got a container it makes a call to GetDevice and returns the wined3d device. We'd have to account for that as well. Or we could write a function to just retrieve the pointer. We would of course also have to get the d3d9 parent of the container.
You can't easily store it in d3d9 since when the wined3d surface's container changes the d3d9 container would have to change as well, but you can't call much of anything in d3d9 without adding another callback.
Can containers change? I thought that the container is set at creation time and can't change?
I'm not sure if it happens or not (and if it is actually required when it happens :)), but the function to change it is there. Of course if we can be sure it never happens that would mean we could just store the pointer to the container in d3d9 during creation time and set it to NULL when releasing the container.
I'm not sure, probably not that much, but it depends on whether the AddRef / Release functions are called a lot. If we use GetContainer, it would make a call to QueryInterface. If the surface hasn't got a container it makes a call to GetDevice and returns the wined3d device. We'd have to account for that as well. Or we could write a function to just retrieve the pointer. We would of course also have to get the d3d9 parent of the container.
I think the returned device pointer is not a problem - just check the returned pointer against your device instead of NULL. As for GetParent / QueryInterface / GetContainer calls, it's necessary to release any unwanted increased refcount. If however, a GetContainer call for an D3D9 texture causes an AddRef or Release of that texture, the approach is impossible. I didn't find any evidence of that yet.
I'm not sure if it happens or not (and if it is actually required when it happens :)), but the function to change it is there. Of course if we can be sure it never happens that would mean we could just store the pointer to the container in d3d9 during creation time and set it to NULL when releasing the container.
That would make everything much easier :)
I'd suggest that you send a patch taking whatever way you like, and ask AJ for his opinion.
On 13/02/06, Stefan Dösinger stefandoesinger@gmx.at wrote:
I'd suggest that you send a patch taking whatever way you like, and ask AJ for his opinion.
If nobody else has a real opinion on the subject, I'll probably just implement your idea, and see what problems I'll run into.
-
H. Verbeet -
Stefan Dösinger