WineD3D refcounting fun part II :)

[Roderick Colenbrander]

Hi,

During the d3d8 -> wined3d transition I noticed one huge refcounting issue 
which affects both d3d8 and d3d9. The issue is illustrated by running 
http://www.codesampler.com/source/dx9_multitexture.zip (the same demo is 
available for dx8) and then modify wined3d's globalChangeGlRam in 
wined3d_main.c to let it output how it adjusts the memory.

If you run the test on d3d9 or using oliver's full d3d8 patch, you'll notice 
that not all memory is returned when quitting the program. (In 3dmark2001 
this is a big issue as because of this we are out of memory after running a 
few demos) The problem appears that wined3d surfaces contain a reference to 
the wined3d device. So when you run a game the d3d8 and wined3d device 
refcounts are out of sync (the wined3d refcount is a lot higher because of 
all the refcounts from the surfaces).

At the moment you quit a game the d3d8 device refcount nicely becomes zero and 
the object tries to kill itself. The release function also calls the wined3d 
device release function but the wined3d device object won't kill itself 
because its refcount doesn't become zero due to all the surfaces which still 
contain a reference to it. (the surfaces which useally are front-, 
backbuffers and more are normally released in the wined3d device release call 
when the refcount becomes zero)

One way to 'fix' the problem is to let d3d8/d3d9 handle the reference counts 
(remove the refcounts from wined3d; and let d3d8/d3d9 call some cleanup 
function to remove the garbage in case its refcount becomes zero)

Does anyone have a better solution?

Roderick

[H. Verbeet]

O right. From the conversation on IRC I got the impression the issue
was the application calling the device's Release before calling the
surface's Release. Similar to the Surface / Texture issue. On windows
that should work, on wine it will probably fail at the moment.

Releasing the device after all its children are deleted should work
though, and it might simply be a matter of the object (be it a surface
or something else) not releasing its references to the device properly
upon deletion.

I also suspect there might be a couple of circular references in the
form of device->stateblock->object->device, but I haven't had time to
really look into that yet. I *think* wined3d shouldn't keep references
to objects in the device's Set* methods, but I remember that when I
removed them in IWineD3DDeviceImpl_SetVertexShader Oliver put them
back again in a later patch for some reason.

[Robert Shearman]

H. Verbeet wrote:
> I also suspect there might be a couple of circular references in the
> form of device->stateblock->object->device, but I haven't had time to
> really look into that yet. I *think* wined3d shouldn't keep references
> to objects in the device's Set* methods, but I remember that when I
> removed them in IWineD3DDeviceImpl_SetVertexShader Oliver put them
> back again in a later patch for some reason.

It would be good to write out on paper or otherwise the hierarchy of 
which lifetimes a given object controls so that you can be sure of this. 
There should be no loops.

I am not at all familiar with the wined3d code, but from what I've seen 
discussed here it would seem that wined3d shouldn't keep references to 
any objects. Instead the other objects should release the corresponding 
wined3d object when they are destroyed.

-- 
Rob Shearman

[H. Verbeet]

On 24/02/06, Robert Shearman <rob(a)codeweavers.com> wrote:
> I am not at all familiar with the wined3d code, but from what I've seen
> discussed here it would seem that wined3d shouldn't keep references to
> any objects. Instead the other objects should release the corresponding
> wined3d object when they are destroyed.

I don't think it does, generally. d3d8/9 objects usually contain a
wined3d object that they wrap, and when the reference count of the
d3d8/9 object reaches 0 it releases its wined3d object. That works
fine as long as the is no relation between the wined3d objects. The
moment a wined3d object keeps a reference to another wined3d object
there is a problem though, because the d3d8/9 object that wraps that
object can be released while there are wined3d objects that still keep
a reference to that wined3d object.

What I mentioned to Roderick on IRC was not really doing refcounts on
wined3d objects, but on the wrapper objects instead. Essentially
calling AddRef on a wined3d surface would just increase the reference
count on the d3d8/9 surface. When the d3d8/9 object's refcount reaches
0 it just calls the wined3d object's cleanup function directly instead
of the release.
> From Roderick's post I don't get the impression that that particular
> issue is the problem here though.

[Stefan Dösinger]

