Created attachment 248074 [details] [review]
meta-shaped-texture: Don't queue redraws for obscured regions

When we get a damage event we update the window by calling
meta_shaped_texture_update_area which queues a redraw on the actor.
We can avoid that for obscured regions by comparing the damage area to
our visible area.

To be able to do that we have to keep the visible regions set even after painting. We did reset them because "they will mess up painting clones of our actors".

Handle the clone case by checking clutter_actor_is_in_clone_paint in
meta_shaped_texture_paint.

Created attachment 248078 [details] [review]
meta-shaped-texture: Don't queue redraws for obscured regions

When we get a damage event we update the window by calling
meta_shaped_texture_update_area which queues a redraw on the actor.
We can avoid that for obscured regions by comparing the damage area to
our visible area.

To be able to do that we have to keep the visible regions set even
after painting. We did reset them because "they will mess up painting clones of our actors".

Handle the clone case by checking clutter_actor_is_in_clone_paint in
meta_shaped_texture_paint.

We also have to ignore the clip region when the actor has any effects applied.


---

This one handles effects and clones better.

Did some testing (CLUTTER_SHOW_FPS=1): 
glxgears in front - before: 60fps - after: 60fps
glxgears behind firefox - before: 60fps - after: 3fps

Bugs:
For some reason wriing a message in xchat and hitting enter has some
paint delay with this patch ...

Do you still need has_mapped_clones? You're not using it in this latest patch...

(In reply to comment #3)
> Do you still need has_mapped_clones? You're not using it in this latest
> patch...

Yes somehow got lost while debugging the xchat thing.

Created attachment 248080 [details] [review]
meta-shaped-texture: Don't queue redraws for obscured regions

When we get a damage event we update the window by calling
meta_shaped_texture_update_area which queues a redraw on the actor.
We can avoid that for obscured regions by comparing the damage area to
our visible area.

To be able to do that we have to keep the visible regions set even
after painting. We did reset them because "they will mess up painting clones of our actors".

Handle the clone case by checking clutter_actor_is_in_clone_paint in
meta_shaped_texture_paint.

We also have to ignore the clip region when the actor has any effects applied.

Created attachment 248082 [details] [review]
meta-shaped-texture: Don't queue redraws for obscured regions

When we get a damage event we update the window by calling
meta_shaped_texture_update_area which queues a redraw on the actor.
We can avoid that for obscured regions by comparing the damage area to
our visible area.

To be able to do that we have to keep the visible regions set even
after painting. We did reset them because "they will mess up painting clones of our actors".

Handle the clone case by checking clutter_actor_is_in_clone_paint in
meta_shaped_texture_paint.

We also have to ignore the clip region when the actor has any effects applied.


-----

OK, found the "xchat bug" ... with this patch the optimization seems to work
fine here without any side effects.

Created attachment 248156 [details] [review]
meta-shaped-texture: Don't queue redraws for obscured regions

When we get a damage event we update the window by calling
meta_shaped_texture_update_area which queues a redraw on the actor.
We can avoid that for obscured regions by comparing the damage area to
our visible area.

To be able to do that we have to keep the visible regions set even
after painting. We did reset them because "they will mess up painting clones of our actors".

Handle the clone case by checking clutter_actor_is_in_clone_paint in
meta_shaped_texture_paint.

We also have to ignore the clip region when the actor has any effects applied.



We need to send frame drawn messages for wm synced apps. To avoid a redraw do that using a timer. I don't know whether 17ms "~ 1 frame" is a good enough estimation but I went with that for now.

Created attachment 248160 [details] [review]
meta-shaped-texture: Don't queue redraws for obscured regions

When we get a damage event we update the window by calling
meta_shaped_texture_update_area which queues a redraw on the actor.
We can avoid that for obscured regions by comparing the damage area to
our visible area.

To be able to do that we have to keep the visible regions set even
after painting. We did reset them because "they will mess up painting clones of our actors".

Handle the clone case by checking clutter_actor_is_in_clone_paint in
meta_shaped_texture_paint.

We also have to ignore the clip region when the actor has any effects applied.


--

We have to send frame drawn messages when the the app uses the sync protocol,
so do that even in case where we do not draw stuff on screen. I am not sure
whether the 17ms delay "~1 frame" is good enough but not sure how to really
test that.

While at it I updated the code to use the more efficent / convient clutter_actor_iter
API.

Created attachment 248178 [details] [review]
meta-shaped-texture: Don't queue redraws for obscured regions

When we get a damage event we update the window by calling
meta_shaped_texture_update_area which queues a redraw on the actor.
We can avoid that for obscured regions by comparing the damage area to
our visible area.

To be able to do that we have to keep the visible regions set even
after painting. We did reset them because "they will mess up painting clones of our actors".

Handle the clone case by checking clutter_actor_is_in_clone_paint in
meta_shaped_texture_paint.

We also have to ignore the clip region when the actor has any effects applied.


---

Fix crash

This seemingly obvious change raises some pretty deep questions about  synchronization works. 

In the draft wm-spec changes for synchronization I wrote in http://fishsoup.net/misc/wm-spec-synchronization.html


  To indicate the end of the frame the client increases
  the value of the extended frame counter to an even value. 
  At this point the window is no longer frozen, and the
  compositor SHOULD redraw the window and send _NET_WM_FRAME_DRAWN
  and _NET_WM_FRAME_TIMINGS messages. The window manager SHOULD
  redraw the window and send the messages even if no damage events
  were received.

  Rational: the reason for redrawing the window even if no damage
  events have been    received is so that there is consistent timing
  for each frame, even if one frame happens to involve no changes to
  the window. It is recommended to behave as if a damage event was
  received containing only a single pixel.

Simply not sending the messages isn't an option - not only does it violate the draft spec, it doesn't work:
 
A) The application may be waiting for the _NET_WM_FRAME_DRAWN in order to do something that will make it become unobscured - for example, a window might be animating itself larger in such a way that it will peek out from behind a window.

