Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[d3d8] RefCount refactoring #122

Merged
merged 5 commits into from
Apr 15, 2023
Merged

[d3d8] RefCount refactoring #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
49b2da3
[d3d8] Ignore Release calls on objects with 0 refCount
WinterSnowfall Apr 15, 2023
1705b7a
[d3d8] Adjust state block texture refcounts
WinterSnowfall Apr 15, 2023
eeb22bc
[d3d8] Adjust texture and subresource refcounts
WinterSnowfall Apr 15, 2023
e26d11c
[d3d8] Adjust DS and RT refcounts
WinterSnowfall Apr 15, 2023
ad97fea
[d3d8] Adjust VB, IB & backbuffer refcounts
WinterSnowfall Apr 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/d3d8/d3d8_blit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ namespace dxvk {
case D3DPOOL_SYSTEMMEM: {

// RT (DEFAULT) -> SYSTEMMEM: Use GetRenderTargetData as fast path if possible
if ((srcDesc.Usage & D3DUSAGE_RENDERTARGET || m_renderTarget == src)) {
if ((srcDesc.Usage & D3DUSAGE_RENDERTARGET || m_renderTarget.ptr() == src.ptr())) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be needed?

Copy link
Author

@WinterSnowfall WinterSnowfall Apr 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// GetRenderTargetData works if the formats and sizes match
if (srcDesc.MultiSampleType == d3d9::D3DMULTISAMPLE_NONE
Expand Down
96 changes: 77 additions & 19 deletions src/d3d8/d3d8_device.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,14 +151,15 @@ namespace dxvk {
UINT iBackBuffer,
D3DBACKBUFFER_TYPE Type,
IDirect3DSurface8** ppBackBuffer) {
InitReturnPtr(ppBackBuffer);

if (iBackBuffer >= m_backBuffers.size() || m_backBuffers[iBackBuffer] == nullptr) {
Com<d3d9::IDirect3DSurface9> pSurface9;
HRESULT res = GetD3D9()->GetBackBuffer(0, iBackBuffer, (d3d9::D3DBACKBUFFER_TYPE)Type, &pSurface9);

if (FAILED(res)) return res;

m_backBuffers[iBackBuffer] = ref(new D3D8Surface(this, std::move(pSurface9)));
m_backBuffers[iBackBuffer] = new D3D8Surface(this, std::move(pSurface9));
*ppBackBuffer = m_backBuffers[iBackBuffer].ref();
AlpyneDreams marked this conversation as resolved.
Show resolved Hide resolved

return res;
Expand Down Expand Up @@ -368,30 +369,76 @@ namespace dxvk {

if (pRenderTarget != NULL) {
D3D8Surface* surf = static_cast<D3D8Surface*>(pRenderTarget);
res = GetD3D9()->SetRenderTarget(0, surf->GetD3D9());

if (FAILED(res)) return res;
D3DSURFACE_DESC rtDesc;
surf->GetDesc(&rtDesc);

if (unlikely(!(rtDesc.Usage & D3DUSAGE_RENDERTARGET)))
return D3DERR_INVALIDCALL;

if(likely(m_renderTarget.ptr() != surf)) {
res = GetD3D9()->SetRenderTarget(0, surf->GetD3D9());

if (FAILED(res)) return res;

D3D8Surface* pRenderTargetSwap = nullptr;
bool isRTSwap = m_renderTargetPrev.ptr() == surf;

if(unlikely(isRTSwap))
// keep a temporary ref on the prev RT to prevent its release
pRenderTargetSwap = m_renderTargetPrev.ref();

m_renderTarget = ref(surf);
m_renderTargetPrev = m_renderTarget;
m_renderTarget = surf;

if(unlikely(isRTSwap && pRenderTargetSwap))
pRenderTargetSwap->Release();
}
}

// SetDepthStencilSurface is a separate call
D3D8Surface* zStencil = static_cast<D3D8Surface*>(pNewZStencil);
res = GetD3D9()->SetDepthStencilSurface(D3D8Surface::GetD3D9Nullable(zStencil));

if (FAILED(res)) return res;
if(pNewZStencil != NULL) {
D3DSURFACE_DESC zsDesc;
zStencil->GetDesc(&zsDesc);

m_depthStencil = ref(zStencil);
return res;
if (unlikely(!(zsDesc.Usage & D3DUSAGE_DEPTHSTENCIL)))
return D3DERR_INVALIDCALL;
}

if(likely(m_depthStencil.ptr() != zStencil)) {
res = GetD3D9()->SetDepthStencilSurface(D3D8Surface::GetD3D9Nullable(zStencil));

if (FAILED(res)) return res;

D3D8Surface* pDepthStencilSwap = nullptr;
bool isDSSwap = m_depthStencilPrev.ptr() == zStencil;

if(unlikely(isDSSwap))
// keep a temporary ref on the prev DS to prevent its release
pDepthStencilSwap = m_depthStencilPrev.ref();

m_depthStencilPrev = m_depthStencil;
m_depthStencil = zStencil;

if(unlikely(isDSSwap && pDepthStencilSwap))
pDepthStencilSwap->Release();
}

return D3D_OK;
}

HRESULT STDMETHODCALLTYPE GetRenderTarget(IDirect3DSurface8** ppRenderTarget) {
InitReturnPtr(ppRenderTarget);

if (unlikely(m_renderTarget == nullptr)) {
Com<d3d9::IDirect3DSurface9> pRT9 = nullptr;
HRESULT res = GetD3D9()->GetRenderTarget(0, &pRT9); // use RT index 0

m_renderTarget = ref(new D3D8Surface(this, std::move(pRT9)));
if(FAILED(res)) return res;

m_renderTarget = new D3D8Surface(this, std::move(pRT9));

*ppRenderTarget = m_renderTarget.ref();
return res;
Expand All @@ -402,12 +449,15 @@ namespace dxvk {
}

HRESULT STDMETHODCALLTYPE GetDepthStencilSurface(IDirect3DSurface8** ppZStencilSurface) {
InitReturnPtr(ppZStencilSurface);

if (unlikely(m_depthStencil == nullptr)) {
Com<d3d9::IDirect3DSurface9> pStencil9 = nullptr;
HRESULT res = GetD3D9()->GetDepthStencilSurface(&pStencil9);

m_depthStencil = ref(new D3D8Surface(this, std::move(pStencil9)));
if(FAILED(res)) return res;

m_depthStencil = new D3D8Surface(this, std::move(pStencil9));

*ppZStencilSurface = m_depthStencil.ref();
return res;
Expand Down Expand Up @@ -567,7 +617,10 @@ namespace dxvk {

D3D8Texture2D* tex = static_cast<D3D8Texture2D*>(pTexture);

m_textures[Stage] = ref(tex);
if(unlikely(m_textures[Stage].ptr() == tex))
return D3D_OK;

m_textures[Stage] = tex;

return GetD3D9()->SetTexture(Stage, D3D8Texture2D::GetD3D9Nullable(tex));
}
Expand Down Expand Up @@ -769,8 +822,11 @@ namespace dxvk {
HRESULT STDMETHODCALLTYPE GetIndices(
IDirect3DIndexBuffer8** ppIndexData,
UINT* pBaseVertexIndex) {
InitReturnPtr(ppIndexData);

*ppIndexData = m_indices.ptr();
*pBaseVertexIndex = m_baseVertexIndex;

return D3D_OK;
}

Expand Down Expand Up @@ -829,9 +885,10 @@ namespace dxvk {
for (UINT i = 0; i < m_presentParams.BackBufferCount; i++) {
m_backBuffers.push_back(nullptr);
}
m_frontBuffer = nullptr;
m_renderTarget = nullptr;
m_renderTargetPrev = nullptr;
m_depthStencil = nullptr;
m_depthStencilPrev = nullptr;
}

friend d3d9::IDirect3DPixelShader9* getPixelShaderPtr(D3D8DeviceEx* device, DWORD Handle);
Expand All @@ -849,23 +906,24 @@ namespace dxvk {
D3D8StateBlock* m_recorder = nullptr;

struct D3D8VBO {
Com<D3D8VertexBuffer> buffer = nullptr;
UINT stride = 0;
Com<D3D8VertexBuffer, false> buffer = nullptr;
UINT stride = 0;
};

// Remember to fill() these in the constructor!
std::array<Com<IDirect3DBaseTexture8>, d8caps::MAX_TEXTURE_STAGES> m_textures;
std::array<Com<D3D8Texture2D, false>, d8caps::MAX_TEXTURE_STAGES> m_textures;
std::array<D3D8VBO, d8caps::MAX_STREAMS> m_streams;

Com<D3D8IndexBuffer> m_indices;
INT m_baseVertexIndex = 0;
Com<D3D8IndexBuffer, false> m_indices;
INT m_baseVertexIndex = 0;

// TODO: Which of these should be a private ref
std::vector<Com<D3D8Surface, false>> m_backBuffers;
Com<D3D8Surface> m_frontBuffer;

Com<D3D8Surface> m_renderTarget;
Com<D3D8Surface, false> m_renderTarget;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@WinterSnowfall WinterSnowfall Apr 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

  1. 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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Com<D3D8Surface, false> m_renderTargetPrev;
Com<D3D8Surface, false> m_depthStencil;
Com<D3D8Surface, false> m_depthStencilPrev;

std::vector<D3D8VertexShaderInfo> m_vertexShaders;
std::vector<d3d9::IDirect3DPixelShader9*> m_pixelShaders;
Expand Down
4 changes: 4 additions & 0 deletions src/d3d8/d3d8_device_child.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ namespace dxvk {
}

ULONG STDMETHODCALLTYPE Release() {
// ignore Release calls on objects with 0 refCount
if(unlikely(!this->m_refCount))
return this->m_refCount;

WinterSnowfall marked this conversation as resolved.
Show resolved Hide resolved
uint32_t refCount = --this->m_refCount;
if (unlikely(!refCount)) {
auto* pDevice = GetDevice();
Expand Down
8 changes: 6 additions & 2 deletions src/d3d8/d3d8_state_block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@ HRESULT dxvk::D3D8StateBlock::Capture() {
if (m_capture.ps) m_device->GetPixelShader(&m_pixelShader);

for (DWORD stage = 0; stage < m_textures.size(); stage++)
if (m_capture.textures.get(stage))
if (m_capture.textures.get(stage)) {
m_device->GetTexture(stage, &m_textures[stage]);
// we are adding a needless ref when calling GetTexture, so release it
if(m_textures[stage])
m_textures[stage]->Release();
}

if (m_capture.indices) m_device->GetIndices(&m_indices, &m_baseVertexIndex);

Expand All @@ -32,7 +36,7 @@ HRESULT dxvk::D3D8StateBlock::Apply() {

for (DWORD stage = 0; stage < m_textures.size(); stage++)
if (m_capture.textures.get(stage))
m_device->SetTexture(stage, m_textures[stage] );
m_device->SetTexture(stage, m_textures[stage]);

if (m_capture.indices) m_device->SetIndices(m_indices, m_baseVertexIndex);

Expand Down
2 changes: 1 addition & 1 deletion src/d3d8/d3d8_texture.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ namespace dxvk {
return ptr;
}

std::vector<Com<SubresourceType>> m_subresources;
std::vector<Com<SubresourceType, false>> m_subresources;
AlpyneDreams marked this conversation as resolved.
Show resolved Hide resolved

};

Expand Down