Merge lp:~vanvugt/compiz/fix-unredirect-flicker into lp:compiz/0.9.8

[Daniel van Vugt]

Fix flickering observed when an "Unredirect Fullscreen Window" gets
redirected.
(LP: #1046664)

See commit message.

[Daniel van Vugt (vanvugt)]

Sadly, this fix only works with GL, not GLES.

It may be possible to fix bug 1046664 by other means** and provide a fix for GLES too, but I'm fairly sure the only way to fix bug 1050749 with GLES would be to disallow unredirection with multiple monitors.

** which I have tried to find and failed so far.

[jenkins (martin-mrazik+qa)]

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

review: Approve (continuous-integration)

[Sam Spilsbury (smspillaz)]

Hmm, I'm a little confused by this. My understanding of this bug is that if you have fullscreen window Y on monitor B and no fullscreen windows on monitor A that unredirecting Y causes monitor B to flicker. Yes? The bounding shape of the output window is clipped in that case, and the way in which we swap buffers should have nothing to do with it.

I had a look into this and it seems like our fullscreen region detection code doesn't quite cover this case. What happens is that the we are doing the detection in terms of each monitor's paint pass.

So on monitor B, we do a pass and detect that a window is covering the monitor region and unredirect it. When we pass monitor A a bit later, we check every single window including the previously unredirected window from monitor B against monitor A, and find that nothing occludes monitor A. So we unredirect B. As a result, B is in a state of constantly being redirected and unredirected, hence the flicker.

Bug 1046664 is a completely different issue. I suspect what's going on there is that the unredirected window + menu are still there in the backbuffer, and then we partial-repaint the menu on top of that backbuffer (so the old menu is still there).

Here's a patch that at least fixes the constant un/redirection

=== modified file 'plugins/opengl/src/fsregion/fsregion.cpp'
--- plugins/opengl/src/fsregion/fsregion.cpp 2012-09-06 08:01:54 +0000
+++ plugins/opengl/src/fsregion/fsregion.cpp 2012-09-17 11:50:53 +0000
@@ -30,7 +30,8 @@

 FullscreenRegion::FullscreenRegion (const CompRect &rect) :
     covered (false),
- untouched (rect)
+ untouched (rect),
+ orig (untouched)
 {
 }

@@ -38,8 +39,19 @@
 FullscreenRegion::isCoveredBy (const CompRegion &region, WinFlags flags)
 {
     bool fullscreen = false;
+ /* We don't want to unredirect desktop or alpha windows, or consider them
+ * as not-occluding other fullscreen windows */
+ const bool canBeConsideredAsCovering = !(flags & (Desktop | Alpha));
+ /* The region of this fullscreen window covers the existing untouched area
+ * of this monitor */
+ const bool coversUntouchedRegion = region == untouched;

- if (!covered && !(flags & (Desktop | Alpha)) && region == untouched)
+ /* This window covers the fullscreen area of this monitor, consider
+ * the monitor covered and don't allow any further windows on this
+ * monitor to be considered fullscreen */
+ if (!covered &&
+ canBeConsideredAsCovering &&
+ coversUntouchedRegion)
     {
  covered = true;
  fullscreen = true;
@@ -50,5 +62,14 @@
     return fullscreen;
 }

+bool
+FullscreenRegion::allowRedirection (const CompRegion &region)
+{
+ /* Don't allow existing unredirected windows that cover this
+ * region to be redirected again as they were probably unredirected
+ * on another monitor */
+ return region.intersects (orig);
+}
+
 } // namespace opengl
 } // namespace compiz

