search

crosire / d3d8to9

A D3D8 pseudo-driver which converts API calls and bytecode shaders to equivalent D3D9 ones.
BSD 2-Clause "Simplified" License
880 stars 78 forks source link

Back buffer reference counting #21

Closed CookiePLMonster closed 6 years ago

CookiePLMonster commented 7 years ago

When investigating on why both d3d8to9 and ENB's d3d8 wrapper flat out crash with a dx8 thrash driver in Need for Speed III (from THIS PATCH) I noticed a couple issues, but the most important fact is that this code loves to do stuff like:

device->GetBackBuffer(0, &surface);
surface->Release();
global_myBackbuffer = surface;

Since d3d8to9 creates a new wrapper class in GetBackBuffer, the call to Release destroys it and any attempts to use this surface fail miserably.

Should this be considered severe API misuse and not handled, or is this something to fix on d3d8to9 side? I mean, this is obviously not how you should be managing reference counters but this doesn't seem to crash with a real dx8 device at all. Maybe device wrapper should account for such odd behaviour, store its backbuffer surface wrappers in an array and return those on demand? This should probably match real device behaviour more closely.

EDIT: Said game also seems to crash consistently inside Direct3DDevice8::SetCurrentTexturePalette but that seems like a different issue. What's worse, it instantiates itself with D3DCREATE_MULTITHREADED and who knows how thread unsafe d3d8to9 device wrapper really is.

crosire commented 7 years ago

While it's a bad idea it's not technically wrong to use the API like that, since the device has a guaranteed backbuffer instance for its entire lifetime. So yes, I guess we should cache the backbuffer in the device wrapper. I don't think we need to handle more than the first backbuffer though. It's rare more than one exists.

As for thread-safety: Most calls are implicitly handled by the critical section the D3D9 runtime acquires, since the same flag is passed on to it as well. However, some calls that modify states in d3d8to9, like Direct3DDevice8::SetRenderTarget are certainly not thread-safe.

CookiePLMonster commented 7 years ago

I don't know what the D3D8 limit is, but D3D9 limit is 3 so it shouldn't be much more effort than handling only the first backbuffer.

Then there are swap chains, though... maybe d3d8 device should store handles to its implicit swap chains and use them for D3D8Device::GetBackBuffer and the others?

EDIT: This thrash driver really is a clusterfuck though - seems to have crashes in more than just this and SetCurrentTexturePalette. Weird stuff.

elishacloud commented 7 years ago

CookiePLMonster, are you sure you put the right link down? I downloaded the 'nfs3_modern_patch.7z' file from this link and followed the steps to install the patch and it runs nfs3.exe using Direct3D9. d3d8.dll does not get loaded at all. So I cannot reproduce this issue.

Also this code seems ok to me with the native Microsoft Direct3D8 driver:

device->GetBackBuffer(0, &surface);
surface->Release();
global_myBackbuffer = surface;

The surface should have at least 1 reference before GetBackBuffer is called. So when GetBackBuffer is called the count jumps to 2. Therefore when they call Release the count should go back to 1. I think the issue is in d3d8to9 where it does not add a reference when GetBackBuffer is called.

Maybe we should do something like this in GetBackBuffer:

*ppBackBuffer = new Direct3DSurface8(this, SurfaceInterface);
(*ppBackBuffer)->AddRef();

I see the same issue in many other functions. Here are the functions I see the same issue with:

CookiePLMonster commented 7 years ago

you need to set the game to use dx8 - it's an ini option somewhere in the game directory but now I can't check where exactly.

1 lip 2017 06:56 "Elisha Riedlinger" notifications@github.com napisał(a):

CookiePLMonster, are you sure you put the right link down? I downloaded the 'nfs3_modern_patch.7z' file from this http://veg.by/en/projects/nfs3/ link and followed the steps to install the patch and it runs nfs3.exe using Direct3D9. d3d8.dll does not get loaded at all. So I cannot reproduce this issue.

Also this code seems ok to me with the native Microsoft Direct3D8 driver:

device->GetBackBuffer(0, &surface); surface->Release(); global_myBackbuffer = surface;

The surface should have at least 1 reference before GetBackBuffer is called. So when GetBackBuffer is called the count jumps to 2. Therefore when they call Release the count should go back to 1. I think the issue is in d3d8to9 where it does not add a reference when GetBackBuffer is called.

