Merge lp:~vanvugt/compiz/XDamageReport2 into lp:compiz/0.9.8

[Daniel van Vugt]

Use the XDamage extension more efficiently (the way it was designed to be
used). This dramatically reduces CPU usage, reduces wakeups, and increases
frame rates. It also solves at least one observed performance bug
(LP: #1007299) and probably several more.

Second attempt...

This time I've been careful to avoid races with the X server that could result in damage artefacts.

The original proposal is here:
https://code.launchpad.net/~vanvugt/compiz/XDamageReport/+merge/108307

[Daniel van Vugt (vanvugt)]

Hmm, I think I can see more potential for races unless some server grabbing is added. Work in progress again.

[Sam Spilsbury (smspillaz)]

BTW, I would suggest using the ServerLock class to do server grabs safely.

ServerLock serverLock (screen->serverGrabInterface ());

It's testable and ensures that you don't get stale grabs caused by missed output buffer flushes.

[Daniel van Vugt (vanvugt)]

No I think my hunch was wrong and the code is right. There is no need for a server grab because each XDamageSubtract is completely independent (different drawables). And in fact XDamageSubtract is synchronous, so the XSync() probably is not required.

[Daniel van Vugt (vanvugt)]

Though, we could probably make the code much safer by using de->geometry (the whole drawable) instead of de->area (the damaged region) and XDamageReportNonEmpty instead of XDamageReportBoundingBox.

Then we'd be guaranteed a maximum of one damage per drawable per frame, and the damage would always be the whole drawable. It would be safer in theory, even if potentially less efficient.

[Sam Spilsbury (smspillaz)]

> Though, we could probably make the code much safer by using de->geometry (the
> whole drawable) instead of de->area (the damaged region) and
> XDamageReportNonEmpty instead of XDamageReportBoundingBox.
>
> Then we'd be guaranteed a maximum of one damage per drawable per frame, and
> the damage would always be the whole drawable. It would be safer in theory,
> even if potentially less efficient.

I think the problem then is that for maximized or fullscreen windows you don't get the optimization of being able to clip painting. Its worth looking into how important this optimization is IMO.

[Daniel van Vugt (vanvugt)]

Ignore the above hypotheses for now. Please review and test the actual code below :)

[Sam Spilsbury (smspillaz)]

Works great, nouveau. Code fine.

I feel as though this code may be easy to break because of the coupling between this:

43 + else if (event->type == damageEvent + XDamageNotify)
44 + {
45 + XDamageNotifyEvent *de = (XDamageNotifyEvent*)event;
46 + damages[de->damage] = de->area;
47 + }

And this:

55 + Display *dpy = screen->dpy ();
56 + std::map<Damage, XRectangle>::iterator d = priv->damages.begin ();
57 + for (; d != priv->damages.end (); d++)
58 + {
59 + XserverRegion sub = XFixesCreateRegion (dpy, &d->second, 1);
60 + if (sub != None)
61 + {
62 + XDamageSubtract (dpy, d->first, sub, None);
63 + XFixesDestroyRegion (dpy, sub);
64 + }
65 + }
66 + XSync (dpy, False);
67 + priv->damages.clear ();

Since if the latter is not done for the former, then things will stop updating. As such, I would strongly recommend adding some unit tests to ensure that this code doesn't break accidentally. So abstaining for now, someone else can approve if they think tests aren't appropriate.

review: Abstain

[Daniel van Vugt (vanvugt)]

The risk, and what needs testing here, is for races between XFunctions and the X server. Testing for such races should be done as part of a system test suite that runs a real compiz instance on a real X server. Unit tests in this case are not relevant.

The next best thing to an automated system test suite, until one exists, is manual testing.

[Sam Spilsbury (smspillaz)]

The reason i say unit tests is because the code itself is tightly coupled and looks easy to subtly break if someone doesnt know what they are doing.

Sent from Samsung Mobile

 Daniel van Vugt <email address hidden> wrote:

The risk, and what needs testing here, is for races between XFunctions and the X server. Testing for such races should be done as part of a system test suite that runs a real compiz instance on a real X server. Unit tests in this case are not relevant.

The next best thing to an automated system test suite, until one exists, is manual testing.
--
https://code.launchpad.net/~vanvugt/compiz/XDamageReport2/+merge/111739
You are reviewing the proposed merge of lp:~vanvugt/compiz/XDamageReport2 into lp:compiz.

[Daniel van Vugt (vanvugt)]

In that case, the maintenance risk is actually between:
   XDamageCreate(..., XDamageReportBoundingBox);
and
   XDamageSubtract().

So to mitigate that risk, we need to ensure:
   1. All plugins that use XDamageCreate with BoundingBox need to depend on composite. Already done.
   2. Maybe add a comment to composite saying "Don't delete this code!". But the same could be said for lots of critical code.

If you happen to delete the XDamageSubtract code by accident, then I can tell you from experience that you'll know about it because the screen won't update at all. :)

[Sam Spilsbury (smspillaz)]

> In that case, the maintenance risk is actually between:
> XDamageCreate(..., XDamageReportBoundingBox);
> and
> XDamageSubtract().
>
> So to mitigate that risk, we need to ensure:
> 1. All plugins that use XDamageCreate with BoundingBox need to depend on
> composite. Already done.
> 2. Maybe add a comment to composite saying "Don't delete this code!". But
> the same could be said for lots of critical code.