=== modified file 'plugins/opengl/src/fsregion/fsregion.h'
--- plugins/opengl/src/fsregion/fsregion.h 2012-09-06 08:01:54 +0000
+++ plugins/opengl/src/fsregion/fsregion.h 2012-09-17 11:51:14 +0000
@@ -36,8 +36,8 @@
 public:
     typedef enum
     {
- Desktop = 1,
- Alpha = ...

review: Needs Information

[Daniel van Vugt (vanvugt)]

"The bounding shape of the output window is clipped in that case, and the way in which we swap buffers should have nothing to do with it."

No, that's incorrect. Buffer swapping is a hardware operation that always swaps the whole screen (all outputs), unconditionally. It does not do clipping of any sort. It's a GLX function, not GL.

[Daniel van Vugt (vanvugt)]

Unrelated... if your solution solves the single monitor case then I'm happy to ignore/disable the multi-monitor case for now.

[Sam Spilsbury (smspillaz)]

> "The bounding shape of the output window is clipped in that case, and the way
> in which we swap buffers should have nothing to do with it."
>
> No, that's incorrect. Buffer swapping is a hardware operation that always
> swaps the whole screen (all outputs), unconditionally. It does not do clipping
> of any sort. It's a GLX function, not GL.

This is correct, however it shouldn't have anything to do with the flickering that you see on top of the unredirected window. When a window is unredirected, the window that compiz displays to is clipped so that it does not cover the area of the unredirected window. It doesn't matter if any part of that buffer will be swapped, it is not visible in any case.

We could be thinking of entirely separate problems here, and I apologize if that's the case. Can you clarify why the unredirected window and compiz are competing for the same regions if buffer swapping is on?

[Daniel van Vugt (vanvugt)]

"the window that compiz displays to is clipped so that it does not cover the area of the unredirected window"

That is relevant to regional sub-suffer redraws but not relevant to buffer swapping. Buffer swapping will swap everything, including unredirected/uninitialized regions. I don't believe that the X server has a hook into GLX to tell the driver to clip the swapped region around unredirected windows.

So why don't we see flickering all the time?...
With a single monitor, it /appears/ to mostly work either by luck or by design of the X server to ensure that unredirected windows are painted immediately after the buffer swap, but before vertical retrace. Either way, I can't find a documented reason why it behaves that way, or why it should work without flickering, ever.

[Daniel van Vugt (vanvugt)]

But as I said, if your solution lets us use more buffer swapping and not have to revert to sub-buffers like mine, then I would prefer yours.

[Sam Spilsbury (smspillaz)]

> "the window that compiz displays to is clipped so that it does not cover the
> area of the unredirected window"
>
> That is relevant to regional sub-suffer redraws but not relevant to buffer
> swapping. Buffer swapping will swap everything, including
> unredirected/uninitialized regions. I don't believe that the X server has a
> hook into GLX to tell the driver to clip the swapped region around
> unredirected windows.

Right, it doesn't clipped the swapped region in the application buffer (eg, compiz' output window). My contention here is that this part of the application buffer is not even visible. The bounding shape of the output window is clipped away in CompositeWindow::updateOutputWindow. If we didn't unredirect the window for instance, but still clipped it, what you'd see is just a blank area of the screen where the unredirected window should be.

The only reason I'm asking is because requiring partial copies may still be relevant if we need to ensure that this part of the OpenGL buffer (eg, not the visible part of the X output window) must remain untouched even while it is invisible.

>
> So why don't we see flickering all the time?...
> With a single monitor, it /appears/ to mostly work either by luck or by design
> of the X server to ensure that unredirected windows are painted immediately
> after the buffer swap, but before vertical retrace. Either way, I can't find a
> documented reason why it behaves that way, or why it should work without
> flickering, ever.

These windows are painted separately, in a region where X does not draw our output window to the screen.

[Daniel van Vugt (vanvugt)]

Again, unredirection should not work at all with buffer swapping. And I am yet to see any documentation explaining how it does partially work, sometimes. Looking at CompositeWindow::updateOutputWindow, the answer may be buried in the X Shape extension, but I doubt it.

Buffer swapping is usually implemented by page flipping. That means all clipping you think you have is totally irrelevant. A page flip just changes the address of the video memory you see on your monitor to point to the alternate buffer. A page flip does not process, change or clip the contents of the video memory. Therefore, unredirected windows on top of swapbuffers can only possibly be working if the unredirect windows are drawn last (over the swapped buffer), but during vblank period so you (usually) only see a single consistent image.

