Created attachment 329966 [details] [review]
window-actor: Take over ignoring damage from surface-actor

Currently the decision of whether damage should be processed or
replaced by a full redraw later is done by the surface actor.
However as it turns out, the surface does not have enough information
to make this decision correctly, and we end up doing full redraws
when we should not. To prepare for a fix, move the the code for
ignoring damage up to the window actor where we will be able to
address the issue.

Created attachment 329967 [details] [review]
window-actor: Only ignore damage while an effect is in progress

We don't process damage while a window is frozen in the hope that this
will stop the corresponding texture_from_pixmap from updating while
an effect is in progress, but mark the surface for a full redrawn once
the window is thawed. However when extended frame synchronisation is
used (for now: GTK3), windows are also frozen while the application
is busy drawing the frame - not processing damage in this case means
that we end up painting the entire window each frame. Fix this by
only ignoring damage when the window is frozen due to an ongoing
effect and processing it normally otherwise.

Why would the effect running not cause damage to be queued?

(In reply to Jonas Ådahl from comment #3)
> Why would the effect running not cause damage to be queued?

Full damage that is, corresponding to what the effect actually does.

(In reply to Jonas Ådahl from comment #4)
> (In reply to Jonas Ådahl from comment #3)
> > Why would the effect running not cause damage to be queued?
> 
> Full damage that is, corresponding to what the effect actually does.

I'm not sure I understand the question - this is about updating the surface texture in response to damage queued by the client. I don't think it is safe to assume that the potential damage queued by the effect includes the previously reported client damage we ignored, in which case the client wouldn't redraw the affected areas. In any case this issue is about ignoring damage events from the client when we should not, so just removing the full update of the texture would make the issue worse (even if it worked for the effects) - usually clients don't start frozen, but we would ignore all changes that happen after the initial frame.

That said, I think an alternative to queuing full updates only when we intend to would be to replace needs_damage_all by an ignored_damage_region and apply missed damage when the surface is thawed - I'll give that a try when I get home.

(In reply to Florian Müllner from comment #5)
> (In reply to Jonas Ådahl from comment #4)
> > (In reply to Jonas Ådahl from comment #3)
> > > Why would the effect running not cause damage to be queued?
> > 
> > Full damage that is, corresponding to what the effect actually does.
> 
> I'm not sure I understand the question - this is about updating the surface
> texture in response to damage queued by the client. I don't think it is safe
> to assume that the potential damage queued by the effect includes the
> previously reported client damage we ignored, in which case the client
> wouldn't redraw the affected areas. In any case this issue is about ignoring
> damage events from the client when we should not, so just removing the full
> update of the texture would make the issue worse (even if it worked for the
> effects) - usually clients don't start frozen, but we would ignore all
> changes that happen after the initial frame.

It was not so much of a question to what the patch did, but why we did what we were doing to begin with.

> 
> That said, I think an alternative to queuing full updates only when we
> intend to would be to replace needs_damage_all by an ignored_damage_region
> and apply missed damage when the surface is thawed - I'll give that a try
> when I get home.

This sounds like a better approach to me.

Created attachment 329970 [details] [review]
surface-actor: Keep track of ignored damage

We ignore all damage while a surface is frozen and queue a full
update instead once it's thawed. While not super efficient, this
isn't overly bad for the intended case of catching up with any
updates that happened during a compositor effect. However when
extended frame sync is used, surfaces are also frozen while the
client is drawing a frame, in which case the current behavior is
pretty damaging (pun intended), as we end up redrawing the entire
window each frame. To address this, keep track of the actual damage
we ignore and apply it when the surface is thawed.


(In reply to Jonas Ådahl from comment #6)
> (In reply to Florian Müllner from comment #5)
> > That said, I think an alternative to queuing full updates only when we
> > intend to would be to replace needs_damage_all by an ignored_damage_region
> > and apply missed damage when the surface is thawed - I'll give that a try
> > when I get home.
> 
> This sounds like a better approach to me.

Yeah, after giving it a shot, this definitively looks nicer to me.

Review of attachment 329970 [details] [review]:

Just a couple of nits, but otherwise looks good to me.

::: src/compositor/meta-surface-actor.c
@@ +27,2 @@
   /* Freeze/thaw accounting */
+  cairo_region_t *ignored_damage;

nit: "pending_damage" makes more sense to me, since we are not ignoring it.

@@ +272,3 @@
        * texture regardless of damage event handling.
        */
+      cairo_rectangle_int_t rect = { x, y, width, height };

nit: if you do { .x = x, .y = y, .width = width, .height = height } you don't need to implicitly rely on knowing the order of the fields in the rect, which is much nicer IMHO.

Pushed with the suggested changes - thanks for the quick review!

Attachment 329970 [details] pushed as 53a9411 - surface-actor: Keep track of ignored damage