GNOME Bugzilla – Bug 788493
Gnome 3.26 does not properly unredirect fullscreen windows
Last modified: 2017-11-27 16:12:41 UTC
There is a performance regression with gaming in Gnome 3.26. It doesn't seem to be properly unredirecting fullscreen windows again. I can tell by the lack of tearing in all my games even though I have vsync off in the game settings. This means compositing is on and forcing vsync on everything. I notice increased input lag and also frame stuttering. This also breaks Nvidia Gsync support. Fedora 27 Nvidia 384.90 drivers
Steps to Reproduce: 1. Launch a fullscreen game or fullscreen Firefox video playback 2. Disable vsync in-game. In Firefox, make sure layers acceleration is on its default setting of Off. 3. Play game in fullscreen. 4. Play a vsync test video on Youtube and fullscreen it. Actual results: Game experiences micro stutters, has vsync forced on by compositor, increased input lag. Expected results: Game experience is smooth due to correct unredirection of fullscreen windows. We will get tearing when in-game vsync is turned off. Youtube videos played in fullscreen on Firefox should also tear.
Can confirm this bug. Fast FPS games are unplayable because of input lag and stutter. Also gnome shell crashes when fullscreen games close normally (Tested this behaviour with Insurgency and CSGO). Arch Linux Gnome 3.26.1 Nvidia 387.12
Can confirm also. This can be seen as g-sync does not work with full screen windows, like they did with 3.24. Tested with Chromium and cs:go. Arch Linux Gnome 3.26.1 Nvidia 387.12
Yet another person who can confirm this bug and all the issues listed above. Arch Linux Gnome 3.26.1 Nvidia 387.12
Confirm too! I switch on Ubuntu 17.10 at the end of august > Gnome 3.24 at this moment. Switch to PPA Gnome 3 staging > Gnome 3.26. All games drop in performance. Purge PPA > Gnome 3.24 All performances refound. All tests with Nvidia 384.90 Can't test today because Ubuntu 17.10 has now Gnome 3.26.1 Ubuntu 17.10 64bits Gnome 3.26.1 Nvidia 387.12
Created attachment 361202 [details] bt of crash when closing fullscreen game Tested on 3 different fullscreen games, it crashes the same way when try to exit them. Arch Linux Gnome 3.26.1 Nvidia 387.12
This is not the same bt as #788666 but can they be related?
Can confirm, also getting a crash when closing Counter-Strike:Global Offensive. Also: I use tint2 to display window lists and set struts to limit the size of maximized windows. CS:GO is now following the struts as well (not maximizing over tint2), where as it did not before (maximizing over tint2 is the preferred behavior). tint2 version did not change with latest system upgrade (i.e., previous versions of gnome-shell and nvidia drivers did not trigger this behavior with the current version of tint2). Arch Linux gnome-shell-3.26.1-1 nvidia-387.12-1
Nick: It seems the patch from #78866 fixed my crash when closing game window. Try to compile mutter from source and see if it fixes it for you too.
Térence: Thanks for the quick response. I've already downgraded back to 3.24.3 to deal with the issue for now. Sorry for not providing more useful feedback.
Nick: If you want, here is the compiled package including the latest commit and a commit revert to fix performance issue with nvidia: https://drive.google.com/open?id=0B-u7ZMo7krOdazk2TGFuWlVVaEE
Térence, is that performance fix aimed at fixing unredirection in Gnome? I am afraid we're conflating two issues here. This bug is for Gnome's failure to unredirect fullscreen windows.
Sorry Benjamin, it only concerns Nick's problem, that is game crashing when trying to close it. I just saw he was also using nvidia prop drivers so I built it using https://aur.archlinux.org/packages/mutter-781835-workaround/ Nick's issue is related to https://bugzilla.gnome.org/show_bug.cgi?id=788666
I'm having this issue with the Intel drivers in Arch under X11.
Same issue on F27 and nvidia. Gsync don't work anymore
I'm also able to reproduce this on Arch. Makes fast FPS games unplayable. Arch Linux gnome-shell-3.26.1+3+g43ec5280b-1 mutter-3.26.1+7+g41f7a5fdf-1 nvidia-387.12-1
Seems to be caused by bug 780485. Reverting e3d5bc0 fixes the issue for me. FWIW, as far as I can see, the issue is that "top_window" (the last window in compositor->windows) is the "mutter" window (an window internal to mutter), so the code checking whether to unredirect doesn't check the top most fullscreen window.
Created attachment 361485 [details] [review] compositor: Ignore offscreen windows when unredirecting When determining whether we should unredirect a window or not, ignore offscreen windows, and just check the top most visible window. Previously this was not an issue, but since 'stack-tracker: Keep override redirect windows on top' we started sorting the UI frames window, which is an offscreen override redirect window, on top, causing the unredirect checking code to always check whether to unredirect the UI frames window. This effectively disabled the compositor bypass functionality.
(In reply to Jonas Ådahl from comment #18) > Created attachment 361485 [details] [review] [review] > compositor: Ignore offscreen windows when unredirecting > > When determining whether we should unredirect a window or not, ignore > offscreen windows, and just check the top most visible window. > > Previously this was not an issue, but since 'stack-tracker: Keep > override redirect windows on top' we started sorting the UI frames > window, which is an offscreen override redirect window, on top, causing > the unredirect checking code to always check whether to unredirect the > UI frames window. This effectively disabled the compositor bypass > functionality. I'm not completely happy with this solution, as it means we have a window actor for a window that we will never ever composite. It could probably be solved by never including any offscreen windows in the compositor window list, but that I suspect might result in brokenness if a window is initially offscreen then later moved into the screen region. This approach, while hacky, at least seems relatively safe.
Jonas, thanks for the quick fix! I can help test the patch if you want some feedback. I take it this applies cleanly on top of mutter 3.26.1? Do you happen to know the quickest way to rebuild mutter on Fedora? If not, I can always look up how to do it.
Check out https://wiki.gnome.org/action/show/Newcomers/BuildSystemComponent (build gnome-shell and run it with --replace).
@Jan, I was thinking of just rebuilding from SRPM with the patch applied. Is the above patch for mutter or gnome-shell?
@Jonas, I tested out the patch. Seems to work well! GSYNC works again. Games feel smoother. No forced vsync. Can we get this into an update for 3.26? This affects gaming performance for a lot of upcoming distro releases. For anyone else here who wants to test, here's how I built it on Fedora 27: 1. sudo dnf install rpmdevtools (install rpm build tools) 2. dnf download --source mutter (download mutter src rpm into the current dir) 3. rpmdev-setuptree (sets up a rpmbuild directory in your home folder where the build tools will create the rpms) 4. rpm -ivh mutter-<version>.src.rpm (installs source package into rpmbuild folder) 5. sudo dnf builddep mutter (install needed build dependencies) 6. download and copy compositor-Ignore-offscreen-windows-when-unredirec.patch to ~/rpmbuild/SOURCES 7. open up ~/rpmbuild/SPECS/mutter.spec and add a line "Patch2: compositor-Ignore-offscreen-windows-when-unredirec.patch" in the appropriate section. 8. rpmbuild -ba ~/rpmbuild/SPECS/mutter.spec 9. sudo dnf reinstall ~/rpmbuild/RPMS/x86_64/mutter-<version>.x86_64.rpm
(In reply to Jonas Ådahl from comment #18) > Previously this was not an issue, but since 'stack-tracker: Keep > override redirect windows on top' we started sorting the UI frames > window, which is an offscreen override redirect window, on top, causing > the unredirect checking code to always check whether to unredirect the > UI frames window. This effectively disabled the compositor bypass > functionality. I suppose alternatively we could do something like: diff --git a/src/core/stack-tracker.c b/src/core/stack-tracker.c index 82afd644a..97cce9798 100644 --- a/src/core/stack-tracker.c +++ b/src/core/stack-tracker.c @@ -1066,6 +1066,10 @@ meta_stack_tracker_keep_override_redirect_on_top (MetaStackTracker *tracker) for (i -= 1; i >= 0; i--) { + if (META_STACK_ID_IS_X11 (stack[i]) && + meta_ui_window_is_dummy (tracker->screen->ui, (Window)stack[i])) + continue; + window = meta_display_lookup_stack_id (tracker->screen->display, stack[i]); if (window && window->layer == META_LAYER_OVERRIDE_REDIRECT) {
@Florian, tested your patch. Seems to work just fine as well.
@Jonas and @Florian Discovered an issue. There's a few apps that seem to disable unredirect even though they're not the top most app. Simply having them open will disable unredirect for other fullscreen apps. Test case #1: 1. Install the Blizzard app via wine. Launch the Blizzard app. 2. Launch another game at the same time. I used Everspace in this case. 3. Fullscreen Everspace. Notice that redirection is still on. G-Sync doesn't work. 4. Close the Blizzard app. Go back to Everspace and notice unredirection works again. 5. Instead of Everspace, you can use a fullscreen Youtube video in Chrome. Test case #2: 1. Install the rpcs3 emulator and launch a game. 2. Fullscreen the game window by double clicking on it. 3. (Optional) Unfullscreen it. 4. Launch another game at the same time. I used Everspace again in this case. 5. Fullscreen Everspace. Notice that redirection is still on, G-Sync is disabled 6. Close the rpcs3 game. Notice that if you go back to Everspace, unredirection will start working again. 7. Instead of Everspace, you can use a fullscreen Youtube video in Chrome. Is this intended behavior? I know certain apps can request unredirection to be disabled, but does that make sense when they're not the top most app or even a fullscreen app? I don't think Gnome should allow disabling of unredirection if they're not the top-most app. Also all Wine games seem to be disabling unredirection even though they're fullscreen. I think that causes huge performance issues. Is this a Gnome or Wine issue?
I've applied the patch from @Florian Is there any sure-way to test if unredirect was turned on? I can't seem to tell if a game is going through the compositor or not.
@Ales, my g-sync monitor has a frame counter that shows FPS. G-sync doesn't work when compositor is on, so if the FPS matches my in-game fps then I know unredirect is working. If it stays constant 144, then I know compositor is still on. I can also tell by loading up a vsync test video in YouTube on Firefox. Firefox with basic acceleration (no hw accelerated layers) will tear when you full screen a YouTube video if unredirect is working.
@Benjamin I see, I'm running with RX580 on a non-special 1080p monitor I'm afraid so a bit harder but it seems to me I can't get tearing no matter what. I do get >60 FPS in e.g. unigine-heaven but I guess that doesn't mean vsync is off. Wondering if I could just add explicit logging to the `meta_stack_tracker_keep_override_redirect_on_top` function to see what's going on.
Created attachment 361644 [details] [review] x11/window: Don't manage InputOnly windows This was dropped by mistake in commit f166240225b6ab110b091520103d0370d51899ac. -- wine seems to create InputOnly windows and we are managing them for no reason.
Review of attachment 361485 [details] [review]: ::: src/compositor/compositor.c @@ +1098,3 @@ return TRUE; + top_window = get_top_visible_window_actor (compositor); this seems fine but I think we can and should avoid doing it in the draw path and instead keep a top_window_actor pointer in the compositor private and update it at the end of meta_compositor_sync_stack()
Review of attachment 361644 [details] [review]: This seems reasonable to me.
Created attachment 361648 [details] [review] compositor: Ignore offscreen windows when unredirecting When determining whether we should unredirect a window or not, ignore offscreen windows, and just check the top most visible window. Previously this was not an issue, but since 'stack-tracker: Keep override redirect windows on top' we started sorting the UI frames window, which is an offscreen override redirect window, on top, causing the unredirect checking code to always check whether to unredirect the UI frames window. This effectively disabled the compositor bypass functionality.
@Rui Matos Tried your patch (along with Jonas's newest patch). Now wine windows work properly and don't stop unredirect from happening. However, testcase #2, the rpcs3 testcase still happens.
Review of attachment 361648 [details] [review]: lgtm
(In reply to Benjamin Xiao from comment #34) > @Rui Matos > > Tried your patch (along with Jonas's newest patch). Now wine windows work > properly and don't stop unredirect from happening. However, testcase #2, the > rpcs3 testcase still happens. Please post the output of 'xwininfo -root -tree' when you're reproducing this? Probably the simplest way to capture it at the right moment is $ sleep 10 && xwininfo -root -tree and then launch or switch to the window that should be unredirected but isn't and wait till those 10 secs elapse.
Attachment 361644 [details] pushed as 4d763e1 - x11/window: Don't manage InputOnly windows Attachment 361648 [details] pushed as 2e99963 - compositor: Ignore offscreen windows when unredirecting
Also backported the current two accepted patches to gnome-3-26.
Created attachment 361714 [details] xwininfo output for rpcs3 and everspace test case @Rui Matos, I've uploaded the xwininfo output for testcase #2. Steps: 1. Launch rpcs3 and start Demon Souls 2. Fullscreen Demon Souls window 3. Unfullscreen Demon Soul window 4. Launch Everspace 5. Capture xwininfo output.
@Jonas, in which release of Gnome 3.26 will these two commits be included? I just want to know which package version to look out for in Fedora.
(In reply to Benjamin Xiao from comment #40) > @Jonas, in which release of Gnome 3.26 will these two commits be included? I > just want to know which package version to look out for in Fedora. They'll be part of 3.26.2, or possibly some 3.26.1-xyz if the patches are backported before the .2 release.
Created attachment 361758 [details] [review] compositor: Avoid a crash if the top window actor is finalized Since we're not holding a reference, the top window actor might be finalized when we paint resulting in a use after free crash. -- Oops, didn't think about this when reviewing Jonas' patch. We could also connect to the destroy signal or add a full reference. Any preference?
Arch Linux mutter was updated to 3.26.1+15+gb48c34979-1 and unreditect is working now, g-sync working in games and Chromium when fullscreen. Thanks!
@Rui Matos, What should I be looking for in the xwininfo output to indicate unredirection?
Review of attachment 361758 [details] [review]: Oops indeed.
(In reply to Rui Matos from comment #42) > Created attachment 361758 [details] [review] [review] > compositor: Avoid a crash if the top window actor is finalized > > Since we're not holding a reference, the top window actor might be > finalized when we paint resulting in a use after free crash. > -- > > Oops, didn't think about this when reviewing Jonas' patch. We could > also connect to the destroy signal or add a full reference. Any > preference? Keeping a reference doesn't seem like a good idea. At first glance, a weak ref seems like a good idea, but then that means we keep a reference to it even if it become unmanaged. Should we maybe instead recalculate it in meta_compositor_remove_window()?
I have a good experience with this bug. When I switch off Vsync then I have much more FPS. I see this with all games that I have. And this bug is not bug for me. No performance dropping, no stuttering, only no tearing. This is good. Usually when I turning off Vsync I have more FPS but also see tearing. I have NVIDIA GTX650. Now I'm reverting this patches for me, because I can play with more FPS without Vsync and witjout tearing. Example with Talos Principle without patches: http://storage2.static.itmages.ru/i/17/1018/h_1508323350_4404193_e154bb73bf.png With patches FPS doesn't change.
@Maxim, there's definitely an increase in input lag and frame stuttering. I think you can't feel it because on your system you're not able to go above 60 fps. For systems that can go above 60 fps, there's a huge difference in performance. You're being bottlenecked by other factors. There is also an extension that does exactly what you want, so you don't need to revert the patches. It will disable unredirection for you, which is the proper way to do it. https://github.com/kazysmaster/gnome-shell-extension-disable-unredirect
(In reply to Benjamin Xiao from comment #48) > There is also an extension that does exactly what you want, so you don't > need to revert the patches. It will disable unredirection for you, which is > the proper way to do it. > > https://github.com/kazysmaster/gnome-shell-extension-disable-unredirect Oh thanks! That what I want.
Created attachment 361868 [details] [review] compositor: Avoid a crash if the top window actor is finalized -- (In reply to Jonas Ådahl from comment #46) > Keeping a reference doesn't seem like a good idea. At first glance, a weak > ref seems like a good idea, but then that means we keep a reference to it > even if it become unmanaged. Both the MetaWindow and the MetaWindowActor instances should get finalized from meta_window_unmanage() so the weak reference would get cleared at the right time. But... > Should we maybe instead recalculate it in > meta_compositor_remove_window()? Yeah, doing it explicity in compositor_remove_window() seems like a better idea. I don't think we need to recalculate anything here though since meta_window_unmanage() queues a stack sync anyway.
(In reply to Rui Matos from comment #50) > Created attachment 361868 [details] [review] [review] > compositor: Avoid a crash if the top window actor is finalized > > -- > > (In reply to Jonas Ådahl from comment #46) > > Keeping a reference doesn't seem like a good idea. At first glance, a weak > > ref seems like a good idea, but then that means we keep a reference to it > > even if it become unmanaged. > > Both the MetaWindow and the MetaWindowActor instances should get > finalized from meta_window_unmanage() so the weak reference would get > cleared at the right time. But... Don't we potentially have lingering refs to those two in GJS waiting to be garbage collected?
Review of attachment 361868 [details] [review]: lgtm
(In reply to Jonas Ådahl from comment #51) > Don't we potentially have lingering refs to those two in GJS waiting to be > garbage collected? Right, so not using weak refs is preferable to be sure though in this case it would still fix the crash.
Attachment 361868 [details] pushed as 3caefd8 - compositor: Avoid a crash if the top window actor is finalized 297027b8c..6eacf9a39 gnome-3-26 -> gnome-3-26 12381d57d..3caefd8fd master -> master
Thanks everyone involved for the fix! Looking forward to see this as an update in Fedora 27.
Can confirm both issues are fixed now on Arch Linux with mutter 3.26.1+19+g193216c2a-1.
This is not fixed in all the cases. I'm always having this this crash when changing scaling level with the fractional scaling enabled. # Pastebin XEK7YcAM
+ Trace 238194
Fix is to add a weak pointer on the compositor's top_window_actor or to ensure we call meta_compositor_remove_window on window finalize too... Let me what you'd prefer, and if it's better to reuse this bug or opening a new one.