Merge lp:~vanvugt/compiz/fix-1037411 into lp:compiz/0.9.8

[Daniel van Vugt]

Workaround SubBuffer performance regression (LP: #1037411), which is actually
Mesa bug #54763.

Workaround SubBuffer performance regression (LP: #1037411), which is actually
Mesa bug #54763.

Make sure copyFrontToBack is _never_ called unless it's actually needed. Or else you will trigger Mesa bug #54763 (which is a /feature/ from what I've been told so far).

[jenkins (martin-mrazik+qa)]

PASSED: Continuous integration, rev:3370
http://jenkins.qa.ubuntu.com/job/compiz-ci/70/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/70/console

review: Approve (continuous-integration)

[Sam Spilsbury (smspillaz)]

Looking good. I'm not sure if I like the stateful design, but this can be dealt with elsewhere.

86 + db.set (DoubleBuffer::NEED_PERSISTENT_BACK_BUFFER, false);

This should be unnecessary, as the class will be re-constructed upon every test run. If its touching static data that's bad.

For the EGL case, if we do want to enable swap-by-copying in cases where framebuffer objects are not available, you can set the surface attrib in glInitContext if postSubBufferNV and framebuffer objects are not available (very unlikely), eg

eglSurfaceAttrib (EGL_SWAP_BEHAVIOUR, EGL_PRESERVED);

[Daniel van Vugt (vanvugt)]

I agree line 86 may be redundant, but I was covering for the case where you may not know which test within the fixture gets executed next. At least it's defensive against future maintenance.

I know EGL has the preservation option. There's a similar extension for GLX, but it was never fully implemented when I tried it last.

[Sam Spilsbury (smspillaz)]

> I agree line 86 may be redundant, but I was covering for the case where you
> may not know which test within the fixture gets executed next. At least it's
> defensive against future maintenance.

This doesn't matter. The object is destructed upon test completion.

>
> I know EGL has the preservation option. There's a similar extension for GLX,
> but it was never fully implemented when I tried it last.

Curious. What was it called ?

[Daniel van Vugt (vanvugt)]

http://www.opengl.org/registry/specs/OML/glx_swap_method.txt

[Sam Spilsbury (smspillaz)]

Can you remove the redundant line? Then I'm happy to merge it.

[lp:~vanvugt/compiz/fix-1037411]

3371. By Daniel van Vugt on 2012-09-12

Remove redundant line

[Daniel van Vugt (vanvugt)]

Done

[jenkins (martin-mrazik+qa)]

PASSED: Continuous integration, rev:3371
http://jenkins.qa.ubuntu.com/job/compiz-ci/73/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/73/console

review: Approve (continuous-integration)

=== modified file 'plugins/opengl/include/opengl/doublebuffer.h'
--- plugins/opengl/include/opengl/doublebuffer.h	2012-07-24 06:34:28 +0000
+++ plugins/opengl/include/opengl/doublebuffer.h	2012-09-12 10:48:18 +0000
@@ -19,11 +19,13 @@
 	virtual void blit (const CompRegion &region) const = 0;
 	virtual bool fallbackBlitAvailable () const = 0;
 	virtual void fallbackBlit (const CompRegion &region) const = 0;
+	virtual void copyFrontToBack () const = 0;
 
 	typedef enum
 	{
 	    VSYNC,
-	    PERSISTENT_BACK_BUFFER,
+	    HAVE_PERSISTENT_BACK_BUFFER,
+	    NEED_PERSISTENT_BACK_BUFFER,
 	    _NSETTINGS
 	} Setting;
 

=== modified file 'plugins/opengl/src/doublebuffer/src/double-buffer.cpp'
--- plugins/opengl/src/doublebuffer/src/double-buffer.cpp	2012-08-09 06:58:19 +0000
+++ plugins/opengl/src/doublebuffer/src/double-buffer.cpp	2012-09-12 10:48:18 +0000
@@ -41,7 +41,8 @@
 DoubleBuffer::DoubleBuffer ()
 {
     setting[VSYNC] = true;
-    setting[PERSISTENT_BACK_BUFFER] = false;
+    setting[HAVE_PERSISTENT_BACK_BUFFER] = false;
+    setting[NEED_PERSISTENT_BACK_BUFFER] = false;
 }
 
 DoubleBuffer::~DoubleBuffer ()
@@ -59,7 +60,14 @@
                       bool fullscreen)
 {
     if (fullscreen)
+    {
 	swap ();
+	if (setting[NEED_PERSISTENT_BACK_BUFFER] &&
+	    !setting[HAVE_PERSISTENT_BACK_BUFFER])
+	{
+	    copyFrontToBack ();
+	}
+    }
     else if (blitAvailable ())
 	blit (region);
     else if (fallbackBlitAvailable ())

=== modified file 'plugins/opengl/src/doublebuffer/tests/test-opengl-double-buffer.cpp'
--- plugins/opengl/src/doublebuffer/tests/test-opengl-double-buffer.cpp	2012-08-09 06:58:19 +0000
+++ plugins/opengl/src/doublebuffer/tests/test-opengl-double-buffer.cpp	2012-09-12 10:48:18 +0000
@@ -18,6 +18,7 @@
 	MOCK_CONST_METHOD1 (blit, void (const CompRegion &));
 	MOCK_CONST_METHOD0 (fallbackBlitAvailable, bool ());
 	MOCK_CONST_METHOD1 (fallbackBlit, void (const CompRegion &));
+	MOCK_CONST_METHOD0 (copyFrontToBack, void ());
 };
 
 class DoubleBufferTest :