Either that or add an abstraction such that the implementation detail is not exposed by default, and the tests expect that XDamageSubtract is called for all damage added to the map. A failing test is possibly a lot more effective in providing feedback than a comment that just says "don't do dis!!" on code a future maintainer might not understand.

>
> If you happen to delete the XDamageSubtract code by accident, then I can tell
> you from experience that you'll know about it because the screen won't update
> at all. :)

[Sam Spilsbury (smspillaz)]

In any case, I'd prefer to have this in than out, even without tests.

review: Approve

=== modified file 'plugins/annotate/src/annotate.cpp'
--- plugins/annotate/src/annotate.cpp	2012-01-30 05:12:24 +0000
+++ plugins/annotate/src/annotate.cpp	2012-06-24 10:15:24 +0000
@@ -73,7 +73,7 @@
 	}
 
 	damage = XDamageCreate (screen->dpy (), pixmap,
-				XDamageReportRawRectangles);
+				XDamageReportBoundingBox);
 
 	surface =
 	    cairo_xlib_surface_create_with_xrender_format (screen->dpy (),

=== modified file 'plugins/composite/src/privates.h'
--- plugins/composite/src/privates.h	2012-06-06 05:12:28 +0000
+++ plugins/composite/src/privates.h	2012-06-24 10:15:24 +0000
@@ -33,6 +33,7 @@
 
 #include <composite/composite.h>
 #include <core/atoms.h>
+#include <map>
 
 #include "pixmapbinding.h"
 #include "composite_options.h"
@@ -110,6 +111,9 @@
 
 	Atom cmSnAtom;
 	Window newCmSnOwner;
+
+	/* Map Damage handle to its bounding box */
+	std::map<Damage, XRectangle> damages;
 };
 
 class PrivateCompositeWindow :

=== modified file 'plugins/composite/src/screen.cpp'
--- plugins/composite/src/screen.cpp	2012-06-06 07:31:59 +0000
+++ plugins/composite/src/screen.cpp	2012-06-24 10:15:24 +0000
@@ -100,6 +100,11 @@
 		    }
 		}
 	    }
+	    else if (event->type == damageEvent + XDamageNotify)
+	    {
+		XDamageNotifyEvent *de = (XDamageNotifyEvent*)event;
+		damages[de->damage] = de->area;
+	    }
 	    break;
     }
 
@@ -793,6 +798,20 @@
 		damageScreen ();
 	}
 
+	Display *dpy = screen->dpy ();
+	std::map<Damage, XRectangle>::iterator d = priv->damages.begin ();
+	for (; d != priv->damages.end (); d++)
+	{
+	    XserverRegion sub = XFixesCreateRegion (dpy, &d->second, 1);
+	    if (sub != None)
+	    {
+		XDamageSubtract (dpy, d->first, sub, None);
+		XFixesDestroyRegion (dpy, sub);
+	    }
+	}
+	XSync (dpy, False);
+	priv->damages.clear ();
+
 	priv->damage = CompRegion ();
 
 	int mask = priv->damageMask;

=== modified file 'plugins/composite/src/window.cpp'
--- plugins/composite/src/window.cpp	2012-06-06 05:12:28 +0000
+++ plugins/composite/src/window.cpp	2012-06-24 10:15:24 +0000
@@ -36,7 +36,7 @@
     if (w->windowClass () != InputOnly)
     {
 	priv->damage = XDamageCreate (s->dpy (), w->id (),
-				      XDamageReportRawRectangles);
+				      XDamageReportBoundingBox);
     }
     else
     {

=== modified file 'plugins/copytex/src/copytex.cpp'
--- plugins/copytex/src/copytex.cpp	2012-05-27 04:32:55 +0000
+++ plugins/copytex/src/copytex.cpp	2012-06-24 10:15:24 +0000
@@ -80,7 +80,7 @@
 					   MIN (h, maxTS)));
 
 
-    cp->damage = XDamageCreate (screen->dpy (), cp->pixmap, XDamageReportRawRectangles);
+    cp->damage = XDamageCreate (screen->dpy (), cp->pixmap, XDamageReportBoundingBox);
     CopytexScreen::get (screen)->pixmaps[cp->damage] = cp;
 
     return cp;

=== modified file 'plugins/decor/src/decor.cpp'
--- plugins/decor/src/decor.cpp	2012-06-19 06:03:04 +0000
+++ plugins/decor/src/decor.cpp	2012-06-24 10:15:24 +0000
@@ -347,7 +347,7 @@
 	textures[0]->setMipmap (false);
 
     damage = XDamageCreate (screen->dpy (), pixmap->getPixmap (),
-			     XDamageReportRawRectangles);
+			     XDamageReportBoundingBox);
 }
 
 /*
@@ -1986,7 +1986,7 @@
 	oldHeight = 0;
 
 	frameDamage = XDamageCreate (screen->dpy (), outputFrame,
-			             XDamageReportRawRectangles);
+			             XDamageReportBoundingBox);
 
 	dScreen->frames[outputFrame] = this;
     }

=== modified file 'plugins/opengl/src/texture.cpp'
--- plugins/opengl/src/texture.cpp	2012-06-05 09:31:34 +0000
+++ plugins/opengl/src/texture.cpp	2012-06-24 10:15:24 +0000
@@ -581,7 +581,7 @@
     glBindTexture (texTarget, 0);
 
     tex->damage = XDamageCreate (screen->dpy (), pixmap,
-			         XDamageReportRawRectangles);
+			         XDamageReportBoundingBox);
     boundPixmapTex[tex->damage] = tex;
 
     return rv;