[Daniel van Vugt (vanvugt)]

Completing the paper trail:
It appears "it just works" because GLX is designed to draw the (unre)directed windows on top of the front buffer (whichever that is), after the swap:
http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.47.4979

So that's why it mostly works already without flickering for a single monitor.

It seems the multi-monitor flickering is just a mistake in the compiz opengl plugin, which is redirecting unredirected windows repeatedly, prematurely.

[Daniel van Vugt (vanvugt)]

OK, I have approved Sam's patch here:
https://code.launchpad.net/~compiz-team/compiz/fix-multimonitor-unredirect/+merge/124855

This branch however is still the only working fix for bug 1046664. But I'll try to find a nicer solution instead of reproposing this one right now.

[lp:~vanvugt/compiz/fix-unredirect-flicker]

3380. By Daniel van Vugt on 2012-09-19

Merge latest lp:compiz

3381. By Daniel van Vugt on 2012-09-19

Allow swapping to continue when overlay windows exist. This means we no longer
need to compromise multi-monitor visual quality.

Swapping is only disallowed on the single frame where the number of
overlays changes.

[Daniel van Vugt (vanvugt)]

I reject this proposal as a much nicer fix/workaround for bug 1046664 has now landed.

Unmerged revisions

3381. By Daniel van Vugt on 2012-09-19

Allow swapping to continue when overlay windows exist. This means we no longer
need to compromise multi-monitor visual quality.

Swapping is only disallowed on the single frame where the number of
overlays changes.

3380. By Daniel van Vugt on 2012-09-19

Merge latest lp:compiz

3379. By Daniel van Vugt on 2012-09-17

Slightly clearer design: Replace FORCE_SINGLE_BUFFER with !SWAP_ALLOWED

3378. By Daniel van Vugt on 2012-09-17

Add tests.

3377. By Daniel van Vugt on 2012-09-17

Don't allow DoubleBuffer::FORCE_SINGLE_BUFFER to be true ifdef USE_GLES.

3376. By Daniel van Vugt on 2012-09-17

First working prototype for avoiding flicker when switching between
redirected and (unre)direct rendering.

=== modified file 'plugins/opengl/include/opengl/doublebuffer.h'
--- plugins/opengl/include/opengl/doublebuffer.h	2012-09-12 03:55:21 +0000
+++ plugins/opengl/include/opengl/doublebuffer.h	2012-09-19 04:02:23 +0000
@@ -26,6 +26,7 @@
 	    VSYNC,
 	    HAVE_PERSISTENT_BACK_BUFFER,
 	    NEED_PERSISTENT_BACK_BUFFER,
+	    SWAP_ALLOWED,
 	    _NSETTINGS
 	} Setting;
 

=== modified file 'plugins/opengl/src/doublebuffer/src/double-buffer.cpp'
--- plugins/opengl/src/doublebuffer/src/double-buffer.cpp	2012-09-12 03:55:21 +0000
+++ plugins/opengl/src/doublebuffer/src/double-buffer.cpp	2012-09-19 04:02:23 +0000
@@ -43,6 +43,7 @@
     setting[VSYNC] = true;
     setting[HAVE_PERSISTENT_BACK_BUFFER] = false;
     setting[NEED_PERSISTENT_BACK_BUFFER] = false;
+    setting[SWAP_ALLOWED] = true;
 }
 
 DoubleBuffer::~DoubleBuffer ()