B) If we don't send _NET_WM_FRAME_DRAWN until the window is unobscured, the first time we draw it unobscured, we will draw it frozen in whatever state it had when it became obscured - so, e.g., a clock on another desktop would show the old time when switched to, then switch to the new time.

But if there is no redraw, no swap, and no waiting for the vertical blank, when do we send these messages? Sending them immediately would put the application into a 100% cpu/gpu infinite loop. In fact, even sending messages back so that the application redraws at 60fps seems like an unnecessary waste of CPU/GPU if it isn't visible to the user. Having the application redraw at 10fps seems like it should be perfectly sufficient.

Additionally, we have a question about what the presentation time offset, refresh interval, and frame delay should be in the _NET_WM_FRAME_TIMINGS message. Should we be using a presentation time offset that represents "if I swapped when I sent _NET_WM_FRAME_DRAWN, the frame would have been drawn at time X", or should 0 be sent indicating that no value is known?

Finally we have the issue of the case when we want to have very exact video/audio synchronization - as represented by tests/video-timer.c in the GTK+ source code. In this case we might have determined that we need to resample audio by 0.1% or something so that it exactly stays in step with the video. If the user switches away from the video and obscures the window and at that point we stop sending relevant _NET_WM_FRAME_TIMING messages, then the application will lose sync and have to reestablish it when the user switches back - resulting in outputs that will be a few ms's misaligned for a few seconds.

(In fact, the video system might switch the refresh rate on the output down if nothing draws to it, to save power.)

My feeling at this point are:

 * Throttle drawing for obscured windows down to ~100ms interval. An algorithm I think would work (haven't re-read your patch yet)

 - don't queue a redraw for the obscured damage; if we have no non-obscured damage:
   - Set up a timer to go off at a multiple of 6*refresh_interval us after the last time we sent a _NET_WM_FRAME_DRAWN for the window. 
 - If we redraw the window (paint method is called) before that timer expires, remove the timer.
 - Exclude windows in the timer-pending state from sending _NET_WM_FRAME_DRAWN in  meta_window_actor_post_paint()
 - If the timer expires, send _NET_WM_FRAME_DRAWN and _NET_WM_FRAME_TIMINGS messages

 * For timer-driven DRAWN/TIMINGS messages, send 0 for presentation_time_offset. Also send 0 for refresh_interval. Send the normal value for frame_delay

 * Ignore the exact-synchronization audio-synchronization case - if you want your screening room movie playback app to to stay 100% sychronized, don't alt-tab away from it.

Review of attachment 248178 [details] [review]:

I dislike having the clip region be used for two different things:

 - The clipped region of a window during the paint
 - The visible region of a window otherwise

What I'd rather see is that when we paint the window group, we walk through and create the visible regions and store them. (Probably on the actor, not on the shaped textures - I'd like to keep shaped textures more low level)

Then compute the clipped regions as the intersection of the visible region with the clip. (This also avoids a lot of duplicated transformation calculations) and use them as currently - set then unset.

You need to document a very important assumption somewhere:

 "If anything changes that would change the computed obscured regions - the transforms on actors, whether actors have clones, whether actors have transforms, then the entire stage will be queued for redraw for each frame and we don't need to care what we individually queue for redraw for each actor"

::: src/compositor/meta-shaped-texture.c
@@ +146,3 @@
+      cairo_region_is_empty (priv->clip_region) &&
+      !clutter_actor_is_in_clone_paint (actor) &&
+      !clutter_actor_has_effects (actor))

I don't know why you added has_effects here - it has to be taken into account in the code that computes clip_region already, right? (For the clone_paint, see my suggestion of doing clip and visible region computations differently)

@@ +231,3 @@
+  if (priv->clip_region &&
+     !clutter_actor_is_in_clone_paint (actor) &&
+     !clutter_actor_has_effects (actor))

Same here

