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

Proposed by Daniel van Vugt on 2012-09-12
Status: Merged
Approved by: Sam Spilsbury on 2012-09-13
Approved revision: 3371
Merged at revision: 3370
Proposed branch: lp:~vanvugt/compiz/fix-1037411
Merge into: lp:compiz/0.9.8
Diff against target: 174 lines (+52/-9)
5 files modified
plugins/opengl/include/opengl/doublebuffer.h (+3/-1)
plugins/opengl/src/doublebuffer/src/double-buffer.cpp (+9/-1)
plugins/opengl/src/doublebuffer/tests/test-opengl-double-buffer.cpp (+27/-0)
plugins/opengl/src/privates.h (+2/-0)
plugins/opengl/src/screen.cpp (+11/-7)
To merge this branch: bzr merge lp:~vanvugt/compiz/fix-1037411
Reviewer Review Type Date Requested Status
jenkins (community) continuous-integration Approve on 2012-09-12
Compiz Maintainers 2012-09-12 Pending
Review via email: mp+123878@code.launchpad.net

Commit message

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

Description of the change

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).

To post a comment you must log in.
jenkins (martin-mrazik+qa) wrote :
review: Approve (continuous-integration)
Sam Spilsbury (smspillaz) wrote :

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) wrote :

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) wrote :

> 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 ?

Sam Spilsbury (smspillaz) wrote :

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

lp:~vanvugt/compiz/fix-1037411 updated on 2012-09-12
3371. By Daniel van Vugt on 2012-09-12

Remove redundant line

Daniel van Vugt (vanvugt) wrote :

Done

