Merge lp:~compiz-team/compiz/compiz.gles2.always-swapbuffers.1 into lp:~compiz-linaro-team/compiz/gles2

[Sam Spilsbury]

Always use *SwapBuffers instead of using glCopyPixels. Introduces a dependency on either having EGL_NV_post_sub_buffer OR GLX_MESA_copy_sub_buffer OR OpenGL 3.0 OR GL_EXT_framebuffer_object.

More of an RFC really.

Related to bug 1013514

[Daniel van Vugt (vanvugt)]

It seems to work very nicely on intel. I have a couple of concerns though:

1. The glCopyPixels fallback works and is reliable. I would like to keep it unless there is a good reason not to. That way we don't raise the minimum system requirements unnecessarily.

2. We need a configuration option for people to be able to choose the old rendering method. There is a significant performance hit in (unthrottled) benchmark results with this change. So people need to have the option of going back to regional updates.

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

> It seems to work very nicely on intel. I have a couple of concerns though:
>
> 1. The glCopyPixels fallback works and is reliable. I would like to keep it
> unless there is a good reason not to. That way we don't raise the minimum
> system requirements unnecessarily.

It is completely broken on NVIDIA (for reasons unknown, and for reasons that I don't think are worth looking into).

The only hardware that was going to be using this fallback path these days was hardware using the binary nvidia driver older than about 2003 (< GF FX 5xxx) and radeon hardware using the binary fglrx driver < Radeon 9xxx (2003 also). Every other driver supports supports either GLX_EXT_framebuffer_object or GL_MESA_copy_sub_buffer. The binary fglrx and nvidia drivers which support that hardware are, to the best of my knowledge, not even supported on modern distributions at all these days.

>
> 2. We need a configuration option for people to be able to choose the old
> rendering method.

I would prefer not to do this.

Back in the Beryl days we had countless configuration options for controlling how rendering worked internally and we had countless bug reports because people used the wrong configuration for their particular hardware. Configuration should only be exposed for user-facing functionality and not for core functionality. I think we can all agree that the existing configuration options which change how rendering work, like providing an option to sync to vblank is already totally insane. Adding an option to say "do you want to handle vsync in software or hardware" also makes no sense to the user.

Assuming that we might be running in the case of glCopyPixels OR glXSwapBuffers presents a significant divergence in codepath internally. It means we need to keep around and maintain a bunch of old vsync code which we know is flaky at best because its not handled internally in the driver. I would prefer to drop support for GL_MESA_copy_sub_buffer, but I at least decided that for now we should support hardware on the open drivers that is more than ten years old.

We will also be heavily depending on shaders and vertex buffer objects in the future, and these codepaths have higher requirements than GLX_EXT_framebuffer_object anyways.

Its time to move on from the past.

> There is a significant performance hit in (unthrottled)
> benchmark results with this change. So people need to have the option of going
> back to regional updates.

I'd like some information on this.

If it is that the performance hit comes from the fact that we're effectively always being vsync'd because we're using glXSwapBuffers / eglSwapBuffers, then I think the best course of action is to leave VSync on by default and use triple buffering so that you don't get the performance hit when you're really under load.

We're a compositor, not a benchmark. I don't think there's a good reason to maintain the old codepaths.

[Achim (ach1m)]

Just FYI the intel driver is using triple buffering by default.

$ cat /var/log/Xorg.0.log | grep "Triple"
[ 15.316] (**) intel(0): Triple buffering? enabled

[Sam Spilsbury (smspillaz)]

Okay, after some discussions on IRC, here is I think what the best course of action should be:

1) Always use glXSwapBuffers if GLX_EXT_framebuffer_object is available
2) If GLX_EXT_framebuffer_object is not available fall back in the following order:
   a) Use glXSwapBuffers if damage is fullscreen
   b) Use GLX_MESA_copy_sub_buffer if available
   c) Use glCopyPixels

What this means is that if you hit 2(c) on NVIDIA, you're going to have a broken desktop. We should probably warn if we hit that case

I would much prefer to do this in future:

With VSync On:

1) Always use glXSwapBuffers if GLX_EXT_framebuffer_object is available
2) If GLX_EXT_framebuffer_object is not available fall back in the following order:
   a) Use glXSwapBuffers if damage is fullscreen
   b) Use GLX_MESA_copy_sub_buffer if available
   c) Use glCopyPixels

With VSync Off:

1) Only render to framebuffer object if plugins needed to do fullscreen postprocessing
2) Use glXSwapBuffers where we are either rendering to an fbo, or doing a fullscreen render
3) Use glCopySubBufferMESA where supported
4) Use glCopyPixels if glCopySubBufferMESA is not supported

The reason being that always using swapbuffers in the no vsync case will effectively slow down unthrottled rendering because we have to do the pipeline stall on binding the fbo. Because the user doesn't care about image perfection with vsync off, we can just use the slightly more glitchy methods.

I will rework the code however to go with the first methods

[Sam Spilsbury (smspillaz)]

As discussed on IRC, we will be dropping the glCopyPixels codepath as it is broken on all drivers not just nvidia

[Sam Spilsbury (smspillaz)]

Hi, resubmitted with the following changes:

1. Got the fallback logic under test, created an abstraction that we could use for both EGL and GLX, which removed some ifdef soup
2. Called SwapBuffers if fullscreen damage or painting to FBO
3. Added startup requirement on GLX_MESA_copy_sub_buffer / EGL_NV_post_sub_buffer OR GLX_EXT_framebuffer_object OR OpenGL ES 2.0
4. Removed EGL_BUFFER_PRESERVED call in eglSurfaceAttrib, we no longer need to do this as the framebuffer object is our main output device (incidentally , this should provide a small speed boost as there is no longer an implicit glReadPixels / glDrawPixels done inside of eglSwapBuffers : see http://www.khronos.org/registry/egl/specs/EGLTechNote0001.html)

[Daniel van Vugt (vanvugt)]

Please use a more appropriate class name that doesn't contain a verb. So change:
   XYZBufferBlit
to
   XYZDoubleBuffer
(where Buffer is a noun in this case, not a verb ;)

Also missing file:

[ 26%] Building CXX object plugins/opengl/src/bufferblit/CMakeFiles/compiz_opengl_buffer_blit.dir/src/buffer-blit.cpp.o
/home/dan/bzr/compiz/tmp.swap/plugins/opengl/src/bufferblit/src/buffer-blit.cpp:4:31: fatal error: opengl/bufferblit.h: No such file or directory
compilation terminated.
make[2]: *** [plugins/opengl/src/bufferblit/CMakeFiles/compiz_opengl_buffer_blit.dir/src/buffer-blit.cpp.o] Error 1
make[1]: *** [plugins/opengl/src/bufferblit/CMakeFiles/compiz_opengl_buffer_blit.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Please use a more appropriate class name that doesn't contain a verb. So change:
   XYZBufferBlit
to
   XYZDoubleBuffer
(where Buffer is a noun in this case, not a verb ;)

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

Rename complete

[lp:~compiz-team/compiz/compiz.gles2.always-swapbuffers.1]

3265. By Sam Spilsbury on 2012-07-06

 lp:~compiz-linaro-team/compiz/gles2

[Daniel van Vugt (vanvugt)]

Works well. Although it feels overcomplicated... I can't put my finger on any exact problems right now.

review: Approve

=== modified file 'plugins/opengl/CMakeLists.txt'
--- plugins/opengl/CMakeLists.txt	2012-05-17 10:41:21 +0000
+++ plugins/opengl/CMakeLists.txt	2012-07-06 10:07:22 +0000
@@ -2,12 +2,17 @@
 
 include (CompizPlugin)
 
+set (INTERNAL_LIBRARIES
+     compiz_opengl_double_buffer)
+
+add_subdirectory (src/doublebuffer)
+
 if (USE_GLES)
-    compiz_plugin(opengl PLUGINDEPS composite CFLAGSADD "-DUSE_GLES -std=c++0x" LIBRARIES ${OPENGLES2_LIBRARIES} INCDIRS ${OPENGLES2_INCLUDE_DIR})
+    compiz_plugin(opengl PLUGINDEPS composite CFLAGSADD "-DUSE_GLES -std=c++0x" LIBRARIES ${OPENGLES2_LIBRARIES} ${INTERNAL_LIBRARIES} INCDIRS ${OPENGLES2_INCLUDE_DIR})
 else (USE_GLES)
     find_package (OpenGL)
     if (OPENGL_FOUND)
-	compiz_plugin(opengl PLUGINDEPS composite CFLAGSADD -std=c++0x LIBRARIES ${OPENGL_gl_LIBRARY} INCDIRS ${OPENGL_INCLUDE_DIR})
+	compiz_plugin(opengl PLUGINDEPS composite CFLAGSADD -std=c++0x LIBRARIES ${OPENGL_gl_LIBRARY} ${INTERNAL_LIBRARIES} INCDIRS ${OPENGL_INCLUDE_DIR})
     endif (OPENGL_FOUND)
 endif (USE_GLES)
 

=== added file 'plugins/opengl/include/opengl/doublebuffer.h'
--- plugins/opengl/include/opengl/doublebuffer.h	1970-01-01 00:00:00 +0000
+++ plugins/opengl/include/opengl/doublebuffer.h	2012-07-06 10:07:22 +0000
@@ -0,0 +1,31 @@
+#ifndef _COMPIZ_OPENGL_BUFFERBLIT_H
+#define _COMPIZ_OPENGL_BUFFERBLIT_H
+
+#include <core/region.h>
+
+namespace compiz
+{
+namespace opengl
+{
+
+const unsigned int  PaintedWithFramebufferObject = (1 << 0);
+const unsigned int  PaintedFullscreen = (1 << 1);
+
+class GLDoubleBufferInterface
+{
+    public:
+
+	virtual ~GLDoubleBufferInterface () {}
+
+	virtual void swapBuffers () const = 0;
+	virtual bool subBufferBlitAvailable () const = 0;
+	virtual void subBufferBlit (const CompRegion &region) const = 0;
+};
+
+void blitBuffers (unsigned int flags,
+		  const CompRegion &blitRegion,
+		  GLDoubleBufferInterface &);
+
+}
+}
+#endif

=== added directory 'plugins/opengl/src/doublebuffer'
=== added file 'plugins/opengl/src/doublebuffer/CMakeLists.txt'
--- plugins/opengl/src/doublebuffer/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ plugins/opengl/src/doublebuffer/CMakeLists.txt	2012-07-06 10:07:22 +0000
@@ -0,0 +1,31 @@
+INCLUDE_DIRECTORIES (  
+  ${compiz_SOURCE_DIR}/src/logmessage/include
+  ${CMAKE_CURRENT_SOURCE_DIR}/../../include
+  ${CMAKE_CURRENT_SOURCE_DIR}/src
+    
+  ${Boost_INCLUDE_DIRS}
+)
+
+LINK_DIRECTORIES (${COMPIZ_LIBRARY_DIRS}) 
+
+SET( 
+  SRCS 
+  ${CMAKE_CURRENT_SOURCE_DIR}/src/double-buffer.cpp
+)
+
+ADD_LIBRARY( 
+  compiz_opengl_double_buffer STATIC
+  
+  ${SRCS}
+)
+
+if (COMPIZ_BUILD_TESTING)
+ADD_SUBDIRECTORY( ${CMAKE_CURRENT_SOURCE_DIR}/tests )
+endif (COMPIZ_BUILD_TESTING)
+
+TARGET_LINK_LIBRARIES(
+  compiz_opengl_double_buffer
+
+  compiz_region
+  compiz_logmessage
+)

=== added directory 'plugins/opengl/src/doublebuffer/src'
=== added file 'plugins/opengl/src/doublebuffer/src/double-buffer.cpp'
--- plugins/opengl/src/doublebuffer/src/double-buffer.cpp	1970-01-01 00:00:00 +0000
+++ plugins/opengl/src/doublebuffer/src/double-buffer.cpp	2012-07-06 10:07:22 +0000
@@ -0,0 +1,34 @@
+#include <iostream>
+
+#include <core/logmessage.h>
+#include <opengl/doublebuffer.h>
+
+#include <cstdlib>
+
+using namespace compiz::opengl;
+
+char programName[] = "compiz_test_opengl_double_buffer";
+bool debugOutput = false;
+
+void
+compiz::opengl::blitBuffers (unsigned int flags,
+			     const CompRegion &tmpRegion,
+			     GLDoubleBufferInterface &blit)
+{
+    if (flags & (PaintedFullscreen |
+		 PaintedWithFramebufferObject))
+    {
+	blit.swapBuffers ();
+    }
+    else if (blit.subBufferBlitAvailable ())
+    {
+	blit.subBufferBlit (tmpRegion);
+    }
+    else
+    {
+	/* FIXME: We need to use compLogMessage here, but for some
+	 * reason it just crashes in the tests */
+	std::cerr << "(compiz) - fatal: no back to front flip methods supported" << std::endl;
+	abort ();
+    }
+}

=== added directory 'plugins/opengl/src/doublebuffer/tests'
=== added file 'plugins/opengl/src/doublebuffer/tests/CMakeLists.txt'
--- plugins/opengl/src/doublebuffer/tests/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ plugins/opengl/src/doublebuffer/tests/CMakeLists.txt	2012-07-06 10:07:22 +0000
@@ -0,0 +1,24 @@
+find_library (GMOCK_LIBRARY gmock)
+find_library (GMOCK_MAIN_LIBRARY gmock_main)
+
+if (NOT GMOCK_LIBRARY OR NOT GMOCK_MAIN_LIBRARY OR NOT GTEST_FOUND)
+  message ("Google Mock and Google Test not found - cannot build tests!")
+  set (COMPIZ_BUILD_TESTING OFF)
+endif (NOT GMOCK_LIBRARY OR NOT GMOCK_MAIN_LIBRARY OR NOT GTEST_FOUND)
+
+include_directories (${GTEST_INCLUDE_DIRS})
+
+link_directories (${COMPIZ_LIBRARY_DIRS})
+
+add_executable (compiz_test_opengl_double_buffer
+                ${CMAKE_CURRENT_SOURCE_DIR}/test-opengl-double-buffer.cpp)
+
+target_link_libraries (compiz_test_opengl_double_buffer
+                       compiz_opengl_double_buffer
+                       ${GTEST_BOTH_LIBRARIES}
+		       ${GMOCK_LIBRARY}
+		       ${GMOCK_MAIN_LIBRARY}
+		       ${CMAKE_THREAD_LIBS_INIT} # Link in pthread.
+                       )
+
+compiz_discover_tests (compiz_test_opengl_double_buffer)

=== added file 'plugins/opengl/src/doublebuffer/tests/test-opengl-double-buffer.cpp'
--- plugins/opengl/src/doublebuffer/tests/test-opengl-double-buffer.cpp	1970-01-01 00:00:00 +0000
+++ plugins/opengl/src/doublebuffer/tests/test-opengl-double-buffer.cpp	2012-07-06 10:07:22 +0000
@@ -0,0 +1,91 @@
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+#include <opengl/doublebuffer.h>
+
+using namespace compiz::opengl;
+using testing::_;
+using testing::StrictMock;
+using testing::Return;
+
+class MockGLDoubleBuffer :
+    public GLDoubleBufferInterface
+{
+    public:
+
+	MOCK_CONST_METHOD0 (swapBuffers, void ());
+	MOCK_CONST_METHOD0 (subBufferBlitAvailable, bool ());
+	MOCK_CONST_METHOD1 (subBufferBlit, void (const CompRegion &));
+};
+
+class CompizOpenGLDoubleBufferTest :
+    public ::testing::Test
+{
+    public:
+
+	MockGLDoubleBuffer mglbb;
+	CompRegion	 blitRegion;
+
+};
+
+class CompizOpenGLDoubleBufferDeathTest :
+    public CompizOpenGLDoubleBufferTest
+{
+};
+
+TEST_F(CompizOpenGLDoubleBufferTest, TestPaintedWithFBOAlwaysSwaps)
+{
+    EXPECT_CALL (mglbb, swapBuffers ());
+
+    blitBuffers (PaintedWithFramebufferObject, blitRegion, mglbb);
+}
+
+TEST_F(CompizOpenGLDoubleBufferTest, TestPaintedFullscreenAlwaysSwaps)
+{
+    EXPECT_CALL (mglbb, swapBuffers ());
+
+    blitBuffers (PaintedFullscreen, blitRegion, mglbb);
+}
+
+TEST_F(CompizOpenGLDoubleBufferTest, TestNoPaintedFullscreenOrFBOAlwaysBlitsSubBuffer)
+{
+    EXPECT_CALL (mglbb, subBufferBlitAvailable ()).WillOnce (Return (true));
+    EXPECT_CALL (mglbb, subBufferBlit (_));
+
+    blitBuffers (0, blitRegion, mglbb);
+}
+
+TEST_F(CompizOpenGLDoubleBufferTest, TestNoPaintedFullscreenOrFBODoesNotBlitIfNotSupported)
+{
+
+}
+
+TEST_F(CompizOpenGLDoubleBufferTest, TestBlitExactlyWithRegionSpecified)
+{
+    CompRegion r1 (0, 0, 100, 100);
+    CompRegion r2 (100, 100, 100, 100);
+    CompRegion r3 (200, 200, 100, 100);
+
+    EXPECT_CALL (mglbb, subBufferBlitAvailable ()).WillRepeatedly (Return (true));
+
+    EXPECT_CALL (mglbb, subBufferBlit (r1));
+    blitBuffers (0, r1, mglbb);
+
+    EXPECT_CALL (mglbb, subBufferBlit (r2));
+    blitBuffers (0, r2, mglbb);
+
+    EXPECT_CALL (mglbb, subBufferBlit (r3));
+    blitBuffers (0, r3, mglbb);
+}
+
+TEST_F(CompizOpenGLDoubleBufferDeathTest, TestNoPaintedFullscreenOrFBODoesNotBlitIfNotSupportedAndDies)
+{
+    StrictMock <MockGLDoubleBuffer> mglbbStrict;
+
+    ON_CALL (mglbbStrict, subBufferBlitAvailable ()).WillByDefault (Return (false));
+
+    ASSERT_DEATH ({
+		    blitBuffers (0, blitRegion, mglbbStrict);
+		  },
+		  ".fatal.");
+}

=== modified file 'plugins/opengl/src/privates.h'
--- plugins/opengl/src/privates.h	2012-05-26 12:52:04 +0000
+++ plugins/opengl/src/privates.h	2012-07-06 10:07:22 +0000
@@ -36,12 +36,77 @@
 #include <opengl/framebufferobject.h>
 #endif
 
+#include <opengl/doublebuffer.h>
+
 #include "privatetexture.h"
 #include "privatevertexbuffer.h"
 #include "opengl_options.h"
 
 extern CompOutput *targetOutput;
 
+class BaseDoubleBuffer
+{
+    public:
+
+	BaseDoubleBuffer (Display *,
+			const CompSize &,
+			const boost::function <bool ()> &);
+
+    protected:
+
+	Display *mDpy;
+	const CompSize &mSize;
+	boost::function <bool ()> getSyncVblank;
+};
+
+#ifndef USE_GLES
+
+class GLXDoubleBuffer :
+    public compiz::opengl::GLDoubleBufferInterface,
+    public BaseDoubleBuffer
+{
+    public:
+
+	GLXDoubleBuffer (Display *,
+		       const CompSize &,
+		       const boost::function <bool ()> &,
+		       Window,
+		       const boost::function <void ()> &);
+
+	void swapBuffers () const;
+	bool subBufferBlitAvailable () const;
+	void subBufferBlit (const CompRegion &region) const;
+
+    protected:
+
+	Window mOutput;
+	boost::function <void ()> waitVSync;
+};
+
+#else
+
+class EGLDoubleBuffer :
+    public compiz::opengl::GLDoubleBufferInterface,
+    public BaseDoubleBuffer
+{
+    public:
+
+	EGLDoubleBuffer (Display *,
+		       const CompSize &,
+		       const boost::function <bool ()> &,
+		       EGLSurface const &);
+
+	void swapBuffers () const;
+	bool subBufferBlitAvailable () const;
+	void subBufferBlit (const CompRegion &region) const;
+
+    private:
+
+	EGLSurface const & mSurface;
+};
+
+#endif
+
 class GLIcon
 {
     public:
@@ -118,10 +183,12 @@
 	#ifdef USE_GLES
 	EGLContext ctx;
 	EGLSurface surface;
+	EGLDoubleBuffer bufferBlit;
 	#else
 	GLXContext ctx;
 
 	GL::GLXGetProcAddressProc getProcAddress;
+	GLXDoubleBuffer bufferBlit;
 	#endif
 
 	GLFramebufferObject *scratchFbo;

=== modified file 'plugins/opengl/src/screen.cpp'
--- plugins/opengl/src/screen.cpp	2012-07-03 18:16:28 +0000
+++ plugins/opengl/src/screen.cpp	2012-07-06 10:07:22 +0000
@@ -341,8 +341,8 @@
 	return false;
     }
 
-    // Currently we rely unconditionally on preserving the buffer contents.
-    eglSurfaceAttrib (dpy, priv->surface, EGL_SWAP_BEHAVIOR, EGL_BUFFER_PRESERVED);
+    // Do not preserve buffer contents on swap
+    eglSurfaceAttrib (dpy, priv->surface, EGL_SWAP_BEHAVIOR, EGL_BUFFER_DESTROYED);
 
     priv->ctx = eglCreateContext (dpy, config, EGL_NO_CONTEXT, context_attribs);
     if (priv->ctx == EGL_NO_CONTEXT)
@@ -415,6 +415,15 @@
 	GL::postSubBuffer = (GL::EGLPostSubBufferNVProc)
 	    eglGetProcAddress ("eglPostSubBufferNV");
 
+    if (!GL::fbo &&
+	!GL::postSubBuffer)
+    {
+	compLogMessage ("opengl", CompLogLevelFatal,
+			"GL_EXT_framebuffer_object or EGL_NV_post_sub_buffer are required");
+	screen->handleCompizEvent ("opengl", "fatal_fallback", o);
+	return false;
+    }
+
     GL::activeTexture = glActiveTexture;
     GL::genFramebuffers = glGenFramebuffers;
     GL::deleteFramebuffers = glDeleteFramebuffers;
@@ -631,6 +640,16 @@
 	    GL::fbo = true;
     }
 
+
+    if (!GL::fbo &&
+	!GL::copySubBuffer)
+    {
+	compLogMessage ("opengl", CompLogLevelFatal,
+			"GL_EXT_framebuffer_object or GLX_MESA_copy_sub_buffer are required");
+	screen->handleCompizEvent ("opengl", "fatal_fallback", o);
+	return false;
+    }
+
     if (strstr (glExtensions, "GL_ARB_vertex_buffer_object"))
     {
 	GL::bindBuffer = (GL::GLBindBufferProc)
@@ -1069,6 +1088,14 @@
     lighting (false),
     #ifndef USE_GLES
     getProcAddress (0),
+    bufferBlit (screen->dpy (), *screen,
+		boost::bind (&PrivateGLScreen::optionGetSyncToVblank, this),
+		cScreen->output (),
+		boost::bind (&PrivateGLScreen::waitForVideoSync, this)),
+    #else
+    bufferBlit (screen->dpy (), *screen,
+		boost::bind (&PrivateGLScreen::optionGetSyncToVblank, this),
+		surface),
     #endif
     scratchFbo (NULL),
     scratchFboBindFailed (false),
@@ -1677,6 +1704,111 @@
         GL::waitForVideoSync ();
 }
 
+BaseDoubleBuffer::BaseDoubleBuffer (Display *d, const CompSize &s,
+				const boost::function <bool ()> &getSyncVblankFunc) :
+    mDpy (d),
+    mSize (s),
+    getSyncVblank (getSyncVblankFunc)
+{
+}
+
+#ifndef USE_GLES
+
+GLXDoubleBuffer::GLXDoubleBuffer (Display *d,
+			      const CompSize &s,
+			      const boost::function <bool ()> &getSyncVblankFunc,
+			      Window output,
+			      const boost::function <void ()> &waitVSyncFunc) :
+    BaseDoubleBuffer (d, s, getSyncVblankFunc),
+    mOutput (output),
+    waitVSync (waitVSyncFunc)
+{
+}
+
+void
+GLXDoubleBuffer::swapBuffers () const
+{
+    GL::controlSwapVideoSync (getSyncVblank ());
+    glXSwapBuffers (mDpy, mOutput);
+}
+
+bool
+GLXDoubleBuffer::subBufferBlitAvailable () const
+{
+    return GL::copySubBuffer ? true : false;
+}
+
+void
+GLXDoubleBuffer::subBufferBlit (const CompRegion &region) const
+{
+    CompRect::vector blitRects (region.rects ());
+    int		     y = 0;
+
+    waitVSync ();
+
+    foreach (const CompRect &r, blitRects)
+    {
+	y = mSize.height () - r.y2 ();
+
+	(*GL::copySubBuffer) (screen->dpy (), mOutput,
+			      r.x1 (), y,
+			      r.width (),
+			      r.height ());
+    }
+}
+
+#else
+
+EGLDoubleBuffer::EGLDoubleBuffer (Display *d,
+			      const CompSize &s,
+			      const boost::function <bool ()> &getSyncVblankFunc,
+			      EGLSurface const & surface) :
+    BaseDoubleBuffer (d, s, getSyncVblankFunc),
+    mSurface (surface)
+{
+}
+
+void
+EGLDoubleBuffer::swapBuffers () const
+{
+    GL::controlSwapVideoSync (getSyncVblank ());
+
+    eglSwapBuffers (eglGetDisplay (mDpy), mSurface);
+    eglWaitGL ();
+    XFlush (mDpy);
+}
+
+bool
+EGLDoubleBuffer::subBufferBlitAvailable () const
+{
+    return GL::postSubBuffer ? true : false;
+}
+
+void
+EGLDoubleBuffer::subBufferBlit (const CompRegion &region) const
+{
+    CompRect::vector blitRects (region.rects ());
+    int		     y = 0;
+
+    GL::controlSwapVideoSync (getSyncVblank ());
+
+    foreach (const CompRect &r, blitRects)
+    {
+	y = mSize.height () - r.y2 ();
+
+	(*GL::postSubBuffer) (eglGetDisplay (screen->dpy ()),
+			      mSurface,
+			      r.x1 (), y,
+			      r.width (),
+			      r.height ());
+    }
+
+    eglWaitGL ();
+    XFlush (screen->dpy ());
+}
+
+#endif
+
 void
 PrivateGLScreen::paintOutputs (CompOutput::ptrList &outputs,
 			       unsigned int        mask,
@@ -1691,11 +1823,21 @@
     GLFramebufferObject *oldFbo = NULL;
     bool useFbo = false;
 
-    if (!scratchFboBindFailed)
+    /* Clear the color buffer where appropriate */
+    if (!scratchFboBindFailed && GL::fbo)
     {
+	if (clearBuffers)
+	    glClear (GL_COLOR_BUFFER_BIT);
+
 	oldFbo = scratchFbo->bind ();
 	useFbo = scratchFbo->checkStatus () && scratchFbo->tex ();
     }
+    else
+    {
+	if (clearBuffers)
+	    if (mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK)
+		glClear (GL_COLOR_BUFFER_BIT);
+    }
 
     if (!useFbo && !scratchFboBindFailed)
     {
@@ -1721,12 +1863,6 @@
     }
 #endif
 
-    if (clearBuffers)
-    {
-	if (mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK)
-	    glClear (GL_COLOR_BUFFER_BIT);
-    }
-
     CompRegion tmpRegion (region);
 
     foreach (CompOutput *output, outputs)
@@ -1747,11 +1883,7 @@
 	    lastViewport = r;
 	}
 
-#ifdef USE_GLES
-	if (mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK || !GL::postSubBuffer)
-#else
 	if (mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK)
-#endif
 	{
 	    GLMatrix identity;
 
@@ -1796,109 +1928,18 @@
 	GLFramebufferObject::rebind (oldFbo);
 
 	// FIXME: does not work if screen dimensions exceed max texture size
-	gScreen->glPaintCompositedOutput (tmpRegion, scratchFbo, mask);
+	gScreen->glPaintCompositedOutput (screen->region (), scratchFbo, mask);
     }
 
-    glFlush ();
-
-#ifdef USE_GLES
-    if (mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK || !GL::postSubBuffer)
-#else
+    unsigned int blitMask = 0;
+
+    if (useFbo)
+	blitMask |= compiz::opengl::PaintedWithFramebufferObject;
+
     if (mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK)
-#endif
-     {
-	/*
-	 * 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.
-	 */
-	#ifdef USE_GLES
-	Display *xdpy = screen->dpy ();
-	GL::controlSwapVideoSync (optionGetSyncToVblank ());
-	eglSwapBuffers (eglGetDisplay (xdpy), surface);
-	eglWaitGL ();
-	XFlush (xdpy);
-	#else
-	GL::controlSwapVideoSync (optionGetSyncToVblank ());
-	glXSwapBuffers (screen->dpy (), cScreen->output ());
-	#endif
-    }
-    else
-    {
-	BoxPtr pBox = const_cast <Region> (tmpRegion.handle ())->rects;
-	int    nBox = const_cast <Region> (tmpRegion.handle ())->numRects;
-	int    y;
-
-	waitForVideoSync ();
-
-	#ifdef USE_GLES
-	Display *xdpy = screen->dpy ();
-
-	GL::controlSwapVideoSync (optionGetSyncToVblank ());
-
-	while (nBox--)
-	{
-	    y = screen->height () - pBox->y2;
-
-	    (*GL::postSubBuffer) (eglGetDisplay (xdpy), surface,
-				  pBox->x1, y,
-				  pBox->x2 - pBox->x1,
-				  pBox->y2 - pBox->y1);
-	    pBox++;
-	}
-
-	eglWaitGL ();
-	XFlush (xdpy);
-
-	#else
-	if (GL::copySubBuffer)
-	{
-	    while (nBox--)
-	    {
-		y = screen->height () - pBox->y2;
-
-		(*GL::copySubBuffer) (screen->dpy (), cScreen->output (),
-				      pBox->x1, y,
-				      pBox->x2 - pBox->x1,
-				      pBox->y2 - pBox->y1);
-
-		pBox++;
-	    }
-	}
-	else
-	{
-	    glEnable (GL_SCISSOR_TEST);
-	    glDrawBuffer (GL_FRONT);
-
-	    while (nBox--)
-	    {
-		y = screen->height () - pBox->y2;
-
-		glBitmap (0, 0, 0, 0,
-			  pBox->x1 - rasterPos.x (),
-			  y - rasterPos.y (),
-			  NULL);
-
-		rasterPos = CompPoint (pBox->x1, y);
-
-		glScissor (pBox->x1, y,
-			   pBox->x2 - pBox->x1,
-			   pBox->y2 - pBox->y1);
-
-		glCopyPixels (pBox->x1, y,
-			      pBox->x2 - pBox->x1,
-			      pBox->y2 - pBox->y1,
-			      GL_COLOR);
-
-		pBox++;
-	    }
-
-	    glDrawBuffer (GL_BACK);
-	    glDisable (GL_SCISSOR_TEST);
-	    glFlush ();
-	}
-	#endif
-    }
+	blitMask |= compiz::opengl::PaintedFullscreen;
+
+    compiz::opengl::blitBuffers (blitMask, tmpRegion, bufferBlit);
 
     lastMask = mask;
 }