Reference count fix for Shaders and StateBlocks to prevent a crash when exiting games

[elishacloud]

This will fix PR #159 so shaders and state blocks are correctly released once the device reference count reaches 1. The previous code would cause a crash on exit in a couple of games, including Indiana Jones and the Emperor's Tomb and State of Emergency.

This fix does the following things:

1. It fixes the ref count issue in both AddRef() and Release() to return the correct value based on how the native Direct3D8 works.
2. It cleans up some code related to the shader count in CreateVertexShader()
3. It fixes a crash in Windows 7 64bit caused by erasing an item in a map while looping through the same map.

I tested this with a couple of games alongside the native d3d8 to ensure it does not crash and it works the same as the native d3d8.

Note: it appears that you need to get the current device reference count by call AddRef() and then Release() before releasing the shaders and state blocks. If you call Release() first then it can lead to a crash in some games.

[elishacloud]

This PR is ready to check-in. It fixes a crash in Windows 7 caused by modifying the unordered map in the middle of looping through the same unordered map. It also fixes the reference count in both AddRef() and Release() to make it match what it would be on native Direct3D8.

[WinterSnowfall]

2. It fixes a state block issue where it should not be inserting a token in `EndStateBlock()`.

I don't think this is incorrect. There are two ways of creating a state block in D3D8:

• CreateStateBlock()
• BeginStateBlock() followed by EndStateBlock()

You'll notice both CreateStateBlock() and EndStateBlock() take a DWORD* pToken which is then returned to the calling app, thus both will create valid ref counted state blocks in D3D9, with separate tokens at D3D8 level. State blocks created by both methods should ultimately be released if not deleted via DeleteStateBlock().

If you want, I can write a test to check it explicitly, but based on the above described behavior it should be pretty clear how it works.

P.S.: Some D3D8 games are known to leak state blocks and will crash in certain situations even with native D3D8.

[elishacloud]

There are two ways of creating a state block in D3D8. [...] You'll notice both CreateStateBlock() and EndStateBlock() take a DWORD* pToken which is then returned to the calling app, thus both will create valid ref counted state blocks in D3D9, with separate tokens at D3D8 level.

Ok, good catch. I have fixed this issue.

P.S.: Some D3D8 games are known to leak state blocks and will crash in certain situations even with native D3D8.

The games I tested did not use StateBlocks at all. One used only Vertex Shaders. Plus, I literally just started the game and exited immediately on the opening menu and it crashed. You don't even need to play the game to see the crash.

The issue with Windows 7 64bit was that when an item in map is modified while looping through the map then the next iteration though the loop would retrieve, and thus send, an invalid handle to DeleteVertexShader() causing a crash. I am not completely sure why, but I believe that for some reason in Windows 7 64bit when erasing an item from the map it would move the map to a new address space, however, the loop would still continue to loop through the map as if it was still in the old address space, thus causing an invalid handle to be retrieved and sent.

Note: If the game crashes even with native d3d8 then it would not be an issue with d3d8to9.

[elishacloud]

If you want, I can write a test to check it explicitly, but based on the above described behavior it should be pretty clear how it works.

You are welcome to test this. More testing is always good. But I did already run this through several games and checked the reference count before and after every call to any create function and every call to the Release and AddRef functions. I tested both native d3d8 and I tested with d3d8to9 to verify the references match.

[WinterSnowfall]

Ok, good catch. I have fixed this issue.

Thank you.

The games I tested did not use StateBlocks at all. One used only Vertex Shaders. Plus, I literally just started the game and exited immediately on the opening menu and it crashed. You don't even need to play the game to see the crash.

Edit: Nevermind on the initial suggestion, missed that sets were used here, not vectors.

Cossacks II is the game to test for anything state block related, or perhaps Supreme Ruler 2010/2020, as the latter spuriously creates state blocks every frame. This is just FYI, if you ever need to test anything state block related.

Note: If the game crashes even with native d3d8 then it would not be an issue with d3d8to9.

Indeed. My point was mostly that crashes due to various leaks, especially on alt+tab-ing, are not unheard of.

You are welcome to test this. More testing is always good. But I did already run this through several games and checked the reference count before and after every call to any create function and every call to the Release and AddRef functions. I tested both native d3d8 and I tested with d3d8to9 to verify the references match.

I was only referring to the state blocks, but I'm pretty sure I have checked this myself as well in the past. Otherwise I see no problem with the shader related changes, if they improve compatibility.

[elishacloud]

Edit: Nevermind on the initial suggestion, missed that sets were used here, not vectors.

Yeah, unfortunately std::unordered_set doesn't support indexing. I don't think using std::move is too bad. Keep in mind the code only runs when the whole device is being deleted anyways.

Otherwise I see no problem with the shader related changes, if they improve compatibility.

Thanks for checking. I appreciate it. More eyes are always better.

[elishacloud]

unfortunately std::unordered_set doesn't support indexing. I don't think using std::move is too bad.

@WinterSnowfall, I found a better way to loop through the maps without moving them to a temporary map.

[WinterSnowfall]

@WinterSnowfall, I found a better way to loop through the maps without moving them to a temporary map.

Nicely done. I was actually considering to suggest getting the set keys in a temp vector and iterating over that rather than copying the whole thing, but this is even better (you've done the same, but in place, which makes more sense).

[elishacloud]

@crosire, this is ready to check in. This fixes a rather prevalent crash that happens on Windows 7. We are hoping to get this fix into the Silent Hill 2 Enhancements project.