Karsten Elfenbein wrote:
- don't prevent IUnknown_Release of pDecl in the exit section and fix
This seems wrong, d3d9 does not keep an internal reference of the vdecl. Try the attached patch instead, let me know if it works...
--- dlls/d3d9/d3d9_private.h | 4 ++++ dlls/d3d9/device.c | 1 + dlls/d3d9/vertexdeclaration.c | 6 +++++- 3 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/dlls/d3d9/d3d9_private.h b/dlls/d3d9/d3d9_private.h index 2d6397e..c5cef9f 100644 --- a/dlls/d3d9/d3d9_private.h +++ b/dlls/d3d9/d3d9_private.h @@ -176,6 +176,10 @@ typedef struct IDirect3DDevice9Impl /* IDirect3DDevice9 fields */ IWineD3DDevice *WineD3DDevice;
+ /* A vertex declaration was converted from setFVF. + * Keep track of it, so it can be properly freed */ + IDirect3DVertexDeclaration9 *convertedDecl; + } IDirect3DDevice9Impl;
diff --git a/dlls/d3d9/device.c b/dlls/d3d9/device.c index 6eeaceb..185a7ef 100644 --- a/dlls/d3d9/device.c +++ b/dlls/d3d9/device.c @@ -731,6 +731,7 @@ HRESULT WINAPI IDirect3DDevice9Impl_Se
hr = IDirect3DDevice9Impl_SetVertexDeclaration(iface, pDecl); if (hr != S_OK) goto exit; + This->convertedDecl = pDecl; pDecl = NULL;
exit: diff --git a/dlls/d3d9/vertexdeclaration.c b/dlls/d3d9/vertexdeclaration.c index 246b52f..88f5909 100644 --- a/dlls/d3d9/vertexdeclaration.c +++ b/dlls/d3d9/vertexdeclaration.c @@ -300,8 +300,12 @@ HRESULT WINAPI IDirect3DDevice9Impl_Se IDirect3DVertexDeclaration9Impl *pDeclImpl = (IDirect3DVertexDeclaration9Impl *)pDecl; HRESULT hr = D3D_OK;
- TRACE("(%p) : Relay\n", iface); + if (This->convertedDecl && This->convertedDecl != pDecl) { + IUnknown_Release(This->convertedDecl); + This->convertedDecl = NULL; + }
+ TRACE("(%p) : Relay\n", iface); hr = IWineD3DDevice_SetVertexDeclaration(This->WineD3DDevice, pDeclImpl == NULL ? NULL : pDeclImpl->wineD3DVertexDeclaration);
return hr;
Ivan Gyurdiev wrote:
Karsten Elfenbein wrote:
- don't prevent IUnknown_Release of pDecl in the exit section and fix
This seems wrong, d3d9 does not keep an internal reference of the vdecl. Try the attached patch instead, let me know if it works...
...better yet, try this one
--- dlls/d3d9/d3d9_private.h | 4 ++++ dlls/d3d9/device.c | 3 +++ dlls/d3d9/vertexdeclaration.c | 6 +++++- 3 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/dlls/d3d9/d3d9_private.h b/dlls/d3d9/d3d9_private.h index 2d6397e..c5cef9f 100644 --- a/dlls/d3d9/d3d9_private.h +++ b/dlls/d3d9/d3d9_private.h @@ -176,6 +176,10 @@ typedef struct IDirect3DDevice9Impl /* IDirect3DDevice9 fields */ IWineD3DDevice *WineD3DDevice;
+ /* A vertex declaration was converted from setFVF. + * Keep track of it, so it can be properly freed */ + IDirect3DVertexDeclaration9 *convertedDecl; + } IDirect3DDevice9Impl;
diff --git a/dlls/d3d9/device.c b/dlls/d3d9/device.c index 6eeaceb..5a836b9 100644 --- a/dlls/d3d9/device.c +++ b/dlls/d3d9/device.c @@ -60,6 +60,8 @@ static ULONG WINAPI IDirect3DDevice9Impl if (ref == 0) { IWineD3DDevice_Uninit3D(This->WineD3DDevice); IWineD3DDevice_Release(This->WineD3DDevice); + if (This->convertedDecl != NULL) + IUnknown_Release(This->convertedDecl); HeapFree(GetProcessHeap(), 0, This); } return ref; @@ -731,6 +733,7 @@ HRESULT WINAPI IDirect3DDevice9Impl_Se
hr = IDirect3DDevice9Impl_SetVertexDeclaration(iface, pDecl); if (hr != S_OK) goto exit; + This->convertedDecl = pDecl; pDecl = NULL;
exit: diff --git a/dlls/d3d9/vertexdeclaration.c b/dlls/d3d9/vertexdeclaration.c index 246b52f..88f5909 100644 --- a/dlls/d3d9/vertexdeclaration.c +++ b/dlls/d3d9/vertexdeclaration.c @@ -300,8 +300,12 @@ HRESULT WINAPI IDirect3DDevice9Impl_Se IDirect3DVertexDeclaration9Impl *pDeclImpl = (IDirect3DVertexDeclaration9Impl *)pDecl; HRESULT hr = D3D_OK;
- TRACE("(%p) : Relay\n", iface); + if (This->convertedDecl && This->convertedDecl != pDecl) { + IUnknown_Release(This->convertedDecl); + This->convertedDecl = NULL; + }
+ TRACE("(%p) : Relay\n", iface); hr = IWineD3DDevice_SetVertexDeclaration(This->WineD3DDevice, pDeclImpl == NULL ? NULL : pDeclImpl->wineD3DVertexDeclaration);
return hr;
Ivan Gyurdiev wrote:
Ivan Gyurdiev wrote:
Karsten Elfenbein wrote:
- don't prevent IUnknown_Release of pDecl in the exit section and fix
This seems wrong, d3d9 does not keep an internal reference of the vdecl. Try the attached patch instead, let me know if it works...
...better yet, try this one
Based on further tests, I don't believe this patch is correct.. but it will fix the memory leak. I'm still unsure what kind of insanity the native API is doing internally - in process of figuring that out. When adding this code, I spent a lot of time testing conversion between the fvf and decl, and that has been shown to be correct - but management of the decl afterwards seems rather wrong to me now, and that it needs additional tests.
Tests Show: Refcount of converted decl is 1 after GetVertexDeclaration() Tests Show: Refcount of converted decl *remains* 1 over a SetFVF call Tests Show: Refcount of converted decl *remains* 1 over a SetVertexDeclaration(NULL) call
I currently suspect that the convertedDecl is shared internally, and refcount behavior does not follow normal code path on Get(). Another possibility is that the internal storage is FVF-based, and conversions are ran in the GetVertexDecl() code [ or the object is cloned ]... but other tests show this not to be the case.
On 14/10/06, Ivan Gyurdiev ivg231@gmail.com wrote:
Ivan Gyurdiev wrote:
Based on further tests, I don't believe this patch is correct.. but it will fix the memory leak. I'm still unsure what kind of insanity the native API is doing internally - in process of figuring that out. When adding this code, I spent a lot of time testing conversion between the fvf and decl, and that has been shown to be correct - but management of the decl afterwards seems rather wrong to me now, and that it needs additional tests.
Tests Show: Refcount of converted decl is 1 after GetVertexDeclaration() Tests Show: Refcount of converted decl *remains* 1 over a SetFVF call Tests Show: Refcount of converted decl *remains* 1 over a SetVertexDeclaration(NULL) call
That's what happens with regular declarations as well, see test_get_set_vertex_declaration() in the d3d9 vertexdeclaration tests. Calling SetVertexDeclaration() doesn't change the refcount, but calling GetVertexDeclaration does (however, note that wined3d internally does change refcounts).
What does GetVertexDeclaration() do for the generated decl?
H. Verbeet wrote:
On 14/10/06, Ivan Gyurdiev ivg231@gmail.com wrote:
Ivan Gyurdiev wrote:
Based on further tests, I don't believe this patch is correct.. but it will fix the memory leak. I'm still unsure what kind of insanity the native API is doing internally - in process of figuring that out. When adding this code, I spent a lot of time testing conversion between the fvf and decl, and that has been shown to be correct - but management of the decl afterwards seems rather wrong to me now, and that it needs additional tests.
Tests Show: Refcount of converted decl is 1 after GetVertexDeclaration() Tests Show: Refcount of converted decl *remains* 1 over a SetFVF call Tests Show: Refcount of converted decl *remains* 1 over a SetVertexDeclaration(NULL) call
That's what happens with regular declarations as well, see test_get_set_vertex_declaration() in the d3d9 vertexdeclaration tests. Calling SetVertexDeclaration() doesn't change the refcount, but calling GetVertexDeclaration does (however, note that wined3d internally does change refcounts).
Exactly... so why is the refcount 1 *after* GetVertexDeclaration.
What does GetVertexDeclaration() do for the generated decl?
I don't think it increases the refcount like it does normally.
On 14/10/06, Ivan Gyurdiev ivg231@gmail.com wrote:
Exactly... so why is the refcount 1 *after* GetVertexDeclaration.
Perhaps the declaration in only generated when calling GetVertexDeclaration(). If you call it twice, does it return the same pointer?
H. Verbeet wrote:
On 14/10/06, Ivan Gyurdiev ivg231@gmail.com wrote:
Exactly... so why is the refcount 1 *after* GetVertexDeclaration.
Perhaps the declaration in only generated when calling GetVertexDeclaration(). If you call it twice, does it return the same pointer?
No.. I tried that too. The same pointer is returned.
Am Samstag 14 Oktober 2006 20:53 schrieb Ivan Gyurdiev:
H. Verbeet wrote:
On 14/10/06, Ivan Gyurdiev ivg231@gmail.com wrote:
Exactly... so why is the refcount 1 *after* GetVertexDeclaration.
Perhaps the declaration in only generated when calling GetVertexDeclaration(). If you call it twice, does it return the same pointer?
No.. I tried that too. The same pointer is returned.
Perhaps windows has a static internal vertex declaration? Does the refcount increase when you call GetVertexDeclaration more often?
Stefan Dösinger wrote:
Am Samstag 14 Oktober 2006 20:53 schrieb Ivan Gyurdiev:
H. Verbeet wrote:
On 14/10/06, Ivan Gyurdiev ivg231@gmail.com wrote:
Exactly... so why is the refcount 1 *after* GetVertexDeclaration.
Perhaps the declaration in only generated when calling GetVertexDeclaration(). If you call it twice, does it return the same pointer?
No.. I tried that too. The same pointer is returned.
Perhaps windows has a static internal vertex declaration? Does the refcount increase when you call GetVertexDeclaration more often?
Yes, the refcount increases.
======================
Here's probably what it does:
-> SetFVF()... sets FVF -> GetVertexDeclaration() ... vdecl is null, but there is an FVF, grab the FVF, and make a declaration out of it. Store the declaration, and return it to the caller with refcount 1. -> GetVertexDeclaration() again - just grab the stored declaration and follow regular code path, increasing the refcount.
Now, if the caller does not release the ref, it will leak memory [ because d3d certainly doesn't free it - not on SetFVF, or on SetVertexDecl ]. On the other hand, if the caller does release it, will it cause damage (1) on subsequent get calls? (2) at draw time... and how can we test that.
This whole part of the API is very strange, and completely undocumented. Perhaps those conversions don't matter at all in real applications. The primary reason they fixed stuff, is probably that the FVF codepath wasn't working together with shaders, so switching to decl internally caused improvement.
Hi
-> SetFVF()... sets FVF -> GetVertexDeclaration() ... vdecl is null, but there is an FVF, grab the FVF, and make a declaration out of it. Store the declaration, and return it to the caller with refcount 1. -> GetVertexDeclaration() again - just grab the stored declaration and follow regular code path, increasing the refcount.
Now, if the caller does not release the ref, it will leak memory [ because d3d certainly doesn't free it - not on SetFVF, or on SetVertexDecl ]. On the other hand, if the caller does release it, will it cause damage (1) on subsequent get calls? (2) at draw time... and how can we test that.
3 tests I can think of:
* If you release the decl, does GetVertexDecl return a different pointer? * If you call SetFVF with a different FVF, do you get a different decl pointer? * If you call SetFVF, GetVertexDecl and AddRef the decl, then SetFVF again with a different FVF, does the refcount of the previously returned declaration change? What happens if you allocate a huge amount of memory and fill it with garbage? Can you still access the vertex declaration?
-
H. Verbeet -
Ivan Gyurdiev -
Stefan Dösinger