GNOME Bugzilla – Bug 683786
mutter-window-actor: Improve unredirect heuristic
Last modified: 2013-02-23 23:21:29 UTC
See patch. For some unknown reason the top panel can sometimes have a part of the unredirected window drawn on top of it after leaving the unredirected window (unfullscreen or close) still need to investiage that ... Otherwise from my testing we seem to do much better at detecting games with this heuristic added.
Created attachment 224002 [details] [review] mutter-window-actor: Improve unredirect heuristic Currently we only unredirect monitor sized override redirect windows. This was supposed to catch fullscreen opengl games and improve their performance. Newer games like fullscreen webgl games and SDL2 using games (like L4D) as well as wine based games do not use override redirect windows so we need a better heuristic to catch them. GLX windows always damage the whole window when calling glxSwapBuffers and never damage sub regions. So we can use that to detect them. The new heuristic unredirects windows fullscreen windows that have damaged the whole window more then 100 times in a row.
Review of attachment 224002 [details] [review]: I think I'm fine with this heuristic even if we get a proper hint. Some concerns about the details of the patch. ::: src/compositor/meta-window-actor.c @@ +1207,3 @@ + if (priv->full_damage_frames_count > 100) + return TRUE; Do we really want to unredirect windows that aren't full-monitor, even if they are on top and updating full-frame? I think generally no we don't - since "full frame" does include decorations it won't come up very often, but it seems to me that updating full-monitor should be a prerequisite. @@ +1243,3 @@ meta_error_trap_push (display); XCompositeRedirectWindow (xdisplay, xwin, CompositeRedirectManual); + XSync (xdisplay, FALSE); Why was this added? the meta error trap already handles the asynchronous nature of X errors @@ +1978,3 @@ priv->received_damage = TRUE; + if (G_UNLIKELY (meta_window_is_fullscreen (priv->window) && g_list_last (info->windows)->data == self)) { Never use G_UNLIKELY unless something *should not* happen - cache prediction works fine in the normal case. Indetation problems here and below. @@ +1999,3 @@ + else if (G_UNLIKELY (priv->full_damage_frames_count != 0)) + { + priv->full_damage_frames_count = 0; It seems to me bad if a partial-frame update - for whatever reason - causes a game to unredirect for 100 frames. I think basically once we apply this heuristic we probably want to keep applying it for the app unless it becomes non-full-monitor. So, I'd probably not check full_damage_frames_count directly from meta_window_actor_should_unredirect(), but rather set a bit on the window when we get 100 consecutive frames, and just leave it.
(In reply to comment #2) > > ::: src/compositor/meta-window-actor.c > @@ +1207,3 @@ > > + if (priv->full_damage_frames_count > 100) > + return TRUE; > > Do we really want to unredirect windows that aren't full-monitor, even if they > are on top and updating full-frame? No and that's not what the patch (is supposed to do). full_damage_frames_count is only incremented for fullscreen windows (which implies full monitor). "meta_window_is_fullscreen (priv->window) && g_list_last (info->windows)->data == self)" > I think generally no we don't - since "full frame" does include decorations it > won't come up very often, but it seems to me that updating full-monitor should > be a prerequisite. Yeah that's what it does. > @@ +1243,3 @@ > meta_error_trap_push (display); > XCompositeRedirectWindow (xdisplay, xwin, CompositeRedirectManual); > + XSync (xdisplay, FALSE); > > Why was this added? the meta error trap already handles the asynchronous nature > of X errors Though "this is missing" while reading the code but didn't think of the trap, will remove. > @@ +1978,3 @@ > priv->received_damage = TRUE; > > + if (G_UNLIKELY (meta_window_is_fullscreen (priv->window) && g_list_last > (info->windows)->data == self)) { > > Never use G_UNLIKELY unless something *should not* happen - cache prediction > works fine in the normal case. OK. > Indetation problems here and below. OK. > @@ +1999,3 @@ > + else if (G_UNLIKELY (priv->full_damage_frames_count != 0)) > + { > + priv->full_damage_frames_count = 0; > > It seems to me bad if a partial-frame update - for whatever reason - causes a > game to unredirect for 100 frames. I think basically once we apply this > heuristic we probably want to keep applying it for the app unless it becomes > non-full-monitor. That's what happens (this branch cannot be hit for fullscreen topmost windows).
Created attachment 226069 [details] [review] mutter-window-actor: Improve unredirect heuristic Currently we only unredirect monitor sized override redirect windows. This was supposed to catch fullscreen opengl games and improve their performance. Newer games like fullscreen webgl games and SDL2 using games (like L4D) as well as wine based games do not use override redirect windows so we need a better heuristic to catch them. GLX windows always damage the whole window when calling glxSwapBuffers and never damage sub regions. So we can use that to detect them. The new heuristic unredirects windows fullscreen windows that have damaged the whole window more then 100 times in a row.
Created attachment 226072 [details] [review] mutter-window-actor: Improve unredirect heuristic Currently we only unredirect monitor sized override redirect windows. This was supposed to catch fullscreen opengl games and improve their performance. Newer games like fullscreen webgl games and SDL2 using games (like L4D) as well as wine based games do not use override redirect windows so we need a better heuristic to catch them. GLX windows always damage the whole window when calling glxSwapBuffers and never damage sub regions. So we can use that to detect them. The new heuristic unredirects windows fullscreen windows that have damaged the whole window more then 100 times in a row. --- Remove debug prints.
Created attachment 226073 [details] [review] mutter-window-actor: Improve unredirect heuristic Currently we only unredirect monitor sized override redirect windows. This was supposed to catch fullscreen opengl games and improve their performance. Newer games like fullscreen webgl games and SDL2 using games (like L4D) as well as wine based games do not use override redirect windows so we need a better heuristic to catch them. GLX windows always damage the whole window when calling glxSwapBuffers and never damage sub regions. So we can use that to detect them. The new heuristic unredirects windows fullscreen windows that have damaged the whole window more then 100 times in a row.
Just a minor comment: would this patch not interfere with some presentational-type software? It would probably be a good idea to calculate some sort of FPS and unredirect if FPS is greater than 20 (or 10, or 30).
This is great. The heuristics needs to be better in time for 3.8 if you are supposed to remove the fallback mode. I get like 60 FPS in fallback and 20 in GNOME Shell. It's a huge penalty for gaming.
(In reply to comment #8) > This is great. The heuristics needs to be better in time for 3.8 if you are > supposed to remove the fallback mode. I get like 60 FPS in fallback and 20 in > GNOME Shell. It's a huge penalty for gaming. This may not be related to unredirection. Please try the patch before we land it. Note that we're also planning a better approach where applications can request unredirection based on a hint. (In reply to comment #7) > Just a minor comment: would this patch not interfere with some > presentational-type software? It would probably be a good idea to calculate > some sort of FPS and unredirect if FPS is greater than 20 (or 10, or 30). Only if it uses GLX and pulls a ton of full-screen redraws. This might apply to something like pinpoint, I'm not sure.
If I run games with proper override-redirect set like Torchlight or SDL1 games like World of Goo or play 1080p with XBMC I get the same FPS in fallback as in the Shell. Games like Team Fortress 2 and Serious Sam 3 and Wine games runs a lot faster in fallback so I'm pretty sure it's caused by composition overhead.
Please double-check with the patch if you can do so. Performance is not an easy thing to guess, especially when dealing with the radically different architectures between GNOME 2 and GNOME 3.
I've done some testing now and it works. I patched mutter 3.6.2 on a laptop and did some testing with FPS shown in game. It's exactly the same FPS in the Shell as in fallback. Some games leave the system cursor visible. I'm not sure about what this does to full damages but it doesn't seem to interfere. HexGL doesn't seem to unredirect though: http://hexgl.bkcore.com/ or does it? Is it because the mouse cursor is never hidden? -- I don't like that you can trigger the message tray when in full screen. Games that did the old override-redirect flag couldn't get interfered by the message tray but now that we have games not setting this flag you can interrupt game play by placing the cursor at the bottom. It's totally inconsistent because you cannot trigger Activities hotspot when in full screen so why should you be able to trigger the message tray? Games like Osmos that uses a mouse cursor shouldn't mess up just because you touched the lower edge of the screen during game play. Also - when watching YouTube full screen and you reach for the button to exit full screen you can trigger the message tray by mistake.
(In reply to comment #12) > I've done some testing now and it works. I patched mutter 3.6.2 on a laptop and > did some testing with FPS shown in game. It's exactly the same FPS in the Shell > as in fallback. Thanks, you rock! > Some games leave the system cursor visible. I'm not sure about what this does > to full damages but it doesn't seem to interfere. The system cursor is put composited onto the final scene by the graphics card, so it shouldn't make a difference. > HexGL doesn't seem to unredirect though: http://hexgl.bkcore.com/ or does it? > Is it because the mouse cursor is never hidden? My guess is because it doesn't do full-screen redraws, but I'll let Adel know about it on Monday. > -- > > I don't like that you can trigger the message tray when in full screen. > Games that did the old override-redirect flag couldn't get interfered by the > message tray but now that we have games not setting this flag you can interrupt > game play by placing the cursor at the bottom. It's totally inconsistent > because you cannot trigger Activities hotspot when in full screen so why should > you be able to trigger the message tray? > > Games like Osmos that uses a mouse cursor shouldn't mess up just because you > touched the lower edge of the screen during game play. Also - when watching > YouTube full screen and you reach for the button to exit full screen you can > trigger the message tray by mistake. You seemed to have found a bug. The trigger here is going to be rewritten for 3.8, but I'll try and make a patch so that that doesn't happen in the mean time...
(In reply to comment #13) > You seemed to have found a bug. The trigger here is going to be rewritten for > 3.8, but I'll try and make a patch so that that doesn't happen in the mean > time... Should I open a separate bug report for this? It's annoying and has been since 3.0.
(In reply to comment #12) > I've done some testing now and it works. I patched mutter 3.6.2 on a laptop and > did some testing with FPS shown in game. It's exactly the same FPS in the Shell > as in fallback. > > Some games leave the system cursor visible. I'm not sure about what this does > to full damages but it doesn't seem to interfere. > > HexGL doesn't seem to unredirect though: http://hexgl.bkcore.com/ or does it? > Is it because the mouse cursor is never hidden? No it is because it isn't fullscreen.
Does this heuristic apply to video players (say, mplayer/xv, totem)? Tearing while playing unredirected video with intel drivers has been a problem for Compiz developers.
Some Flash videos are being unredirected and some are not. I don't know if this is bugs in 3.6.2 but it's kind of buggy when changing volume using media keys. The volume indicator that shows up when changin volume brings up the last stored render of the full screen window when being unredirected. Why not just turn unredirection on when you have a full screen window without overpainted child windows or panels. The transition seems fluid. I believe KDE has a check box to unredirect all full screen windows and Unity 13.04 is doing this by default. I remember an argument was the v-sync given free to all apps when being composited. I rather have apps control it's own v-sync when being full screen. XBMC has perfect v-sync without being composited and games actually have better v-sync when not being composited. There's an open bug about the Shell tearing when using Intel / NVIDIA drivers. Games are not tearing when being unredirected.
Team Fortress 2 runs smooth 720p on r600g driver when in fallback but is totally unplayable in 640x480 when in (unpatched) Shell on my computer. Important fix if you are removing fallback IMO.
(In reply to comment #3) > (In reply to comment #2) > > > > > ::: src/compositor/meta-window-actor.c > > @@ +1207,3 @@ > > > > + if (priv->full_damage_frames_count > 100) > > + return TRUE; > > > > Do we really want to unredirect windows that aren't full-monitor, even if they > > are on top and updating full-frame? > > No and that's not what the patch (is supposed to do). full_damage_frames_count > is only incremented for fullscreen windows (which implies full monitor). If a window unfullscreens, could this happen for a frame before damage is received at the new size? It seems to me that we should make "does full screen update" an alternative to "is override redirect" as a qualifying criteria, and not bypass everything else. [...] > > @@ +1999,3 @@ > > + else if (G_UNLIKELY (priv->full_damage_frames_count != 0)) > > + { > > + priv->full_damage_frames_count = 0; > > > > It seems to me bad if a partial-frame update - for whatever reason - causes a > > game to unredirect for 100 frames. I think basically once we apply this > > heuristic we probably want to keep applying it for the app unless it becomes > > non-full-monitor. > > That's what happens (this branch cannot be hit for fullscreen topmost windows). It seems like I commented on the wrong branch, but there's another branch - also in the current patch that seems to run even in the fullscreen topmost case. I think you either need to have something like: if (priv->full_damage_frames_count < 100) priv->full_damage_frame_counts = 0; or use a boolean variable to track the "reached 100 consecutive frames" - which would probably be clearer.
(In reply to comment #17) > Some Flash videos are being unredirected and some are not. I don't know if this > is bugs in 3.6.2 but it's kind of buggy when changing volume using media keys. > The volume indicator that shows up when changin volume brings up the last > stored render of the full screen window when being unredirected. This should be fixed by the patch in bug 693042
(In reply to comment #20) > (In reply to comment #17) > > Some Flash videos are being unredirected and some are not. I don't know if this > > is bugs in 3.6.2 but it's kind of buggy when changing volume using media keys. > > The volume indicator that shows up when changin volume brings up the last > > stored render of the full screen window when being unredirected. > > This should be fixed by the patch in bug 693042 Great. But still - some Flash videos are not triggering the new heuristics even though they are fullscreen OpenGL through Chrome's Skia (or whatever). We have tried opt-in with little success (override-redirect). The problem is, not every developer are aware of GNOME and will never opt-in (Osmos, Valve, Wine, etc..). Heuristics may work in some cases (it's a great start for 3.8) but I really believe in opt-out from the default that is unredirection of all fullscreen apps that have no child windows. Apps that are not aware of GNOME and it's opt-out will obviously not be counting on being composited by Mutter so they are responsible for v-sync anyway (XBMC, Chrome, all games, etc..). If an app don't want to play along with Mutter they are responsible of handling v-sync. Otherwise it's a bug in the app, not in Mutter IMO.
(In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #17) > > > Some Flash videos are being unredirected and some are not. I don't know if this > > > is bugs in 3.6.2 but it's kind of buggy when changing volume using media keys. > > > The volume indicator that shows up when changin volume brings up the last > > > stored render of the full screen window when being unredirected. > > > > This should be fixed by the patch in bug 693042 > > Great. > > But still - some Flash videos are not triggering the new heuristics even though > they are fullscreen OpenGL through Chrome's Skia (or whatever). I doubt this. Unless the videos do not damage the whole screen. In that case you don't really need unredirection anyway. Any specific example? > We have tried opt-in with little success (override-redirect). That wasn't opt in. That was a heuristic as well. > The problem is, > not every developer are aware of GNOME That's why we have added a hint to the wm-spec (that is not GNOME specific). >and will never opt-in (Osmos, Valve, > Wine, etc..). I have no idea what Osmos is. Valve just uses SDL2 which is caught by this heuristic (and we can just fix SDL2 without any Valve involvement). For wine this works better because only apps that need it will get unredirected (like games running in wine). And not just all > Heuristics may work in some cases (it's a great start for 3.8) > but I really believe in opt-out from the default that is unredirection of all > fullscreen apps that have no child windows. > > Apps that are not aware of GNOME and it's opt-out will obviously not be > counting on being composited by Mutter so they are responsible for v-sync > anyway (XBMC, Chrome, all games, etc..). If an app don't want to play along > with Mutter they are responsible of handling v-sync. Otherwise it's a bug in > the app, not in Mutter IMO. X does not have any vsync mechanism. So no unless the app uses opengl (or a video overlay like Xv) it cannot just "handle vsync".
> X does not have any vsync mechanism. So no unless the app uses opengl (or a > video overlay like Xv) it cannot just "handle vsync". And a fullscreen GL app has no choice but to damage the whole screen (when calling swap buffers). So I have no idea what problem you are trying to describe / solve.
(In reply to comment #19) > (In reply to comment #3) > > (In reply to comment #2) > > > > > > > > ::: src/compositor/meta-window-actor.c > > > @@ +1207,3 @@ > > > > > > + if (priv->full_damage_frames_count > 100) > > > + return TRUE; > > > > > > Do we really want to unredirect windows that aren't full-monitor, even if they > > > are on top and updating full-frame? > > > > No and that's not what the patch (is supposed to do). full_damage_frames_count > > is only incremented for fullscreen windows (which implies full monitor). > > If a window unfullscreens, could this happen for a frame before damage is > received at the new size? It seems to me that we should make "does full screen > update" an alternative to "is override redirect" as a qualifying criteria, and > not bypass everything else. > OK. > > > > @@ +1999,3 @@ > > > + else if (G_UNLIKELY (priv->full_damage_frames_count != 0)) > > > + { > > > + priv->full_damage_frames_count = 0; > > > > > > It seems to me bad if a partial-frame update - for whatever reason - causes a > > > game to unredirect for 100 frames. I think basically once we apply this > > > heuristic we probably want to keep applying it for the app unless it becomes > > > non-full-monitor. > > > > That's what happens (this branch cannot be hit for fullscreen topmost windows). > > It seems like I commented on the wrong branch, but there's another branch - > also in the current patch that seems to run even in the fullscreen topmost > case. > I think you either need to have something like: > > if (priv->full_damage_frames_count < 100) > priv->full_damage_frame_counts = 0; > > or use a boolean variable to track the "reached 100 consecutive frames" - which > would probably be clearer. OK.
@drago01 Okay then. You sure know this better than I (former Windows guy). Let's try this new heuristics in 3.8. It will be a huge improvement over previous releases.
Created attachment 235071 [details] [review] mutter-window-actor: Improve unredirect heuristic Currently we only unredirect monitor sized override redirect windows. This was supposed to catch fullscreen opengl games and improve their performance. Newer games like fullscreen webgl games and SDL2 using games (like L4D) as well as wine based games do not use override redirect windows so we need a better heuristic to catch them. GLX windows always damage the whole window when calling glxSwapBuffers and never damage sub regions. So we can use that to detect them. The new heuristic unredirects windows fullscreen windows that have damaged the whole window more then 100 times in a row.
Review of attachment 235071 [details] [review]: ::: src/compositor/meta-window-actor.c @@ +1177,1 @@ !meta_window_requested_bypass_compositor (metaWindow)) Argh - the checks are turning into spaghetti! I have a lot of trouble deciding if this is correct or not. My suggestion for a cleanup. Do the disqualifications first, then the qualifications: if (meta_window_requested_dont_bypass_compositor (metaWindow)) return FALSE; if (priv->opacity != 0xff) return FALSE; if (metaWindow->has_shape) return FALSE; if (priv->argb32 && !meta_window_requested_bypass_compositor (metaWindow)) return FALSE; Then determine if the window is fullscreen or fullscreen-override-redirect - if neither, is true, then we're also disqualified. gboolean occupies_full_monitors = FALSE; if (meta_window_is_fullscreen (metaWindow)) { occupies_full_monitors = TRUE; } else if (meta_window_is_override_redirect (metaWindow)) { /* 'fullscreen' hint doesn't apply to O-R windows, so check manually */ /* Do manual checks */ } if (!occupies_full_monitors) return FALSE; Then handle the bypass compositor hint: if (meta_window_requested_bypass_compositor (metaWindow)) return TRUE; Then do the heuristics: if (meta_window_is_override_redirect (metaWindow)) return TRUE; if (priv->does_full_damage) return TRUE; Something like that.
Created attachment 235227 [details] [review] mutter-window-actor: Improve unredirect heuristic Currently we only unredirect monitor sized override redirect windows. This was supposed to catch fullscreen opengl games and improve their performance. Newer games like fullscreen webgl games and SDL2 using games (like L4D) as well as wine based games do not use override redirect windows so we need a better heuristic to catch them. GLX windows always damage the whole window when calling glxSwapBuffers and never damage sub regions. So we can use that to detect them. The new heuristic unredirects windows fullscreen windows that have damaged the whole window more then 100 times in a row. --- 'Unspaghettified' the checks.
Review of attachment 235227 [details] [review]: Looks good
Attachment 235227 [details] pushed as 90f2a3a - mutter-window-actor: Improve unredirect heuristic
*** Bug 694550 has been marked as a duplicate of this bug. ***