Merge lp:~vanvugt/compiz-core/fix-880707 into lp:compiz-core/0.9.5

Proposed by Daniel van Vugt on 2011-11-09
Status: Superseded
Proposed branch: lp:~vanvugt/compiz-core/fix-880707
Merge into: lp:compiz-core/0.9.5
Diff against target: 145 lines (+11/-59)
2 files modified
plugins/composite/src/screen.cpp (+7/-8)
plugins/opengl/src/screen.cpp (+4/-51)
To merge this branch: bzr merge lp:~vanvugt/compiz-core/fix-880707
Reviewer Review Type Date Requested Status
Sam Spilsbury 2011-11-09 Approve on 2011-11-10
Review via email: mp+81737@code.launchpad.net

Description of the change

Reduced screen tearing still visible with "Sync To VBlank" enabled. (LP: #880707)

The composite plugin's frame timing logic did not correctly synchronize with the vblank support of other plugins (opengl). This would lead to excessive visible tearing even when "Sync To VBlank" is enabled in opengl.

This fix does NOT claim to eliminate all tearing, but it does eliminate most of it. To completely eliminate tearing more work has to be done to make the glXSwapBuffers code (opengl plugin) fast enough to use on every frame.

To post a comment you must log in.
Daniel van Vugt (vanvugt) wrote :

I suspect this will fix bug 92599 too, so long as "Sync To VBlank" is enabled in OpenGL. Because composite will now ignore the rubbish framerate it gets from NVIDIA-Xrandr and will always use the natural refresh rate of the monitor instead.

Sam Spilsbury (smspillaz) wrote :

Hi Daniel,

Just to clarify, this returns to using glXWaitVideoSync and glXGetVideoSync rather than glXSwapInterval?

As for moving to using glXSwapBuffers - I agree with that, however the plugins generally expect that the damaged regions are going to be the ones copied over on to the frontbuffer from the backbuffer, rather than the entire back-to-front copy. Our current paint system allows for sort of "spill overs" on to the backbuffer, a much larger work item here is to make every plugin set its paint clipping region explicitly for each thing that it paints, so that the backbuffer is "sacred".

review: Approve
Daniel van Vugt (vanvugt) wrote :

Returning back to WaitVideoSync was necessary, because SwapInterval doesn't block. And we need the blocking in WaitVideoSync for accurate frame timing and to always stay in sync with the monitor. Composite's reasoning that it can magically stay in sync with the monitor's vsync with artificial timer delays was very misguided. If you have Sync to VBlank enabled then the only blocking should be the call to WaitVideoSync. This solves the problem of compiz painting out of phase, and even slightly out of frequency.

SwapInterval was originally used to try and "unblock the CPU". However I now realize that the main CPU blocking issue was this one -- bad timing in composite. SwapInterval made little to no difference and can be safely removed.

The critical fix for bug 763005 is still present:
  (*GL::waitVideoSync) (1, 0, &frameno);

Daniel van Vugt (vanvugt) wrote :

Tearing is further reduced by combining this proposal with another:
https://code.launchpad.net/~vanvugt/compiz-core/fix-priority/+merge/81952

Daniel van Vugt (vanvugt) wrote :

Work in progress again. I think I can improve on this significantly.

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/composite/src/screen.cpp'
2--- plugins/composite/src/screen.cpp 2011-10-15 11:00:51 +0000
3+++ plugins/composite/src/screen.cpp 2011-11-09 14:27:01 +0000
4@@ -706,7 +706,7 @@
5 (FPSLimiterMode == CompositeFPSLimiterModeVSyncLike ||
6 (pHnd && pHnd->hasVSync ()));
7
8- if (idle || hasVSyncBehavior)
9+ if (idle)
10 {
11 if (timeMult > 1)
12 {
13@@ -715,6 +715,11 @@
14 timeMult--;
15 }
16 }
17+ else if (hasVSyncBehavior)
18+ {
19+ frameStatus = 0;
20+ redrawTime = diff;
21+ }
22 else
23 {
24 int next;
25@@ -755,13 +760,7 @@
26 }
27 }
28
29- if (diff >= redrawTime)
30- return 1;
31-
32- if (hasVSyncBehavior)
33- return (redrawTime - diff) * 0.7;
34-
35- return redrawTime - diff;
36+ return (diff >= redrawTime) ? 1 : redrawTime - diff;
37 }
38
39 int
40
41=== modified file 'plugins/opengl/src/screen.cpp'
42--- plugins/opengl/src/screen.cpp 2011-10-26 23:15:02 +0000
43+++ plugins/opengl/src/screen.cpp 2011-11-09 14:27:01 +0000
44@@ -31,9 +31,6 @@
45 #include <math.h>
46
47 namespace GL {
48- typedef int (*GLXSwapIntervalProc) (int interval);
49-
50- GLXSwapIntervalProc swapInterval = NULL;
51 GLXBindTexImageProc bindTexImage = NULL;
52 GLXReleaseTexImageProc releaseTexImage = NULL;
53 GLXQueryDrawableProc queryDrawable = NULL;
54@@ -413,12 +410,6 @@
55 getProcAddress ("glXWaitVideoSyncSGI");
56 }
57
58- if (strstr (glxExtensions, "GLX_SGI_swap_control"))
59- {
60- GL::swapInterval = (GL::GLXSwapIntervalProc)
61- getProcAddress ("glXSwapIntervalSGI");
62- }
63-
64 fbConfigs = (*GL::getFBConfigs) (dpy, s->screenNum (), &nElements);
65
66 for (i = 0; i <= MAX_DEPTH; i++)
67@@ -1076,44 +1067,18 @@
68 priv->lastViewport.height);
69 }
70
71-namespace GL
72-{
73-
74 void
75-waitForVideoSync ()
76+PrivateGLScreen::waitForVideoSync ()
77 {
78+ // Docs: http://www.opengl.org/registry/specs/SGI/video_sync.txt
79 if (GL::waitVideoSync)
80 {
81- // Don't wait twice. Just in case.
82- if (GL::swapInterval)
83- (*GL::swapInterval) (0);
84-
85- // Docs: http://www.opengl.org/registry/specs/SGI/video_sync.txt
86 unsigned int frameno;
87 (*GL::waitVideoSync) (1, 0, &frameno);
88 }
89 }
90
91 void
92-controlSwapVideoSync (bool sync)
93-{
94- // Docs: http://www.opengl.org/registry/specs/SGI/swap_control.txt
95- if (GL::swapInterval)
96- (*GL::swapInterval) (sync ? 1 : 0);
97- else if (sync)
98- waitForVideoSync ();
99-}
100-
101-} // namespace GL
102-
103-void
104-PrivateGLScreen::waitForVideoSync ()
105-{
106- if (optionGetSyncToVblank ())
107- GL::waitForVideoSync ();
108-}
109-
110-void
111 PrivateGLScreen::paintOutputs (CompOutput::ptrList &outputs,
112 unsigned int mask,
113 const CompRegion &region)
114@@ -1183,22 +1148,11 @@
115 targetOutput = &screen->outputDevs ()[0];
116
117 glFlush ();
118+ if (optionGetSyncToVblank ())
119+ waitForVideoSync ();
120
121- /*
122- * FIXME: Actually fix the composite plugin to be more efficient;
123- * If GL::swapInterval == NULL && GL::waitVideoSync != NULL then the
124- * composite plugin should not be imposing any framerate restriction
125- * (ie. blocking the CPU) at all. Because the framerate will be controlled
126- * and optimized here:
127- */
128 if (mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK)
129 {
130- /*
131- * controlSwapVideoSync is much faster than waitForVideoSync because
132- * it won't block the CPU. The waiting is offloaded to the GPU.
133- * Unfortunately it only works with glXSwapBuffers in most drivers.
134- */
135- GL::controlSwapVideoSync (optionGetSyncToVblank ());
136 glXSwapBuffers (screen->dpy (), cScreen->output ());
137 }
138 else
139@@ -1206,7 +1160,6 @@
140 BoxPtr pBox;
141 int nBox, y;
142
143- waitForVideoSync ();
144 pBox = const_cast <Region> (tmpRegion.handle ())->rects;
145 nBox = const_cast <Region> (tmpRegion.handle ())->numRects;
146

Subscribers

People subscribed via source and target branches