compiz team mailing list archive

[Daniel van Vugt <vanvugt@xxxxxxxxx>]

Daniel van Vugt has proposed merging lp:~vanvugt/compiz-core/fix-763005 into lp:compiz-core.

Requested reviews:
  Sam Spilsbury (smspillaz)
  compiz packagers (compiz)

For more details, see:
https://code.launchpad.net/~vanvugt/compiz-core/fix-763005/+merge/73625

Fix slow/stuttering display when Sync To VBlank is enabled. (LP: #763005)

For fglrx the cause appears to be an ATI/AMD driver bug where glXGetVideoSyncSGI and glXWaitVideoSyncSGI are particularly slow, so we can't afford to call both on every redraw or we end up with 1/3-1/2 framerate. For intel and really all drivers, another cause appears to be excessive waiting in the compiz architecture not allowing enough CPU time to draw the full frame before the next vertical refresh.

This proposal addresses both possible problems, providing optimal framerates with sync-to-vblank support, most of the time.
-- 
https://code.launchpad.net/~vanvugt/compiz-core/fix-763005/+merge/73625
Your team compiz packagers is requested to review the proposed merge of lp:~vanvugt/compiz-core/fix-763005 into lp:compiz-core.

=== modified file 'plugins/opengl/include/opengl/opengl.h'
--- plugins/opengl/include/opengl/opengl.h	2011-02-24 07:52:09 +0000
+++ plugins/opengl/include/opengl/opengl.h	2011-09-01 03:50:21 +0000
@@ -98,11 +98,13 @@
 					  int	  width,
 					  int	  height);
 
-    typedef int (*GLXGetVideoSyncProc)  (unsigned int *count);
     typedef int (*GLXWaitVideoSyncProc) (int	  divisor,
 					 int	  remainder,
 					 unsigned int *count);
 
+    // http://www.opengl.org/registry/specs/SGI/swap_control.txt
+    typedef int (*GLXSwapIntervalProc) (int interval);
+
     #ifndef GLX_VERSION_1_3
     typedef struct __GLXFBConfigRec *GLXFBConfig;
     #endif
@@ -163,8 +165,8 @@
     extern GLXReleaseTexImageProc   releaseTexImage;
     extern GLXQueryDrawableProc     queryDrawable;
     extern GLXCopySubBufferProc     copySubBuffer;
-    extern GLXGetVideoSyncProc      getVideoSync;
     extern GLXWaitVideoSyncProc     waitVideoSync;
+    extern GLXSwapIntervalProc      swapInterval;
     extern GLXGetFBConfigsProc      getFBConfigs;
     extern GLXGetFBConfigAttribProc getFBConfigAttrib;
     extern GLXCreatePixmapProc      createPixmap;

=== modified file 'plugins/opengl/src/privates.h'
--- plugins/opengl/src/privates.h	2009-03-16 09:18:16 +0000
+++ plugins/opengl/src/privates.h	2011-09-01 03:50:21 +0000
@@ -70,6 +70,7 @@
 
 	void prepareDrawing ();
 
