GNOME Bugzilla – Bug 597014
Unredirect fullscreen windows
Last modified: 2011-08-29 21:14:49 UTC
Created attachment 144516 [details] [review] Make mutter_window_detach() public The attached patch makes the mutter_window_detach() function public; this allows a mutter plugin to force rebinding of the texture associated with MutterWindow. (As a background to this, we are looking at addressing some performance issues in Moblin; one particularly unsatisfactory situation performance-wise is when a user is viewing a fullscreen video. As a way of addressing this, we are experimenting with dropping out of the compositor when a fullscreen application is present. This is quite simple to do from the plugin, but requires to rebind all MutterWindows when we return back into the compositing mode.)
Comment on attachment 144516 [details] [review] Make mutter_window_detach() public * pixmap for a new size. + * + * This function is intended for internal use; if you are calling it externally, + * you are likely doing something you should not. That doesn't really make sense to me - our "users" are plugins like mutter-moblin and gnome-shell. We can't have an API that is "internal" but used by mutter-moblin. That doesn't make sense. The comment should instead explain why you might want to use it. I do think that the better API here is mutter_window_set_redirected() Let Mutter worry about dropping the texture when unredirecting and reattaching when redirecting the window again.
(In reply to comment #1) > (From update of attachment 144516 [details] [review]) > * pixmap for a new size. > + * > + * This function is intended for internal use; if you are calling it > externally, > + * you are likely doing something you should not. > > That doesn't really make sense to me - our "users" are plugins like > mutter-moblin and gnome-shell. We can't have an API that is "internal" but used > by mutter-moblin. That doesn't make sense. That comment reflects my misgivings about exposing this function. A cleaner solution would be to fold the entire redirection/unredirection into mutter, but I am not entirely convinced this whole approach is a particularly satisfactory way of addressing the performance problem, so not sure we want to land this code (though small) in mutter itself. I can prepare a patch for that if you think that would be a preferable.
Created attachment 145120 [details] [review] original patch making mutter_window_detach publin, but with updated documentation Updated the documentation comment on the original patch (the actual change is still required for the following patch).
Created attachment 145121 [details] [review] Temporary disabling of compositing The attached patch extracts the code we currently have into mutter-moblin for temporary disabling of compositing, as mentioned in comment#0. This patch requires the previous patch as well.
Review of attachment 145121 [details] [review]: Is it possible to just unredirect the one window rather than all of them? Unredirecting all of them temporarily saves memory, but will force all apps to be swapped in and to process exposes when you redirect the window again, which is going to be quite disruptive to performance and appearance. Various comments in detail. ::: src/compositor/compositor.c @@ +1097,3 @@ +mutter_detach_all_mutter_windows (MetaScreen *screen) +{ + GList* l = mutter_get_windows (screen); GList *l not GList* l @@ +1099,3 @@ + GList* l = mutter_get_windows (screen); + + while (l) I'd use a for () loop @@ +1102,3 @@ + { + MutterWindow *m = l->data; + if (m) How could this be NULL? @@ +1104,3 @@ + if (m) + { + /* we need to repair the window pixmap here */ This comment is clearly in the wrong place - this function is called detached_all_mutter_windows(), having an explanation about why it calls mutter_window_detach() doesn't make sense @@ +1115,3 @@ + * mutter_set_compositing_disabled_for_screen: + * @screen: a #MetaScreen + * @disable: boolean value indiacating whether compositing should be disabled set_disabled() should take a boolean called 'disabled' not 'disable'; but would making this the other way around - mutter_set_compositing_enabled_for_screen() cause less double-negatives? @@ +1127,3 @@ + MetaCompScreen *info; + MetaDisplay *display; + Display *xdpy; In compositor/ we use xdisplay in 17 places and xdpy in 7 @@ +1137,3 @@ + if ((info->compositing_disabled && disable) || + (!info->compositing_disabled && !disable)) + return; disable = disable != FALSE; if (info->compositing_disabled == disable) return; @@ +1163,3 @@ + * immediately at the correct size, with correct contents, but there is a + * delay before the contents of the screen behind the application and + * the application frame are drawn. (In the oposite order, we get a brief A fullscreen application has neither a window border nor "screen behind it". Not sure what is meant here. @@ +1167,3 @@ + * application. + */ + XMapWindow (xdpy, overlay); If the overlay has background None, then the contents of the screen before compositing was reenabled should stick around until we draw another frame. @@ +1171,3 @@ + xroot, + CompositeRedirectManual); + XSync (xdpy, FALSE); Why? @@ +1194,3 @@ + + if (info->compositing_disabled) + return TRUE; Why not just return info->compositing_disabled;
(In reply to comment #5) > Review of attachment 145121 [details] [review]: > > Is it possible to just unredirect the one window rather than all of them? > Unredirecting all of them temporarily saves memory, but will force all apps to > be swapped in and to process exposes when you redirect the window again, which > is going to be quite disruptive to performance and appearance. I briefly considered doing this, but running in a mixed mode is tricky. We do not redirect individual windows, but redirect subwindows for the root window, so if you just unredirect the fullscreen window, and some other window pops up, things become interesting, as the second window is redirected and painted by the compositor into the overlay. Unredirecting the whole shebang is lot simpler, and, to my surprise, not that particularly disruptive. > @@ +1115,3 @@ > + * mutter_set_compositing_disabled_for_screen: > + * @screen: a #MetaScreen > + * @disable: boolean value indiacating whether compositing should be disabled > > set_disabled() should take a boolean called 'disabled' not 'disable'; but would > making this the other way around - mutter_set_compositing_enabled_for_screen() > cause less double-negatives? I love double negatives :). No really, the reason for calling it disabled is that the normal, default, state is enabled, and you only use this function if you want to disrupt that normality. > > @@ +1127,3 @@ > + MetaCompScreen *info; > + MetaDisplay *display; > + Display *xdpy; > > In compositor/ we use xdisplay in 17 places and xdpy in 7 We should clean it up; which do you prefer ? > @@ +1163,3 @@ > + * immediately at the correct size, with correct contents, but there is > a > + * delay before the contents of the screen behind the application and > + * the application frame are drawn. (In the oposite order, we get a > brief > > A fullscreen application has neither a window border nor "screen behind it". > Not sure what is meant here. The comment assumes you are toggling the compositing on and off because of an fullscreen application (though admittedly, this only makes sense in the mutter-moblin contex). When you again redirect the root window and its subwidnows, there is a short, but observable, delay in the compositing coming online. During this interval you get to see see the a black background, which iirc is the overlay window, and application windows without decorations painted. > > @@ +1167,3 @@ > + * application. > + */ > + XMapWindow (xdpy, overlay); > > If the overlay has background None, then the contents of the screen before > compositing was reenabled should stick around until we draw another frame. That does not seem to be the case, perhaps because the overlay window is not the clutter stage window, so it does not have any contents to start with ? > @@ +1171,3 @@ > + xroot, > + CompositeRedirectManual); > + XSync (xdpy, FALSE); > > Why? IIRC, that was necessary for things to work, but I will need to check.
(In reply to comment #6) > (In reply to comment #5) > > Review of attachment 145121 [details] [review] [details]: > > > > Is it possible to just unredirect the one window rather than all of them? > > Unredirecting all of them temporarily saves memory, but will force all apps to > > be swapped in and to process exposes when you redirect the window again, which > > is going to be quite disruptive to performance and appearance. > > I briefly considered doing this, but running in a mixed mode is tricky. We do > not redirect individual windows, but redirect subwindows for the root window, > so if you just unredirect the fullscreen window, and some other window pops up, > things become interesting, as the second window is redirected and painted by > the compositor into the overlay. Unredirecting the whole shebang is lot > simpler, and, to my surprise, not that particularly disruptive. Certainly it's way simpler - I'm OK with a patch going in like this; I'm not sure it's going to be smooth enough that I'd want to do it this way for gnome-shell, but we can see how it works out. > > @@ +1115,3 @@ > > + * mutter_set_compositing_disabled_for_screen: > > + * @screen: a #MetaScreen > > + * @disable: boolean value indiacating whether compositing should be disabled > > > > set_disabled() should take a boolean called 'disabled' not 'disable'; but would > > making this the other way around - mutter_set_compositing_enabled_for_screen() > > cause less double-negatives? > > I love double negatives :). No really, the reason for calling it disabled is > that the normal, default, state is enabled, and you only use this function if > you want to disrupt that normality. Yeah, there are competing tensions here. I would go would set_compositing_enabled(), but I see where you are coming from. > > @@ +1127,3 @@ > > + MetaCompScreen *info; > > + MetaDisplay *display; > > + Display *xdpy; > > > > In compositor/ we use xdisplay in 17 places and xdpy in 7 > > We should clean it up; which do you prefer ? I definitely prefer xdisplay. > > @@ +1163,3 @@ > > + * immediately at the correct size, with correct contents, but there is > > a > > + * delay before the contents of the screen behind the application and > > + * the application frame are drawn. (In the oposite order, we get a > > brief > > > > A fullscreen application has neither a window border nor "screen behind it". > > Not sure what is meant here. > > The comment assumes you are toggling the compositing on and off because of an > fullscreen application (though admittedly, this only makes sense in the > mutter-moblin contex). When you again redirect the root window and its > subwidnows, there is a short, but observable, delay in the compositing coming > online. During this interval you get to see see the a black background, which > iirc is the overlay window, and application windows without decorations > painted. That doesn't really make sense to me... once the overlay window is mapped, what is going on behind it won't matter. > > @@ +1167,3 @@ > > + * application. > > + */ > > + XMapWindow (xdpy, overlay); > > > > If the overlay has background None, then the contents of the screen before > > compositing was reenabled should stick around until we draw another frame. > > That does not seem to be the case, perhaps because the overlay window is not > the clutter stage window, so it does not have any contents to start with ? Hmm, you might be on to something there - you should try adding code to make the Clutter stage window background=None, I bet things will look a bit better, though you probably will still get some artifacts as clutter draws frames before everything has finished redrawing. > > @@ +1171,3 @@ > > + xroot, > > + CompositeRedirectManual); > > + XSync (xdpy, FALSE); > > > > Why? > > IIRC, that was necessary for things to work, but I will need to check. Can't imagine that it is necessary, or if it is, something else bad is going on.
Review of attachment 145120 [details] [review]: Fine, if the prototype is moved to mutter-window-private.h ::: src/include/mutter-window.h @@ +68,3 @@ const char * mutter_window_get_description (MutterWindow *mcw); gboolean mutter_window_showing_on_its_workspace (MutterWindow *mcw); +void mutter_window_detach (MutterWindow *mcw); Shouldn't this be in mutter-window-private.h?
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > Review of attachment 145121 [details] [review] [details] [details]: > > > > > > Is it possible to just unredirect the one window rather than all of them? > > > Unredirecting all of them temporarily saves memory, but will force all apps to > > > be swapped in and to process exposes when you redirect the window again, which > > > is going to be quite disruptive to performance and appearance. > > > > I briefly considered doing this, but running in a mixed mode is tricky. We do > > not redirect individual windows, but redirect subwindows for the root window, > > so if you just unredirect the fullscreen window, and some other window pops up, > > things become interesting, as the second window is redirected and painted by > > the compositor into the overlay. Unredirecting the whole shebang is lot > > simpler, and, to my surprise, not that particularly disruptive. > > Certainly it's way simpler - I'm OK with a patch going in like this; I'm not > sure it's going to be smooth enough that I'd want to do it this way for > gnome-shell, but we can see how it works out. Well without testing this, this might result in all kind of weirdness on dual (multi) display setups. Lets say we unredirect fullscreen windows using this function. The user runs a fullscreen presentation on the secondary monitor and we just unredirect everything. Wouldn't this mean that everything on the primary monitor would behave weird?
(In reply to comment #9) > (In reply to comment #7) > > (In reply to comment #6) > > > (In reply to comment #5) > > > > Review of attachment 145121 [details] [review] [details] [details] [details]: > > > > > > > > Is it possible to just unredirect the one window rather than all of them? > > > > Unredirecting all of them temporarily saves memory, but will force all apps to > > > > be swapped in and to process exposes when you redirect the window again, which > > > > is going to be quite disruptive to performance and appearance. > > > > > > I briefly considered doing this, but running in a mixed mode is tricky. We do > > > not redirect individual windows, but redirect subwindows for the root window, > > > so if you just unredirect the fullscreen window, and some other window pops up, > > > things become interesting, as the second window is redirected and painted by > > > the compositor into the overlay. Unredirecting the whole shebang is lot > > > simpler, and, to my surprise, not that particularly disruptive. > > > > Certainly it's way simpler - I'm OK with a patch going in like this; I'm not > > sure it's going to be smooth enough that I'd want to do it this way for > > gnome-shell, but we can see how it works out. > > Well without testing this, this might result in all kind of weirdness on dual > (multi) display setups. > > Lets say we unredirect fullscreen windows using this function. > > The user runs a fullscreen presentation on the secondary monitor and we just > unredirect everything. > > Wouldn't this mean that everything on the primary monitor would behave weird? OK, as we have to unmap the COW this can't be avoided anyway ...
(In reply to comment #10) > > Lets say we unredirect fullscreen windows using this function. > > > > The user runs a fullscreen presentation on the secondary monitor and we just > > unredirect everything. > > > > Wouldn't this mean that everything on the primary monitor would behave weird? > > OK, as we have to unmap the COW this can't be avoided anyway ... Could we shape the COW? (output shape - we currently shape the input shape)
(In reply to comment #11) > (In reply to comment #10) > > > Lets say we unredirect fullscreen windows using this function. > > > > > > The user runs a fullscreen presentation on the secondary monitor and we just > > > unredirect everything. > > > > > > Wouldn't this mean that everything on the primary monitor would behave weird? > > > > OK, as we have to unmap the COW this can't be avoided anyway ... > > Could we shape the COW? (output shape - we currently shape the input shape) Yeah it is "just a window" after all so I can't think of a reason why it wouldn't work.
Created attachment 161088 [details] [review] Add meta_window_is_fullscreen Add accessor for the fullscreen property.
Created attachment 161089 [details] [review] Unredirect fullscreen windows Some apps that do a lot of rendering on the screen like games, mostly run in fullscreen where there is no need for them to be redirected doing so does add an overhead; while performance is critical for those apps. Some benchmarks (openarena on GM45): Before: 840 frames 27.2 seconds 30.9 fps 8.0/32.3/62.0/8.6 ms 840 frames 27.8 seconds 30.2 fps 12.0/33.1/54.0/3.3 ms After: 840 frames 16.1 seconds 52.1 fps 8.0/19.2/45.0/5.9 ms 840 frames 16.2 seconds 51.9 fps 8.0/19.3/47.0/6.0 ms This patch achieves this by unredirecting the window instead, other windows are still redirected, the COW is shaped so in a multi screen configuration other screens aren't affected. (screen = monitor). It contains code to unredirect both fullscreen windows and screen sized override-redirect windows. Just doing the later seems to catch every game I tried (as SDL which most games use does fullscreening this way). Unredirecting all fullscreen windows has the side effect that we catch windows like fullscreen firefox where opening a submenu can cause flicker doing to redirecting it again. The only case we miss with the override-redirect heuristic is video playback; but the overhead does not seem to an issue here as most work is the actuall decoding rather then drawing on screen. Anyway the patch contains both codepaths so that those can be tested.
Review of attachment 161088 [details] [review]: Looks good to commit after minor fixes ::: src/core/window.c @@ +8856,3 @@ + * @window: A #MetaWindow + * + * Returns if this window is a fullscreen window. Not a real doc comment, should be: Return value: %TRUE if this is a fullscreen window @@ +8860,3 @@ +gboolean +meta_window_is_fullscreen (MetaWindow *window) +{ I've been asking people to add: g_return_val_if_fail (META_IS_WINDOW (window), NULL); To new publicly exported functions in Mutter, as if Mutter were a normal library.
Review of attachment 161089 [details] [review]: I like this general approach - shaping the COW seems like a good way to handle multihead, and 'fullscreen override-redirect windows' seems like a good heuristic. For the multihead case, it might be nice to eventually set up a clip on the stage when painting so we don't spend time painting the stuff that's underneath the unredirected window. But my expectation is that the normal case is: - Lots of drawing on the redirected window - Very little drawing of anything else So that's a minor optimization that can be left for the distant future. Various comments below about the implementation. Suppressing or reducing the flash when redirecting/unredirecting might also be possible with enough care - see some comments above about making the ClutterStage background none. But also not very important if what we are trying to handle is full-screen games that go fullscreen and stay fullscreen. ::: src/compositor/compositor.c @@ +97,3 @@ + MutterWindow *cw; + MetaScreen *screen; + MetaCompScreen *info; Weird alignment. Avoid tabs except at the start of the line. You don't need to line up local variables, you can just have 'MetaScreen *screen;'. (I don't mind having local variables lined up, but don't really like realigning them when adding or removing variables, so I tend not to do so. This is different from function parameter lists which don't change as often) @@ +824,3 @@ GList *tmp; + MutterWindow *top_most = (g_list_last (windows))->data; a) topmost is one word. b) This will crash if windows == NULL. @@ +838,3 @@ + meta_window_get_outer_rect (window, &window_rect); + meta_screen_get_size (screen, &width, &height); + /* FIXME: restrict to monitor */ This FIXME needs to be fixed before landing the patch, I think; though you might need to handle *either* monitor-sized or screen-sized windows, since games played multihead or gnome-screensaver may have a single window covering multiple monitors (maybe check the gnome-screensaver sources to see what it does.) You may also need to handle the monitor <=> screen transition - imagine a preference in a game about multihead behavior. It's fine to handle it with an redirect/unredirect like the redirected window changed, though. @@ +839,3 @@ + meta_screen_get_size (screen, &width, &height); + /* FIXME: restrict to monitor */ + if (window_rect.width == width && window_rect.height == height) Should check x,y as well. Generally, it bothers me to have the concept: "unredirect the window if it's an override redirect window that is at the top of the stack and is the size of the screen/current monitor" But only to check this when checking stacking and not when the window is moved/resized. I'd rather see that enforced accurately. So, you need to have code in mutter_window_sync_actor_position() as well. What I'd do is something like update a window->is_topmost flag here, and update a window->is_monitor_sized flag in sync_actor_position(), then have the actual logic for updating redirection in a shared function. @@ +871,3 @@ + int width, height; + rect.x = 0; + rect.y = 0; some blank lines would make things more readable; I'd also probably move setting up rect to where you use it and call it screen_rect @@ +879,3 @@ + info->unredirected_window = top_most; + XCompositeUnredirectWindow (xdisplay, xwin, CompositeRedirectManual); + info->output_region = XFixesCreateRegionFromWindow (xdisplay, xwin, WindowRegionBounding); Is there any reason to save the output region rather than just destroying it immediately.
(In reply to comment #14) > It contains code to unredirect both fullscreen windows and screen sized > override-redirect windows. Just doing the later seems to catch every game > I tried (as SDL which most games use does fullscreening this way). Doing 'fullscreening' this way in an application is broken; we have to live with it on the WM level, but we should not consider it the normal case, and need to have the normal case (below) handled as a matter of priority (i.e., optimizing for applications that are broken whilst leaving EWMH compliant applications to suffer performance hit in comparison is bad policy). > Unredirecting all fullscreen windows has the side effect that we catch windows > like fullscreen firefox where opening a submenu can cause flicker doing to > redirecting it again. This is a problem that is inherent to the single toplevel window unredirection (i.e., it will apply to the OR screen sized windows above as well, it's just that such apps will be creating transients less often). Unless we can find a way to get the transition flickerless, I don't think we can go down this route, but rather need to unredirect any window that is in a fullscreen state, and all of its transients. The only real benefit in partial unredirection is for a multi-monitor set up, but then you end up with the shell overlay not working as expected, and hard to predict in what way not working depending which monitor the fullscreen app is on. IMO it is questionable whether from the user experience point of view this is not worse than having the compositing turned off completely, and knowing that is the case. There are serious drawbacks to turning compositing off in any shape an form, and I am wondering if it might not be better in the long run to invest into improving the compositing performance instead (e.g., in Moblin 2.1 we had a 3rd party vendor that implemented a translucent volume OSD using an argb window, and they were very unhappy that their OSD was no longer translucent when watching a fullscreen video, so unhappy that they preferred to take the performance hit on the playback instead).
(In reply to comment #17) > (In reply to comment #14) > > It contains code to unredirect both fullscreen windows and screen sized > > override-redirect windows. Just doing the later seems to catch every game > > I tried (as SDL which most games use does fullscreening this way). > > Doing 'fullscreening' this way in an application is broken; we have to live > with it on the WM level, but we should not consider it the normal case, and > need to have the normal case (below) handled as a matter of priority (i.e., > optimizing for applications that are broken whilst leaving EWMH compliant > applications to suffer performance hit in comparison is bad policy). That is a heuristic to catch the apps that suffer most when being redirected and where unredirecting them has basically no effect on the UX. If there is a better heuristic for this we should try it, but just unredirecting everything (or even turning off compositing for everything) is not really an option imo. > > Unredirecting all fullscreen windows has the side effect that we catch windows > > like fullscreen firefox where opening a submenu can cause flicker doing to > > redirecting it again. > > This is a problem that is inherent to the single toplevel window unredirection > (i.e., it will apply to the OR screen sized windows above as well, it's just > that such apps will be creating transients less often). Yeah, you'd get a short flicker when doing so but compared to other apps it does not happen all the time. > Unless we can find a > way to get the transition flickerless, I don't think we can go down this route, > but rather need to unredirect any window that is in a fullscreen state, and all > of its transients. For the shell that would mean that stuff like the appSwitcher would be invisible while running a fullscreen applications, and make applications look out of place when running fullscreened (ex: no menu shadows etc.). > The only real benefit in partial unredirection is for a multi-monitor set up, > but then you end up with the shell overlay not working as expected, and hard to > predict in what way not working depending which monitor the fullscreen app is > on. IMO it is questionable whether from the user experience point of view this > is not worse than having the compositing turned off completely, and knowing > that is the case. What does "knowing that is the case." actually means? For the shell it would mean that a lot of stuff will stop working (messagetray. appswitcher, workspace switcher, overview). So basically everything but the fullscreen window works as expected. Knowing that "stuff will be broken when running fullscreen apps" doesn't really make sense. > There are serious drawbacks to turning compositing off in any shape an form, > and I am wondering if it might not be better in the long run to invest into > improving the compositing performance instead Yeah, well we can't the performance hit go away, but if we can get it to an acceptable level we wouldn't have this discussion at all. (As my numbers show currently there are cases where we cut the framerate in half which is just too much). But because it has drawbacks we should only do it when we really have to. It is pretty pointless to unredirect a fullscreen browser window; it causes more problems than it solves. > (e.g., in Moblin 2.1 we had a 3rd > party vendor that implemented a translucent volume OSD using an argb window, > and they were very unhappy that their OSD was no longer translucent when > watching a fullscreen video, so unhappy that they preferred to take the > performance hit on the playback instead). Are they really cases where compositing adds a noticeable overhead for video playback?
(In reply to comment #18) > That is a heuristic to catch the apps that suffer most when being redirected > and where unredirecting them has basically no effect on the UX. > > If there is a better heuristic for this we should try it, The '#if 0' in the patch means you are only doing your optimatization for applications that do not follow the EWMH standard and ignoring apps that conform; I happen to care about the latter lot more than the former :) > but just > unredirecting everything (or even turning off compositing for everything) is > not really an option imo. I am not necessarily arguing you should unredirect everything, I am suggesting we need to unredirect the transients of the fullscreen window in addition to the fullscreen window itself. Yep, your transients will not have shadows, but that is lot better IMO than the application window flickering every time you pop a menu. > > IMO it is questionable whether from the user experience point of view this > > is not worse than having the compositing turned off completely, and knowing > > that is the case. > > What does "knowing that is the case." actually means? It means you make it clear that when using an application in fullscreen mode, the fancy shell is not available and the users needs to return to normal mode before being able to use it. > For the shell it would mean that a lot of stuff will stop working (messagetray. > appswitcher, workspace switcher, overview). Surely, that will be the case anyway; any parts of the shell UI that happen to be on the same monitor as the unredirected window will not be rendered. > (As my numbers show currently there are cases where we cut the framerate in > half which is just too much). The numbers are not disputed, nor the need to address this. > But because it has drawbacks we should only do it when we really have to. > It is pretty pointless to unredirect a fullscreen browser window; it causes > more problems than it solves. Unless you are watching a video in your browser, then it is not pointless at all; this specifically is one case I care lot more about that SDL games. > Are they really cases where compositing adds a noticeable overhead for video > playback? Yes; video playback is the main reason why the Moblin/MeeGo plugin is turning compositing off when it detects a fullscreen application.
(In reply to comment #19) > (In reply to comment #18) > > That is a heuristic to catch the apps that suffer most when being redirected > > and where unredirecting them has basically no effect on the UX. > > > > If there is a better heuristic for this we should try it, > > The '#if 0' in the patch means you are only doing your optimatization for > applications that do not follow the EWMH standard and ignoring apps that > conform; I happen to care about the latter lot more than the former :) Yeah but the intend wasn't "don't do it for compliant apps" but "do it for apps where it matters most, so to make the 'UX hit' minimal" > > > IMO it is questionable whether from the user experience point of view this > > > is not worse than having the compositing turned off completely, and knowing > > > that is the case. > > > > What does "knowing that is the case." actually means? > > It means you make it clear that when using an application in fullscreen mode, > the fancy shell is not available and the users needs to return to normal mode > before being able to use it. The "fancy shell" is not just eye candy but without it a lot of stuff would be just useless (in the multihead case your second monitor); but yeah if the fullscreen app runs on the primary screen most of the "fancy shell" will be dead too. We could update the output shape of the COW and actually show them (everything but the overview); not sure if it would cause any visual glitches or not though. > > For the shell it would mean that a lot of stuff will stop working (messagetray. > > appswitcher, workspace switcher, overview). > > Surely, that will be the case anyway; any parts of the shell UI that happen to > be on the same monitor as the unredirected window will not be rendered. Yeah but in case you do a fullscreen presentation on your second screen; or (to pick up your video playback example) play a video on your TV; why should that affect what you are doing on the primary screen. > > (As my numbers show currently there are cases where we cut the framerate in > > half which is just too much). > > The numbers are not disputed, nor the need to address this. Yeah was just saying that unless we can make it faster to the point where the overhead isn't noticeable we should do *something* to address it. > > But because it has drawbacks we should only do it when we really have to. > > It is pretty pointless to unredirect a fullscreen browser window; it causes > > more problems than it solves. > > Unless you are watching a video in your browser, then it is not pointless at > all; this specifically is one case I care lot more about that SDL games. This is a case where having the window redirected might be actually a benefit though. Browses tend to render the videos on screen directly and not try to sync to vblank at all; while having the window redirected can allow the compositor to sync and therefore avoid video tearing. > > Are they really cases where compositing adds a noticeable overhead for video > > playback? > > Yes; video playback is the main reason why the Moblin/MeeGo plugin is turning > compositing off when it detects a fullscreen application. Ugh ... OK wasn't aware of that but I suppose Moblin/MeeGo runs on hardware where smaller hits are more likely to get noticed than on the hardware I have here to test with. So not sure where to go from here; I don't want to enforce anything in mutter which results into a worse user experience when running the shell but I don't want to break the Moblin/MeeGo case either (and keeping the status quo isn't ideal either). We could add a generic API and let the plugins decide what they actually want to unredirect or if they want to unredirect anything at all (ex: when running the shell's builtin screencast recorder unredirecting anything is a bad idea).
(In reply to comment #20) > > Unless you are watching a video in your browser, then it is not pointless at > > all; this specifically is one case I care lot more about that SDL games. > > This is a case where having the window redirected might be actually a benefit > though. > > Browses tend to render the videos on screen directly and not try to sync to > vblank at all; while having the window redirected can allow the compositor to > sync and therefore avoid video tearing. Except having the window redirected means the contents of the window get drawn twice; first by the application into the window and then by the compositor into the cow, which is the main reason why the fps drops. IIRC, the Intel driver now implements optimization for GL applications such that if the window is not decorated the intermediate blit is avoided (the GL object size must match the GL window size, which is only the case for undecorated windows), but unfortunately most apps do not fall into this category. > So not sure where to go from here; I don't want to enforce anything in mutter > which results into a worse user experience when running the shell but I don't > want to break the Moblin/MeeGo case either (and keeping the status quo isn't > ideal either). > > We could add a generic API and let the plugins decide what they actually want > to unredirect or if they want to unredirect anything at all (ex: when running > the shell's builtin screencast recorder unredirecting anything is a bad idea). If this genuinely improves things for the Gnome shell, I would settle just for an additional API that would allow me to turn this feature on / off, and for having the mutter_window_detach() available for the plugins as per the original patch (this way I would have the option of implement complete turning off of the compositor if I need to).
(In reply to comment #16) > Review of attachment 161089 [details] [review]: > > I like this general approach - shaping the COW seems like a good way to handle > multihead, and 'fullscreen override-redirect windows' seems like a good > heuristic. > > For the multihead case, it might be nice to eventually set up a clip on the > stage when painting so we don't spend time painting the stuff that's underneath > the unredirected window. But my expectation is that the normal case is: > > - Lots of drawing on the redirected window > - Very little drawing of anything else > > So that's a minor optimization that can be left for the distant future. Various > comments below about the implementation. Well this could become tricky when the number of monitors exceeds 2. Ex: 3 monitor setup with the fullscreen app centered; we can't set a clip for two rectangles. > Suppressing or reducing the flash when redirecting/unredirecting might also be > possible with enough care - see some comments above about making the > ClutterStage background none. But also not very important if what we are trying > to handle is full-screen games that go fullscreen and stay fullscreen. > > ::: src/compositor/compositor.c > @@ +97,3 @@ > + MutterWindow *cw; > + MetaScreen *screen; > + MetaCompScreen *info; > > Weird alignment. Avoid tabs except at the start of the line. You don't need to > line up local variables, you can just have 'MetaScreen *screen;'. OK > (I don't mind having local variables lined up, but don't really like realigning > them when adding or removing variables, so I tend not to do so. This is > different from function parameter lists which don't change as often) > > @@ +824,3 @@ > GList *tmp; > > + MutterWindow *top_most = (g_list_last (windows))->data; > > a) topmost is one word. b) This will crash if windows == NULL. OK > @@ +838,3 @@ > + meta_window_get_outer_rect (window, &window_rect); > + meta_screen_get_size (screen, &width, &height); > + /* FIXME: restrict to monitor */ > > This FIXME needs to be fixed before landing the patch, I think; though you > might need to handle *either* monitor-sized or screen-sized windows, since > games played multihead or gnome-screensaver may have a single window covering > multiple monitors (maybe check the gnome-screensaver sources to see what it > does.) You may also need to handle the monitor <=> screen transition - imagine > a preference in a game about multihead behavior. It's fine to handle it with an > redirect/unredirect like the redirected window changed, though. OK > @@ +839,3 @@ > + meta_screen_get_size (screen, &width, &height); > + /* FIXME: restrict to monitor */ > + if (window_rect.width == width && window_rect.height == height) > > Should check x,y as well. Generally, it bothers me to have the concept: > > "unredirect the window if it's an override redirect window that is at the top > of > the stack and is the size of the screen/current monitor" > > But only to check this when checking stacking and not when the window is > moved/resized. > I'd rather see that enforced accurately. So, you need to have code in > mutter_window_sync_actor_position() as well. What I'd do is something like > update a window->is_topmost flag here, and update a window->is_monitor_sized > flag in sync_actor_position(), then have the actual logic for updating > redirection in a shared function. So you are suggesting to do the actual redirection from both places? (i.e sync_actor_position and sync_actor_stacking ?) > @@ +871,3 @@ > + int width, height; > + rect.x = 0; > + rect.y = 0; > > some blank lines would make things more readable; I'd also probably move > setting up rect to where you use it and call it screen_rect OK > @@ +879,3 @@ > + info->unredirected_window = top_most; > + XCompositeUnredirectWindow (xdisplay, xwin, CompositeRedirectManual); > + info->output_region = XFixesCreateRegionFromWindow (xdisplay, xwin, > WindowRegionBounding); > > Is there any reason to save the output region rather than just destroying it > immediately. Actually no. (In reply to comment #21) > (In reply to comment #20) > > > Unless you are watching a video in your browser, then it is not pointless at > > > all; this specifically is one case I care lot more about that SDL games. > > > > This is a case where having the window redirected might be actually a benefit > > though. > > > > Browses tend to render the videos on screen directly and not try to sync to > > vblank at all; while having the window redirected can allow the compositor to > > sync and therefore avoid video tearing. > > Except having the window redirected means the contents of the window get drawn > twice; first by the application into the window and then by the compositor into > the cow, which is the main reason why the fps drops. Yeah that is the whole point here. > IIRC, the Intel driver now > implements optimization for GL applications such that if the window is not > decorated the intermediate blit is avoided (the GL object size must match the > GL window size, which is only the case for undecorated windows), but > unfortunately most apps do not fall into this category. Hmm.... interesting the optimization does not seem to hit a lot of apps though. > > So not sure where to go from here; I don't want to enforce anything in mutter > > which results into a worse user experience when running the shell but I don't > > want to break the Moblin/MeeGo case either (and keeping the status quo isn't > > ideal either). > > > > We could add a generic API and let the plugins decide what they actually want > > to unredirect or if they want to unredirect anything at all (ex: when running > > the shell's builtin screencast recorder unredirecting anything is a bad idea). > > If this genuinely improves things for the Gnome shell, I would settle just for > an additional API that would allow me to turn this feature on / off, and for > having the mutter_window_detach() available for the plugins as per the original > patch (this way I would have the option of implement complete turning off of > the compositor if I need to). Well if we are going this route I rather add a way for plugins to turn this on/off at any time _and_ to be able to decide "screensized fullscreen windows only" or "every fullscreen window" or "no window".
(In reply to comment #21) > If this genuinely improves things for the Gnome shell, I would settle just for > an additional API that would allow me to turn this feature on / off, and for > having the mutter_window_detach() available for the plugins as per the original > patch (this way I would have the option of implement complete turning off of > the compositor if I need to). OK, I have opted for this as it introduces the minimum amount of complexity. I made it opt-in rather opt-out and allow plugins to toggle it at runtime in case they want to (to handle the screencast case).
Created attachment 163329 [details] [review] Unredirect fullscreen windows Some apps that do a lot of rendering on the screen like games, mostly run in fullscreen where there is no need for them to be redirected doing so does add an overhead; while performance is critical for those apps. Some benchmarks (openarena on GM45): Before: 840 frames 27.2 seconds 30.9 fps 8.0/32.3/62.0/8.6 ms 840 frames 27.8 seconds 30.2 fps 12.0/33.1/54.0/3.3 ms After: 840 frames 16.1 seconds 52.1 fps 8.0/19.2/45.0/5.9 ms 840 frames 16.2 seconds 51.9 fps 8.0/19.3/47.0/6.0 ms We use a heuristic to catch games by unredirecting monitor-sized / screen-sized to minimize the visual disruption and only apply it when really needed. The feature is opt-in plugins need to explicitly enable it using mutter_plugin_set_auto_unredirect. They also have the ability to enable / disable it at runtime.
(In reply to comment #23) > OK, I have opted for this as it introduces the minimum amount of complexity. > I made it opt-in rather opt-out and allow plugins to toggle it at runtime in > case they want to (to handle the screencast case). Sorry, I think we misunderstood each other; could the plugin also have the option to enable this for all screen-sized windows, not just ORs (e.g., via an extra bool parameter to mutter_plugin_set_auto_unredirect()) ?
(In reply to comment #25) > (In reply to comment #23) > > OK, I have opted for this as it introduces the minimum amount of complexity. > > I made it opt-in rather opt-out and allow plugins to toggle it at runtime in > > case they want to (to handle the screencast case). > > Sorry, I think we misunderstood each other; could the plugin also have the > option to enable this for all screen-sized windows, not just ORs (e.g., via an > extra bool parameter to mutter_plugin_set_auto_unredirect()) ? Yeah can add that.
(In reply to comment #8) > Review of attachment 145120 [details] [review]: > > Fine, if the prototype is moved to mutter-window-private.h > > ::: src/include/mutter-window.h > @@ +68,3 @@ > const char * mutter_window_get_description (MutterWindow *mcw); > gboolean mutter_window_showing_on_its_workspace (MutterWindow *mcw); > +void mutter_window_detach (MutterWindow *mcw); > > Shouldn't this be in mutter-window-private.h? The reason for exposing this is so it can be used by the plugin, hence the public prototype.
Created attachment 163363 [details] [review] Unredirect fullscreen windows *) Make the "only screen/monitor sized OR windows" heuristic optional
Created attachment 163366 [details] [review] Unredirect fullscreen windows *) Minior cleanups
Created attachment 163370 [details] [review] Unredirect fullscreen windows *) Remove hack
For additional info: Here are some quick benchmarks I did using Unigine Heaven. These benchmarks are against Metacity and Mutter. I was not running Gnome Shell at the time. I did two benchmarks: one without tessellation and one with it set to "normal". *Settings for Both Render: opengl Mode: 1600x1200 fullscreen Shaders: high Textures: high Filter: trilinear Anisotropy: 16x Occlusion: enabled Refraction: enabled Volumetric: enabled Replication: disabled *Hardware Binary: Linux 32bit GCC 4.3.2 Release May 20 2010 Operating system: Linux 2.6.35-25-generic i686 CPU model: Intel(R) Core(TM)2 Quad CPU Q9550 @ 2.83GHz CPU flags: 2833MHz MMX SSE SSE2 SSE3 SSSE3 SSE41 HTT GPU model: GeForce GTX 460 PCI Express 270.26 768Mb *Metacity (First Run/No Tess) FPS: 51.8 Scores: 1305 Min FPS: 21.1 Max FPS: 99.8 *Mutter (First Run/No Tess) FPS: 49.8 Scores: 1253 Min FPS: 20.9 Max FPS: 94.6 *Metacity (Second Run/No Tess) FPS: 52.1 Scores: 1312 Min FPS: 32.6 Max FPS: 100.0 *Mutter (Second Run/No Tess) FPS: 50.0 Scores: 1260 Min FPS: 31.7 Max FPS: 95.1 *Metacity (First Run/Tess) FPS: 37.3 Scores: 941 Min FPS: 7.1 Max FPS: 85.1 *Mutter (First Run/Tess) FPS: 36.1 Scores: 911 Min FPS: 7.0 Max FPS: 80.4 *Metacity (Second Run/Tess) FPS: 37.7 Scores: 951 Min FPS: 25.6 Max FPS: 85.3 *Mutter (Second Run/Tess) FPS: 36.6 Scores: 921 Min FPS: 23.9 Max FPS: 80.3 So objectively there was about an average 2fps drop using Mutter, with the maximum fps being reduced by 5. There's also about a 1-2fps increase in the minimum when using Metacity vs Mutter. Subjectively I noticed more high frame rates (for longer) watching the fps counter while using Metacity. Just not enough to drastically effect the average. Though the first run numbers are important, the second run numbers should be what you get if there's no anomalies (due to loading hitches, etc).
*** Bug 654786 has been marked as a duplicate of this bug. ***
are these patches already included in newest gnome versions?
No, if they were, the bug would have been closed.
So, one interesting thing is that the patch I reviewed in: http://lists.x.org/archives/xorg-devel/2011-July/023953.html plus patches that already landed in the X server this spring should allow us to unredirect and display a fullscreen window without any flash. (Probably hard to get vertical reblank synchronization on unredirection, but that's really the only visible artifact.) I'm not really sure that changes anything though - it seems like it still doesn't make sense to unredirect something like a fullscreen browser if it's not asking to be unredirected. That is, browsers are likely not handling vblank synchronization, so you would get tearing that would be otherwise be eliminated by the compositor. And redirecting and unredirecting for browser autocomplete popups, for the gedit slide-in fullscreen menus, etc, seems difficult to do without *any* visual artifacts or inefficiency. Have we checked a variety of games to see that they are reliably using override-redirect windows for their fullscreen version? If that's the case, my recommendation would be to: * Unredirect for override-redirect fullscreen windows * Try to standardize through EWMH an X property that means that an app can handle direct access to the frame buffer and will benefit from it.
Review of attachment 163370 [details] [review]: I'd like to go ahead on this - I think it does make sense, at least in terms of looking good in phoronix benchmarks. Right now you are updating the unredirect state when various things change. I think it's a bit better to do it in compositor:meta_repaint_func() - Get the topmost window - Is it a candidate for redirection? - Adjust the redirect state to match ::: src/compositor/mutter-plugin.c @@ +584,3 @@ +mutter_plugin_set_auto_unredirect (MutterPlugin *plugin, + gboolean state, + gboolean no_heuristic) A) Let's stop pushing stuff through MetaPlugn and just add the MetaCompositor method B) I'd rather keep this simple and give us the flexibility to keep making it better, which means not exposing the heuristic to the plugin/library user. My suggested interface is: meta_compositor_disable_unredirect(compositor, screen) meta_compositor_enable_unredirect(compositor, screen) where these update a disable_unredirect_count so that multiple sections of code can independently disable unredirection (say, chrome.js and overview.js and the shell recorder code) ::: src/include/mutter-window.h @@ +55,3 @@ + guint is_topmost : 1; + guint is_monitor_sized : 1; I'd suggest not adding these and just computing stuff as needed in the pre-paint function; I don't particularly like how is_monitor_sized means here "is an unredirect candidate, depending on selected options"
Created attachment 192331 [details] [review] MetaWindowActor: Make meta_window_actor_detach() public This function allows its caller to force rebinding of the texture bound to MutterWindow. --- *) Rebased
Created attachment 192332 [details] [review] Unredirect fullscreen windows Some apps that do a lot of rendering on the screen like games, mostly run in fullscreen where there is no need for them to be redirected doing so does add an overhead; while performance is critical for those apps. This can be disabled / enabled at runtime using meta_enable_unredirect_for_screen / meta_disable_unredirect_for_screen -- *) Reworked and rebased based on review
(In reply to comment #36) > Review of attachment 163370 [details] [review]: > > I'd like to go ahead on this - I think it does make sense, at least in terms of > looking good in phoronix benchmarks. > > Right now you are updating the unredirect state when various things change. I > think it's a bit better to do it in compositor:meta_repaint_func() > > - Get the topmost window > - Is it a candidate for redirection? > - Adjust the redirect state to match > > ::: src/compositor/mutter-plugin.c > @@ +584,3 @@ > +mutter_plugin_set_auto_unredirect (MutterPlugin *plugin, > + gboolean state, > + gboolean no_heuristic) > > A) Let's stop pushing stuff through MetaPlugn and just add the MetaCompositor > method OK. > B) I'd rather keep this simple and give us the flexibility to keep making it > better, which means not exposing the heuristic to the plugin/library user. My > suggested interface is: OK. > meta_compositor_disable_unredirect(compositor, screen) > meta_compositor_enable_unredirect(compositor, screen) I ignored the "compositor" parts because the whole compositor API does that (just meta_foo_...(screen) ) > where these update a disable_unredirect_count so that multiple sections of code > can independently disable unredirection (say, chrome.js and overview.js and the > shell recorder code) OK > ::: src/include/mutter-window.h > @@ +55,3 @@ > > + guint is_topmost : 1; > + guint is_monitor_sized : 1; > > I'd suggest not adding these and just computing stuff as needed in the > pre-paint function; I don't particularly like how is_monitor_sized means here > "is an unredirect candidate, depending on selected options" Done that.
Comment on attachment 161088 [details] [review] Add meta_window_is_fullscreen Pushed a while ago.
(In reply to comment #35) > (Due to some bugzilla and/or gmail nonsense that mail got delayed so I somehow never saw this comment until now). > So, one interesting thing is that the patch I reviewed in: > > http://lists.x.org/archives/xorg-devel/2011-July/023953.html > > plus patches that already landed in the X server this spring should allow us to > unredirect and display a fullscreen window without any flash. (Probably hard to > get vertical reblank synchronization on unredirection, but that's really the > only visible artifact.) > > I'm not really sure that changes anything though - it seems like it still > doesn't make sense to unredirect something like a fullscreen browser if it's > not asking to be unredirected. That is, browsers are likely not handling vblank > synchronization, so you would get tearing that would be otherwise be eliminated > by the compositor. And redirecting and unredirecting for browser autocomplete > popups, for the gedit slide-in fullscreen menus, etc, seems difficult to do > without *any* visual artifacts or inefficiency. Yeah that makes sense. > Have we checked a variety of games to see that they are reliably using > override-redirect windows for their fullscreen version? Yes I did that back then when I decided on that heuristics, even "benchmark only" stuff like unengine heaven does that. > If that's the case, my > recommendation would be to: > > * Unredirect for override-redirect fullscreen windows > * Try to standardize through EWMH an X property that means that an app can > handle direct access to the frame buffer and will benefit from it. OK patch does 1) will send a re 2)
Review of attachment 192331 [details] [review]: I don't want to make meta_window_detach() public. Unredirecting a window is pretty tricky... e.g., you have to handle damage events differently for the window while it's unredirected. I don't think leaking out one potential piece of the puzzle to the public API makes sense.
Review of attachment 192332 [details] [review]: Bugs here: * You need to handle the case where the unredirected window is destroyed (in meta_compositor_remove_window) * Because you are dropping damage events on the floor while the window is redirected, window_actor->priv->received_damage is likely FALSE, but the damage object will have a non-empty rectangle, so damage events won't be sent. The simplest thing is probably to keep track of unredirection in MetaWindowActor and when you get damage for a unredirected window, set received_damage before returning, so when it's painted again at redirection the damage object gets cleared. Things that can be improved: * In order to take advantage of the no-flicker redirection support that's landing in Xorg (see above mailing list link), we need to order things as follows: When unredirecting a window: - First set the shape on the COW, revealing the root window, but not repainting it (because we've redirected subwindows and that suppresses background painting with the queued up patch and according to the spec) - Then unredirect the window, copying the relevant parts of the offscreen pixmap onto the now visible unredirected window When redirecting a window: - First redirect the window, this will copy the bits of the window from the root window to the offscreen pixmap - Then shape the COW to hide everything again * This patch doesn't suppress painting as much as I'd like - remember, the backing pixmap is not clipped to the shape of the COW, and drawing to it won't be clipped, so if anything else changes, then we'll redraw everything onto the backing pixmap, then it will get clipped at swap buffers time. One thing we can do to avoid this is to use a better starting region in MetaWindowGroup - like the comment: /* Get the clipped redraw bounds from Clutter so that we can avoid * painting shadows on windows that don't need to be painted in this * frame. In the case of a multihead setup with mismatched monitor * sizes, we could intersect this with an accurate union of the * monitors to avoid painting shadows that are visible only in the * holes. */ but also subtract out the unredirected window. Then we'll skip all window and background painting for the covered area. It isn't perfect since we'll still be going through the paint cycle, just not drawing anything and having the "copy area" to the front buffer clipped out. We might want to consider whether we can suppress the paint cycle entirely in the case where the entire stage is obscured, but I can't think of an easy way to do it offhand. Style: * I'd like to move anything that relates to the state of the window (like the handling of damage and detaching) into meta-window-actor.c
Created attachment 194907 [details] [review] Unredirect fullscreen windows Some apps that do a lot of rendering on the screen like games, mostly run in fullscreen where there is no need for them to be redirected doing so does add an overhead; while performance is critical for those apps. This can be disabled / enabled at runtime using meta_enable_unredirect_for_screen / meta_disable_unredirect_for_screen --- *) Handle the case where the unredirected window is destroyed *) Fix damage event handling *) Reorder operations to avoid flicker *) Substract region in MetaWindowGroup *) Don't require the "make meta_window_actor_detach public" patch
Created attachment 194911 [details] [review] Unredirect fullscreen windows Some apps that do a lot of rendering on the screen like games, mostly run in fullscreen where there is no need for them to be redirected doing so does add an overhead; while performance is critical for those apps. This can be disabled / enabled at runtime using meta_enable_unredirect_for_screen / meta_disable_unredirect_for_screen --- Some cleanups
Created attachment 194914 [details] [review] Unredirect fullscreen windows Some apps that do a lot of rendering on the screen like games, mostly run in fullscreen where there is no need for them to be redirected doing so does add an overhead; while performance is critical for those apps. This can be disabled / enabled at runtime using meta_enable_unredirect_for_screen / meta_disable_unredirect_for_screen
Created attachment 194970 [details] [review] Unredirect fullscreen windows Some apps that do a lot of rendering on the screen like games, mostly run in fullscreen where there is no need for them to be redirected doing so does add an overhead; while performance is critical for those apps. This can be disabled / enabled at runtime using meta_enable_unredirect_for_screen / meta_disable_unredirect_for_screen ---- *) Substract the unredirected window region outside of the loop
Review of attachment 194970 [details] [review]: needs some more structural work, I don't like the manipulation of compositor data structures inside MetaWindowActor - things need to be organized to keep objects only manipulating what they "own" ::: src/compositor/compositor.c @@ +112,3 @@ + info = meta_screen_get_compositor_data (screen); + + meta_window_actor_process_damage (window_actor, event, CLUTTER_ACTOR (window_actor) == info->unredirected_window); Just keep track of a unredirected flag inside of MetaWindowActor @@ +637,3 @@ + + if (CLUTTER_ACTOR (window_actor) == info->unredirected_window) + meta_window_actor_update_redirect_state (window_actor); I'd expect rather something like: meta_window_actor_set_redirected (window_actor, TRUE); info->unredirected_window = NULL; (if the first line is necessary). I'd expect this code to be maintaining what info->unredirected_window should be, and the code inside MetaWindowActor to be handling the details of how to redirect and unredirect. @@ +1114,3 @@ + if (info->windows) + meta_window_actor_update_redirect_state (g_list_last (info->windows)->data); Again, I'd expect info->unredirected_window and what window is unredirceted to be handled here. So, you know: top_window = g_list_last (info->windows); if (meta_window_actor_should_redirect (top_window)) expected_unredirected_window = top_window; else expected_unredirected_window = NULL; if (info->unredirected_window != expected_unredirected_window) { meta_window_actor_set_redirected (info->unredirected_window, FALSE); ... } }
Created attachment 195092 [details] [review] Unredirect fullscreen windows Some apps that do a lot of rendering on the screen like games, mostly run in fullscreen where there is no need for them to be redirected doing so does add an overhead; while performance is critical for those apps. This can be disabled / enabled at runtime using meta_enable_unredirect_for_screen / meta_disable_unredirect_for_screen --- Restructured
Review of attachment 194970 [details] [review]: Looks almost perfect (or I'm just tired and low on my bug-finding mojo at the moment :-) but there was one thing from my last comment not handled here: * Because you are dropping damage events on the floor while the window is redirected, window_actor->priv->received_damage is likely FALSE, but the damage object will have a non-empty rectangle, so damage events won't be sent. The simplest thing is probably to keep track of unredirection in MetaWindowActor and when you get damage for a unredirected window, set received_damage before returning, so when it's painted again at redirection the damage object gets cleared.
Comment on attachment 195092 [details] [review] Unredirect fullscreen windows Attachment 195092 [details] pushed as d383172 - Unredirect fullscreen windows