[d3d8] RefCount refactoring

[WinterSnowfall]

Com objects already ref to the return pointer, so the current way of doing things will actually double refcount on certain calls, as can be seen in some traces. Doesn't fix any game per se as it stands and not sure if it will ultimately go anywhere, but some refcounts now appear to be more sane.

A good game to use for chasing the refs and comparing against how WineD3D refcounts is #92 . It in particular will crash due to an invalid ref count on an IDirect3DTexture8 object, but it also creates and releases a whole bunch of other things on startup before crashing which have invalid ref counts (some of which are addressed by this PR).

Next steps, based on the trace I'm looking at at least, would indicate:

• render target references need to be made private, however that blows up subresouces spectacularly, and m_container references no longer work for some reason (haven't figured it out yet)
• there is one more extra refcount on the parent IDirect3DTexture8 object which prevents its release, even if render targets are made private (this kinds of works if commenting out subresource refcounts to m_container in AddRef() and Release() ). I haven't yet found its annoyingly camouflaged hiding place.

Edit: Fixes #57, fixes #78, fixes #90, fixes #92, fixes #97, fixes #123.

[WinterSnowfall]

For the second iteration, I've managed to pull off making the render target refs private without blowing up everything (hopefully). I also found the extra ref, which I couldn't figure out before, it was m_textures in the device, which I've also switched to private.

In its current state the PR fixes #78 (which now works perfectly, without any crashes on exit or anything).

I haven't been able to get BloodRayne 1 and Blowout to work though. Both seem to have issues with setting the render target to the primary display for some reason. I'd like to figure that one out as well before I mark this as ready for review, but it may take some time, as I'm working on my C++-fu as well.

[WinterSnowfall]

Alright, I think I've reached the limit of what I can do/understand here. I believe this PR improves the ref/refcount situation to some degree. It fixes #78 #57 and #92.

Sadly I could not fix the Railroad Tycoon 3 crash (#90) (which is also refcount related) or #123, although I know what would fix them, just not how to fix them without breaking other games. I'll comment about my findings on those issues.

P.S.: I've tried my best not to regress anything, but I guess anything is possible.

[WinterSnowfall]

I hope I'm at the final iteration... this properly fixes refcounting on textures/subresources, backbuffers + vertex & index buffers, but for some reason that entirely escapes me regresses Hitman Contracts, which will now crash on startup. I've tested over 50 other games and they're all fine.

The final piece of the puzzle was to not call back to d3d9 in SetRenderTarget if the render target or depth stencil addresses are unchanged, but rather just derp out and rely on what's cached for future fetch calls. This prevents crashing in #57 and #92. I have ported over some validations as well from d3d9 while I was at it, so we can more eagerly error out if we need to.

As to what it fixes, basically everything that was refcount related and mentioned above, namely: #57, #78, #90, #92 and #123.

Overall device count is still off compared to what I'm seeing in WineD3D traces, but games don't really seem to care about that too much, so meh. Might be worth looking into at some point though.

[WinterSnowfall]

I have solved the Hitman: Contracts regression with this last update.

The TLDR of it is the game constantly switches between two sets of render targets and depth stencils for some reason, so we need to cater for this case. The d3d8 spec also says with regards to SetRenderTarget:

The previous depth-stencil surface's contents persist after a call to SetRenderTarget to disassociate the previous depth-stencil surface from the device.

So we need to keep the previous depth stencil around, in case it gets re-associated. Apparently, Hitman also expects the same behavior for the render target, although this is not mentioned anywhere in the specs. In any case, the ref counting for the game in the d8vk trace now looks identical to what I'm seeing in WineD3D, so it's definitely doing the right thing.

I've retested the previously fixed titles as well and they are still fixed, as expected. I now hope absolutely nothing will regress because of this PR.

[WinterSnowfall]

Addressed another regression uncovered while testing Motor City Online. The game is just spuriously releasing stuff until it gets a 0 refcount (nice system, guys...). I've never seen any other games doing something this "innovative", but have added a sanity check to prevent unsigned underruns on subresource release, just to be safe. WineD3D seems to be handling it in a similar fashion.

[WinterSnowfall]

The refcount saga continues. While playing around a bit with the D3D8 SDK samples, I noticed some of them were erroring out while trying to go fullscreen, complaining about objects not being released properly. I've traced it down to overblown refcounts in relation to SetTexture() and stateblock Capture()s, and those bits are also fixed now. I'm not aware of any games being affected, but it's good to behave well here in any case.

[WinterSnowfall]

Updated again to fix a regression in Enclave (also present on main). The game depends on us derping out early in GetDepthStencilSurface in case of errors (added the equivalent for render targets for good measure).

[WinterSnowfall]

So, turns out I was being stupid (who would have guessed, right?) and abused SAFE_RELEASE, which also nulls the pointer that gets passed to it. State block derp.

This bit should also be fine now. Anyway, I've only seen this affect the "Water" D3D8 SDK sample, and possibly some of the other samples as well, but no actual games. All refcounts should now actually look identical to those in traces captured with WineD3D, including device refcounts, which is nice.

Even if arguably there could be room for improvement in some places, the refcounts are now correct, if not also in a very delicate balance (we really need to be careful not to add any additional public refs in the future). Would appreciate it if this PR gets merged in its current state and then later improved upon if needed, because it has been tested to hell and back by now.

Thanks to @Blisto91 for testing some of the heavy hitters that I don't own (think GTA SA & friends). I've also gone through most, if not all, of my 100+ d3d8 games collection and can confirm nothing regresses and that the above mentioned fixes actually work properly.

Note that I've also included some non-related regression fixes (Enclave) in this PR. Perhaps not the best idea, but it sort of blended into the refactoring work really (and it's getting rather hard to keep track of all my other fixes filed as separate PRs).

[AlpyneDreams]

This PR has the right idea, and the fact that it supposedly fixes all those games is very good, but I am not certain we're doing things right here... See my review for more.

[AlpyneDreams]

I don't think these should all be private.

1. d3d8 docs say that SetRenderTarget increases ref on the new RT, but a private Com I think would not affect the public refcount?
2. Higher up you use SAFE_RELEASE, which calls Release(), after m_renderTargetPrev.ref() on these pointers, so you are increasing the private refcount, but decreasing the public refcount, which seems wrong. Ditto for m_depthStencilPrev

Similar deal for the depth-stencils, SetIndices and m_indices, SetStreamSource and D3D8VBO.

[WinterSnowfall]

1. The idea with private COMs in all these places is to have tighter control over the lifecycle of the cached objects and get to decide when they are released. The public refcount can still be adjusted on a private COM via .ref() (or ref(object) for that matter, they both do the same thing), while assignment and other operations will automagically handle the private refcount. Here is the COM object code I am referring to:

    T* ref() const {
      return dxvk::ref(m_ptr);
    }

  template<typename T>
  T* ref(T* object) {
    if (object != nullptr)
      object->AddRef();
    return object;
  }

Notice the latter is calling AddRef() directly, not incRef, so it is increasing the public refcount even on private COM objects.

Again, the main idea here is to keep the cached objects around as long as we need them, not according to what a game might decide by messing with the public refcount, while trying to match native public refcounts with privateComObject.ref(). As far as I can see from all the native traces I have compared against those captured with this PR, they now behave identically.

2. This bit is not ideal and I am open to suggestions on how to handle it better, however Hitman: Contracts seems to rely on this sort of behavior. What the game is doing on every frame is swapping between two render targets and depth stencils. It is not holding any public references to them, so their lifecycle is entirely controlled by our private refcounts.

If I do not add a temporary ref here and release it after the swap is finalized, the game will crash. This is because the object that is about to be set is equal to the prev object, and the first operation, namely:

m_depthStencilPrev = m_depthStencil;

will 0 the private reference on m_depthStencilPrev, causing the object's destruction (which we don't want, because this is essentially equal to zStencil in case of a swap). So to keep the object around while I'm swapping it I am indeed increasing its public ref by 1 and releasing it when done.

