Re: [dx9-12] Major change to desgin

[Alexandre Julliard]

"Ann and Jason Edmeades"  writes:
> I think this patch should keep you happy (It doesn't do any of the
> IWineUserDevice stuff as the code isn't ready for it, but does the dual ref
> count + adds Set/Get Stream source). Let me know if you have problems with
> it.

Looks much better yes, thanks. There's still something strange here:
> - /* Not recording... */
> - if (oldSrc != NULL) {
> -
>    IWineD3DVertexBuffer_Release(oldSrc);
> -
>    IUnknown_Release((IUnknown *)((IWineD3DVertexBufferImpl *)oldSrc)->resource.parent);  /* Released an internal reference to a VB */
> - }
> - if (pStreamData != NULL) {
> -
>    IWineD3DVertexBuffer_AddRef(pStreamData);
> -
>    IUnknown_AddRef((IUnknown *)((IWineD3DVertexBufferImpl *)pStreamData)->resource.parent); /* Gained an internal reference to a VB */
> - }

You shouldn't need to AddRef the parent, that's up to the object that
holds the pointer (also you should do the AddRef before the Release in
case old and new are the same object).

[dx9-12] Major change to design

[Ann and Jason Edmeades]

> Looks much better yes, thanks.

Thanks!
> There's still something strange here:
> > - /* Not recording... */
> > - if (oldSrc != NULL) {
> > -
> >    IWineD3DVertexBuffer_Release(oldSrc);
> > -
> >    IUnknown_Release((IUnknown *)((IWineD3DVertexBufferImpl

*)oldSrc)->resource.parent);  /* Released an internal reference to a VB */
> > - }
> > - if (pStreamData != NULL) {
> > -
> >    IWineD3DVertexBuffer_AddRef(pStreamData);
> > -
> >    IUnknown_AddRef((IUnknown *)((IWineD3DVertexBufferImpl

*)pStreamData)->resource.parent); /* Gained an internal reference to a VB */
> > - }
> You shouldn't need to AddRef the parent, that's up to the object that
> holds the pointer (also you should do the AddRef before the Release in
> case old and new are the same object).

Well, that's where I come unstuck :-)

The problem is that in the caller, I could do the addref, but I cant do the
release because the caller doesn't know nor have access to (nor should know
/ have access to!) the vb which it is replacing. It made more 'sense' to do
both in the same place because it is then 'obvious' when one is wrong.

In this case (very pseudo code!)

D3D9 SetStreamSource (idx, d3d9_vb):
   Call WineD3D SetStreamSource(idx, d3d9_vb->wined3d_vb)

WineD3D SetStreamSource:
   In an array store the new vb and addref, and release the old one.

If I just addref/release the wined3d interface, I am not maintaining correct
reference counting for the d3d9 object (Think: Create, SetStreamSource,
Release. We would release the d3d9 one when we shouldn't).

The alternative is to pass back the thing I have freed, which is both very
unintuitive and also may cause problems later on when the same function
needs calling from within wined3d. It also means the caller needs to know
more of the internals whereas I have tried to keep the d3d9 code as trivial
as possible.

Suggestions welcome - I spent ages trying to get my head around how to keep
the dual reference counting correct, and that's the only model I cam up with
which worked!

Jason

[dx9-12] Major change to design

[Alexandre Julliard]

"Ann and Jason Edmeades"  writes:
> In this case (very pseudo code!)
>
> D3D9 SetStreamSource (idx, d3d9_vb):
>    Call WineD3D SetStreamSource(idx, d3d9_vb->wined3d_vb)
>
> WineD3D SetStreamSource:
>    In an array store the new vb and addref, and release the old one.
>
> If I just addref/release the wined3d interface, I am not maintaining correct
> reference counting for the d3d9 object (Think: Create, SetStreamSource,
> Release. We would release the d3d9 one when we shouldn't).

Yes, but that should be up to the wined3d_vb object then, if it holds
a pointer it has to maintain the reference count; users of wined3d_vb
don't have to know that it contains a parent pointer.

[dx9-12] Major change to design

[Ann and Jason Edmeades]

Hi, Firstly apologies for the long delay between responses - I had to take a
week out to sort something else out!

Anyway, where were we! I'll start with a summary, feel free to skip near the
bottom if you remember it!

You last said:
> > Looks much better yes, thanks. There's still something strange here:
> > - /* Not recording... */
> > - if (oldSrc != NULL) {
> > -
> >    IWineD3DVertexBuffer_Release(oldSrc);
> > -
> >    IUnknown_Release((IUnknown *)((IWineD3DVertexBufferImpl
>
> *)oldSrc)->resource.parent);  /* Released an internal reference to a VB */
> > - }
> > - if (pStreamData != NULL) {
> > -
> >    IWineD3DVertexBuffer_AddRef(pStreamData);
> > -
> >    IUnknown_AddRef((IUnknown *)((IWineD3DVertexBufferImpl

*)pStreamData)->resource.parent); /* Gained an internal reference to a VB */
> > - }

Because you didn't like me addrefing the parent during a function of the
child. I responded...
> The problem is that in the caller, I could do the addref, but I cant do the
> release because the caller doesn't know nor have access to (nor should know
> / have access to!) the vb which it is replacing. It made more 'sense' to do
> both in the same place because it is then 'obvious' when one is wrong.
> > In this case (very pseudo code!)
> >
> > D3D9 SetStreamSource (idx, d3d9_vb):
> >    Call WineD3D SetStreamSource(idx, d3d9_vb->wined3d_vb)
> >
> > WineD3D SetStreamSource:
> >    In an array store the new vb and addref, and release the old one.
> >
> > If I just addref/release the wined3d interface, I am not maintaining

correct
> > reference counting for the d3d9 object (Think: Create, SetStreamSource,
> > Release. We would release the d3d9 one when we shouldn't).

To which you said
> Yes, but that should be up to the wined3d_vb object then, if it holds
> a pointer it has to maintain the reference count; users of wined3d_vb
> don't have to know that it contains a parent pointer.

And this is where I am stuck...

My only 'solution' is for AddRef in the wined3d code (eg. The
wined3d_vertexbuffer object) to also generically addref the parent (eg. The
direct3dvertexbuffer8 object), and hence the SetStreamSource just addrefs
the wined3d object (which would in turn addref the d3dx object) - which I
viewed as a more complex solution than that above. It also causes problems
because release the wined3d object needs to call release of the d3dx object,
which may in turn (once it gets to 0) re-call release of the wined3d object
(I know this is handleable by putting the code in the right order.

Is this what you mean or am I missing the point.

Thanks
Jason

This is the point at which I started... How!

Are you saying that when I call addref the wined3dvb interface, I need to
addref on the d3dvb interface, and similarly on release, call release on the
d3d interface from release on the wined3dvb?

If I do that are there any ordering issues (esp. release which will end up
circular). I suspect it should be:

Wined3dvb addref:
   Call d3dvb addref
   Return InterlockedIncrement

Wined3dvb release
    Call d3dvb release (Note this could result in a call back to this fn,
but that should be ok)
    InterlockedDecrement
    If refcount == 0
        Free as usual
    endif

This will fail when a VB is created and never goes through SetStreamSource.
If I set them up 'correctly' then d3dvb has a ref count of 2 (one ptr to
user, one ptr in wined3d interface), wined3d has