@@ -38,6 +39,7 @@
 TEST_F(DoubleBufferTest, TestPaintedFullAlwaysSwaps)
 {
     EXPECT_CALL (db, swap ());
+    EXPECT_CALL (db, copyFrontToBack ()).Times (0);
 
     db.render (blitRegion, true);
 }
@@ -46,6 +48,31 @@
 {
     EXPECT_CALL (db, blitAvailable ()).WillOnce (Return (true));
     EXPECT_CALL (db, blit (_));
+    EXPECT_CALL (db, copyFrontToBack ()).Times (0);
+
+    db.render (blitRegion, false);
+}
+
+TEST_F(DoubleBufferTest, SwapWithoutFBO)
+{
+    db.set (DoubleBuffer::HAVE_PERSISTENT_BACK_BUFFER, false);
+    db.set (DoubleBuffer::NEED_PERSISTENT_BACK_BUFFER, true);
+
+    EXPECT_CALL (db, swap ());
+    EXPECT_CALL (db, copyFrontToBack ()).Times (1);
+
+    db.render (blitRegion, true);
+}
+
+TEST_F(DoubleBufferTest, BlitWithoutFBO)
+{
+    db.set (DoubleBuffer::HAVE_PERSISTENT_BACK_BUFFER, false);
+    db.set (DoubleBuffer::NEED_PERSISTENT_BACK_BUFFER, false);
+
+    EXPECT_CALL (db, blitAvailable ()).WillRepeatedly (Return (true));
+    EXPECT_CALL (db, blit (_));
+    EXPECT_CALL (db, swap ()).Times (0);
+    EXPECT_CALL (db, copyFrontToBack ()).Times (0);
 
     db.render (blitRegion, false);
 }

=== modified file 'plugins/opengl/src/privates.h'
--- plugins/opengl/src/privates.h	2012-07-30 07:10:52 +0000
+++ plugins/opengl/src/privates.h	2012-09-12 10:48:18 +0000
@@ -74,6 +74,7 @@
 	void blit (const CompRegion &region) const;
 	bool fallbackBlitAvailable () const;
 	void fallbackBlit (const CompRegion &region) const;
+	void copyFrontToBack () const;
 
     protected:
 
@@ -96,6 +97,7 @@
 	void blit (const CompRegion &region) const;
 	bool fallbackBlitAvailable () const;
 	void fallbackBlit (const CompRegion &region) const;
+	void copyFrontToBack () const;
 
     private:
 

=== modified file 'plugins/opengl/src/screen.cpp'
--- plugins/opengl/src/screen.cpp	2012-08-17 06:54:37 +0000
+++ plugins/opengl/src/screen.cpp	2012-09-12 10:48:18 +0000
@@ -1722,8 +1722,8 @@
 
 #ifndef USE_GLES
 
-static void
-copyFrontToBack()
+void
+GLXDoubleBuffer::copyFrontToBack() const
 {
     int w = screen->width ();
     int h = screen->height ();
@@ -1761,9 +1761,6 @@
     GL::controlSwapVideoSync (setting[VSYNC]);
 
     glXSwapBuffers (mDpy, mOutput);
-
-    if (!setting[PERSISTENT_BACK_BUFFER])
-	copyFrontToBack ();
 }
 
 bool
@@ -1891,6 +1888,11 @@
 {
 }
 
+void
+EGLDoubleBuffer::copyFrontToBack() const
+{
+}
+
 #endif
 
 void
@@ -2023,13 +2025,15 @@
 	gScreen->glPaintCompositedOutput (screen->region (), scratchFbo, mask);
     }
 
+    bool alwaysSwap = optionGetAlwaysSwapBuffers ();
     bool fullscreen = useFbo ||
-                      optionGetAlwaysSwapBuffers () ||
+                      alwaysSwap ||
                       ((mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK) &&
                        commonFrontbuffer);
 
     doubleBuffer.set (DoubleBuffer::VSYNC, optionGetSyncToVblank ());
-    doubleBuffer.set (DoubleBuffer::PERSISTENT_BACK_BUFFER, useFbo);
+    doubleBuffer.set (DoubleBuffer::HAVE_PERSISTENT_BACK_BUFFER, useFbo);
+    doubleBuffer.set (DoubleBuffer::NEED_PERSISTENT_BACK_BUFFER, alwaysSwap);
     doubleBuffer.render (tmpRegion, fullscreen);
 
     lastMask = mask;