GNOME Bugzilla – Bug 688309
Background drawing issues with multiple screens on lockscreen
Last modified: 2013-11-15 15:57:12 UTC
As you can see on this video : http://vps10963.ovh.net/~djdeath/VID_20121114_110228.m4v with 2 monitors of different sizes, there is a issue with drawing the background on the lockscreen. This is happen because we use a single actor to draw the background. The single actor is actually the root window. But because of the monitors configurations, this leaves part of the root window undefined (because usually not visible). To work around this problem you must create a group of actor composed of one actor per monitor using the corresponding sizes.
Created attachment 228955 [details] [review] Patch for mutter
Created attachment 228956 [details] [review] Patch for gnome-shell
Hi Lionel, Thanks for the patches! Is this a duplicate of bug 681743 ?
Hi Ray, No it seems like a different issue. #681743 seems to be related to the clock position. This bug is about the animation when unlocking the screen.
Look at the blue rectangle on the laptop's screen in the video. It shouldn't be there. That blue color is (I believe) the default stage background color. On other laptop I can see that being black or even garbage.
I believe this issue is fixed in master already. Can you confirm? Incidentally, you're correct that the stage color used to be blue.
What commit fixes that problem on master? 3.6.2 still has the same issue.
So I just gave this a try and I was wrong. not fixed!
Created attachment 230014 [details] [review] Patch for gnome-shell Updated patch for master.
*** Bug 683251 has been marked as a duplicate of this bug. ***
Hello, and thank you for your patch. Sorry it took this long to review, I was hoping to skip this by doing the rendering in mutter (bug 682427), but even after changing paint to split the texture into individual rectangles, I just got myself to change from GnomeBG blue to StBin black.
Review of attachment 228955 [details] [review]: ::: src/compositor/meta-background-actor.c @@ +323,3 @@ guint8 color_component; int width, height; + ClutterActorBox input, output, alloc; Make input and output a pointer, and you avoid a copy. @@ +459,3 @@ + g_boxed_free (CLUTTER_TYPE_ACTOR_BOX, priv->input_box); + priv->input_box = g_value_dup_boxed (value); + clutter_actor_queue_redraw (CLUTTER_ACTOR (self)); It would be nice to split this into helper methods.
Review of attachment 230014 [details] [review]: ::: js/ui/screenShield.js @@ +445,3 @@ + _onMonitorsChanged: function() { + this._backgroundGroup.remove_all_children(); destroy_all_children() @@ +457,3 @@ + y1: 0, + x2: monitor.width, + backgroundTexture.input_box = new Clutter.ActorBox({ x1: monitor.x, Looking at this further, is there any reason to require output_box? Can we just assume that output_box is the allocation (and then set width and height on the actor explicitly)?
After trying a simplified version of the patch, which uses only the input box, I saw that there is a huge performance drop in adding multiple background StBins, so this is blocked on bug 689858.
The performance drop is interesting. Merging 689858 (when it's ready) would probably save a lot of memory, but I'm not sure it would provide a lot of performance in term of memory bandwidth, because it would still write to the same amount of output pixels. Actually the patches attached here should already improve the performance, because it prevents drawing quite a large area that is outside of the visible monitors. Thanks for the review anyway, the output area was a bit of the stretch to have something generic. I might not be useful, so I can remove it.
The box-shadow optimization should be good enough now. If you any information about the performance drop or a way to measure it, please add this to the bug.
I believe this was fixed by the new background rework Ray did in 3.8.