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.

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

[D3D9] Fix for back buffer data loss during D3D9 present #2967

Closed

Conversation

adamjer
Copy link
Contributor

@adamjer adamjer commented Sep 28, 2022

Acording to the D3D9 spec the content of the back buffer can be altered only if the SwapEffect is D3DSWAPEFFECT_DISCARD. The current implementation of the front buffer caused the previous back buffers to have their content lost.

This change keeps a similar present back buffer swapping logic for D3DSWAPEFFECT_DISCARD. For other swap effects the front buffer is a copy of the last presented back buffer.

@K0bin
Copy link
Collaborator

K0bin commented Sep 28, 2022

This might fix #2915

src/d3d9/d3d9_swapchain.cpp.bak Outdated Show resolved Hide resolved
@@ -673,6 +673,24 @@ namespace dxvk {
m_context->beginRecording(
m_device->createCommandList());

if (m_presentParams.SwapEffect != D3DSWAPEFFECT_DISCARD)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

if (...) {

@@ -673,6 +673,24 @@ namespace dxvk {
m_context->beginRecording(
m_device->createCommandList());

if (m_presentParams.SwapEffect != D3DSWAPEFFECT_DISCARD)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about flip?

Copy link
Contributor

Choose a reason for hiding this comment

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

According to the D3D9 spec:
When a swap chain is created with a swap effect of D3DSWAPEFFECT_FLIP or D3DSWAPEFFECT_COPY, the runtime will guarantee that an IDirect3DDevice9::Present operation will not affect the content of any of the back buffers.

This implies that the content of the back buffer that is presented needs to be copied to the front buffer image instead of swapping the reference (this would hide the content of the back buffer from the application).

@adamjer adamjer closed this Sep 28, 2022
@adamjer adamjer force-pushed the ProperFrontBufferImplementation branch from dd5103c to 49854db Compare September 28, 2022 17:17
@adamjer adamjer reopened this Sep 28, 2022
@@ -48,7 +48,6 @@ namespace dxvk {
this->strictPow = config.getOption<bool> ("d3d9.strictPow", true);
this->lenientClear = config.getOption<bool> ("d3d9.lenientClear", false);
this->numBackBuffers = config.getOption<int32_t> ("d3d9.numBackBuffers", 0);
this->noExplicitFrontBuffer = config.getOption<bool> ("d3d9.noExplicitFrontBuffer", false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Also noExplicitFrontBuffer variable declared in d3d9_options.h

@adamjer adamjer closed this Oct 3, 2022
@adamjer adamjer force-pushed the ProperFrontBufferImplementation branch from 5ae7cb4 to 49854db Compare October 3, 2022 15:12
@misyltoad
Copy link
Collaborator

?

Acording to the D3D9 spec the content of the back buffer can be altered only if the SwapEffect is D3DSWAPEFFECT_DISCARD. The current implementation of the front buffer caused the previous back buffers to have their content lost.

This change keeps a similar present back buffer swapping logic for D3DSWAPEFFECT_DISCARD. For other swap effects the front buffer is a copy of the last presented back buffer.
@adamjer adamjer reopened this Oct 4, 2022
@adamjer
Copy link
Contributor Author

adamjer commented Oct 4, 2022

First we need to merge #2964 to merge this.

Our coleague, responsible for #2964, is on sick leave, will be back 10.10.2022

Copy link
Contributor

@Blisto91 Blisto91 left a comment

Choose a reason for hiding this comment

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

Please remove the reference to your internal issue in the commit message as it points to random issues here 🙂

@adamjer adamjer changed the title [D3D9] Fix for back buffer data loss during D3D9 present. (#16) [D3D9] Fix for back buffer data loss during D3D9 present Oct 10, 2022
@frosty5689
Copy link

Any update on this PR and related PR? Hoping this would fix some dx9 games that have the partial black screen issue and doesn't get fixed with d3d9.shaderModel = 1.

@Blisto91
Copy link
Contributor

@frosty5689 what games have this issue?
If they don't already have an issue report then it would be great if you could make some.

@Blisto91
Copy link
Contributor

Blisto91 commented Nov 5, 2022

@adamjer Sorry for the relative silence in this PR and the others. There is currently a semi code freeze until the next release, which hopefully shouldn't be too long.

I finally got around to try and help the devs test this. I picked the commit from #2964 and the two from this into master and tried checking out the apitrace for the game Fantasy Ground Classic in this issue #1554 which is one of the two games the d3d9.noExplicitFrontBuffer configs were supposed to help with.
It seem that this PR reintroduces the original issue where the game will have the backgrounds blinking black.

@misyltoad
Copy link
Collaborator

I'd like to come back to this after DXVK 2.0 is released, there is a lot that can and probably will regress from this either in perf or functionality.

Copy link
Collaborator

@K0bin K0bin left a comment

Choose a reason for hiding this comment

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

auto swapImageView = m_backBuffers[0]->GetImageView(false);

m_blitter->presentImage(m_context.ptr(),
        m_imageViews.at(imageIndex), dstRect,
        swapImageView, srcRect);

Shouldn't we present the front buffer here?

@adamjer adamjer requested a review from mpyrzows March 1, 2023 08:45
@Blisto91
Copy link
Contributor

Blisto91 commented Mar 5, 2023

We've done a bunch of testing in some issues that we had hoped might have been fixed or affected by this PR (plus #2964). Posting the tested issues here for easy overview so it won't get lost.
Most of these are believed to be partial presentation issues and weren't expected to change, but were given a spin anyway.

#3250 no change. Thx @besentv
#2915 no change
#2732 no change
#2240 no change
#1476 no change
#2568 no change
#2542 no change
#2456 no change
#3116 no change
#3196 no change
#3208 no change
#3206 no change in apitrace
#2756 no change
#1554 apitrace regresses. Can't reproduce in launcher
#2860 Untested. Wine issue
#3005 Untested. Black screen
#3238 fixes issue 🎉
#1481 fixes issue 🎉

@w-flo
Copy link
Contributor

w-flo commented Mar 6, 2023

#3250 no change. Thx @besentv

This PR can't help Zusi because Zusi uses the DISCARD swapeffect (last time I checked, probably because it wants MSAA) but still expects COPY semantics, i.e. unchanged backbuffer contents after Present. This usually works fine on Windows in windowed mode, even though it is invalid use of the DirectX API. (Apparently flipping buffers would not help with performance when compositing is required, so there is no buffer flipping in windowed mode?)

I guess Zusi is not the only application (ab)using this behavior, so some kind of workaround would probably still be needed even when non-DISCARD swap effects are implemented correctly. So I'd ask to keep the noExplicitFrontBuffer config option that is removed in this PR. It would make dxvk unusable for Zusi otherwise.

Or maybe GetFrontBufferData() could be implemented similar to this to get rid of the "internal" dxvk frontbuffer, then the "internal" swapchain could be backbuffers only, so no "internal" flipping when there's only 1 backbuffer. I believe most/all (?) Vulkan implementations support VK_IMAGE_USAGE_TRANSFER_SRC_BIT for swapchain images.

@Blisto91
Copy link
Contributor

Blisto91 commented Mar 6, 2023

@w-flo Thank you for your thoughts on some of these issues. I don't have the technical knowledge to add anything myself to what you write sadly hehe.
If you want to then i am sure you are welcome to join the Linux Gaming Dev discord where some might have more insightful comments than me.

@pchome
Copy link
Contributor

pchome commented Mar 6, 2023

@Blisto91

Thank you for your thoughts on some of these issues. I don't have the technical knowledge to add anything myself to what you write sadly hehe. If you want to then i am sure you are welcome to join the Linux Gaming Dev discord where some might have more insightful comments than me.

If only other users was not able to read the issues in this repo and leave comments. This is perfect place for technical information.

@w-flo
Copy link
Contributor

w-flo commented Mar 7, 2023

@Blisto91 To be honest I'm not sure if I have the technical knowledge to do it either, and I'm not sure if it's even possible in a way that is "similar enough" to Windows behavior while not affecting performance in a bad way. I want to give it a try, maybe I can get some proof of concept to work. Or maybe the only feasible way forward is to keep the current "you can take a screenshot using GetFrontBufferData(), or you can avoid back buffer loss, but not both" situation (and maybe a third option "you can actually have both, but performance will suffer"?). Plus game-specific settings for which of the two/three options games prefer.

Also I don't have a discord account and I'm reluctant to create one.

@K0bin
Copy link
Collaborator

K0bin commented Apr 28, 2023

D3DSWAPEFFECT_FLIP

The swap chain might include multiple back buffers and is best envisaged as a circular queue that includes the front buffer. Within this queue, the back buffers are always numbered sequentially from 0 to (n - 1), where n is the number of back buffers, so that 0 denotes the least recently presented buffer. When Present is invoked, the queue is "rotated" so that the front buffer becomes back buffer (n - 1), while the back buffer 0 becomes the new front buffer.

That is pretty much exactly what DXVK already does.

@K0bin
Copy link
Collaborator

K0bin commented May 4, 2023

Superseded by #3392

@K0bin K0bin closed this May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants