crosire / d3d8to9

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

Direct3DDevice8::Release #4

Closed CookiePLMonster closed 7 years ago

CookiePLMonster commented 7 years ago

From what I see, wrappers do not attempt to synchronize reference counters with their underlying types (eg. GetSurfaceLevel should probably return the same interface every time it gets called on the same texture but in this case, each time a new wrapper is constructed) and it's hidden in the implementation by returning real resource refcounts from AddRef/Release, with wrappers' destruction dictacted by their own refcounts.

Direct3DDevice8::Release seems to be an exception here and I'm not sure if this is intentional. It's being deleted only if underlying type's refcount is 0, which probably causes wrapper's memory to leak a lot (and therefore break Direct3D8 refcounting). Shouldn't it call its own delete on its own refcounter, like all the other interfaces?

CookiePLMonster commented 7 years ago

I tested that and noticed it in fact crashes on app exit when the device is killed (wrapper's refcount is 0, underlying interface's refcount is 2)... So the question is, where do the refcounts desync?

CookiePLMonster commented 7 years ago

UPDATE: The exit crash seems to solved by re-reading the reference counter after releasing render target/depth target with myRef = InterlockedExchange(&_ref, ref);. Thus, the initial question in the issue is valid once again.

crosire commented 7 years ago

Was done because of the crash you mentioned and at the time this was the quickest solution.

CookiePLMonster commented 7 years ago

Something as simple as inserting this

// Obtain the reference counter after releasing surfaces again myRef = InterlockedExchange(&_ref, ref);

below proxy->Release() seems to work great though. One could just --myRef when releasing rendertargets but I feel re-reading the actual refcount is safer.

crosire commented 7 years ago

Technically it would be better to put it one line above the _proxy->Release() call to keep constistent with all other Release implementations. Should still work.

CookiePLMonster commented 7 years ago

Sure. It matters little but if consistency can be preserved, best to go for it!

CookiePLMonster commented 7 years ago

While you're here, I know it's not really related but I'm not a linkedin user and leaving a message here is probably the best - are you fine with a game specific fork of d3d8to9 maintained in a never-to-be-merged branch? Here it's about a GameMaker generated executable so we don't really have control on how d3d8 calls are issued (and there is a LOT of redundancy which cannot be easily avoided) and it'd be easiest to address from inside this dll.

CookiePLMonster commented 7 years ago

I have identified the root cause of the refcount issue (device release ended up in a would-be infinite loop only bailed from thanks to refcount underflow) and updated #2 to include the fix!

crosire commented 7 years ago

@CookiePLMonster Sure, you can use it however you see fit as long as it follows the license.