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
1=== modified file 'plugins/opengl/include/opengl/doublebuffer.h'
2--- plugins/opengl/include/opengl/doublebuffer.h 2012-07-24 06:34:28 +0000
3+++ plugins/opengl/include/opengl/doublebuffer.h 2012-09-12 10:48:18 +0000
4@@ -19,11 +19,13 @@
5 virtual void blit (const CompRegion &region) const = 0;
6 virtual bool fallbackBlitAvailable () const = 0;
7 virtual void fallbackBlit (const CompRegion &region) const = 0;
8+ virtual void copyFrontToBack () const = 0;
9
10 typedef enum
11 {
12 VSYNC,
13- PERSISTENT_BACK_BUFFER,
14+ HAVE_PERSISTENT_BACK_BUFFER,
15+ NEED_PERSISTENT_BACK_BUFFER,
16 _NSETTINGS
17 } Setting;
18
19
20=== modified file 'plugins/opengl/src/doublebuffer/src/double-buffer.cpp'
21--- plugins/opengl/src/doublebuffer/src/double-buffer.cpp 2012-08-09 06:58:19 +0000
22+++ plugins/opengl/src/doublebuffer/src/double-buffer.cpp 2012-09-12 10:48:18 +0000
23@@ -41,7 +41,8 @@
24 DoubleBuffer::DoubleBuffer ()
25 {
26 setting[VSYNC] = true;
27- setting[PERSISTENT_BACK_BUFFER] = false;
28+ setting[HAVE_PERSISTENT_BACK_BUFFER] = false;
29+ setting[NEED_PERSISTENT_BACK_BUFFER] = false;
30 }
31
32 DoubleBuffer::~DoubleBuffer ()
33@@ -59,7 +60,14 @@
34 bool fullscreen)
35 {
36 if (fullscreen)
37+ {
38 swap ();
39+ if (setting[NEED_PERSISTENT_BACK_BUFFER] &&
40+ !setting[HAVE_PERSISTENT_BACK_BUFFER])
41+ {
42+ copyFrontToBack ();
43+ }
44+ }
45 else if (blitAvailable ())
46 blit (region);
47 else if (fallbackBlitAvailable ())
48
49=== modified file 'plugins/opengl/src/doublebuffer/tests/test-opengl-double-buffer.cpp'
50--- plugins/opengl/src/doublebuffer/tests/test-opengl-double-buffer.cpp 2012-08-09 06:58:19 +0000
51+++ plugins/opengl/src/doublebuffer/tests/test-opengl-double-buffer.cpp 2012-09-12 10:48:18 +0000
52@@ -18,6 +18,7 @@
53 MOCK_CONST_METHOD1 (blit, void (const CompRegion &));
54 MOCK_CONST_METHOD0 (fallbackBlitAvailable, bool ());
55 MOCK_CONST_METHOD1 (fallbackBlit, void (const CompRegion &));
56+ MOCK_CONST_METHOD0 (copyFrontToBack, void ());
57 };
58
59 class DoubleBufferTest :
60@@ -38,6 +39,7 @@
61 TEST_F(DoubleBufferTest, TestPaintedFullAlwaysSwaps)
62 {
63 EXPECT_CALL (db, swap ());
64+ EXPECT_CALL (db, copyFrontToBack ()).Times (0);
65
66 db.render (blitRegion, true);
67 }
68@@ -46,6 +48,31 @@
69 {
70 EXPECT_CALL (db, blitAvailable ()).WillOnce (Return (true));
71 EXPECT_CALL (db, blit (_));
72+ EXPECT_CALL (db, copyFrontToBack ()).Times (0);
73+
74+ db.render (blitRegion, false);
75+}
76+
77+TEST_F(DoubleBufferTest, SwapWithoutFBO)
78+{
79+ db.set (DoubleBuffer::HAVE_PERSISTENT_BACK_BUFFER, false);
80+ db.set (DoubleBuffer::NEED_PERSISTENT_BACK_BUFFER, true);
81+
82+ EXPECT_CALL (db, swap ());
83+ EXPECT_CALL (db, copyFrontToBack ()).Times (1);
84+
85+ db.render (blitRegion, true);
86+}
87+
88+TEST_F(DoubleBufferTest, BlitWithoutFBO)
89+{
90+ db.set (DoubleBuffer::HAVE_PERSISTENT_BACK_BUFFER, false);
91+ db.set (DoubleBuffer::NEED_PERSISTENT_BACK_BUFFER, false);
92+
93+ EXPECT_CALL (db, blitAvailable ()).WillRepeatedly (Return (true));
94+ EXPECT_CALL (db, blit (_));
95+ EXPECT_CALL (db, swap ()).Times (0);
96+ EXPECT_CALL (db, copyFrontToBack ()).Times (0);
97
98 db.render (blitRegion, false);
99 }
100
101=== modified file 'plugins/opengl/src/privates.h'
102--- plugins/opengl/src/privates.h 2012-07-30 07:10:52 +0000
103+++ plugins/opengl/src/privates.h 2012-09-12 10:48:18 +0000
104@@ -74,6 +74,7 @@
105 void blit (const CompRegion &region) const;
106 bool fallbackBlitAvailable () const;
107 void fallbackBlit (const CompRegion &region) const;
108+ void copyFrontToBack () const;
109
110 protected:
111
112@@ -96,6 +97,7 @@
113 void blit (const CompRegion &region) const;
114 bool fallbackBlitAvailable () const;
115 void fallbackBlit (const CompRegion &region) const;
116+ void copyFrontToBack () const;
117
118 private:
119
120
121=== modified file 'plugins/opengl/src/screen.cpp'
122--- plugins/opengl/src/screen.cpp 2012-08-17 06:54:37 +0000
123+++ plugins/opengl/src/screen.cpp 2012-09-12 10:48:18 +0000
124@@ -1722,8 +1722,8 @@
125
126 #ifndef USE_GLES
127
128-static void
129-copyFrontToBack()
130+void
131+GLXDoubleBuffer::copyFrontToBack() const
132 {
133 int w = screen->width ();
134 int h = screen->height ();
135@@ -1761,9 +1761,6 @@
136 GL::controlSwapVideoSync (setting[VSYNC]);
137
138 glXSwapBuffers (mDpy, mOutput);
139-
140- if (!setting[PERSISTENT_BACK_BUFFER])
141- copyFrontToBack ();
142 }
143
144 bool
145@@ -1891,6 +1888,11 @@
146 {
147 }
148
149+void
150+EGLDoubleBuffer::copyFrontToBack() const
151+{
152+}
153+
154 #endif
155
156 void
157@@ -2023,13 +2025,15 @@
158 gScreen->glPaintCompositedOutput (screen->region (), scratchFbo, mask);
159 }
160
161+ bool alwaysSwap = optionGetAlwaysSwapBuffers ();
162 bool fullscreen = useFbo ||
163- optionGetAlwaysSwapBuffers () ||
164+ alwaysSwap ||
165 ((mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK) &&
166 commonFrontbuffer);
167
168 doubleBuffer.set (DoubleBuffer::VSYNC, optionGetSyncToVblank ());
169- doubleBuffer.set (DoubleBuffer::PERSISTENT_BACK_BUFFER, useFbo);
170+ doubleBuffer.set (DoubleBuffer::HAVE_PERSISTENT_BACK_BUFFER, useFbo);
171+ doubleBuffer.set (DoubleBuffer::NEED_PERSISTENT_BACK_BUFFER, alwaysSwap);
172 doubleBuffer.render (tmpRegion, fullscreen);
173
174 lastMask = mask;

Subscribers

People subscribed via source and target branches