GNOME Bugzilla – Bug 694988
Avoid unnecessary full stage redraws
Last modified: 2013-03-04 23:03:51 UTC
The patches I am going to attach here fix: Clutter-Message: [ +7514]:[CLIPPING]:./clutter-actor.c:16927: Bail from get_paint_volume (<unnamed>[<MetaWindowGroup>:0x2154080]): Actor failed to report a volume Clutter-Message: [ +7526]:[CLIPPING]:./clutter-actor.c:3388: Bail from update_last_paint_volume (<unnamed>[<MetaWindowGroup>:0x2154080]): Actor failed to report a paint volume Clutter-Message: [ +7546]:[CLIPPING]:./clutter-actor.c:3340: Bail from cull_actor without culling (<unnamed>[Clutter-Message: [ +262]:[CLIPPING]:./cogl/clutter-stage-cogl.c:533: Stage clip pushed: x=0, y=0, width=1920, height=1080 Clutter-Message: [ +319]:[CLIPPING]:./clutter-stage.c:661: Setting stage clip too: x=0,000000, y=0,000000, width=1920,000000, height=1080,000000 Clutter-Message: [ +346]:[CLIPPING]:./clutter-actor.c:16916: Bail from get_paint_volume (<unnamed>[<ClutterStage>:0x20ec4a0]): Actor has "paint" signal handlers Clutter-Message: [ +368]:[CLIPPING]:./clutter-actor.c:3388: Bail from update_last_paint_volume (<unnamed>[<ClutterStage>:0x20ec4a0]): Actor failed to report a paint volume Clutter-Message: [ +385]:[CLIPPING]:./clutter-actor.c:3340: Bail from cull_actor without culling (<unnamed>[<ClutterStage>:0x20ec4a0]): ->last_paint_volume_valid == FALSE Clutter-Message: [ 12889683294]:[CLIPPING]:./clutter-stage.c:4032: stage_queue_actor_redraw (actor=<unnamed>[<MetaBackgroundGroup>:0x28eee40], clip=(nil)): Clutter-Message: [ +114]:[CLIPPING]:./clutter-stage.c:4032: stage_queue_actor_redraw (actor=<unnamed>[<MetaBackgroundGroup>:0x28eeaf0], clip=(nil)): Clutter-Message: [ +193]:[CLIPPING]:./clutter-actor.c:16876: Bail from get_paint_volume (<unnamed>[<MetaBackgroundGroup>:0x28eeaf0]): Actor needs allocation Clutter-Message: [ +235]:[CLIPPING]:./cogl/clutter-stage-cogl.c:492: Reusing back buffer - repairing region: x=0, y=0, width=1920, height=1080 <MetaWindowGroup>:0x2154080]): ->last_paint_volume_valid == FALSE Clutter-Message: [ +8031]:[CLIPPING]:./clutter-actor.c:16927: Bail from get_paint_volume (<unnamed>[<MetaWindowGroup>:0x21543a0]): Actor failed to report a volume Clutter-Message: [ +8048]:[CLIPPING]:./clutter-actor.c:3388: Bail from update_last_paint_volume (<unnamed>[<MetaWindowGroup>:0x21543a0]): Actor failed to report a paint volume Clutter-Message: [ +85305]:[CLIPPING]:./clutter-stage.c:661: Setting stage clip too: x=0,000000, y=0,000000, width=1920,000000, height=1080,000000 Clutter-Message: [ +85331]:[CLIPPING]:./clutter-actor.c:16916: Bail from get_paint_volume (<unnamed>[<ClutterStage>:0x128b4a0]): Actor has "paint" signal handlers Clutter-Message: [ +85342]:[CLIPPING]:./clutter-actor.c:3388: Bail from update_last_paint_volume (<unnamed>[<ClutterStage>:0x128b4a0]): Actor failed to report a paint volume Clutter-Message: [ +85356]:[CLIPPING]:./clutter-actor.c:3340: Bail from cull_actor without culling (<unnamed>[<ClutterStage>:0x128b4a0]): ->last_paint_volume_valid == FALSE p>:0x21543a0]): ->last_paint_volume_valid == FALSE
Created attachment 237784 [details] [review] meta-window-group: Report a paint volume We never paint outside of the allocation so we can simply use clutter_paint_volume_set_from_allocation.
Created attachment 237785 [details] [review] meta-background-group: Report a paint volume We never paint outside of the allocation so we can simply use clutter_paint_volume_set_from_allocation.
Created attachment 237786 [details] [review] compositor: Don't connect to the stage's paint signal Doing so causes useless full stage redraws and breaks culling as clutter cannot know how the signal handler affects painting. So use clutter_threads_add_repaint_func_full with the CLUTTER_REPAINT_FLAGS_POST_PAINT flag instead.
Created attachment 237787 [details] [review] shell-global: Don't connect to the stage's paint signal Doing so causes useless full stage redraws and breaks culling as clutter cannot know how the signal handler affects painting. So use clutter_threads_add_repaint_func_full instead.
Created attachment 237789 [details] [review] st-icon: Don't cause a redraw by replacing the icon with itself Check if the two icons match before setting a new icon, if they do don't do anything. --- Owen posted this one on IRC a few days ago ... worth adding to the "kill redraws" patches.
Review of attachment 237789 [details] [review]: I'd rather you use g_icon_equal() instead, and to avoid having a different behaviors for icon_name (which is the semi-deprecated interface) and gicon with a GThemedIcon Also, the power part should be unnecessary, once you do that in StIcon.
Review of attachment 237784 [details] [review]: Are we sure that we never paint outside the allocation (including during workspace switching effects)? Also, why isn't clutter doing the right thing by default here?
Review of attachment 237785 [details] [review]: Same here: are we sure we *never* paint outside the allocation? What about the blur effect in MetaBackground?
(In reply to comment #8) > Review of attachment 237785 [details] [review]: > > Same here: are we sure we *never* paint outside the allocation? What about the > blur effect in MetaBackground? Yes look at meta_background_paint_content.
(In reply to comment #7) > Review of attachment 237784 [details] [review]: > > Are we sure that we never paint outside the allocation (including during > workspace switching effects)? Yes I cannot think of any case where we would paint outside of the allocation (the workspace switching effect reparents the windows away from us so this does not really matter). > Also, why isn't clutter doing the right thing by default here? Clutter cannot assume that we don't draw outside of our allocation, that would be kind of an API break.
Review of attachment 237786 [details] [review]: Ok
Review of attachment 237787 [details] [review]: Ok
Comment on attachment 237786 [details] [review] compositor: Don't connect to the stage's paint signal Attachment 237786 [details] pushed as 4f2bb58 - compositor: Don't connect to the stage's paint signal
Comment on attachment 237787 [details] [review] shell-global: Don't connect to the stage's paint signal Attachment 237787 [details] pushed as 071a4e5 - shell-global: Don't connect to the stage's paint signal
(In reply to comment #6) > Review of attachment 237789 [details] [review]: > > I'd rather you use g_icon_equal() instead, and to avoid having a different > behaviors for icon_name (which is the semi-deprecated interface) and gicon with > a GThemedIcon > Also, the power part should be unnecessary, once you do that in StIcon. Yeah that makes sense .. all I did was just adding a commit message other then "stuff" and attach it here.
(In reply to comment #10) > (In reply to comment #7) > > Review of attachment 237784 [details] [review] [details]: > > > > Are we sure that we never paint outside the allocation (including during > > workspace switching effects)? > > Yes I cannot think of any case where we would paint outside of the allocation > (the workspace switching effect reparents the windows away from us so this does > not really matter). No it doesn't: it reparents window actors inside groups, but then the groups are added to the window_group (windowManager.js:573) > > Also, why isn't clutter doing the right thing by default here? > > Clutter cannot assume that we don't draw outside of our allocation, that would > be kind of an API break. But Clutter implements the default paint volume function by unioning the paint volume of all children, which should give the right result here. (I understand it is slower, because of software transforms, so I'm ok with the shortcut, if we are sure that we never paint outside) (In reply to comment #9) > (In reply to comment #8) > > Review of attachment 237785 [details] [review] [details]: > > > > Same here: are we sure we *never* paint outside the allocation? What about the > > blur effect in MetaBackground? > > Yes look at meta_background_paint_content. Uh, you're right.
(In reply to comment #16) > (In reply to comment #10) > > (In reply to comment #7) > > > Review of attachment 237784 [details] [review] [details] [details]: > > > > > > Are we sure that we never paint outside the allocation (including during > > > workspace switching effects)? > > > > Yes I cannot think of any case where we would paint outside of the allocation > > (the workspace switching effect reparents the windows away from us so this does > > not really matter). > > No it doesn't: it reparents window actors inside groups, but then the groups > are added to the window_group (windowManager.js:573) Hmm indeed. > > > Also, why isn't clutter doing the right thing by default here? > > > > Clutter cannot assume that we don't draw outside of our allocation, that would > > be kind of an API break. > > But Clutter implements the default paint volume function by unioning the paint > volume of all children, which should give the right result here. It doesn't it fails to report a paint volume in a lot of cases with: Clutter-Message: [ +7514]:[CLIPPING]:./clutter-actor.c:16927: Bail from get_paint_volume (<unnamed>[<MetaWindowGroup>:0x2154080]): Actor failed to report a volume Which was the reason for the patch. > (I understand it is slower, because of software transforms, so I'm ok with the > shortcut, if we are sure that we never paint outside) Being slower is not the only issue though, breaking culling / clipped redraws is .. if we can't take the shortcut we need to go and figure out what causes us to get no paint volume.
(In reply to comment #16) > (In reply to comment #9) > > (In reply to comment #8) > > > Review of attachment 237785 [details] [review] [details] [details]: > > > > > > Same here: are we sure we *never* paint outside the allocation? What about the > > > blur effect in MetaBackground? > > > > Yes look at meta_background_paint_content. > > Uh, you're right. Does this mean that this one is ACN ?
(In reply to comment #18) > (In reply to comment #16) > > (In reply to comment #9) > > > (In reply to comment #8) > > > > Review of attachment 237785 [details] [review] [details] [details] [details]: > > > > > > > > Same here: are we sure we *never* paint outside the allocation? What about the > > > > blur effect in MetaBackground? > > > > > > Yes look at meta_background_paint_content. > > > > Uh, you're right. > > Does this mean that this one is ACN ? Err didn't saw that you set it acn ignore that.
Comment on attachment 237785 [details] [review] meta-background-group: Report a paint volume Attachment 237785 [details] pushed as e426900 - meta-background-group: Report a paint volume
Created attachment 237851 [details] [review] st-icon: Don't cause a redraw by replacing the icon with itself Check if the two icons match before setting a new icon, if they do don't do anything. Based on patch from Owen Taylor <otaylor@fishsoup.net>
(In reply to comment #16) > But Clutter implements the default paint volume function by unioning the paint > volume of all children, which should give the right result here. > (I understand it is slower, because of software transforms, so I'm ok with the > shortcut, if we are sure that we never paint outside) Are you looking at the same code I am? http://git.gnome.org/browse/clutter/tree/clutter/clutter-actor.c#n5961 If there's a custom paint function, it returns FALSE, which means that the paint volume may not be correct, and we bail early and queue a full-stage redraw.
(In reply to comment #22) > (In reply to comment #16) > > But Clutter implements the default paint volume function by unioning the paint > > volume of all children, which should give the right result here. > > (I understand it is slower, because of software transforms, so I'm ok with the > > shortcut, if we are sure that we never paint outside) > > Are you looking at the same code I am? > > http://git.gnome.org/browse/clutter/tree/clutter/clutter-actor.c#n5961 > > If there's a custom paint function, it returns FALSE, which means that the > paint volume may not be correct, and we bail early and queue a full-stage > redraw. Oh ... this explains why it keeps returning FALSE so we have to provide a custom get_paint_volume anyway.
(In reply to comment #16) > No it doesn't: it reparents window actors inside groups, but then the groups > are added to the window_group (windowManager.js:573) This seems to be correct -- it will paint outside its allocation. But the allocation should be the full stage size (it seems we query the children right now and compute a maximum size, but this is an easy cleanup), in which case it won't matter.
(In reply to comment #24) > (In reply to comment #16) > > No it doesn't: it reparents window actors inside groups, but then the groups > > are added to the window_group (windowManager.js:573) > > This seems to be correct -- it will paint outside its allocation. But the > allocation should be the full stage size (it seems we query the children right > now and compute a maximum size, but this is an easy cleanup), in which case it > won't matter. Yeah that makes sense. Run that patch for a day now and have not noticed any wrong drawing.
Comment on attachment 237784 [details] [review] meta-window-group: Report a paint volume Attachment 237784 [details] pushed as c996dde - meta-window-group: Report a paint volume
Review of attachment 237851 [details] [review]: Yes
Attachment 237851 [details] pushed as a6b49fe - st-icon: Don't cause a redraw by replacing the icon with itself