jenkins (martin-mrazik+qa) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
19 virtual void blit (const CompRegion &region) const = 0;19 virtual void blit (const CompRegion &region) const = 0;
20 virtual bool fallbackBlitAvailable () const = 0;20 virtual bool fallbackBlitAvailable () const = 0;
21 virtual void fallbackBlit (const CompRegion &region) const = 0;21 virtual void fallbackBlit (const CompRegion &region) const = 0;
22 virtual void copyFrontToBack () const = 0;
2223
23 typedef enum24 typedef enum
24 {25 {
25 VSYNC,26 VSYNC,
26 PERSISTENT_BACK_BUFFER,27 HAVE_PERSISTENT_BACK_BUFFER,
28 NEED_PERSISTENT_BACK_BUFFER,
27 _NSETTINGS29 _NSETTINGS
28 } Setting;30 } Setting;
2931
3032
=== 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 @@
41DoubleBuffer::DoubleBuffer ()41DoubleBuffer::DoubleBuffer ()
42{42{
43 setting[VSYNC] = true;43 setting[VSYNC] = true;
44 setting[PERSISTENT_BACK_BUFFER] = false;44 setting[HAVE_PERSISTENT_BACK_BUFFER] = false;
45 setting[NEED_PERSISTENT_BACK_BUFFER] = false;
45}46}
4647
47DoubleBuffer::~DoubleBuffer ()48DoubleBuffer::~DoubleBuffer ()
@@ -59,7 +60,14 @@
59 bool fullscreen)60 bool fullscreen)
60{61{
61 if (fullscreen)62 if (fullscreen)
63 {
62 swap ();64 swap ();
65 if (setting[NEED_PERSISTENT_BACK_BUFFER] &&
66 !setting[HAVE_PERSISTENT_BACK_BUFFER])
67 {
68 copyFrontToBack ();
69 }
70 }
63 else if (blitAvailable ())71 else if (blitAvailable ())
64 blit (region);72 blit (region);
65 else if (fallbackBlitAvailable ())73 else if (fallbackBlitAvailable ())
6674
=== 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 @@
18 MOCK_CONST_METHOD1 (blit, void (const CompRegion &));18 MOCK_CONST_METHOD1 (blit, void (const CompRegion &));
19 MOCK_CONST_METHOD0 (fallbackBlitAvailable, bool ());19 MOCK_CONST_METHOD0 (fallbackBlitAvailable, bool ());
20 MOCK_CONST_METHOD1 (fallbackBlit, void (const CompRegion &));20 MOCK_CONST_METHOD1 (fallbackBlit, void (const CompRegion &));
21 MOCK_CONST_METHOD0 (copyFrontToBack, void ());
21};22};
2223
23class DoubleBufferTest :24class DoubleBufferTest :
@@ -38,6 +39,7 @@
38TEST_F(DoubleBufferTest, TestPaintedFullAlwaysSwaps)39TEST_F(DoubleBufferTest, TestPaintedFullAlwaysSwaps)
39{40{
40 EXPECT_CALL (db, swap ());41 EXPECT_CALL (db, swap ());
42 EXPECT_CALL (db, copyFrontToBack ()).Times (0);
4143
42 db.render (blitRegion, true);44 db.render (blitRegion, true);
43}45}
@@ -46,6 +48,31 @@
46{48{
47 EXPECT_CALL (db, blitAvailable ()).WillOnce (Return (true));49 EXPECT_CALL (db, blitAvailable ()).WillOnce (Return (true));
48 EXPECT_CALL (db, blit (_));50 EXPECT_CALL (db, blit (_));
51 EXPECT_CALL (db, copyFrontToBack ()).Times (0);
52
53 db.render (blitRegion, false);
54}
55
56TEST_F(DoubleBufferTest, SwapWithoutFBO)
57{
58 db.set (DoubleBuffer::HAVE_PERSISTENT_BACK_BUFFER, false);
59 db.set (DoubleBuffer::NEED_PERSISTENT_BACK_BUFFER, true);
60
61 EXPECT_CALL (db, swap ());
62 EXPECT_CALL (db, copyFrontToBack ()).Times (1);
63
64 db.render (blitRegion, true);
65}
66
67TEST_F(DoubleBufferTest, BlitWithoutFBO)
68{
69 db.set (DoubleBuffer::HAVE_PERSISTENT_BACK_BUFFER, false);
70 db.set (DoubleBuffer::NEED_PERSISTENT_BACK_BUFFER, false);
71
72 EXPECT_CALL (db, blitAvailable ()).WillRepeatedly (Return (true));
73 EXPECT_CALL (db, blit (_));
74 EXPECT_CALL (db, swap ()).Times (0);
75 EXPECT_CALL (db, copyFrontToBack ()).Times (0);
4976
50 db.render (blitRegion, false);77 db.render (blitRegion, false);
51}78}
5279
=== 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 @@
74 void blit (const CompRegion &region) const;74 void blit (const CompRegion &region) const;
75 bool fallbackBlitAvailable () const;75 bool fallbackBlitAvailable () const;
76 void fallbackBlit (const CompRegion &region) const;76 void fallbackBlit (const CompRegion &region) const;
77 void copyFrontToBack () const;
7778
78 protected:79 protected:
7980
@@ -96,6 +97,7 @@
96 void blit (const CompRegion &region) const;97 void blit (const CompRegion &region) const;
97 bool fallbackBlitAvailable () const;98 bool fallbackBlitAvailable () const;
98 void fallbackBlit (const CompRegion &region) const;99 void fallbackBlit (const CompRegion &region) const;
100 void copyFrontToBack () const;
99101
100 private:102 private:
101103
102104
=== 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 @@
17221722
1723#ifndef USE_GLES1723#ifndef USE_GLES
17241724
1725static void1725void
1726copyFrontToBack()1726GLXDoubleBuffer::copyFrontToBack() const
1727{1727{
1728 int w = screen->width ();1728 int w = screen->width ();
1729 int h = screen->height ();1729 int h = screen->height ();
@@ -1761,9 +1761,6 @@
1761 GL::controlSwapVideoSync (setting[VSYNC]);1761 GL::controlSwapVideoSync (setting[VSYNC]);
17621762
1763 glXSwapBuffers (mDpy, mOutput);1763 glXSwapBuffers (mDpy, mOutput);
1764
1765 if (!setting[PERSISTENT_BACK_BUFFER])
1766 copyFrontToBack ();
1767}1764}
17681765
1769bool1766bool
@@ -1891,6 +1888,11 @@
1891{1888{
1892}1889}
18931890
1891void
1892EGLDoubleBuffer::copyFrontToBack() const
1893{
1894}
1895
1894#endif1896#endif
18951897
1896void1898void
@@ -2023,13 +2025,15 @@
2023 gScreen->glPaintCompositedOutput (screen->region (), scratchFbo, mask);2025 gScreen->glPaintCompositedOutput (screen->region (), scratchFbo, mask);
2024 }2026 }
20252027
2028 bool alwaysSwap = optionGetAlwaysSwapBuffers ();
2026 bool fullscreen = useFbo ||2029 bool fullscreen = useFbo ||
2027 optionGetAlwaysSwapBuffers () ||2030 alwaysSwap ||
2028 ((mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK) &&2031 ((mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK) &&
2029 commonFrontbuffer);2032 commonFrontbuffer);
20302033
2031 doubleBuffer.set (DoubleBuffer::VSYNC, optionGetSyncToVblank ());2034 doubleBuffer.set (DoubleBuffer::VSYNC, optionGetSyncToVblank ());
2032 doubleBuffer.set (DoubleBuffer::PERSISTENT_BACK_BUFFER, useFbo);2035 doubleBuffer.set (DoubleBuffer::HAVE_PERSISTENT_BACK_BUFFER, useFbo);
2036 doubleBuffer.set (DoubleBuffer::NEED_PERSISTENT_BACK_BUFFER, alwaysSwap);
2033 doubleBuffer.render (tmpRegion, fullscreen);2037 doubleBuffer.render (tmpRegion, fullscreen);
20342038
2035 lastMask = mask;2039 lastMask = mask;

Subscribers

People subscribed via source and target branches