@@ +444,3 @@
+  if ((priv->clip_region &&
+      cairo_region_contains_rectangle (priv->clip_region, &clip) != CAIRO_REGION_OVERLAP_OUT) ||
+      !priv->clip_region ||

I think if we're tracking all this, it makes sense to actually queue the redraw only for the bounding box of the intersection, not queue the redraw on the full damage rectangle if there is an intersection.

::: src/compositor/meta-window-actor.c
@@ +946,3 @@
+
+  if (!redraw_queued)
+    clutter_actor_queue_redraw (priv->actor);

I don't understand the point of this - damage all is simply "damaging all" - if it was fine not to queue a redraw if all the window is damaged, it's fine not to queue a redraw here

@@ +1930,3 @@
+  priv->needs_frame_drawn = TRUE;
+  frame->sync_request_serial = priv->window->sync_request_serial;
+  MetaWindowActorPrivate *priv = self->priv;

I'm not sure whether we'll end up needing this method or not, but I don't like the duplication with meta_window_actor_queue_frame_drawn().

@@ +1996,3 @@
+
+  if (priv->window->extended_sync_request_counter && !priv->repaint_scheduled)
+      g_timeout_add_full (META_PRIORITY_REDRAW, 17, fake_post_paint, self, NULL);

This timeout could crash if the actor was destroyed in the meantime.

=== 

Isn't it the case that meta_window_actor_queue_frame_drawn() is going to be called from sync protocol at the end of the frame for such a window - and if repaint is not scheduled, is going to do the "A frame was marked by the client without actually doing any damage" code path and create damage 1x1 damage?

I think we probably want to distinguish two cases:

 - Window is completely obscured - do the 100ms thing described in my comment.

 - Window is not completely obscured, but no damage or only obscured damage - keep the display hot and actually trigger a stage repaint as we do now for no damage. Ideally, we'd have some clutter method to queue 0x0 damage - to force a swap, but not specifically queue anything for redraw - to avoid the "bounding box" problem we get with the 1x1 damage where we might end up redrawing a large area of the stage unless the 1x1 damage is the only thing we redraw. (But could keep 1x1 damage for the moment)

This is a little bit of a change from what I said in my comment, and the reasoning is that we shouldn't be changing timing methods depending on exactly what portion of an animation is visible.

[I could be convinced that we should do the 100ms thing in all cases as well... advantages or disadvanges either way]

::: src/compositor/meta-window-group.c
@@ +89,2 @@
 static void
+update_visible_regions (ClutterActor *actor, cairo_rectangle_int_t visible_rect)

Don't pass structures by value, each parameter on a separate line

@@ +152,3 @@
+   */
+  clutter_actor_iter_init (&iter, actor);
+  while (clutter_actor_iter_prev (&iter, &child))

This is a good change, but making it part of the same patch makes the changes to this file very hard to read.

Created attachment 249371 [details] [review]
meta-window-group: Use clutter's iteration API

Use the clutter iteration API instead of copying the list of children.
This is more efficent.

---

Split out iterator changes.

(In reply to comment #11)
> Review of attachment 248178 [details] [review]:
> 
> I dislike having the clip region be used for two different things:
> 
>  - The clipped region of a window during the paint
>  - The visible region of a window otherwise
> 
> What I'd rather see is that when we paint the window group, we walk through and
> create the visible regions and store them. (Probably on the actor, not on the
> shaped textures - I'd like to keep shaped textures more low level)
> 
> Then compute the clipped regions as the intersection of the visible region with
> the clip. (This also avoids a lot of duplicated transformation calculations)
> and use them as currently - set then unset.

OK this makes sense.

> You need to document a very important assumption somewhere:
> 
>  "If anything changes that would change the computed obscured regions - the
> transforms on actors, whether actors have clones, whether actors have
> transforms, then the entire stage will be queued for redraw for each frame and
> we don't need to care what we individually queue for redraw for each actor"

Well just because there are transformations or clones does not necessary mean that the entire stage will get redrawn. But yeah that case should be documented.

> ::: src/compositor/meta-shaped-texture.c
> @@ +146,3 @@
> +      cairo_region_is_empty (priv->clip_region) &&
> +      !clutter_actor_is_in_clone_paint (actor) &&
> +      !clutter_actor_has_effects (actor))
> 
> I don't know why you added has_effects here - it has to be taken into account
> in the code that computes clip_region already, right? (For the clone_paint, see
> my suggestion of doing clip and visible region computations differently)

Yeah will remove.


> 
> @@ +444,3 @@
> +  if ((priv->clip_region &&
> +      cairo_region_contains_rectangle (priv->clip_region, &clip) !=
> CAIRO_REGION_OVERLAP_OUT) ||
> +      !priv->clip_region ||
> 
> I think if we're tracking all this, it makes sense to actually queue the redraw
> only for the bounding box of the intersection, not queue the redraw on the full
> damage rectangle if there is an intersection.

Makes sense.

> ::: src/compositor/meta-window-actor.c
> @@ +946,3 @@
> +
> +  if (!redraw_queued)
> +    clutter_actor_queue_redraw (priv->actor);
> 
> I don't understand the point of this - damage all is simply "damaging all" - if
> it was fine not to queue a redraw if all the window is damaged, it's fine not
> to queue a redraw here

OK

> @@ +1930,3 @@
> +  priv->needs_frame_drawn = TRUE;
> +  frame->sync_request_serial = priv->window->sync_request_serial;
> +  MetaWindowActorPrivate *priv = self->priv;
> 
> I'm not sure whether we'll end up needing this method or not, but I don't like
> the duplication with meta_window_actor_queue_frame_drawn().

OK

> @@ +1996,3 @@
> +
> +  if (priv->window->extended_sync_request_counter && !priv->repaint_scheduled)
> +      g_timeout_add_full (META_PRIORITY_REDRAW, 17, fake_post_paint, self,
> NULL);
> 
> This timeout could crash if the actor was destroyed in the meantime.

Oh ... indeed.

> === 
> 
> Isn't it the case that meta_window_actor_queue_frame_drawn() is going to be
> called from sync protocol at the end of the frame for such a window - and if
> repaint is not scheduled, is going to do the "A frame was marked by the client
> without actually doing any damage" code path and create damage 1x1 damage?
> 
> I think we probably want to distinguish two cases:
> 
>  - Window is completely obscured - do the 100ms thing described in my comment.
> 
>  - Window is not completely obscured, but no damage or only obscured damage -
> keep the display hot and actually trigger a stage repaint as we do now for no
> damage. Ideally, we'd have some clutter method to queue 0x0 damage - to force a
> swap, but not specifically queue anything for redraw - to avoid the "bounding
> box" problem we get with the 1x1 damage where we might end up redrawing a large
> area of the stage unless the 1x1 damage is the only thing we redraw. (But could
> keep 1x1 damage for the moment)

Alternatively we could ask clutter to give us a pixel in an area that will be redrawn anyway. We currently do this for picking when using buffer_age (to avoid the same problem), but we'd need new API in clutter for that as well (the current internal one won't work for the non buffer age case). But if we are going to add API to clutter anyway maybe the 0x0 damage is better.

> This is a little bit of a change from what I said in my comment, and the
> reasoning is that we shouldn't be changing timing methods depending on exactly
> what portion of an animation is visible.

OK

> [I could be convinced that we should do the 100ms thing in all cases as well...
> advantages or disadvanges either way]
> 
> ::: src/compositor/meta-window-group.c
> @@ +89,2 @@
>  static void
> +update_visible_regions (ClutterActor *actor, cairo_rectangle_int_t
> visible_rect)
> 
> Don't pass structures by value, each parameter on a separate line

OK

> @@ +152,3 @@
> +   */
> +  clutter_actor_iter_init (&iter, actor);
> +  while (clutter_actor_iter_prev (&iter, &child))
> 
> This is a good change, but making it part of the same patch makes the changes
> to this file very hard to read.

Split out see comment 12

Review of attachment 249371 [details] [review]:

OK.

Comment on attachment 249371 [details] [review]
meta-window-group: Use clutter's iteration API

Attachment 249371 [details] pushed as e3855c7 - meta-window-group: Use clutter's iteration API

Created attachment 249685 [details] [review]
window-actor: Fix doc comment

Created attachment 249686 [details] [review]
meta-shaped-texture: Don't queue redraws for obscured regions

When we get a damage event we update the window by calling
meta_shaped_texture_update_area which queues a redraw on the actor.
We can avoid that for obscured regions by comparing the damage area to
our visible area.

Created attachment 249687 [details] [review]
meta-window-actor: Throttle obscured frame synced apps

We must send frame_drawn and frame_timing messages to even when
we don't actually queue a redraw on screen to comply with the
WM sync spec.

So throttle such apps to down to a ~100ms interval.

Created attachment 249696 [details] [review]
meta-window-actor: Throttle obscured frame synced apps

We must send frame_drawn and frame_timing messages to even when
we don't actually queue a redraw on screen to comply with the
WM sync spec.

So throttle such apps to down to a ~100ms interval.

Review of attachment 249685 [details] [review]:

Looks good

Review of attachment 249686 [details] [review]:

I'd like to see some mention in the commit message like:

 This patch causes _NET_WM_FRAME_DRAWN messages to be not sent in some cases where
 they should be sent; they will be added back in a later commit.

::: src/compositor/meta-shaped-texture.c
@@ +421,1 @@
 meta_shaped_texture_update_area (MetaShapedTexture *stex,

Can you add a doc comment - the operation of this function becomes obscure with your patch - what does the return value mean? what is visible_region? It's not obvious from the prototype.

@@ +439,3 @@
   meta_texture_tower_update_area (priv->paint_tower, x, y, width, height);
 
+  if (visible_region && !clutter_actor_has_mapped_clones (CLUTTER_ACTOR (stex)))

It might make more sense if the mapped_clones check was in the caller? I think it's basically a check if "visible_region" passed in is correct or not - so it would be better if the caller did the check and passed in a NULL visible region.

::: src/compositor/meta-window-actor.c
@@ +1668,3 @@
  *
  * Provides a hint as to what areas of the window need to be
  * drawn. Regions not in @visible_region are completely obscured.

The doc comments should explicitly say what the difference between set_visible_region and set_clip_region are - so here, something like "unlike meta_window_actor_set_clip_region(), the region here doesn't take into account any clipping that is in effect while drawing"

@@ +1695,3 @@
+ *
+ * Provides a hint as to what areas of the window need to be
+ * drawn. Regions not in @clip_region are completely obscured.

"are completely obscured or not drawn in this frame"

@@ +1700,3 @@
+void
+meta_window_actor_set_clip_region (MetaWindowActor *self,
+                                      cairo_region_t  *clip_region)

Check alignment here - it looks like this line might not have been re-indented after renaming.

@@ +1992,2 @@
 static cairo_region_t *
+scan_clip_region (guchar         *mask_data,

This function had nothing to do with priv->visible_region

::: src/compositor/meta-window-group.c
@@ +93,2 @@
   cairo_region_t *visible_region;
+  cairo_region_t *intersection;

I know I suggested doing the intersection thing, but I was more thinking (incorrectly) that visible_region could avoid being updated on every paint. As the code came out here, it doesn't really make any sense - you are doing all the subtractions, translations, etc, but constantly creating and destroying 'intersection'. It will read cleaner and I think be more efficient if you simply parallel update both clip_region and visible_region as you go down the stack.

@@ +251,2 @@
           if (META_IS_BACKGROUND_GROUP (child))
+            meta_background_group_set_visible_region (META_BACKGROUND_GROUP (child), intersection);

You probably need to update the name of this to be clip_region

Review of attachment 249696 [details] [review]:

In terms of testing, the basic thing would be to use GDK_DEBUG=frames and run gtk+/tests/animated-resizing (with and without -n) and check behavior with different levels of exposure.

::: src/compositor/meta-shaped-texture.c
@@ -455,3 @@
           clutter_actor_queue_redraw_with_clip (CLUTTER_ACTOR (stex), &damage_rect);
           cairo_region_destroy (intersection);
-

Spurious change to this file.

::: src/compositor/meta-window-actor.c
@@ +82,3 @@
   cairo_region_t   *visible_region;
 
+  gint              frame_timer;

frame_timer is too generic - send_frame_messages_timer

@@ +656,3 @@
   MetaShadow *shadow = appears_focused ? priv->focused_shadow : priv->unfocused_shadow;
 
+  if (priv->frame_timer != 0)

This needs a comment - something like:

 /* This window got damage when obscured; we set up a timer
  * to send frame completion events, but since we're drawing
  * the window now (for some other reason) cancel the timer
  * and send the completion events normally */

@@ +924,3 @@
 
+static
+gboolean fake_post_paint (gpointer data)

I don't like the name "fake_post_paint" - send_frame_messages_timeout() or something.

@@ +929,3 @@
+  MetaWindowActorPrivate *priv = self->priv;
+
+  prepare_frame (self);

I'm not OK with linking in a frame into priv->frames so that do_send_frame_drawn() finds it, and then unlinking it immediately - this is really ugly. Consider passing the frame into do_send_frame_drawn().

@@ +932,3 @@
+
+  do_send_frame_drawn (self);
+  do_send_frame_timings (self, priv->frames->data, 0, 0);

Shouldn't you be setting needs_frame_drawn to false here?

@@ +934,3 @@
+  do_send_frame_timings (self, priv->frames->data, 0, 0);
+
+  priv->frames = g_list_delete_link (priv->frames, priv->frames);

Leaking the FrameData.

@@ +948,3 @@
+  gint64 current_time = meta_compositor_monotonic_time_to_server_time (display, g_get_monotonic_time ());
+  gint interval = 16667 * 6; // FIXME: Don't hardcode refresh_interval ...
+  gint offset = (priv->frame_drawn_time + interval * ((current_time - priv->frame_drawn_time) / interval + 1) - current_time) / 1000;

Quantization here doesn't make sense - just do:

 gint offset = MAX (0, current_time - last_time + minimum_interval);

@@ +950,3 @@
+  gint offset = (priv->frame_drawn_time + interval * ((current_time - priv->frame_drawn_time) / interval + 1) - current_time) / 1000;
+
+  priv->frame_timer = g_timeout_add_full (META_PRIORITY_REDRAW, offset, fake_post_paint, self, NULL);

Add a comment here about the exact semantics, since it's non-obvious

 /* The clutter master clock source has already been added with META_PRIORITY_REDRAW,
  * so the timer will run *after* the clutter frame handling, if a frame is ready
  * to be drawn when the timer expires.
  */

@@ +975,3 @@
+  if (priv->window->extended_sync_request_counter &&
+     !priv->repaint_scheduled && priv->frame_timer == 0)
+    queue_fake_post_paint (self);

You don't need the fake post paint here. You can just *or* the repaint scheduled into priv->repaint_scheduled. Then any fake post paint goes in meta_window_actor_queue_frame_drawn() just replacing the current if (!priv->repaint_scheduled).

And see what I said in comment 11 - "I think we probably want to distinguish two cases..."

@@ +2030,3 @@
     return;
 
+  priv->repaint_scheduled = meta_shaped_texture_update_area (META_SHAPED_TEXTURE (priv->actor),

Again *or* into priv->repaint_scheduled and do the real work in meta_window_actor_queue_frame_drawn().

@@ +2443,2 @@
+  if (priv->frame_timer != 0)
+    return;

This needs a comment. something like:

 /* This window had damage, but wasn't actually redrawn because
  * it is obscured. So we should wait until timer expiration
  * before sending _NET_WM_FRAME_* messages.
  */

Comment on attachment 249685 [details] [review]
window-actor: Fix doc comment

Attachment 249685 [details] pushed as 4862872 - window-actor: Fix doc comment

(In reply to comment #22)
> Review of attachment 249696 [details] [review]:
> 
> In terms of testing, the basic thing would be to use GDK_DEBUG=frames and run
> gtk+/tests/animated-resizing (with and without -n) and check behavior with
> different levels of exposure.

OK

> ::: src/compositor/meta-window-actor.c
> @@ +82,3 @@
>    cairo_region_t   *visible_region;
> 
> +  gint              frame_timer;
> 
> frame_timer is too generic - send_frame_messages_timer

OK

> @@ +656,3 @@
>    MetaShadow *shadow = appears_focused ? priv->focused_shadow :
> priv->unfocused_shadow;
> 
> +  if (priv->frame_timer != 0)
> 
> This needs a comment - something like:
> 
>  /* This window got damage when obscured; we set up a timer
>   * to send frame completion events, but since we're drawing
>   * the window now (for some other reason) cancel the timer
>   * and send the completion events normally */

OK

> @@ +924,3 @@
> 
> +static
> +gboolean fake_post_paint (gpointer data)
> 
> I don't like the name "fake_post_paint" - send_frame_messages_timeout() or
> something.

Yeah the name is kind of crappy.

> @@ +929,3 @@
> +  MetaWindowActorPrivate *priv = self->priv;
> +
> +  prepare_frame (self);
> 
> I'm not OK with linking in a frame into priv->frames so that
> do_send_frame_drawn() finds it, and then unlinking it immediately - this is
> really ugly. Consider passing the frame into do_send_frame_drawn().

OK

> @@ +932,3 @@
> +
> +  do_send_frame_drawn (self);
> +  do_send_frame_timings (self, priv->frames->data, 0, 0);
> 
> Shouldn't you be setting needs_frame_drawn to false here?

Yeah.


> @@ +948,3 @@
> +  gint64 current_time = meta_compositor_monotonic_time_to_server_time
> (display, g_get_monotonic_time ());
> +  gint interval = 16667 * 6; // FIXME: Don't hardcode refresh_interval ...
> +  gint offset = (priv->frame_drawn_time + interval * ((current_time -
> priv->frame_drawn_time) / interval + 1) - current_time) / 1000;
> 
> Quantization here doesn't make sense - just do:
> 
>  gint offset = MAX (0, current_time - last_time + minimum_interval);
> 
> @@ +950,3 @@
> +  gint offset = (priv->frame_drawn_time + interval * ((current_time -
> priv->frame_drawn_time) / interval + 1) - current_time) / 1000;
> +
> +  priv->frame_timer = g_timeout_add_full (META_PRIORITY_REDRAW, offset,
> fake_post_paint, self, NULL);
> 
> Add a comment here about the exact semantics, since it's non-obvious
> 
>  /* The clutter master clock source has already been added with
> META_PRIORITY_REDRAW,
>   * so the timer will run *after* the clutter frame handling, if a frame is
> ready
>   * to be drawn when the timer expires.
>   */

OK

> @@ +975,3 @@
> +  if (priv->window->extended_sync_request_counter &&
> +     !priv->repaint_scheduled && priv->frame_timer == 0)
> +    queue_fake_post_paint (self);
> 
> You don't need the fake post paint here. You can just *or* the repaint
> scheduled into priv->repaint_scheduled. Then any fake post paint goes in
> meta_window_actor_queue_frame_drawn() just replacing the current if
> (!priv->repaint_scheduled).
> 
> And see what I said in comment 11 - "I think we probably want to distinguish
> two cases..."

Yeah ebassi wasn't really happy when I asked for the "flip with no redraw" API.
Will just do the 1x1 thing for now.

> @@ +2030,3 @@
>      return;
> 
> +  priv->repaint_scheduled = meta_shaped_texture_update_area
> (META_SHAPED_TEXTURE (priv->actor),
> 
> Again *or* into priv->repaint_scheduled and do the real work in
> meta_window_actor_queue_frame_drawn().

OK.

> @@ +2443,2 @@
> +  if (priv->frame_timer != 0)
> +    return;
> 
> This needs a comment. something like:
> 
>  /* This window had damage, but wasn't actually redrawn because
>   * it is obscured. So we should wait until timer expiration
>   * before sending _NET_WM_FRAME_* messages.
>   */

OK.

Created attachment 252054 [details] [review]
meta-shaped-texture: Don't queue redraws for obscured regions

When we get a damage event we update the window by calling
meta_shaped_texture_update_area which queues a redraw on the actor.
We can avoid that for obscured regions by comparing the damage area to
our visible area.

This patch causes _NET_WM_FRAME_DRAWN messages to be not sent in some cases
where they should be sent; they will be added back in a later commit.

Created attachment 252055 [details] [review]
meta-window-actor: Throttle obscured frame synced apps

We must send frame_drawn and frame_timing messages to even when
we don't actually queue a redraw on screen to comply with the
WM sync spec.

So throttle such apps to down to a ~100ms interval.

Created attachment 252058 [details] [review]
meta-shaped-texture: Don't queue redraws for obscured regions

When we get a damage event we update the window by calling
meta_shaped_texture_update_area which queues a redraw on the actor.
We can avoid that for obscured regions by comparing the damage area to
our visible area.

This patch causes _NET_WM_FRAME_DRAWN messages to be not sent in some cases
where they should be sent; they will be added back in a later commit.

Created attachment 252059 [details] [review]
meta-window-actor: Throttle obscured frame synced apps

We must send frame_drawn and frame_timing messages to even when
we don't actually queue a redraw on screen to comply with the
WM sync spec.

So throttle such apps to down to a ~100ms interval.

*** Bug 706156 has been marked as a duplicate of this bug. ***

Created attachment 252215 [details] [review]
meta-shaped-texture: Don't queue redraws for obscured regions

When we get a damage event we update the window by calling
meta_shaped_texture_update_area which queues a redraw on the actor.
We can avoid that for obscured regions by comparing the damage area to
our visible area.

This patch causes _NET_WM_FRAME_DRAWN messages to be not sent in some cases
where they should be sent; they will be added back in a later commit.

---

Style fixes.

Created attachment 252216 [details] [review]
meta-window-actor: Throttle obscured frame synced apps

We must send frame_drawn and frame_timing messages to even when
we don't actually queue a redraw on screen to comply with the
WM sync spec.

So throttle such apps to down to a ~100ms interval.

--

Use the "real" refresh_rate.

Created attachment 252217 [details] [review]
meta-shaped-texture: Don't queue redraws for obscured regions

When we get a damage event we update the window by calling
meta_shaped_texture_update_area which queues a redraw on the actor.
We can avoid that for obscured regions by comparing the damage area to
our visible area.

This patch causes _NET_WM_FRAME_DRAWN messages to be not sent in some cases
where they should be sent; they will be added back in a later commit.

Created attachment 252218 [details] [review]
meta-window-actor: Throttle obscured frame synced apps

We must send frame_drawn and frame_timing messages to even when
we don't actually queue a redraw on screen to comply with the
WM sync spec.

So throttle such apps to down to a ~100ms interval.

---

Reatched the correct patches in the correct order.

Created attachment 253157 [details] [review]
meta-window-actor: Throttle obscured frame synced apps

We must send frame_drawn and frame_timing messages to even when
we don't actually queue a redraw on screen to comply with the
WM sync spec.

So throttle such apps to down to a ~100ms interval.

---

Turned out that doing
priv->repaint_scheduled || meta_shaped_texture_update_area(...) 
is not a good idea because when repaint_scheduled is true update_area 
will never get called.

So do this a bit more explicit.

Review of attachment 252217 [details] [review]:

::: src/compositor/meta-background-actor-private.h
@@ +12,1 @@
 cairo_region_t *meta_background_actor_get_visible_region (MetaBackgroundActor *self);

so if you're going to rename the setter to set_clip_region, the getter should probably be renamed to get_clip_region.

::: src/compositor/meta-window-actor-private.h
@@ +63,1 @@
                                                    cairo_region_t  *beneath_region);

spacing issue

::: src/compositor/meta-window-group.c
@@ +103,3 @@
+  ClutterActor *stage = clutter_actor_get_stage (actor);
+
+  /* Unset the visible regions */

Why isn't this done at the bottom along side where the clip region gets reset?

(In reply to comment #35)
> Review of attachment 252217 [details] [review]:

> ::: src/compositor/meta-window-group.c
> @@ +103,3 @@
> +  ClutterActor *stage = clutter_actor_get_stage (actor);
> +
> +  /* Unset the visible regions */
> 
> Why isn't this done at the bottom along side where the clip region gets reset?

Because the two are different. The clip region is only valid / used during painting. While the visible region is needed for the damage tracking. So we reset it before setting the new values at the beginning of the function and leave it set intentionally afterwards.

Review of attachment 253157 [details] [review]:

::: src/compositor/meta-window-actor.c
@@ +83,3 @@
   cairo_region_t   *visible_region;
 
+  gint              send_frame_messages_timer;

should be guint

@@ +943,3 @@
+  frame_data_free (frame);
+
+  return FALSE;

might make sense to use G_SOURCE_REMOVE here for clarity, although, most idles/timeouts in mutter don't so maybe not worth it.

@@ +1062,3 @@
+      /* Find out whether the window is completly obscured */
+      if (priv->visible_region)
+      {

wrong spacing

@@ +1079,3 @@
        */
+      if (is_obscured)
+        queue_send_frame_messages_timeout (self);

needs braces since the else has braces

Review of attachment 252217 [details] [review]:

::: src/compositor/meta-window-actor.c
@@ +1683,3 @@
+      cairo_region_destroy (priv->visible_region);
+      priv->visible_region = NULL;
+    }

no need to nullify this, since you set it below. alternatively, if you are going to nullify it, could use g_clear_pointer and turn 5 lines into 1.

@@ +1688,3 @@
+    priv->visible_region = cairo_region_copy (visible_region);
+  else
+    priv->visible_region = NULL;

if you nullify it above, you can drop the else.

Created attachment 253167 [details] [review]
meta-shaped-texture: Don't queue redraws for obscured regions

When we get a damage event we update the window by calling
meta_shaped_texture_update_area which queues a redraw on the actor.
We can avoid that for obscured regions by comparing the damage area to
our visible area.

This patch causes _NET_WM_FRAME_DRAWN messages to be not sent in some cases
where they should be sent; they will be added back in a later commit.

--

Properly rename visible_region to clip_region for the background case 
and fix style issues.

Created attachment 253168 [details] [review]
meta-window-actor: Throttle obscured frame synced apps

We must send frame_drawn and frame_timing messages to even when
we don't actually queue a redraw on screen to comply with the
WM sync spec.

So throttle such apps to down to a ~100ms interval.

--

Style fixes.

Review of attachment 253167 [details] [review]:

a few more comments, but nothing critical. feel free to ignore anything i'm saying here and push.

::: src/compositor/meta-background-actor.c
@@ +44,1 @@
 

since all the changes in this file and meta-background-group are strictly to rename, it may make sense to do that stuff as a separate commit (to make it clear to the reader that there's no semantic changes slipping in)

::: src/compositor/meta-shaped-texture.c
@@ +427,3 @@
+ * @visible_region: The unobscured region of the window or %NULL if
+ * there is no valid one (like when the actor is transformed or
+ * has a mapped clone)

maybe instead of "if there is no valid one", "if @stex should be treated as completely unobscured".

I actually like the word, "unobscured" you picked there.  Maybe all the instances of "visible_region" should be changed to unobscured_region.  It's a little more specific and clearer.

::: src/compositor/meta-window-actor-private.h
@@ +59,3 @@
 
+void meta_window_actor_set_clip_region         (MetaWindowActor *self,
+                                                   cairo_region_t  *clip_region);

spacing issue.

::: src/compositor/meta-window-actor.c
@@ +1669,3 @@
  *
  * Provides a hint as to what areas of the window need to be
  * drawn. Regions not in @visible_region are completely obscured.

maybe instead of "need to be drawn" maybe "need to queue redraws when damaged".

@@ +1697,3 @@
+ * drawn. Regions not in @clip_region are completely obscured or
+ * not drawn in this frame.
+ * This will be set before painting then unset afterwards.

s/will be set/should be set/

::: src/compositor/meta-window-group.c
@@ +103,3 @@
+  ClutterActor *stage = clutter_actor_get_stage (actor);
+
+  /* Unset the visible regions */

Start off by treating all windows as completely unobscured, so damage anywhere in a window queues redraws, but confine it more below.

Review of attachment 253168 [details] [review]:

seems to work okay in basic testing.

::: src/compositor/meta-window-actor.c
@@ +83,3 @@
   cairo_region_t   *visible_region;
 
+  guint              send_frame_messages_timer;

spacing issue.

@@ +937,3 @@
+
+  do_send_frame_drawn (self, frame);
+  do_send_frame_timings (self, frame, 0, 0);

what implications does setting 0 for the refresh interval have here?

(In reply to comment #42)
> Review of attachment 253168 [details] [review]:
> 
> seems to work okay in basic testing.
> 
> ::: src/compositor/meta-window-actor.c
> @@ +83,3 @@
>    cairo_region_t   *visible_region;
> 
> +  guint              send_frame_messages_timer;
> 
> spacing issue.
> 
> @@ +937,3 @@
> +
> +  do_send_frame_drawn (self, frame);
> +  do_send_frame_timings (self, frame, 0, 0);
> 
> what implications does setting 0 for the refresh interval have here?

See comment 10 (or if you don't want to read all of it): 

"* For timer-driven DRAWN/TIMINGS messages, send 0 for
presentation_time_offset. Also send 0 for refresh_interval. Send the normal
value for frame_delay"

(In reply to comment #41)
> Review of attachment 253167 [details] [review]:
> 
> a few more comments, but nothing critical. feel free to ignore anything i'm
> saying here and push.
> 
> ::: src/compositor/meta-background-actor.c
> @@ +44,1 @@
> 
> 
> since all the changes in this file and meta-background-group are strictly to
> rename, it may make sense to do that stuff as a separate commit (to make it
> clear to the reader that there's no semantic changes slipping in)

OK, makes sense.

> ::: src/compositor/meta-shaped-texture.c
> @@ +427,3 @@
> + * @visible_region: The unobscured region of the window or %NULL if
> + * there is no valid one (like when the actor is transformed or
> + * has a mapped clone)
> 
> maybe instead of "if there is no valid one", "if @stex should be treated as
> completely unobscured".

OK.

> I actually like the word, "unobscured" you picked there.  Maybe all the
> instances of "visible_region" should be changed to unobscured_region.  It's a
> little more specific and clearer.

OK.

> ::: src/compositor/meta-window-actor-private.h
> @@ +59,3 @@
> 
> +void meta_window_actor_set_clip_region         (MetaWindowActor *self,
> +                                                   cairo_region_t 
> *clip_region);
> 
> spacing issue.
> 
> ::: src/compositor/meta-window-actor.c
> @@ +1669,3 @@
>   *
>   * Provides a hint as to what areas of the window need to be
>   * drawn. Regions not in @visible_region are completely obscured.
> 
> maybe instead of "need to be drawn" maybe "need to queue redraws when damaged".

OK.

> @@ +1697,3 @@
> + * drawn. Regions not in @clip_region are completely obscured or
> + * not drawn in this frame.
> + * This will be set before painting then unset afterwards.
> 
> s/will be set/should be set/

OK.

> ::: src/compositor/meta-window-group.c
> @@ +103,3 @@
> +  ClutterActor *stage = clutter_actor_get_stage (actor);
> +
> +  /* Unset the visible regions */
> 
> Start off by treating all windows as completely unobscured, so damage anywhere
> in a window queues redraws, but confine it more below.

OK.

Pushed with suggested changes.

Attachment 253167 [details] pushed as 691c107 - meta-shaped-texture: Don't queue redraws for obscured regions
Attachment 253168 [details] pushed as d0210c1 - meta-window-actor: Throttle obscured frame synced apps