Hi,
> If you run the test on d3d9 or using oliver's full d3d8 patch, you'll
> notice that not all memory is returned when quitting the program. (In
> 3dmark2001 this is a big issue as because of this we are out of memory
> after running a few demos) The problem appears that wined3d surfaces
> contain a reference to the wined3d device. So when you run a game the d3d8
> and wined3d device refcounts are out of sync (the wined3d refcount is a lot
> higher because of all the refcounts from the surfaces).
> Are you sure that the Direct3DDevice9 should be released? Windows addrefs the 
> DirectDraw interface when a surface is created, I'd be surprised if Microsoft 
> changed that behavior in d3d8 or d3d9. From your description I'd say that 
> there are still Direct3D9 surfaces hanging, which shouldn't happen.

It could be that some surfaces(or textures) are in use by the WineD3DDevice, 
for example they are set as textures with IWineD3DDevice::SetTexture. 
Settexure addrefs the texture interface, which makes sense and is done by 
Windows too. This means that the textures must be unset before the surfaces 
and the device can be destroyed, if an app doesn't do so, I'd consider it 
buggy.

I'll look at the multitexture demo, and I'll get an IRC client running. I've 
never used IRC before(shame on me), but I think it makes discussing this 
easier.

Stefan

[H. Verbeet]

On 24/02/06, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
> Are you sure that the Direct3DDevice9 should be released? Windows addrefs the
> DirectDraw interface when a surface is created, I'd be surprised if Microsoft
> changed that behavior in d3d8 or d3d9. From your description I'd say that
> there are still Direct3D9 surfaces hanging, which shouldn't happen.
> I assume Roderick is talking about the final Release on the d3d
> device, ie, the one you got from CreateDevice. The surfaces are
> holding a couple of references to the device, and those should get
> released when the surface is destroyed, yes. But the application is
> also holding a single reference to de device.

Looking at the trace from the multitexture demo, it appears
GetContainer sometimes adds an extra reference to the device. I've
submitted a patch for this to wine-patches.

[Stefan Dösinger]

Hi,
> From the multitexture demo:
> void loadTexture( void )
> {
>   D3DXCreateTextureFromFile( g_pd3dDevice, "test.bmp", &g_pTexture_0 );
>   D3DXCreateTextureFromFile( g_pd3dDevice, "checker.bmp", &g_pTexture_1 );
>
> g_pd3dDevice->SetSamplerState(0, D3DSAMP_MINFILTER, D3DTEXF_LINEAR);
>   g_pd3dDevice->SetSamplerState(0, D3DSAMP_MAGFILTER, D3DTEXF_LINEAR);
> }
> Two textures are created
> //-----------------------------------------------------------------------------
> // Name: shutDown()
> // Desc: 
> //-----------------------------------------------------------------------------
> void shutDown( void )
> {
>     if( g_pTexture_0 != NULL ) 
>         g_pTexture_0->Release();
>
> if( g_pVertexBuffer != NULL )
>         g_pVertexBuffer->Release();
>
> if( g_pd3dDevice != NULL )
>         g_pd3dDevice->Release();
>
> if( g_pD3D != NULL )
>         g_pD3D->Release();
> }
> One texture is released
> void render( void )
> {
> <snip>
>     g_pd3dDevice->SetTexture( 0, g_pTexture_0 );
>   g_pd3dDevice->SetTexture( 1, g_pTexture_1 );
> The texture stage is set -> Texure AddRef. The texture isn't unset.

The missing release of the 2nd texture is definitly an application bug. What 
remains open is if not unsetting the texture stages is a bug too. Can someone 
check on Windows if (a) creating a surface or texture addrefs the 
Direct3DDevice, and if (b) assigning a texture to a texture stage addrefs the 
Texture? If (a) isn't true, we should unset all texture stages when the 
WineD3DDevice is released, if (b) is false we shouldn't do that too. If (a) 
and (b) are true, the application is buggy too. I'm sorry that I can't test, 
because I've no D3D9-Equipped windows machine available ATM.

[H. Verbeet]

On 24/02/06, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
> remains open is if not unsetting the texture stages is a bug too. Can someone
> check on Windows if (a) creating a surface or texture addrefs the
> Direct3DDevice, and if (b) assigning a texture to a texture stage addrefs the
> Texture? If (a) isn't true, we should unset all texture stages when the
> WineD3DDevice is released, if (b) is false we shouldn't do that too. If (a)
> and (b) are true, the application is buggy too. I'm sorry that I can't test,
> because I've no D3D9-Equipped windows machine available ATM.
> CreateTexture adds a reference to the device. SetTexture doesn't add a
> reference to the texture.