Merge lp:~vanvugt/compiz-core/fix-880707.2 into lp:compiz-core/0.9.5
- fix-880707.2
- Merge into 0.9.5
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Sam Spilsbury | Approve | ||
Compiz Maintainers | Pending | ||
Tim Penhey | Pending | ||
Review via email: mp+83472@code.launchpad.net |
This proposal supersedes a proposal from 2011-11-14.
Description of the change
Introduced "tickless" frame timing to the composite plugin. This means we no longer need to poll for repaints. Composite only wakes up when a repaint is required and it's the right time to do so. This not only improves vsync accuracy, but also reduces CPU usage and power consumption.
Tim Penhey (thumper) wrote : Posted in a previous version of this proposal | # |
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I disagree...
That file already contains multiple variables per line. So I am not violating the existing standard.
And I tried using initializer lists, but gcc gave warnings about variable initialization order. Those =false lines were the only way I could make the warnings go away.
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
222 + /*
223 + * Note the use of delay = 1 instead of 0, even though 0 would be better.
224 + * A delay of zero is presently broken due to CompTimer bugs;
225 + * 1. Infinite loop in CompTimeoutSour
226 + * timer is set.
227 + * 2. Priority set too high in CompTimeoutSour
228 + * causing the glib main event loop to be starved of X events.'
Just use the delay of zero and mark the other branches as prerequisites to this one :)
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Oh, it might also be good to get this code into unit tests, since regressions that could be introduced on frame timing would end up very difficult to debug down the road.
Feel free to send me an email about it, and I'll work on it next week. Thanks for all this, its excellent :)
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
> Just use the delay of zero and mark the other branches as prerequisites to
> this one :)
I tested a delay of zero. While it made a huge difference in the old composite code (and exposed the CompTimer bugs), using 0 instead of 1 makes no perceivable performance difference in this new version (tested with fixes for CompTimer applied). So maybe my comment is a bit wrong and should not say "even though 0 would be better". In fact, in theory, a delay of zero would increase CPU usage. And since it provides no perceivable benefit any more, I don't think we should aim to change it to zero.
Also, creating dependencies on other branches when you don't need to is not ideal. I would prefer to keep things simple as they are...
Tim Penhey (thumper) wrote : Posted in a previous version of this proposal | # |
The reason you get warnings in the initializer list is that the member variables are initialized in the order that they are declared in the class. The initializer list expects that the order in the list is the same as the order that they are defined. If you don't, it is possible that a developer would assume that something earlier in the initializer list was initialized when in fact it wasn't.
Sam, what does the compiz coding standards say about initializer lists (if anything)?
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Awesome. Testing with NVIDIA-280, this fix appears to solve bug 92599 and bug 888039.
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Found a problem when testing with "nouveau". Nouveau advertises support for GLX_SGI_video_sync, however it is quietly disabled by default and returns immediately. This causes the code in this proposal to attempt to run unthrottled at 1000 FPS, which obviously is a little laggy.
The workaround for nouveau is in /etc/X11/xorg.conf:
Section "Device"
Identifier "My Graphics"
Option "GLXVBlank" "on"
EndSection
However, I will have to build a workaround into this proposal so that users don't need to edit xorg.conf.
Mikkel Kamstrup Erlandsen (kamstrup) wrote : Posted in a previous version of this proposal | # |
Or - query upstream as to why it is disabled? Maybe it's something we
can safely distropatch, they already have it in the pipes?
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I will research the issue with nouveau separately. However it's now clear that we need to occasionally expect graphics drivers to be a little broken. Compiz should be able to handle it without becoming unusable. I will look at adding detection for such driver problems into compiz (opengl plugin?) today.
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Interestingly, I only realized today that the fix for bug 763005 is a prerequisite for this proposal. It probably wouldn't work otherwise. Lucky the fix for 763005 is already committed...
Daniel van Vugt (vanvugt) wrote : | # |
Sam Spilsbury (smspillaz) wrote : | # |
This will be merged as soon as the testing branch in https:/
Sam Spilsbury (smspillaz) wrote : | # |
(I also don't like working around driver bugs inside of the opengl plugin since these can break if the driver developers change something. If you need to add a workaround, add it to the workarounds plugin by replacing the GL::getVideoSync function pointer with your work-around wrapper)
Daniel van Vugt (vanvugt) wrote : | # |
I'm generalizing the "driver bug workaround" today because similar code is needed for all drivers in some circumstances.
Daniel van Vugt (vanvugt) wrote : | # |
Done. The changes to plugins/
Daniel van Vugt (vanvugt) wrote : | # |
Major improvement today. Reduced tearing enough to also resolve bug 755841.
Omer Akram (om26er) wrote : | # |
Daniel, is there a specific reason this change won't make into Precise? I have noticed real positive improvements on on Intel(smooth minimize animations), nvidia-current (no tearing) and nouvea (no tearing)
Daniel van Vugt (vanvugt) wrote : | # |
Omer, you misinterpreted my comment in bug 880707. What I meant was that this fix is so complex that it won't go into any oneiric updates. But getting it into precise is my absolute top priority.
I agree; the number of improvements this branch brings is much larger than I ever envisioned. I never even noticed the stuttering bugs until I saw they were fixed by this code. It was a nice surprise.
We do absolutely want everyone to have these improvements by the time precise is released. :)
Sam Spilsbury (smspillaz) wrote : | # |
I'm going to have another look at how we can do automated testing from this branch again tomorrow.
Daniel van Vugt (vanvugt) wrote : | # |
I already did automate testing... using an army of testers now subscribed to ppa:vanvugt/compiz ;)
OK, maybe you meant a different kind of automated...
Sam Spilsbury (smspillaz) wrote : | # |
Indeed :)
I've started updating lp:~smspillaz/compiz-core/fix-880707.2.test with the stuff we discussed there earlier. Namely testing for phase (eg, that it doesn't take > 4ms to get from a vblank begin fence to finishing a swapbuffers / paint) and also testing that we don't fall out of phase when we are throttling the framerate with the composite plugin.
I don't really think its possible to be testing for an exact fix to bug 880707 without instantiating a separate opengl context and running out constructs in there, but doing that is going to be far too much effort than its worth really.
Please let me know of any other parts of the composite plugin's code that you think can be tested.
Sam Spilsbury (smspillaz) wrote : | # |
(correction: lp:~smspillaz/compiz-core/compiz-core.fix_880707.2.test)
Sam Spilsbury (smspillaz) wrote : | # |
(and thank you for setting up the PPA)
Daniel van Vugt (vanvugt) wrote : | # |
Comments in the other branch.
Daniel van Vugt (vanvugt) wrote : | # |
More work may be required on this proposal. Some anecdotal evidence from a minority of testers, and a brief test on a slow netbook, both suggest that this proposal can make performance worse. I will need to look into this.
Sam Spilsbury (smspillaz) wrote : | # |
Sure. I'll keep working on testcases for the current stuff we've got and rebase on any other work you come up with when that happens.
Daniel van Vugt (vanvugt) wrote : | # |
Fixed the slow-motion animations problem in r2900.
Sam Spilsbury (smspillaz) wrote : | # |
I agree with fixing the plugins for what its worth.
Sam Spilsbury (smspillaz) wrote : | # |
Hey Daniel,
I had a think about the problem that you were encountering with the plugins. As you said earlier, the plugins are expecting a situation where msSinceLastPaint is going to be reflective of optimalRedrawTime, but with the tickless algorithm, msSinceLastPaint could be quite large. On slower systems, if it took > optimalRedrawTime to actually process a paint, then forcing msSinceLastPaint to be optimalRedrawTime would result in slow animations since frame skipping would not work correctly.
Since the only case we really wanted to cover was the case where the screen went from idle to active, what do you think about doing something like this:
struct timeval tv;
int timeDiff;
gettimeofday (&tv, 0);
timeDiff = TIMEVALDIFF (&tv, &mLastRedraw);
/* handle clock rollback */
if (timeDiff < 0)
timeDiff = 0;
/*
* Now that we use a "tickless" timing algorithm, timeDiff could be
* very large if the screen is truely idle.
* However plugins expect the old behaviour where timeDiff is never
* larger than the frame rate (optimalRedrawT
* So enforce this to keep animations timed correctly and smooth...
*/
if (timeDiff > mOptimalRedrawTime && !reschedule)
timeDiff = mOptimalRedrawTime;
...
At least on your branch, reschedule is going to be set every time a plugin requests a redraw in donePaint or addWindowDamage (which is the correct way to do it), so that means that in those cases, msSinceLastPaint is always going to be accurate. And where we have switched from inactive to active, msSinceLastPaint is always going to be mOptimalRedrawTime (in fact, it could be zero, but that might require further testing).
I've updated my changes to reflect this.
Daniel van Vugt (vanvugt) wrote : | # |
Your suggestion looks incorrect. priv->reschedule is always false there. I don't think it will work.
Best to use my r2900 below. Mine has been tested for a few days, whereas yours would appear to probably not work (or compile in that form :)
Sam Spilsbury (smspillaz) wrote : | # |
I forgot to mention - the check would have to be moved to before priv->reschedule has been set to false (I've roughly transposed the code from my branch to yours)
Sam Spilsbury (smspillaz) wrote : | # |
(It works quite well here, and I've tested it with all the plugins)
Sam Spilsbury (smspillaz) wrote : | # |
I think that checking if the timeDiff is more than 100 only really works around the original problem which I outlined earlier as well. There's a case where some really slow plugin could take more than 100 ms per frame to process some animation, which would result in perceived slowness in its animation code ;-)
Daniel van Vugt (vanvugt) wrote : | # |
"I forgot to mention - the check would have to be moved to before priv->reschedule has been set to false (I've roughly transposed the code from my branch to yours)"
That still makes no sense in my mind. But if you can reproduce the slow animations bug consistently, and have tested and verified your fix works then go nuts. If however this is another code change you're proposing without actually testing it, then please reconsider.
Sam Spilsbury (smspillaz) wrote : | # |
On Thu, 22 Dec 2011, Daniel van Vugt wrote:
> "I forgot to mention - the check would have to be moved to before priv->reschedule has been set to false (I've roughly transposed the code from my branch to yours)"
>
> That still makes no sense in my mind. But if you can reproduce the slow animations bug consistently, and have tested and verified your fix works then go nuts. If however this is another code change you're proposing without actually testing it, then please reconsider.
I'm running it right now and haven't run into any problems whatsoever. I
can probably give it a try on some of my other systems as well, but I put
some cases in to force it to run slowly and it still works then.
> --
> https:/
> You are requested to review the proposed merge of lp:~vanvugt/compiz-core/fix-880707.2 into lp:compiz-core.
>
Daniel van Vugt (vanvugt) wrote : | # |
Yep, a simple sleep or something to make painting slow should allow you to reproduce the problem and then verify you're actually fixing it.
In my real-world example, I have an old netbook which takes between 2-4 frames to paint each frame (up to 64ms) during animations. So it's easy to reproduce "slow motion animations" on a machine that only achieves 15-30 FPS.
Oddly enough, as I have mentioned before, this "slow" machine gets a perfect 60 FPS with "Force full screen redraw (buffer swap) on repaint" enabled. But only when using this branch. The difference is staggering.
- 2901. By Daniel van Vugt
-
Merge with upstream changes (lp:compiz-core).
- 2902. By Daniel van Vugt
-
Merge upstream changes.
- 2903. By Daniel van Vugt
-
Merge from upstream.
Sam Spilsbury (smspillaz) wrote : | # |
OK, I think there's been enough discussion here.
I'm confident with the changes this branch is introducing, and think we just need to land it now. I have created an entire testing framework for it, which can be found at lp:~smspillaz/compiz-core/compiz-core.fix_880707.2.test . That test tests for both phase and period timings. On a period of aggressive vblank scheduling for 70 seconds, that test passes each time on my machine.
The testing branch will need to land a little bit after the main one does, once Daniel is ok with it, but ideally it needs to be in before feature freeze.
Preview Diff
1 | === modified file 'plugins/composite/src/privates.h' |
2 | --- plugins/composite/src/privates.h 2011-10-15 11:00:51 +0000 |
3 | +++ plugins/composite/src/privates.h 2012-01-16 10:07:25 +0000 |
4 | @@ -64,7 +64,7 @@ |
5 | |
6 | void detectRefreshRate (); |
7 | |
8 | - int getTimeToNextRedraw (struct timeval *tv); |
9 | + void scheduleRepaint (); |
10 | |
11 | public: |
12 | |
13 | @@ -95,13 +95,9 @@ |
14 | int overlayWindowCount; |
15 | |
16 | struct timeval lastRedraw; |
17 | - int nextRedraw; |
18 | int redrawTime; |
19 | int optimalRedrawTime; |
20 | - int frameStatus; |
21 | - int timeMult; |
22 | - bool idle; |
23 | - int timeLeft; |
24 | + bool scheduled, painting, reschedule; |
25 | |
26 | bool slowAnimations; |
27 | |
28 | @@ -110,7 +106,6 @@ |
29 | compiz::composite::PaintHandler *pHnd; |
30 | |
31 | CompositeFPSLimiterMode FPSLimiterMode; |
32 | - int frameTimeAccumulator; |
33 | |
34 | CompWindowList withDestroyedWindows; |
35 | }; |
36 | |
37 | === modified file 'plugins/composite/src/screen.cpp' |
38 | --- plugins/composite/src/screen.cpp 2012-01-12 17:49:40 +0000 |
39 | +++ plugins/composite/src/screen.cpp 2012-01-16 10:07:25 +0000 |
40 | @@ -279,17 +279,14 @@ |
41 | exposeRects (), |
42 | windowPaintOffset (0, 0), |
43 | overlayWindowCount (0), |
44 | - nextRedraw (0), |
45 | redrawTime (1000 / 50), |
46 | optimalRedrawTime (1000 / 50), |
47 | - frameStatus (0), |
48 | - timeMult (1), |
49 | - idle (true), |
50 | - timeLeft (0), |
51 | + scheduled (false), |
52 | + painting (false), |
53 | + reschedule (false), |
54 | slowAnimations (false), |
55 | pHnd (NULL), |
56 | FPSLimiterMode (CompositeFPSLimiterModeDefault), |
57 | - frameTimeAccumulator (0), |
58 | withDestroyedWindows () |
59 | { |
60 | gettimeofday (&lastRedraw, 0); |
61 | @@ -422,10 +419,6 @@ |
62 | |
63 | showOutputWindow (); |
64 | |
65 | - priv->paintTimer.start |
66 | - (boost::bind (&CompositeScreen::handlePaintTimeout, this), |
67 | - priv->optimalRedrawTime); |
68 | - |
69 | return true; |
70 | } |
71 | |
72 | @@ -469,11 +462,9 @@ |
73 | void |
74 | CompositeScreen::damageScreen () |
75 | { |
76 | - if (priv->damageMask == 0) |
77 | - priv->paintTimer.setTimes (priv->paintTimer.minLeft ()); |
78 | - |
79 | priv->damageMask |= COMPOSITE_SCREEN_DAMAGE_ALL_MASK; |
80 | priv->damageMask &= ~COMPOSITE_SCREEN_DAMAGE_REGION_MASK; |
81 | + priv->scheduleRepaint (); |
82 | } |
83 | |
84 | void |
85 | @@ -482,9 +473,6 @@ |
86 | if (priv->damageMask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK) |
87 | return; |
88 | |
89 | - if (priv->damageMask == 0) |
90 | - priv->paintTimer.setTimes (priv->paintTimer.minLeft ()); |
91 | - |
92 | priv->damage += region; |
93 | priv->damageMask |= COMPOSITE_SCREEN_DAMAGE_REGION_MASK; |
94 | |
95 | @@ -495,15 +483,14 @@ |
96 | |
97 | if (priv->damage.numRects () > 100) |
98 | damageScreen (); |
99 | + priv->scheduleRepaint (); |
100 | } |
101 | |
102 | void |
103 | CompositeScreen::damagePending () |
104 | { |
105 | - if (priv->damageMask == 0) |
106 | - priv->paintTimer.setTimes (priv->paintTimer.minLeft ()); |
107 | - |
108 | priv->damageMask |= COMPOSITE_SCREEN_DAMAGE_PENDING_MASK; |
109 | + priv->scheduleRepaint (); |
110 | } |
111 | |
112 | unsigned int |
113 | @@ -673,6 +660,7 @@ |
114 | mOptions[CompositeOptions::DetectRefreshRate].value ().set (false); |
115 | screen->setOptionForPlugin ("composite", "refresh_rate", value); |
116 | mOptions[CompositeOptions::DetectRefreshRate].value ().set (true); |
117 | + optimalRedrawTime = redrawTime = 1000 / value.i (); |
118 | } |
119 | else |
120 | { |
121 | @@ -693,77 +681,47 @@ |
122 | priv->FPSLimiterMode = newMode; |
123 | } |
124 | |
125 | -int |
126 | -PrivateCompositeScreen::getTimeToNextRedraw (struct timeval *tv) |
127 | +void |
128 | +PrivateCompositeScreen::scheduleRepaint () |
129 | { |
130 | - int diff; |
131 | - |
132 | - diff = compiz::core::timer::timeval_diff (tv, &lastRedraw); |
133 | - |
134 | - /* handle clock rollback */ |
135 | - if (diff < 0) |
136 | - diff = 0; |
137 | - |
138 | - bool hasVSyncBehavior = |
139 | - (FPSLimiterMode == CompositeFPSLimiterModeVSyncLike || |
140 | - (pHnd && pHnd->hasVSync ())); |
141 | - |
142 | - if (idle || hasVSyncBehavior) |
143 | - { |
144 | - if (timeMult > 1) |
145 | - { |
146 | - frameStatus = -1; |
147 | - redrawTime = optimalRedrawTime; |
148 | - timeMult--; |
149 | - } |
150 | + if (painting) |
151 | + { |
152 | + reschedule = true; |
153 | + return; |
154 | + } |
155 | + |
156 | + if (scheduled) |
157 | + return; |
158 | + |
159 | + scheduled = true; |
160 | + |
161 | + int delay; |
162 | + if (FPSLimiterMode == CompositeFPSLimiterModeVSyncLike || |
163 | + (pHnd && pHnd->hasVSync ())) |
164 | + { |
165 | + delay = 1; |
166 | } |
167 | else |
168 | { |
169 | - int next; |
170 | - if (diff > redrawTime) |
171 | - { |
172 | - if (frameStatus > 0) |
173 | - frameStatus = 0; |
174 | - |
175 | - next = optimalRedrawTime * (timeMult + 1); |
176 | - if (diff > next) |
177 | - { |
178 | - frameStatus--; |
179 | - if (frameStatus < -1) |
180 | - { |
181 | - timeMult++; |
182 | - redrawTime = diff = next; |
183 | - } |
184 | - } |
185 | - } |
186 | - else if (diff < redrawTime) |
187 | - { |
188 | - if (frameStatus < 0) |
189 | - frameStatus = 0; |
190 | - |
191 | - if (timeMult > 1) |
192 | - { |
193 | - next = optimalRedrawTime * (timeMult - 1); |
194 | - if (diff < next) |
195 | - { |
196 | - frameStatus++; |
197 | - if (frameStatus > 4) |
198 | - { |
199 | - timeMult--; |
200 | - redrawTime = next; |
201 | - } |
202 | - } |
203 | - } |
204 | - } |
205 | + struct timeval now; |
206 | + gettimeofday (&now, 0); |
207 | + int elapsed = compiz::core::timer::timeval_diff (&now, &lastRedraw); |
208 | + if (elapsed < 0) |
209 | + elapsed = 0; |
210 | + delay = elapsed < optimalRedrawTime ? optimalRedrawTime - elapsed : 1; |
211 | } |
212 | - |
213 | - if (diff >= redrawTime) |
214 | - return 1; |
215 | - |
216 | - if (hasVSyncBehavior) |
217 | - return (redrawTime - diff) * 0.7; |
218 | - |
219 | - return redrawTime - diff; |
220 | + /* |
221 | + * Note the use of delay = 1 instead of 0, even though 0 would be better. |
222 | + * A delay of zero is presently broken due to CompTimer bugs; |
223 | + * 1. Infinite loop in CompTimeoutSource::callback when a zero |
224 | + * timer is set. |
225 | + * 2. Priority set too high in CompTimeoutSource::CompTimeoutSource |
226 | + * causing the glib main event loop to be starved of X events. |
227 | + * Fixes for both of these issues are being worked on separately. |
228 | + */ |
229 | + paintTimer.start |
230 | + (boost::bind (&CompositeScreen::handlePaintTimeout, cScreen), |
231 | + delay); |
232 | } |
233 | |
234 | int |
235 | @@ -782,8 +740,9 @@ |
236 | CompositeScreen::handlePaintTimeout () |
237 | { |
238 | struct timeval tv; |
239 | - int timeToNextRedraw; |
240 | |
241 | + priv->painting = true; |
242 | + priv->reschedule = false; |
243 | gettimeofday (&tv, 0); |
244 | |
245 | if (priv->damageMask) |
246 | @@ -798,21 +757,18 @@ |
247 | /* handle clock rollback */ |
248 | if (timeDiff < 0) |
249 | timeDiff = 0; |
250 | - |
251 | - if (priv->slowAnimations) |
252 | - { |
253 | - int msSinceLastPaint; |
254 | - |
255 | - if (priv->FPSLimiterMode == CompositeFPSLimiterModeDisabled) |
256 | - msSinceLastPaint = 1; |
257 | - else |
258 | - msSinceLastPaint = |
259 | - priv->idle ? 2 : (timeDiff * 2) / priv->redrawTime; |
260 | - |
261 | - preparePaint (msSinceLastPaint); |
262 | - } |
263 | - else |
264 | - preparePaint (priv->idle ? priv->redrawTime : timeDiff); |
265 | + /* |
266 | + * Now that we use a "tickless" timing algorithm, timeDiff could be |
267 | + * very large if the screen is truely idle. |
268 | + * However plugins expect the old behaviour where timeDiff is rarely |
269 | + * larger than the frame rate (optimalRedrawTime). |
270 | + * So enforce this to keep animations timed correctly and smooth... |
271 | + */ |
272 | + if (timeDiff > 100) |
273 | + timeDiff = priv->optimalRedrawTime; |
274 | + |
275 | + priv->redrawTime = timeDiff; |
276 | + preparePaint (priv->slowAnimations ? 1 : timeDiff); |
277 | |
278 | /* substract top most overlay window region */ |
279 | if (priv->overlayWindowCount) |
280 | @@ -876,39 +832,15 @@ |
281 | break; |
282 | } |
283 | } |
284 | - |
285 | - priv->idle = false; |
286 | - } |
287 | - else |
288 | - { |
289 | - priv->idle = true; |
290 | } |
291 | |
292 | priv->lastRedraw = tv; |
293 | - gettimeofday (&tv, 0); |
294 | - |
295 | - if (priv->FPSLimiterMode == CompositeFPSLimiterModeDisabled) |
296 | - { |
297 | - const int msToReturn1After = 100; |
298 | - |
299 | - priv->frameTimeAccumulator += priv->redrawTime; |
300 | - if (priv->frameTimeAccumulator > msToReturn1After) |
301 | - { |
302 | - priv->frameTimeAccumulator %= msToReturn1After; |
303 | - timeToNextRedraw = 1; |
304 | - } |
305 | - else |
306 | - timeToNextRedraw = 0; |
307 | - } |
308 | - else |
309 | - timeToNextRedraw = priv->getTimeToNextRedraw (&tv); |
310 | - |
311 | - if (priv->idle) |
312 | - priv->paintTimer.setTimes (timeToNextRedraw, MAXSHORT); |
313 | - else |
314 | - priv->paintTimer.setTimes (timeToNextRedraw); |
315 | - |
316 | - return true; |
317 | + priv->painting = false; |
318 | + priv->scheduled = false; |
319 | + if (priv->reschedule) |
320 | + priv->scheduleRepaint (); |
321 | + |
322 | + return false; |
323 | } |
324 | |
325 | void |
326 | |
327 | === modified file 'plugins/opengl/src/screen.cpp' |
328 | --- plugins/opengl/src/screen.cpp 2012-01-09 15:12:20 +0000 |
329 | +++ plugins/opengl/src/screen.cpp 2012-01-16 10:07:25 +0000 |
330 | @@ -78,6 +78,9 @@ |
331 | |
332 | bool canDoSaturated = false; |
333 | bool canDoSlightlySaturated = false; |
334 | + |
335 | + unsigned int vsyncCount = 0; |
336 | + unsigned int unthrottledFrames = 0; |
337 | } |
338 | |
339 | CompOutput *targetOutput = NULL; |
340 | @@ -1077,15 +1080,29 @@ |
341 | void |
342 | waitForVideoSync () |
343 | { |
344 | + GL::unthrottledFrames++; |
345 | if (GL::waitVideoSync) |
346 | { |
347 | // Don't wait twice. Just in case. |
348 | if (GL::swapInterval) |
349 | (*GL::swapInterval) (0); |
350 | |
351 | + /* |
352 | + * While glXSwapBuffers/glXCopySubBufferMESA are meant to do a |
353 | + * flush before they blit, it is best to not let that happen. |
354 | + * Because that flush would occur after GL::waitVideoSync, causing |
355 | + * a delay and the final blit to be slightly out of sync resulting |
356 | + * in tearing. So we need to do a glFinish before we wait for |
357 | + * vsync, to absolutely minimize tearing. |
358 | + */ |
359 | + glFinish (); |
360 | + |
361 | // Docs: http://www.opengl.org/registry/specs/SGI/video_sync.txt |
362 | - unsigned int frameno; |
363 | - (*GL::waitVideoSync) (1, 0, &frameno); |
364 | + unsigned int oldCount = GL::vsyncCount; |
365 | + (*GL::waitVideoSync) (1, 0, &GL::vsyncCount); |
366 | + |
367 | + if (GL::vsyncCount != oldCount) |
368 | + GL::unthrottledFrames = 0; |
369 | } |
370 | } |
371 | |
372 | @@ -1094,7 +1111,10 @@ |
373 | { |
374 | // Docs: http://www.opengl.org/registry/specs/SGI/swap_control.txt |
375 | if (GL::swapInterval) |
376 | + { |
377 | (*GL::swapInterval) (sync ? 1 : 0); |
378 | + GL::unthrottledFrames++; |
379 | + } |
380 | else if (sync) |
381 | waitForVideoSync (); |
382 | } |
383 | @@ -1177,15 +1197,6 @@ |
384 | |
385 | targetOutput = &screen->outputDevs ()[0]; |
386 | |
387 | - glFlush (); |
388 | - |
389 | - /* |
390 | - * FIXME: Actually fix the composite plugin to be more efficient; |
391 | - * If GL::swapInterval == NULL && GL::waitVideoSync != NULL then the |
392 | - * composite plugin should not be imposing any framerate restriction |
393 | - * (ie. blocking the CPU) at all. Because the framerate will be controlled |
394 | - * and optimized here: |
395 | - */ |
396 | if (mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK) |
397 | { |
398 | /* |
399 | @@ -1257,7 +1268,8 @@ |
400 | bool |
401 | PrivateGLScreen::hasVSync () |
402 | { |
403 | - return (GL::waitVideoSync && optionGetSyncToVblank ()); |
404 | + return GL::waitVideoSync && optionGetSyncToVblank () && |
405 | + GL::unthrottledFrames < 5; |
406 | } |
407 | |
408 | bool |
> bool scheduled, painting, reschedule;
Many coding standards say only one variable declaration per line. I'm not entirely sure what the compiz-core one says.
Also, please prefer initializer lists to setting variables in the constructor.
+ scheduled = false;
+ painting = false;
+ reschedule = false;
These should all be in the initializer list.