GNOME Bugzilla – Bug 630932
St: Implement get_paint_volume virtual method
Last modified: 2010-11-29 16:28:01 UTC
We currently only do clipped redraws for TFP actors, starting with clutter 1.5 there is support for it for regular actors too, so implement get_paint_volume virtual for StWidget to take advantage of it and potentially improve performance and reduce power consumption. This results into a hard requirement on clutter >= 1.5.
Created attachment 171355 [details] [review] St: Implement get_paint_volume virtual method To allow clutter to queue clipped redraws for St widgets we must report the paint volume using the get_paint_volume method. We currently assume that we never paint outside of an allocation in St. Also bump the minimum clutter requirement to 1.5.1 .
Oh and before someone ask "where are the numbers" ... say hi to http://bugzilla.clutter-project.org/show_bug.cgi?id=2349 ;)
(In reply to comment #1) > Created an attachment (id=171355) [details] [review] > St: Implement get_paint_volume virtual method > > We currently assume that we never paint outside of an allocation in > St. That assumption is wrong: 1) outlines and shadows are painted outside the widget's allocation - there's st_theme_node_get_paint_box() to get the actual box 2) StGroup allows placing children at will, including outside the container's allocation - not sure if it is possible to implement get_paint_volume in this case, but you might want to have a look at ClutterGroup, which StGroup copies
(In reply to comment #3) > (In reply to comment #1) > > Created an attachment (id=171355) [details] [review] [details] [review] > > St: Implement get_paint_volume virtual method > > > > We currently assume that we never paint outside of an allocation in > > St. > > That assumption is wrong: > > 1) outlines and shadows are painted outside the widget's > allocation - there's st_theme_node_get_paint_box() to > get the actual box OK > 2) StGroup allows placing children at will, including outside > the container's allocation - not sure if it is possible to > implement get_paint_volume in this case, but you might want > to have a look at ClutterGroup, which StGroup copies OK, this one is fixable too. StGroup would have to override StWidgets get_paint_volume and have one similar to ClutterGroups (which returns an union of its children's paint volumes transformed into its coordinate space).
Created attachment 172044 [details] [review] [WIP] St: Take advantage of clipped redraws In order to take advantage of clipped redraws (only redraw the parts that actually changed), we have to inform clutter about our paint_volume by implementing the get_paint_volume virtual method. As this feature had been added in in clutter 1.5.x we now require that. ---- I have found the clutter bug that was causing clipped redraws not to work (http://bugzilla.clutter-project.org/show_bug.cgi?id=2349#c7 , still needs review) so I got back working on this. This patch mostly works there is still an issue with the top panel, not being drawn at all sometimes.
(In reply to comment #5) > Created an attachment (id=172044) [details] [review] > [WIP] St: Take advantage of clipped redraws > > In order to take advantage of clipped redraws (only redraw the > parts that actually changed), we have to inform clutter about > our paint_volume by implementing the get_paint_volume virtual > method. > > As this feature had been added in in clutter 1.5.x we now require > that. > > ---- > > I have found the clutter bug that was causing clipped redraws not to work > (http://bugzilla.clutter-project.org/show_bug.cgi?id=2349#c7 , still needs > review) so I got back working on this. > > This patch mostly works there is still an issue with the top panel, not being > drawn at all sometimes. Forgot to add this with CLUTTER_PAINT=redraws set the "would be redrawn" parts are highlighted with a red rectangle (which in fact full redraws are used). This way the paint volumes can be seen without the corruption (top panel).
Created attachment 172045 [details] [review] St: Take advantage of clipped redraws In order to take advantage of clipped redraws (only redraw the parts that actually changed), we have to inform clutter about our paint_volume by implementing the get_paint_volume virtual method. As this feature had been added in in clutter 1.5.x we now require that. ---- I forgot about ShellGenericContainer, this version works as expected.
(In reply to comment #7) > I forgot about ShellGenericContainer, this version works as expected. OK, after using it for a while it seems that there are cases where it isn't correct yet (text input, window drag and drop in overview, closing of the lg pane).
Review of attachment 172045 [details] [review]: ::: src/shell-generic-container.c @@ +232,3 @@ + ClutterPaintVolume *volume) +{ + GList *l, *children; You need to start off with the paint volume that StWidget would allocate - a ShellGenericContainer can have CSS.... so chain up with: CLUTTER_ACTOR_CLASS (shell_generic_container_parent_class)->get_paint_volume (actor, volume); (Note that for Shell.GenericContainers that don't have CSS, this overestimates - it's possible children would only occupy a small fraction of the parent. We could do better for pure-container subclasses of St.Widget if we had a st_theme_node_get_paint_box() variant that returned empty when there was no styling, it might matter if we had a St.GenericContainer() or St.Group that took up the entire screen and allocated it's child as only a small part. We might even do that for Alt-Tab ... have't checked. But the simple thing of allocation-of-container+paint_box+allocation-of-children is a good first step.) @@ +234,3 @@ + GList *l, *children; + + children = clutter_container_get_children (CLUTTER_CONTAINER (actor)); clutter_container_get_children() returns a newly allocated list of children that you have to free with g_list_free(), but instead use st_container_get_children_list (ST_CONTAINER (actor)); - that function returns the internal list without copying. @@ +237,3 @@ + + if (children == NULL) + return TRUE; This check is pointless - we don't do anything after this if children is NULL ::: src/st/st-group.c @@ +234,3 @@ +static gboolean +st_group_get_paint_volume (ClutterActor *actor, + ClutterPaintVolume *volume) All the same comments as for Shell.GenericContainer ::: src/st/st-widget.c @@ +639,3 @@ + clutter_actor_get_allocation_geometry (self, &paint_geometry); + clutter_paint_volume_set_width (volume, paint_geometry.width); + clutter_paint_volume_set_height (volume, paint_geometry.height); Logic here just doesn't make a lot of sense. get_allocation_geometry() call ovewrites the stuff you put into paint_geometry, etc. Here's the diff to a local version that seems to work pretty well for me. - ClutterGeometry paint_geometry; ClutterActorBox paint_box, alloc_box; StThemeNode *theme_node; + ClutterVertex origin; /* Setting the paint volume does not make sense when we don't have any allocation */ if (!clutter_actor_has_allocation (self)) @@ -659,20 +659,17 @@ st_widget_get_paint_volume (ClutterActor *self, ClutterPaintVolume *volume) theme_node = st_widget_get_theme_node (ST_WIDGET(self)); clutter_actor_get_allocation_box (self, &alloc_box); st_theme_node_get_paint_box (theme_node, &alloc_box, &paint_box); - - paint_geometry.x = paint_box.x1; - paint_geometry.y = paint_box.y1; - paint_geometry.width = paint_box.x2 - paint_box.x1; - paint_geometry.height = paint_box.y2 - paint_box.y1; - clutter_actor_get_allocation_geometry (self, &paint_geometry); - clutter_paint_volume_set_width (volume, paint_geometry.width); - clutter_paint_volume_set_height (volume, paint_geometry.height); + origin.x = paint_box.x1 - alloc_box.x1; + origin.y = paint_box.y1 - alloc_box.y1; + + clutter_paint_volume_set_origin (volume, &origin); + clutter_paint_volume_set_width (volume, paint_box.x2 - paint_box.x1); + clutter_paint_volume_set_height (volume, paint_box.y2 - paint_box.y1);
(In reply to comment #9) > Review of attachment 172045 [details] [review]: > > ::: src/shell-generic-container.c > @@ +232,3 @@ > + ClutterPaintVolume *volume) > +{ > + GList *l, *children; > > You need to start off with the paint volume that StWidget would allocate - a > ShellGenericContainer can have CSS.... so chain up with: > > CLUTTER_ACTOR_CLASS (shell_generic_container_parent_class)->get_paint_volume > (actor, volume); OK > (Note that for Shell.GenericContainers that don't have CSS, this overestimates > - it's possible children would only occupy a small fraction of the parent. We > could do better for pure-container subclasses of St.Widget if we had a > st_theme_node_get_paint_box() variant that returned empty when there was no > styling, it might matter if we had a St.GenericContainer() or St.Group that > took up the entire screen and allocated it's child as only a small part. We > might even do that for Alt-Tab ... have't checked. But the simple thing of > allocation-of-container+paint_box+allocation-of-children is a good first step.) OK > @@ +234,3 @@ > + GList *l, *children; > + > + children = clutter_container_get_children (CLUTTER_CONTAINER (actor)); > > clutter_container_get_children() returns a newly allocated list of children > that you have to free with g_list_free(), but instead use > st_container_get_children_list (ST_CONTAINER (actor)); - that function returns > the internal list without copying. OK > @@ +237,3 @@ > + > + if (children == NULL) > + return TRUE; > > This check is pointless - we don't do anything after this if children is NULL > > ::: src/st/st-group.c > @@ +234,3 @@ > +static gboolean > +st_group_get_paint_volume (ClutterActor *actor, > + ClutterPaintVolume *volume) > > All the same comments as for Shell.GenericContainer OK > ::: src/st/st-widget.c > @@ +639,3 @@ > + clutter_actor_get_allocation_geometry (self, &paint_geometry); > + clutter_paint_volume_set_width (volume, paint_geometry.width); > + clutter_paint_volume_set_height (volume, paint_geometry.height); > > Logic here just doesn't make a lot of sense. get_allocation_geometry() call > ovewrites the stuff you put into paint_geometry, etc. Here's the diff to a > local version that seems to work pretty well for me. D'oh .. What do you mean by "work pretty well for me" (i.e how did you test it) ? Using my hack from http://bugzilla.clutter-project.org/show_bug.cgi?id=2349 it seems to work just the same as the other version .. text isn't drawn when entered (in alt-f2, lg) and dnd of windows in the overview behaves weird when hitting the screen edge, Note that CLUTTER_PAINT=redraws actually *disables* clipped redraws (you'd have to test again without the env var set).
(In reply to comment #10) > D'oh .. > > What do you mean by "work pretty well for me" (i.e how did you test it) ? > > Using my hack from http://bugzilla.clutter-project.org/show_bug.cgi?id=2349 it > seems to work just the same as the other version .. text isn't drawn when > entered (in alt-f2, lg) and dnd of windows in the overview behaves weird when > hitting the screen edge, > > Note that CLUTTER_PAINT=redraws actually *disables* clipped redraws (you'd have > to test again without the env var set). With your patch, fixed up as described above, with the connections to "paint" in shell-global.c comented out (see bug 633516 and the linked Clutter bug), and with unmodified clutter master, what I observe is: - CLUTTER_DEBUG=backend reports sub-stage updates - CLUTTER_PAINT=redraws shows plausible looking boxes for most changes (accounting for the fact that the Mutter actors aren't reporting bounding volumes nor are the ClutterClones we use for windows in the overview) - Without CLUTTER_PAINT=redraws things seem to look correct; Alt-f2, lg, window dnd, ec. Maybe the corruptions you are seeing are those caused by what rib describes in: http://bugzilla.clutter-project.org/show_bug.cgi?id=2349#c8 Certainly sounds plausible that it would cause text not to update correctly when changing.
(In reply to comment #11) > With your patch, fixed up as described above, with the connections to "paint" > in shell-global.c comented out (see bug 633516 and the linked Clutter bug), and > with unmodified clutter master, Oh, I missed that one removing it makes it work with an unmodified clutter. > what I observe is: > > - CLUTTER_DEBUG=backend reports sub-stage updates > - CLUTTER_PAINT=redraws shows plausible looking boxes for most changes > (accounting for the fact that the Mutter actors aren't reporting bounding > volumes nor are the ClutterClones we use for windows in the overview) > - Without CLUTTER_PAINT=redraws things seem to look correct; Alt-f2, lg, > window dnd, ec. Same here have not seen any corruption yet (and the ones I reported above are gone). > Maybe the corruptions you are seeing are those caused by what rib describes in: > > http://bugzilla.clutter-project.org/show_bug.cgi?id=2349#c8 > > Certainly sounds plausible that it would cause text not to update correctly > when changing. Yeah.
Created attachment 173555 [details] [review] St: Take advantage of clipped redraws In order to take advantage of clipped redraws (only redraw the parts that actually changed), we have to inform clutter about our paint_volume by implementing the get_paint_volume virtual method. As this feature had been added in in clutter 1.5.x we now require that.
(In reply to comment #12) > > - CLUTTER_DEBUG=backend reports sub-stage updates > > - CLUTTER_PAINT=redraws shows plausible looking boxes for most changes > > (accounting for the fact that the Mutter actors aren't reporting bounding > > volumes nor are the ClutterClones we use for windows in the overview) > > - Without CLUTTER_PAINT=redraws things seem to look correct; Alt-f2, lg, > > window dnd, ec. > > Same here have not seen any corruption yet (and the ones I reported above are > gone). One thing I am seeing is that when I pop down a menu, the underlying window isn't repainted. Trying to track that down now.
(In reply to comment #14) > (In reply to comment #12) > > > - CLUTTER_DEBUG=backend reports sub-stage updates > > > - CLUTTER_PAINT=redraws shows plausible looking boxes for most changes > > > (accounting for the fact that the Mutter actors aren't reporting bounding > > > volumes nor are the ClutterClones we use for windows in the overview) > > > - Without CLUTTER_PAINT=redraws things seem to look correct; Alt-f2, lg, > > > window dnd, ec. > > > > Same here have not seen any corruption yet (and the ones I reported above are > > gone). > > One thing I am seeing is that when I pop down a menu, the underlying window > isn't repainted. Trying to track that down now. Was about to submit this: --- > Same here have not seen any corruption yet (and the ones I reported above are > gone). OK, I found two cases of corruption: 1) Sometimes window previews (of gnome-terminal) aren't drawn at all in altTab 2) OR windows can leave a trace when closed, reproduce able in firefox by doing: Open the context menu; give it focus (i.e move the mouse outside of it); right click somewhere outside of the menu, notice that a part of it (or at least its shadow) is still visible --- But hit a midair collision.
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #12) > > > > - CLUTTER_DEBUG=backend reports sub-stage updates > > > > - CLUTTER_PAINT=redraws shows plausible looking boxes for most changes > > > > (accounting for the fact that the Mutter actors aren't reporting bounding > > > > volumes nor are the ClutterClones we use for windows in the overview) > > > > - Without CLUTTER_PAINT=redraws things seem to look correct; Alt-f2, lg, > > > > window dnd, ec. > > > > > > Same here have not seen any corruption yet (and the ones I reported above are > > > gone). > > > > One thing I am seeing is that when I pop down a menu, the underlying window > > isn't repainted. Trying to track that down now. Tracked it down to http://bugzilla.clutter-project.org/show_bug.cgi?id=2389. Don't immediately know if that's the same as the other issue you are seeing - I don't see that in a quick test, but you say that the previews are only sometimes missing.
(In reply to comment #16) > > Tracked it down to http://bugzilla.clutter-project.org/show_bug.cgi?id=2389. This seems to fix the menu corruption for me to. > Don't immediately know if that's the same as the other issue you are seeing - I > don't see that in a quick test, but you say that the previews are only > sometimes missing. As for the thumbnails thing, it doesn't. A diff like that: ----- index b3f074d..3271f48 100644 --- a/js/ui/altTab.js +++ b/js/ui/altTab.js @@ -112,6 +112,7 @@ AltTabPopup.prototype = { let [childMinHeight, childNaturalHeight] = this._thumbnails.actor. childBox.y2 = childBox.y1 + childNaturalHeight; this._thumbnails.actor.allocate(childBox, flags); + this._thumbnails.actor.queue_relayout(); } }, ---- does but this is obviously wrong as it triggers warnings like --- (mutter:12245): Clutter-WARNING **: The actor 'altTabPopup' is currently inside an allocation cycle; calling clutter_actor_queue_relayout() is not recommended (mutter:12245): Clutter-WARNING **: The actor 'ClutterGroup' is currently inside an allocation cycle; calling clutter_actor_queue_relayout() is not recommended --- I don't really understand why it is needed though, I just noticed then when pressing the arrow key the missing thumbnail appears.
Created attachment 174386 [details] [review] StImText: add get_paint_volume() Since StImText isn't a StWidget, it needs it's own implementation of get_paint_volume() to enable clipped redraws.
Created attachment 174390 [details] [review] StBoxLayout: report correct paint volume when scrolled When scrolled, st_box_layout_apply_transform() includes the scroll offset and affects paint volumes. This is right for our children, but our paint volume is determined by our allocation and borders and doesn't scroll, so we need to reverse-compensate, the same as we do when painting.
Created attachment 174400 [details] [review] Initialize ClutterVertex.z (squash with main StWidget patch) ClutterVertex has three components; we should set all of them.
In addition to the patches above, needed are patches from: http://bugzilla.clutter-project.org/show_bug.cgi?id=2396 (fixes the alt-tab thumbnails problem) http://bugzilla.clutter-project.org/show_bug.cgi?id=2416 (fixes problems with missing updates when typing in LookingGlass) I think when those land and Clutter we can go ahead and land this. It would be possible to make this all conditional with: +#if CLUTTER_CHECK_VERSION(1, 5, 2) checks, but I dont' really want to support two different versions of clutter with very different code paths and performance characteristics, so it seems better to just up the clutter requirement.
Review of attachment 173555 [details] [review]: Code here looks good except for the things noted below. Good to commit once the Clutter fixes land. ::: src/st/st-group.c @@ +231,3 @@ } +/* Based on implementation from clutter-group.c */ Should add a 'Copyright 2010 Intel Corporation' to the copyright headers ::: src/st/st-widget.c @@ +647,3 @@ +static gboolean +st_widget_get_paint_volume (ClutterActor *self, ClutterPaintVolume *volume) Should add yourself to the copyright header (This is on the margin of the "10 lines or more" criteria, but I generally included copyright notices for ST for the marginal cases especially when they were making the code do something that it wasn't doing before as opposed to mechanical fixes.) @@ +662,3 @@ + + origin.x = paint_box.x1 - alloc_box.x1; + origin.y = paint_box.y1 - alloc_box.y1; My patch to set origin.z should be merged in here before pushing
Assigning to Adel for review of my patches
Review of attachment 174386 [details] [review]: This looks fine and is rather obvious, but can be done without code duplication see below. ::: src/st/st-im-text.c @@ +187,3 @@ +{ + /* Same logic as the private + * _clutter_actor_set_default_paint_volume() There is clutter_paint_volume_set_from_allocation() which is public so you just can do return clutter_paint_volume_set_from_allocation (volume, self);
Review of attachment 174390 [details] [review]: Looks good.
Created attachment 175379 [details] [review] St: Take advantage of clipped redraws *) Squashed with origin.z patch *) Adjusted copyright headers *) Bumped clutter requirement to 1.5.7
Created attachment 175480 [details] [review] StImText: add get_paint_volume() Since StImText isn't a StWidget, it needs it's own implementation of get_paint_volume() to enable clipped redraws.
I've gone ahead and pushed your changes and my changes since Clutter 1.5.8 is out with the necessary dependent patches (with your suggested change to simplify the StImText patch and an increase in the required version from 1.5.7 to 1.5.8.) Thanks for the work on this! Attachment 174390 [details] pushed as d870fef - StBoxLayout: report correct paint volume when scrolled Attachment 175379 [details] pushed as 56fb7e2 - St: Take advantage of clipped redraws Attachment 175480 [details] pushed as 6b723ed - StImText: add get_paint_volume()