+	void controlSwapVideoSync ();
 	void waitForVideoSync ();
 
 	void paintBackground (const CompRegion &region,

=== modified file 'plugins/opengl/src/screen.cpp'
--- plugins/opengl/src/screen.cpp	2011-02-24 07:52:09 +0000
+++ plugins/opengl/src/screen.cpp	2011-09-01 03:50:21 +0000
@@ -35,8 +35,8 @@
     GLXReleaseTexImageProc   releaseTexImage = NULL;
     GLXQueryDrawableProc     queryDrawable = NULL;
     GLXCopySubBufferProc     copySubBuffer = NULL;
-    GLXGetVideoSyncProc      getVideoSync = NULL;
     GLXWaitVideoSyncProc     waitVideoSync = NULL;
+    GLXSwapIntervalProc      swapInterval = NULL;
     GLXGetFBConfigsProc      getFBConfigs = NULL;
     GLXGetFBConfigAttribProc getFBConfigAttrib = NULL;
     GLXCreatePixmapProc      createPixmap = NULL;
@@ -217,13 +217,16 @@
 
     if (strstr (glxExtensions, "GLX_SGI_video_sync"))
     {
-	GL::getVideoSync = (GL::GLXGetVideoSyncProc)
-	    getProcAddress ("glXGetVideoSyncSGI");
-
 	GL::waitVideoSync = (GL::GLXWaitVideoSyncProc)
 	    getProcAddress ("glXWaitVideoSyncSGI");
     }
 
+    if (strstr (glxExtensions, "GLX_SGI_swap_control"))
+    {
+	GL::swapInterval = (GL::GLXSwapIntervalProc)
+	    getProcAddress ("glXSwapIntervalSGI");
+    }
+
     glXMakeCurrent (dpy, CompositeScreen::get (s)->output (), priv->ctx);
 
     glExtensions = (const char *) glGetString (GL_EXTENSIONS);
@@ -986,19 +989,28 @@
 }
 
 void
+PrivateGLScreen::controlSwapVideoSync ()
+{
+    bool sync = optionGetSyncToVblank ();
+    // Docs: http://www.opengl.org/registry/specs/SGI/swap_control.txt
+    if (GL::swapInterval)
+	(*GL::swapInterval) (sync ? 1 : 0);
+    else if (sync)
+	waitForVideoSync ();
+}
+
+void
 PrivateGLScreen::waitForVideoSync ()
 {
-    unsigned int sync;
-
-    if (!optionGetSyncToVblank ())
-	return;
-
-    if (GL::getVideoSync)
+    if (optionGetSyncToVblank () && GL::waitVideoSync)
     {
-	glFlush ();
+	// Don't wait twice. Just in case.
+	if (GL::swapInterval)
+	    (*GL::swapInterval) (0);
 
-	(*GL::getVideoSync) (&sync);
-	(*GL::waitVideoSync) (2, (sync + 1) % 2, &sync);
+	// Docs: http://www.opengl.org/registry/specs/SGI/video_sync.txt
+	unsigned int frameno;
+	(*GL::waitVideoSync) (1, 0, &frameno);
     }
 }
 
@@ -1071,10 +1083,23 @@
 
     targetOutput = &screen->outputDevs ()[0];
 
-    waitForVideoSync ();
+    glFlush ();
 
+    //
+    // FIXME: Actually fix the composite plugin to be more efficient;
+    // If GL::swapInterval == NULL && GL::waitVideoSync != NULL then the
+    // composite plugin should not be imposing any framerate restriction
+    // (ie. blocking the CPU) at all. Because the framerate will be controlled
+    // and optimized here:
+    //
     if (mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK)
     {
+	//
+	// controlSwapVideoSync is much faster than waitForVideoSync because
+	// it won't block the CPU. The waiting is offloaded to the GPU.
+	// Unfortunately it only works with glXSwapBuffers in most drivers.
+	//
+	controlSwapVideoSync ();
 	glXSwapBuffers (screen->dpy (), cScreen->output ());
     }
     else
@@ -1082,6 +1107,7 @@
 	BoxPtr pBox;
 	int    nBox, y;
 
+	waitForVideoSync ();
 	pBox = const_cast <Region> (tmpRegion.handle ())->rects;
 	nBox = const_cast <Region> (tmpRegion.handle ())->numRects;
 
@@ -1137,7 +1163,7 @@
 bool
 PrivateGLScreen::hasVSync ()
 {
-   return (GL::getVideoSync && optionGetSyncToVblank ());
+   return (GL::waitVideoSync && optionGetSyncToVblank ());
 }
 
 void
@@ -1145,7 +1171,8 @@
 {
     if (pendingCommands)
     {
-	glFinish ();
+	// glFlush! glFinish would block the CPU, which is bad.
+	glFlush ();
 	pendingCommands = false;
     }
 }