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

[dxgi] Leave fullscreen mode when window looses focus #2675

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

gofman
Copy link
Contributor

@gofman gofman commented Jun 9, 2022

Fixes Street Fighter V not being able to alt-tab out (and likely a few other games not properly atltabbing out or staying in maximized state after alttab, e. g., TEKKEN 7).

Game-wise I was only checking Street Fighter V so far, the alttab issue is reproduced on Windows with dxvk exactly as in Proton.

There is existing Wine dxgi test (dlls/dxgi/tests/dxgi.c: test_swapchain_present() which covers exactly the concerned functionality. The PR is fixing most of TODOs there for d3d10/11 (I am attaching the patch to vkd3d-proton for reference which fixes the same test with d3d12). I am also attaching the patch to Wine dxgi tests which removes fixed todos and comments out unrelated tests.

I also was making separate ad-hoc testing confirming that window fullscreen mode is restored on d3d10-11 even if _Present is never called at all, only _GetFullScreenState.

This is a workaround as:

  • the class of cases when the window is considered occluded is broader and not exactly that is not being foreground. But detecting the actual occlusion on X is very tricky if even possible;
  • On Windows this mechanics is related to output exclusive ownership which is taken when fullscreen mode is set and withdrawn by D3DKMT when window becomes occluded. So implementing that like on Windows would require implementing related D3DKMT functions in Wine, IDXGIOutput TakeOwnership, ReleaseOwnership in dxgi on top of that, interaction of those with Present and fullscreen mode in dxgi.

So I hope this will solve most of the practical issues related to the occlusion handling: unability to alttab at all or window staying fullscreen in a bunch of games.

The change is a bit risky, with the most risky part is not even introducing the bugs directly in dxgi but triggering bugs and races in winex11 by kicking window out of fullscreen as a quick reaction on some other window being focused. I already spotted and fixed one in Experimental BE which was reproducible right with the referenced test (bug was Proton specific, ValveSoftware/wine@5e9580b).

So I'd suggest the following:

  • I'd appreciate some comment if this solution is a go in principle, or maybe something instead it may be done;
  • Then, before committing, we may probably give it a bit of testing with more games in Proton to catch and fix some potential regressions early.

@gofman
Copy link
Contributor Author

gofman commented Jun 9, 2022

referenced_patches.zip

@K0bin K0bin added the dxgi label Jun 12, 2022
Copy link
Owner

@doitsujin doitsujin left a comment

Choose a reason for hiding this comment

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

Sorry for the super late review. Overall I'm okay with doing a change like this, but I have a few questions, see comments.



if (!m_descFs.Windowed && GetForegroundWindow() != m_window)
SetFullscreenState(FALSE, nullptr);
Copy link
Owner

Choose a reason for hiding this comment

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

I'm concerned that changing fullscreen modes here might cause infinite recursion in case the app tries to react to the window messages we're implicitly generating in SetFullscreenState; we've had problems like that before. Unless this particular change is known to fix an actual appliction I'd prefer to leave this one out for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as my debugging went exactly this part DxgiSwapChain::GetFullscreenState() is fixing Street Figther 5. Yes, the change is risky (both parts actually) and that's why I am suggesting some broader testing first before committing (internally in Proton) if this otherwise looks ok.

{
if (!(PresentFlags & DXGI_PRESENT_TEST))
SetFullscreenState(FALSE, nullptr);
return DXGI_STATUS_OCCLUDED;
Copy link
Owner

Choose a reason for hiding this comment

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

Is it intended to return DXGI_STATUS_OCCLUDED for only one single frame here?

Also, if the window becomes current again, does native DXGI automatically go into fullscreen mode again or does that require app intervention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DXGI_STATUS_OCCLUDED is returned whenever swapchain stays fullscreen while we are out of focus. Yes, in practice that will mean that once we presented and got out of fullscreen the next frame will be presented ok and won't return DXGI_STATUS_OCCLUDED (unless that was DXGI_PRESENT_TEST). I think this DXGI_STATUS_OCCLUDED return is covered by the linked test. Not sure what is the concern here, or am I probably missing something?

Regarding going fullscreen mode again, I think dxgi doesn't go fullscreen automatically. Also, I don't see a straightforward mechanism in its logic how would it do it. It gets kicked out of fullscreen by loosing exclusive ownership on the output (which is managed by D3DKMT upon detecting that the window got occluded). I can't immediately think of the mechanism which would do the opposite and somehow suggest dxgi that it should go back fullscreen.

@alasky17
Copy link

alasky17 commented Jan 9, 2023

The necessary wine changes were released with Proton 7.0-6. If it would be helpful, it is possible for the CW QA team to run the proposed dxvk change through our windowing testing plan. This is quite a time and labor intensive process, so we would only want to do it if there was a good chance of the change being accepted into dxvk after testing :) Please just let me know if you would like us to do any testing.

@gofman gofman force-pushed the dxgi_leave_fullscreen branch from c2385db to a3fd9d7 Compare March 24, 2023 00:53
@gofman
Copy link
Contributor Author

gofman commented Mar 24, 2023

dxgi should account for d3d12 differences now, I've updated the PR. Also attaching rebased test update (it doesn't add anything new to Wine tests, just removes the todo's fixed by the PR and comments out unrelated tests)
Uploading 0001-dxgi-tests-Remove-fixed-TODOs.txt…
.

@gofman gofman force-pushed the dxgi_leave_fullscreen branch from a3fd9d7 to a454ac2 Compare March 24, 2023 01:16
@gofman
Copy link
Contributor Author

gofman commented Mar 24, 2023

Now also moved minimize and occlusion state detection to wsi part.

@GloriousEggroll
Copy link
Contributor

GloriousEggroll commented May 6, 2023

msg'd paul directly but putting thoughts here also in case they are useful:

i had a user that was confused about MH:RISE on my build going from fullscreen to windowed mode when alt+tabbing because it doesn't happen in normal proton.

I informed him it was due to this patch, which is expected, and that Borderless Windowed Mode works fine and that's literally what it exists for.

Based on that I'm wondering if the action when alt+tabbing from a fullscreen window should be changed to a proper minimize instead of changing the window mode (like it does on windows when you alt+tab from a fullscreen)

Another idea on top of this is to gate the patch with an envvar -- so that games that do not have borderless fullscreen can still retain the old method of staying maximized/fullscreened when the envvar is active, or alternatively inverse where we only apply the envvar to a list of games via proton script and game ID like we do with various mfplat and nvapi games or via dxvk config.

some other comments/concerns brought up about it:

well, it's sort of annoying since a 2560x1440 window is slightly larger than that space...

mainly I'm just thinking about when kwin wayland's allow tearing option is fully implemented, will that count borderless windowed too?

just the way it works also makes rise's window go black for a second with a hitch while it's switching modes

@gofman
Copy link
Contributor Author

gofman commented May 6, 2023

Yes, certainly worth a check, but a couple of notes:

  • should be verified on Windows first if that is not Windows behaviour; also on Proton, not derivatives of it;
  • I think game specific option is not on the table here, it will create more hastle than patch is supposed to solve;
  • the issue may equally be in winex11 / fshack, if the issue is indeed present needs to be debugged before guessing solutions.

@WinterSnowfall
Copy link
Contributor

Based on that I'm wondering if the action when alt+tabbing from a fullscreen window should be changed to a proper minimize instead of changing the window mode (like it does on windows when you alt+tab from a fullscreen)

A bit of feedback on this bit, based on something I randomly ran into. It looks like The King of Fighters XIV also behaves like this when alt+tabbing out, namely you can't restore proper fullscreen unless you first minimize the window (using the taskbar) and then refocus it. Which makes me think it was expecting to be minimized when alt+tabbing out in the first place.

@gofman gofman force-pushed the dxgi_leave_fullscreen branch from a454ac2 to b809209 Compare August 20, 2024 21:20
@gofman
Copy link
Contributor Author

gofman commented Aug 20, 2024

Changes:

  • add more tests (attached): also test with d3d11 device and all the swapchain effects; test present count, present a few times to minimized window;
  • attribute occluded result with minimized window to flip swapchains and not d3d12;
  • still perform presentation when returning occluded status due to focus loss.

0001-dxgi-tests-Add-more-tests-for-swapchain-present.txt

@gofman gofman force-pushed the dxgi_leave_fullscreen branch 4 times, most recently from 352cb8b to e5b5543 Compare August 20, 2024 22:34
@simifor
Copy link
Contributor

simifor commented Aug 27, 2024

@GloriousEggroll MH Rise does become a window when alt-tabbing with this MR, but the game does the same on Windows. So this would be the expected behavior and not a regression.

@gofman gofman force-pushed the dxgi_leave_fullscreen branch from e5b5543 to e82abd6 Compare September 6, 2024 03:21
@gofman
Copy link
Contributor Author

gofman commented Sep 6, 2024

There was a regression found with this PR with Sekiro: Shadows Die Twice (814380): when going to settings being in fullscreen mode, changing resolution and then alt-tabbing out, instead of properly minimizing the game window starts frantically jumping between fullscreen and minimized state (instead of just becoming minimized).

The relevant parts of game behaviour in fullscreen mode are:

  1. It calls GetFullscreenState regularly from the main thread. If that become not fullscreen (like happens with alt-tabbing out with the present PR), it minimizes game window.
  2. When it receives WM_SIZE message it calls GetFullscreenState() from window procedure. If the state is not fullscreen it calls SetFullscreenState(TRUE).

So what happens when alt-tabbing out after resolution change is that during executing SetFullscreenMode from GetFullscreenState (due to "occlusion" detection) it receives WM_SIZE and that recursively calls SetFullscreenState, things go haywire in this process.

What happens on Windows, as I tested with updated unit test, is that just like with the present PR WM_SIZE is sent during IDXGISwapChain::GetFullscreenState(), and GetFullscreenState also returns not-fullscreen state. But SetFullscreenState returns DXGI_STATUS_MODE_CHANGE_IN_PROGRESS, good status perfectly explaining what is going on. Doing the same fixes the issue with the game. This seems to be othrogonal to what the original patch is doing (and the original patch only triggers the regression while so far doesn't seem to introduce wrong behaviour with the game), I added the fix a separate patch to the PR.

Attaching updated test.

0001-dxgi-tests-Add-more-tests-for-swapchain-present.txt

Note that currently it produces some test failures unrelated to the PR:

  • wrong refcount in the end of the test (Windows now has some extra for some reason, it wasn't the case back then when this patch was originally introduced);
  • multiple swapchain presents on minimized window (besides the very first one) produce DXGI_STATUS_OCCLUDED when it should not on Windows. It is coming from m_presenter->Present(), not from the logic introduced by the patch.

@gofman gofman force-pushed the dxgi_leave_fullscreen branch from e82abd6 to 4f4bf46 Compare September 6, 2024 15:50
@alasky17
Copy link

@gofman @doitsujin We tested these 2 commits with our classic test games, in addition to some games that are known to be improved (Dark Souls III, Sekiero: Shadows Die Twice, Street Fighter V and 6), and we didn't find any regressions. There were several games with improved behavior that match Windows better now :). In my (QA) opinion, this looks good to push.

@netborg-afps
Copy link

I have checked out the branch to see if it fixes an alt-tab blackscreen bug in Dirt Rally 2. Unfortunately it doesn't and my Gnome X11 session gets incredibly laggy/stuttery and there are weird effects like when opening system monitor, its window got filled with other graphics.

@doitsujin doitsujin merged commit ed9ffa6 into doitsujin:master Sep 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants