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

Jump to bottom

[d3d9] Fix for crashes on BACK buffer allocation failure due to OOM #2964

Closed
wants to merge 1 commit into from
Closed

[d3d9] Fix for crashes on BACK buffer allocation failure due to OOM #2964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions src/d3d9/d3d9_device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7282,8 +7282,9 @@ namespace dxvk {
}

if (m_implicitSwapchain != nullptr) {
if (FAILED(m_implicitSwapchain->Reset(pPresentationParameters, pFullscreenDisplayMode)))
return D3DERR_INVALIDCALL;
HRESULT hr = m_implicitSwapchain->Reset(pPresentationParameters, pFullscreenDisplayMode);
if (FAILED(hr))
return hr;
}
else
m_implicitSwapchain = new D3D9SwapChainEx(this, pPresentationParameters, pFullscreenDisplayMode);
Expand Down
35 changes: 27 additions & 8 deletions src/d3d9/d3d9_swapchain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,9 +492,7 @@ namespace dxvk {
if (changeFullscreen)
SetGammaRamp(0, &m_ramp);

CreateBackBuffers(m_presentParams.BackBufferCount);

return D3D_OK;
return CreateBackBuffers(m_presentParams.BackBufferCount);
}


Expand Down Expand Up @@ -645,6 +643,8 @@ namespace dxvk {
m_parent->EndFrame();
m_parent->Flush();

ValidateBackBuffersPtrs();

// Retrieve the image and image view to present
auto swapImage = m_backBuffers[0]->GetCommonTexture()->GetImage();
auto swapImageView = m_backBuffers[0]->GetImageView(false);
Expand Down Expand Up @@ -836,14 +836,16 @@ namespace dxvk {


void D3D9SwapChainEx::DestroyBackBuffers() {
for (auto& backBuffer : m_backBuffers)
backBuffer->ClearContainer();
for (uint32_t i = 0; i < m_backBuffers.size(); i++) {
if (GetBackBuffer(i))
m_backBuffers[i]->ClearContainer();
}

m_backBuffers.clear();
}


void D3D9SwapChainEx::CreateBackBuffers(uint32_t NumBackBuffers) {
HRESULT D3D9SwapChainEx::CreateBackBuffers(uint32_t NumBackBuffers) {
// Explicitly destroy current swap image before
// creating a new one to free up resources
DestroyBackBuffers();
Expand All @@ -867,8 +869,14 @@ namespace dxvk {
desc.IsBackBuffer = TRUE;
desc.IsAttachmentOnly = FALSE;

for (uint32_t i = 0; i < m_backBuffers.size(); i++)
m_backBuffers[i] = new D3D9Surface(m_parent, &desc, this, nullptr);
try {
for (uint32_t i = 0; i < m_backBuffers.size(); i++)
m_backBuffers[i] = new D3D9Surface(m_parent, &desc, this, nullptr);
}
catch (const DxvkError& e) {
Logger::err(e.message());
return D3DERR_OUTOFVIDEOMEMORY;
}

auto swapImage = m_backBuffers[0]->GetCommonTexture()->GetImage();

Expand All @@ -892,6 +900,8 @@ namespace dxvk {

m_device->submitCommandList(
m_context->endRecording());

return D3D_OK;
}


Expand Down Expand Up @@ -1160,4 +1170,13 @@ namespace dxvk {
return this->GetParent()->IsExtended() ? "D3D9Ex" : "D3D9";
}

void D3D9SwapChainEx::ValidateBackBuffersPtrs() {
for (uint32_t i = 0; i < m_backBuffers.size(); i++) {
if (!GetBackBuffer(i))
{
throw DxvkError("D3D9SwapChainEx: Back buffer pointer is invalid - nullptr");
}
}
}

Comment on lines +1173 to +1181
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be possible to hit unless the app ignores the Reset error and goes on trying to present black frame buffers...

If you want to sanity check, just check m_backBuffers[0] being non-null at present time and log+error, all of them don't need to be checked.

Copy link

@wdebkows-intel wdebkows-intel Oct 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This shouldn't be possible to hit unless the app ignores the Reset error and goes on trying to present black frame buffers...

Yes, agree, that makes sense, but there is a test, which behaves this way and goes with Present despite of OOM error on Reset.

If you want to sanity check, just check m_backBuffers[0] being non-null at present time and log+error, all of them don't need to be checked.

Actually, in my scenario, only m_backBuffers[1] is null, m_backBuffers[0] is fine, so unless we're okay with crash on Present called after failed Reset, we should validate both backbuffers.

}
3 changes: 2 additions & 1 deletion src/d3d9/d3d9_swapchain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ namespace dxvk {

void DestroyBackBuffers();

void CreateBackBuffers(
HRESULT CreateBackBuffers(
uint32_t NumBackBuffers);

void CreateBlitter();
Expand Down Expand Up @@ -188,6 +188,7 @@ namespace dxvk {

std::string GetApiName();

void ValidateBackBuffersPtrs();
};

}