Maybe we should do something like this in GetBackBuffer:

ppBackBuffer = new Direct3DSurface8(this, SurfaceInterface); (ppBackBuffer)->AddRef();

I see the same issue in many other functions. Here are the functions I see the same issue with:

  • Direct3DDevice8::GetBackBuffer()
  • Direct3DDevice8::GetTexture()
  • Direct3DDevice8::GetStreamSource()
  • Direct3DDevice8::GetIndices()
  • Direct3DSwapChain8::GetBackBuffer()
  • Direct3DTexture8::GetSurfaceLevel()
  • Direct3DCubeTexture8::GetCubeMapSurface()
  • Direct3DVolumeTexture8::GetVolumeLevel()

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/crosire/d3d8to9/issues/21#issuecomment-312410697, or mute the thread https://github.com/notifications/unsubscribe-auth/AHlExTX0xbz3AhPZ1IKFlu17YskQswjnks5sJdF2gaJpZM4NS7bo .

elishacloud commented 7 years ago

Ok, I found it. If you open 'nfs3.ini' and change ThrashDriver=nglide to ThrashDriver=dx8 it will use Direct3D8 and d3d8.dll.

elishacloud commented 7 years ago

Calling AddRef() for each get surface and texture request as described in my comments above fixed the reference count issue.

The palette issue appears to be that they are calling SetCurrentTexturePalette with NULL palette peFlags. This was allowed in IDirect3D8, see here. But as far as I can tell SetCurrentTexturePalette will crash in IDirect3D9 unless the peFlags are set to PC_NOCOLLAPSE. This site also seems to indicate that you must set peFlags to PC_NOCOLLAPSE.

Adding this line solved the crash in SetCurrentTexturePalette:

PaletteNumber = (PaletteNumber & 0xFFF) | (PC_NOCOLLAPSE << 12);

Note the line should be added to both of these functions:

Code changes to fix these issues are posted here 7d64f1a.

CookiePLMonster commented 7 years ago

Looks great! Though won't that leak the d3d wrapper objects if one requests the same backbuffer multiple times? There's no way for the application to release them safely and the wrapper itself won't ever release them either, because each one will have a dangling reference.

1 lip 2017 22:32 "Elisha Riedlinger" notifications@github.com napisał(a):

Calling AddRef() for each get surface and texture request as described in my comments above https://github.com/crosire/d3d8to9/issues/21#issuecomment-312410697 fixed the reference count issue.

The palette issue appears to be that they are calling SetCurrentTexturePalette with NULL palette peFlags https://msdn.microsoft.com/en-us/library/windows/desktop/bb147253(v=vs.85).aspx. This was allowed in IDirect3D8, see here https://msdn.microsoft.com/en-us/library/ms910636.aspx. But as far as I can tell SetCurrentTexturePalette will crash in IDirect3D9 unless the peFlags are set to PC_NOCOLLAPSE. This site http://www.yaldex.com/games-programming/0672323699_ch06lev1sec5.html also seems to indicate that you must set peFlags to PC_NOCOLLAPSE.

Adding this line solved the crash in SetCurrentTexturePalette:

PaletteNumber = (PaletteNumber & 0xFFF) | (PC_NOCOLLAPSE << 12);

Note the line should be added to both of these functions:

  • Direct3DDevice8::SetPaletteEntries
  • Direct3DDevice8::SetCurrentTexturePalette

Code changes to fix these issues are posted here 7d64f1a https://github.com/elishacloud/d3d8to9/commit/7d64f1ad48e8f8979de12f66d215e44ebcd3ddbb .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/crosire/d3d8to9/issues/21#issuecomment-312454176, or mute the thread https://github.com/notifications/unsubscribe-auth/AHlExVLZwfHbRs7zMqiyDWN63jb7oVhDks5sJqzsgaJpZM4NS7bo .

crosire commented 7 years ago

Don't just AddRef, that will create a memory leak. Caching is the right way to go here.

elishacloud commented 7 years ago

Ok, I added caching to fix the ref counting issue. Take a look at the code here and let me know what you think. I tested this with Need for Speed III along with 9 other Direct3D8 games.

Updated link with latest check-in that simplifies the caching function.

elishacloud commented 7 years ago

I have made a few changes and updates. You might want to just look at the branch I created for this here.