I've used SAFE_RELEASE, because it also nulls the "swap" pointer after calling Release(), but simply calling Release in case of a not null pointer works as well. It's just that SAFE_RELEASE already has the needed logic in place.

[AlpyneDreams]

Okay, I see, I always assumed ref called .incRef rather than ->AddRef. I am a little concerned about private refcounts purely because there may be cases where we forget to manually increase the refcount when a game expects a certain thing, but we forget to do it.

I think it could probably be solved another way, but if this increases compatability then it might be worth it.

[AlpyneDreams]

Shouldn't be needed?

[WinterSnowfall]

One is now a private COM while the other is a public one so I get an error when I don't use .ptr() here.

[AlpyneDreams]

That's interesting, because Com<T, B> should have == implemented for any B

    template<bool Public_>
    bool operator == (const Com<T, Public_>& other) const { return m_ptr == other.m_ptr; }
    template<bool Public_>
    bool operator != (const Com<T, Public_>& other) const { return m_ptr != other.m_ptr; }

But if the compiler is being dumb about it then I suppose leave it like that.

[WinterSnowfall]

Hopefully the RT/DS swap logic is a bit more clear now, as it's gated to temporarily bump the public refcount only in case an actual swap is taking place. I've only seen Hitman: Contracts rely on this in practice, so other games shouldn't care much anyway.

Also removed the extra checks on SetIndeces/SetStreamSource which weren't crucial, as they weren't affecting refcounts.

[AlpyneDreams]

Looks good now.

[AlpyneDreams]

Okay, I see, I always assumed ref called .incRef rather than ->AddRef. I am a little concerned about private refcounts purely because there may be cases where we forget to manually increase the refcount when a game expects a certain thing, but we forget to do it.

I think it could probably be solved another way, but if this increases compatability then it might be worth it.

[AlpyneDreams]

That's interesting, because Com<T, B> should have == implemented for any B

    template<bool Public_>
    bool operator == (const Com<T, Public_>& other) const { return m_ptr == other.m_ptr; }
    template<bool Public_>
    bool operator != (const Com<T, Public_>& other) const { return m_ptr != other.m_ptr; }

But if the compiler is being dumb about it then I suppose leave it like that.