@@ -59,7 +60,7 @@
 DoubleBuffer::render (const CompRegion &region,
                       bool fullscreen)
 {
-    if (fullscreen)
+    if (fullscreen && setting[SWAP_ALLOWED])
     {
 	swap ();
 	if (setting[NEED_PERSISTENT_BACK_BUFFER] &&

=== modified file 'plugins/opengl/src/doublebuffer/tests/test-opengl-double-buffer.cpp'
--- plugins/opengl/src/doublebuffer/tests/test-opengl-double-buffer.cpp	2012-09-12 10:45:58 +0000
+++ plugins/opengl/src/doublebuffer/tests/test-opengl-double-buffer.cpp	2012-09-19 04:02:23 +0000
@@ -44,6 +44,28 @@
     db.render (blitRegion, true);
 }
 
+TEST_F(DoubleBufferTest, TestSingleFullscreenNeverSwaps)
+{
+    EXPECT_CALL (db, swap ()).Times (0);
+    EXPECT_CALL (db, copyFrontToBack ()).Times (0);
+    EXPECT_CALL (db, blitAvailable ()).WillRepeatedly (Return (true));
+
+    db.set (DoubleBuffer::SWAP_ALLOWED, false);
+    db.render (blitRegion, true);
+}
+
+TEST_F(DoubleBufferTest, TestSingleRegionNeverSwaps)
+{
+    CompRegion reg (12, 34, 56, 78);
+
+    EXPECT_CALL (db, swap ()).Times (0);
+    EXPECT_CALL (db, copyFrontToBack ()).Times (0);
+    EXPECT_CALL (db, blitAvailable ()).WillRepeatedly (Return (true));
+
+    db.set (DoubleBuffer::SWAP_ALLOWED, false);
+    db.render (reg, false);
+}
+
 TEST_F(DoubleBufferTest, TestNoPaintedFullscreenOrFBOAlwaysBlitsSubBuffer)
 {
     EXPECT_CALL (db, blitAvailable ()).WillOnce (Return (true));

=== modified file 'plugins/opengl/src/privates.h'
--- plugins/opengl/src/privates.h	2012-09-12 04:01:02 +0000
+++ plugins/opengl/src/privates.h	2012-09-19 04:02:23 +0000
@@ -211,6 +211,8 @@
 
 	Pixmap rootPixmapCopy;
 	CompSize rootPixmapSize;
+
+	int prevOverlayCount;
 };
 
 class PrivateGLWindow :

=== modified file 'plugins/opengl/src/screen.cpp'
--- plugins/opengl/src/screen.cpp	2012-09-13 08:35:17 +0000
+++ plugins/opengl/src/screen.cpp	2012-09-19 04:02:23 +0000
@@ -1134,7 +1134,8 @@
     shaderCache (),
     autoProgram (new GLScreenAutoProgram(gs)),
     rootPixmapCopy (None),
-    rootPixmapSize ()
+    rootPixmapSize (),
+    prevOverlayCount (0)
 {
     ScreenInterface::setHandler (screen);
 }
@@ -2044,6 +2045,14 @@
     doubleBuffer.set (DoubleBuffer::VSYNC, optionGetSyncToVblank ());
     doubleBuffer.set (DoubleBuffer::HAVE_PERSISTENT_BACK_BUFFER, useFbo);
     doubleBuffer.set (DoubleBuffer::NEED_PERSISTENT_BACK_BUFFER, alwaysSwap);
+
+#ifndef USE_GLES
+    int overlayCount = cScreen->overlayWindowCount ();
+    doubleBuffer.set (DoubleBuffer::SWAP_ALLOWED,
+                      overlayCount == prevOverlayCount);
+    prevOverlayCount = overlayCount;
+#endif
+
     doubleBuffer.render (tmpRegion, fullscreen);
 
     lastMask = mask;
@@ -2070,7 +2079,9 @@
 PrivateGLScreen::updateRenderMode ()
 {
 #ifndef USE_GLES
-    GL::fboEnabled = GL::fboSupported && optionGetFramebufferObject ();
+    GL::fboEnabled = GL::fboSupported &&
+                     optionGetFramebufferObject () &&
+                     cScreen->overlayWindowCount () == 0;
     GL::vboEnabled = GL::vboSupported && optionGetVertexBufferObject ();
 #endif
 }