Wrong handling of reference counter?

[GoogleCodeExporter]

I think the reference counting is not handled right in the surface
bindings. I don't know if this is also an issue in other classes, but it
might be.

My problem is that I save the current rendertarget in one place of my code
and restore it later in case I changed it somewhere.

It's like:
store = GetRenderTarget
Draw
SetRenderTarget = store

In the draw function I get a reference to the current backbuffer, which is
the same surface as the stored rendertarget. After using the backbuffer I
Dispose() the pointer to decrease the reference counter again.

When I restore the saved rendertarget my app crashes. Debugging the issue I
found the internal mUnknown of the stored rendertarget surface was set to 0.

I think in the code (SlimDX, surface.cpp)...

    Surface^ Surface::FromPointer( IDirect3DSurface9* pointer )
    {
        if( pointer == 0 )
            return nullptr;

        Surface^ tableEntry = safe_cast<Surface^>( ObjectTable::Find(
static_cast<IntPtr>( pointer ) ) );
        if( tableEntry != nullptr )
        {
            pointer->Release();
            return tableEntry;
        }

        return gcnew Surface( pointer );
    }

... the "pointer->Release()" should not be there. As I'm "cleaning up" the
reference counter myself anytime I get a surface, with "pointer->Release()"
in Surface::FromPointer() the surface gets released too often and is freed.

Even if I'm getting more than one reference to the same object, the
reference counter has to be increased and I'm responsible for decreasing it
again using Dispose().

Test case (copied from my code, mD3d is a DX9 Device):

Surface mRenderTarget;
mRenderTarget = mD3D.GetRenderTarget( 0 );

Surface dest = mD3D.GetBackBuffer( 0, 0 ); // <-- Gets backbuffer and
DECREASES REFCOUNT WRONGLY
// here I'm doing something that needs a reference to the backbuffer
dest.Dispose();
dest = null;

mD3D.SetRenderTarget( 0, mRenderTarget ); // <-- BOOM :)
if( mRenderTarget != null )
  mRenderTarget.Dispose();
mRenderTarget = null;

I can't remove the "dest.Dispose()", because in some cases, when I'm not
saving the rendertarget, I have to do it to balance the refcount.

Original issue reported on code.google.com by michael.stoyke@gmx.de on 26 Oct 2008 at 3:48

[GoogleCodeExporter]

After debugging some more I think the problem is even worse.

I took a closer look at "ComObject::Destruct()", as ComObject is used for all DX
resources.

It's here:
m_Unknown->Release();
m_Unknown = NULL;

After releasing my first reference, m_Unknown is set to NULL. But as it's 
pointing to
the same ComObject as my second reference, m_Unknown is also NULL there.

I understand you need to lookup the objects in the ObjectTable to always 
provide the
same managed wrapper object for the same unmanaged DX object. The only solution 
I see
at the moment is doing "your own" refcounting in the ComObject class.

A solution might be (in pseudocode):

m_Unknown->Release();
if(m_Unknown.IsFreedBecauseRefCountIsZero())
  m_Unknown = NULL;

I hope you can understand the problem as I explained it here, english is not my
native language...

Refcounting has always been a nightmare to deal with, that's why I don't like 
COM :)

Original comment by michael.stoyke@gmx.de on 26 Oct 2008 at 4:31

[GoogleCodeExporter]

Dispose() does not "decrease the reference count" -- rather, while the code 
path may 
call Release(), that's an implementation detail and not the documented 
behavior. 
Dispose() is what you do to an object you created (with "new") when you are no 
longer using it.

You should only Dispose() what you explicitly create, so the fact that you are 
Dispose()ing the result of GetBackBuffer() is an error. If you have a case 
where 
omitting the Dispose() call creates a problem, then that is a bug and you 
should 
file a seperate bug report with the details.

See these blog entries for more detail:
http://scientificninja.com/development/com-and-slimdx-part-i
http://scientificninja.com/development/com-and-slimdx-part-ii

Original comment by josh.petrie on 26 Oct 2008 at 4:54

• Changed state: Invalid

[GoogleCodeExporter]

Thanks for pointing me to the blog, I should've known about that earlier, would 
have
stopped me from writing this "bug report" :)

Now I understand the design decisions for refcounters in SlimDX. After thinking 
about
it for some time I think the way you do it is much better than the "DirectX 
way".
I'll change my code and want to check if I'm still experiencing leaked 
resources,
like I did before using Dispose everywhere.

If there are more online resources about SlimDX besides the wiki and the blog, 
I'd
like to kindly ask you to send me the links (my username is also my email 
address).

I'm sorry for giving you guys so much trouble today, but I just started using 
SlimDX
yesterday and it looked to me like "just a plain 1:1 wrapper of DirectX for 
.NET".

I'm usually not rushing into using a "new" library that I have little knowledge
about, but I want to convince our team of using SlimDX instead of MDX for our 
next
project. The only way to achieve this is to have something up and running by 
monday :)

Original comment by michael.stoyke@gmx.de on 26 Oct 2008 at 5:54

[GoogleCodeExporter]

The next version of the documentation that I'm working on tries to call a lot 
of 
this stuff out, rather than just being a plain class reference. I'm aiming to 
have 
the preliminary version of that finished for the November release, so hopefully 
this 
sort of thing will be more clear than it has in the past. I realize that right 
now 
things are very much not ideal.

Original comment by promit....@gmail.com on 26 Oct 2008 at 1:36