Merge lp:~compiz-team/compiz/compiz.fix_1016366 into lp:compiz/0.9.8

[Sam Spilsbury]

== Problem ==
There are a few cases where textures can become invalid because a pixmap went away and a plugin is keeping the texture around for animations (eg, unmapRefCnt).

This is anywhere where CompositeWindow::release () can be called from, which includes frame updates, map, unmap, resize etc. In those cases, some drivers may not report a failure to bind the texture and we will have an invalid texture.

== Solution ==
Don't allow unbinds of textures kept around for animations in
any case, not just resizing.

== Test ==
Covered by existing unit tests.

[Daniel van Vugt (vanvugt)]

Works nicely on intel. I'll retest with NVIDIA later.

I would suggest making frozen() const. But it looks like that might not be possible because it uses CompWindow which is not const-friendly.

[Daniel van Vugt (vanvugt)]

Also OK on NVIDIA.

For future reference, I would suggest not using names like:
   class PixmapFreezerInterface
And instead use something like:
   class FreezablePixmap

review: Approve

=== modified file 'plugins/composite/include/composite/composite.h'
--- plugins/composite/include/composite/composite.h	2012-06-06 05:12:28 +0000
+++ plugins/composite/include/composite/composite.h	2012-06-22 05:29:36 +0000
@@ -371,6 +371,11 @@
 	bool redirected ();
 	bool overlayWindow ();
 
+	/**
+	 * Returns true if pixmap updates are frozen
+	 */
+	bool frozen ();
+
 	void damageTransformedRect (float          xScale,
 				    float          yScale,
 				    float          xTranslate,

=== modified file 'plugins/composite/src/pixmapbinding/include/pixmapbinding.h'
--- plugins/composite/src/pixmapbinding/include/pixmapbinding.h	2012-05-31 07:15:37 +0000
+++ plugins/composite/src/pixmapbinding/include/pixmapbinding.h	2012-06-22 05:29:36 +0000
@@ -61,6 +61,15 @@
 	virtual void allowFurtherRebindAttempts () = 0;
 };
 
+class PixmapFreezerInterface
+{
+    public:
+
+	virtual ~PixmapFreezerInterface () {}
+
+	virtual bool frozen () = 0;
+};
+
 class WindowAttributesGetInterface
 {
     public:
@@ -163,6 +172,7 @@
 	PixmapBinding (const NewPixmapReadyCallback &,
 			WindowPixmapGetInterface *,
 			WindowAttributesGetInterface *,
+			PixmapFreezerInterface *,
 			ServerGrabInterface *);
 
 	~PixmapBinding ();
@@ -184,6 +194,7 @@
 
 	WindowPixmapGetInterface *windowPixmapRetreiver;
 	WindowAttributesGetInterface *windowAttributesRetreiver;
+	PixmapFreezerInterface        *pixmapFreezer;
 	ServerGrabInterface *serverGrab;
 
 };

=== modified file 'plugins/composite/src/pixmapbinding/src/pixmapbinding.cpp'
--- plugins/composite/src/pixmapbinding/src/pixmapbinding.cpp	2012-05-31 07:15:37 +0000
+++ plugins/composite/src/pixmapbinding/src/pixmapbinding.cpp	2012-06-22 05:29:36 +0000
@@ -33,6 +33,7 @@
 PixmapBinding::PixmapBinding (const NewPixmapReadyCallback &cb,
 				WindowPixmapGetInterface *pmg,
 				WindowAttributesGetInterface *wag,
+				PixmapFreezerInterface       *pf,
 				ServerGrabInterface *sg) :
     mPixmap (),
     mSize (),
@@ -41,6 +42,7 @@
     newPixmapReadyCallback (cb),
     windowPixmapRetreiver (pmg),
     windowAttributesRetreiver (wag),
+    pixmapFreezer (pf),
     serverGrab (sg)
 {
 }
@@ -124,7 +126,8 @@
 void
 PixmapBinding::release ()
 {
-    needsRebind = true;
+    if (!pixmapFreezer->frozen ())
+	needsRebind = true;
 }
 
 void

=== modified file 'plugins/composite/src/pixmapbinding/tests/test-composite-pixmapbinding.cpp'
--- plugins/composite/src/pixmapbinding/tests/test-composite-pixmapbinding.cpp	2012-06-01 07:49:55 +0000
+++ plugins/composite/src/pixmapbinding/tests/test-composite-pixmapbinding.cpp	2012-06-22 05:29:36 +0000
@@ -41,6 +41,14 @@
 {
 };
 
+class MockPixmapFreezer :
+    public PixmapFreezerInterface
+{
+    public:
+
+	MOCK_METHOD0 (frozen, bool ());
+};
+
 class MockWindowPixmapGet :
     public WindowPixmapGetInterface
 {
@@ -136,6 +144,7 @@
     MockWindowPixmapGet mwpg;
     MockWindowAttributesGet mwag;
     MockServerGrab          msg;
+    MockPixmapFreezer       mpf;
     XWindowAttributes xwa;
 
     xwa.width = 100;
@@ -152,6 +161,7 @@
     PixmapBinding pr (readyCb,
 			&mwpg,
 			&mwag,
+			&mpf,
 			&msg);
 
     EXPECT_CALL (msg, grabServer ());
@@ -181,6 +191,7 @@
     MockWindowPixmapGet mwpg;
     MockWindowAttributesGet mwag;
     MockServerGrab          msg;
+    MockPixmapFreezer       mpf;
     XWindowAttributes xwa;
 
     xwa.width = 100;
@@ -197,6 +208,7 @@
     PixmapBinding pr (readyCb,
 			&mwpg,
 			&mwag,
+			&mpf,
 			&msg);
 
     EXPECT_CALL (msg, grabServer ());
@@ -227,54 +239,114 @@
     MockWindowPixmapGet mwpg;
     MockWindowAttributesGet mwag;
     MockServerGrab          msg;
-    XWindowAttributes xwa;
-
-    xwa.width = 100;
-    xwa.height = 200;
-    xwa.map_state = IsViewable;
-    xwa.border_width = 1;
-
-    FakeWindowAttributesGet fwag (xwa);
-
-    MockPixmapReady ready;
-
-    boost::function <void ()> readyCb (boost::bind (&PixmapReadyInterface::ready, &ready));
-
-    PixmapBinding pr (readyCb,
-			&mwpg,
-			&mwag,
-			&msg);
-
-    EXPECT_CALL (msg, grabServer ());
-    EXPECT_CALL (msg, syncServer ()).Times (2);
-    EXPECT_CALL (mwag, getAttributes (_)).WillOnce (Invoke (&fwag, &FakeWindowAttributesGet::getAttributes));
-    EXPECT_CALL (mwpg, getPixmap ()).WillOnce (Return (boost::shared_static_cast <WindowPixmapInterface> (wp)));
-
-    EXPECT_CALL (*wp, pixmap ()).WillOnce (Return (1));
-
-    EXPECT_CALL (ready, ready ());
-    EXPECT_CALL (msg, ungrabServer ());
-
-    EXPECT_TRUE (pr.bind ());
-
-    EXPECT_CALL (*wp, pixmap ()).WillOnce (Return (1));
-
-    EXPECT_EQ (pr.pixmap (), 1);
-    EXPECT_EQ (pr.size (), CompSize (102, 202));
-
-    EXPECT_CALL (*wp, releasePixmap ());
-
-    pr.release ();
-
-    EXPECT_CALL (msg, grabServer ());
-    EXPECT_CALL (msg, syncServer ()).Times (2);
-    EXPECT_CALL (mwag, getAttributes (_)).WillOnce (Invoke (&fwag, &FakeWindowAttributesGet::getAttributes));
-    EXPECT_CALL (mwpg, getPixmap ()).WillOnce (Return (boost::shared_static_cast <WindowPixmapInterface> (wp)));
-
-    EXPECT_CALL (*wp, pixmap ()).WillOnce (Return (1));
-
-    EXPECT_CALL (ready, ready ());
-    EXPECT_CALL (msg, ungrabServer ());
+    MockPixmapFreezer       mpf;
+    XWindowAttributes xwa;
+
+    xwa.width = 100;
+    xwa.height = 200;
+    xwa.map_state = IsViewable;
+    xwa.border_width = 1;
+
+    FakeWindowAttributesGet fwag (xwa);
+
+    MockPixmapReady ready;
+
+    boost::function <void ()> readyCb (boost::bind (&PixmapReadyInterface::ready, &ready));
+
+    PixmapBinding pr (readyCb,
+			&mwpg,
+			&mwag,
+			&mpf,
+			&msg);
+
+    EXPECT_CALL (msg, grabServer ());
+    EXPECT_CALL (msg, syncServer ()).Times (2);
+    EXPECT_CALL (mwag, getAttributes (_)).WillOnce (Invoke (&fwag, &FakeWindowAttributesGet::getAttributes));
+    EXPECT_CALL (mwpg, getPixmap ()).WillOnce (Return (boost::shared_static_cast <WindowPixmapInterface> (wp)));
+
+    EXPECT_CALL (*wp, pixmap ()).WillOnce (Return (1));
+
+    EXPECT_CALL (ready, ready ());
+    EXPECT_CALL (msg, ungrabServer ());
+
+    EXPECT_TRUE (pr.bind ());
+
+    EXPECT_CALL (mpf, frozen ()).WillOnce (Return (false));
+    EXPECT_CALL (*wp, pixmap ()).WillOnce (Return (1));
+
+    EXPECT_EQ (pr.pixmap (), 1);
+    EXPECT_EQ (pr.size (), CompSize (102, 202));
+
+    EXPECT_CALL (*wp, releasePixmap ());
+
+    pr.release ();
+
+    EXPECT_CALL (msg, grabServer ());
+    EXPECT_CALL (msg, syncServer ()).Times (2);
+    EXPECT_CALL (mwag, getAttributes (_)).WillOnce (Invoke (&fwag, &FakeWindowAttributesGet::getAttributes));
+    EXPECT_CALL (mwpg, getPixmap ()).WillOnce (Return (boost::shared_static_cast <WindowPixmapInterface> (wp)));
+
+    EXPECT_CALL (*wp, pixmap ()).WillOnce (Return (1));
+
+    EXPECT_CALL (ready, ready ());
+    EXPECT_CALL (msg, ungrabServer ());
+
+    EXPECT_TRUE (pr.bind ());
+
+    EXPECT_CALL (*wp, pixmap ()).WillOnce (Return (1));
+
+    EXPECT_EQ (pr.pixmap (), 1);
+    EXPECT_EQ (pr.size (), CompSize (102, 202));
+
+    EXPECT_CALL (*wp, releasePixmap ());
+}
+
+TEST(CompositePixmapBinderTest, TestNoRebindAfterReleaseWhenFrozen)
+{
+    MockPixmap::Ptr wp (boost::make_shared <MockPixmap> ());
+
+    MockWindowPixmapGet mwpg;
+    MockWindowAttributesGet mwag;
+    MockServerGrab          msg;
+    MockPixmapFreezer       mpf;
+    XWindowAttributes xwa;
+
+    xwa.width = 100;
+    xwa.height = 200;
+    xwa.map_state = IsViewable;
+    xwa.border_width = 1;
+
+    FakeWindowAttributesGet fwag (xwa);
+
+    MockPixmapReady ready;
+
+    boost::function <void ()> readyCb (boost::bind (&PixmapReadyInterface::ready, &ready));
+
+    PixmapBinding pr (readyCb,
+			&mwpg,
+			&mwag,
+			&mpf,
+			&msg);
+
+    EXPECT_CALL (msg, grabServer ());
+    EXPECT_CALL (msg, syncServer ()).Times (2);
+    EXPECT_CALL (mwag, getAttributes (_)).WillOnce (Invoke (&fwag, &FakeWindowAttributesGet::getAttributes));
+    EXPECT_CALL (mwpg, getPixmap ()).WillOnce (Return (boost::shared_static_cast <WindowPixmapInterface> (wp)));
+
+    EXPECT_CALL (*wp, pixmap ()).WillOnce (Return (1));
+
+    EXPECT_CALL (ready, ready ());
+    EXPECT_CALL (msg, ungrabServer ());
+
+    EXPECT_TRUE (pr.bind ());
+
+    EXPECT_CALL (mpf, frozen ()).WillOnce (Return (true));
+    EXPECT_CALL (*wp, pixmap ()).WillOnce (Return (1));
+
+    EXPECT_EQ (pr.pixmap (), 1);
+    EXPECT_EQ (pr.size (), CompSize (102, 202));
+
+    pr.release ();
 
     EXPECT_TRUE (pr.bind ());
 
@@ -291,6 +363,7 @@
     MockWindowPixmapGet mwpg;
     MockWindowAttributesGet mwag;
     MockServerGrab          msg;
+    MockPixmapFreezer       mpf;
     XWindowAttributes xwa;
 
     xwa.width = 100;
@@ -303,6 +376,7 @@
     PixmapBinding pr (boost::function <void ()> (),
 			&mwpg,
 			&mwag,
+			&mpf,
 			&msg);
 
     EXPECT_CALL (msg, grabServer ());
@@ -322,6 +396,7 @@
     MockWindowPixmapGet mwpg;
     MockWindowAttributesGet mwag;
     MockServerGrab          msg;
+    MockPixmapFreezer       mpf;
     XWindowAttributes xwa;
 
     xwa.width = 0;
@@ -334,6 +409,7 @@
     PixmapBinding pr (boost::function <void ()> (),
 			&mwpg,
 			&mwag,
+			&mpf,
 			&msg);
 
     EXPECT_CALL (msg, grabServer ());
@@ -355,6 +431,7 @@
     MockWindowPixmapGet mwpg;
     MockWindowAttributesGet mwag;
     MockServerGrab          msg;
+    MockPixmapFreezer       mpf;
     XWindowAttributes xwa;
 
     xwa.width = 100;
@@ -367,6 +444,7 @@
     PixmapBinding pr (boost::function <void ()> (),
 			&mwpg,
 			&mwag,
+			&mpf,
 			&msg);
 
     EXPECT_CALL (msg, grabServer ());
@@ -391,6 +469,7 @@
     MockWindowPixmapGet mwpg;
     MockWindowAttributesGet mwag;
     MockServerGrab          msg;
+    MockPixmapFreezer       mpf;
     XWindowAttributes xwa;
 
     xwa.width = 100;
@@ -403,6 +482,7 @@
     PixmapBinding pr (boost::function <void ()> (),
 			&mwpg,
 			&mwag,
+			&mpf,
 			&msg);
 
     EXPECT_CALL (msg, grabServer ());

=== modified file 'plugins/composite/src/privates.h'
--- plugins/composite/src/privates.h	2012-06-06 05:12:28 +0000
+++ plugins/composite/src/privates.h	2012-06-22 05:29:36 +0000
@@ -116,7 +116,8 @@
     public WindowInterface,
     public CompositePixmapRebindInterface,
     public WindowPixmapGetInterface,
-    public WindowAttributesGetInterface
+    public WindowAttributesGetInterface,
+    public PixmapFreezerInterface
 {
     public:
 	PrivateCompositeWindow (CompWindow *w, CompositeWindow *cw);
@@ -132,6 +133,7 @@
 	void release ();
 	void setNewPixmapReadyCallback (const boost::function <void ()> &);
 	void allowFurtherRebindAttempts ();
+	bool frozen ();
 
 	static void handleDamageRect (CompositeWindow *w,
 				      int             x,
@@ -159,6 +161,7 @@
 	XRectangle *damageRects;
 	int        sizeDamage;
 	int        nDamage;
+
     private:
 
 	bool getAttributes (XWindowAttributes &);

=== modified file 'plugins/composite/src/window.cpp'
--- plugins/composite/src/window.cpp	2012-06-06 05:12:28 +0000
+++ plugins/composite/src/window.cpp	2012-06-22 05:29:36 +0000
@@ -108,6 +108,7 @@
     mPixmapBinding (boost::function <void ()> (),
 		     this,
 		     this,
+		     this,
 		     screen->serverGrabInterface ()),
     damage (None),
     damaged (false),
@@ -184,6 +185,21 @@
     return false;
 }
 
+bool
+PrivateCompositeWindow::frozen ()
+{
+    /* keep old pixmap for windows that are unmapped on the client side,
+     * but not yet on our side as it's pretty likely that plugins are
+     * currently using it for animations
+     */
+
+    bool pendingUnmap = !window->mapNum () && window->isViewable ();
+    bool hidden = window->state () & CompWindowStateHiddenMask;
+    bool animated = window->hasUnmapReference ();
+
+    return (pendingUnmap || hidden) && animated;
+}
+
 Pixmap
 CompositeWindow::pixmap ()
 {
@@ -260,6 +276,12 @@
     return priv->overlayWindow;
 }
 
+bool
+CompositeWindow::frozen ()
+{
+    return priv->frozen ();
+}
+
 void
 CompositeWindow::damageTransformedRect (float          xScale,
 					float          yScale,
@@ -605,19 +627,7 @@
 	cScreen->damageRegion (CompRegion (CompRect (x1, y1, x2 - x1, y2 - y1)));
     }
 
-    if (((!window->mapNum () && window->isViewable ()) ||
-	   window->state () & CompWindowStateHiddenMask) && window->hasUnmapReference ())
-    {
-       /* keep old pixmap for windows that are unmapped on the client side,
-	* but not yet on our side as it's pretty likely that plugins are
-	* currently using it for animations
-	*/
-    }
-    else
-    {
-	cWindow->release ();
-    }
-
+    cWindow->release ();
     cWindow->addDamage ();
 }
 

=== modified file 'plugins/opengl/src/window.cpp'
--- plugins/opengl/src/window.cpp	2012-05-26 02:48:05 +0000
+++ plugins/opengl/src/window.cpp	2012-06-22 05:29:36 +0000
@@ -104,37 +104,42 @@
 bool
 GLWindow::bind ()
 {
-    if ((priv->needsRebind && !priv->cWindow->bind ()))
+    if (priv->needsRebind)
     {
-	if (!priv->textures.empty ())
-	{
-	    /* Getting a new pixmap failed, recycle the old texture */
+	if (!priv->cWindow->bind ())
+	{
+	    if (!priv->textures.empty ())
+	    {
+		/* Getting a new pixmap failed, recycle the old texture */
+		priv->needsRebind = false;
+		return true;
+	    }
+	    else
+		return false;
+	}
+
+	GLTexture::List textures =
+	    GLTexture::bindPixmapToTexture (priv->cWindow->pixmap (),
+					    priv->cWindow->size ().width (),
+					    priv->cWindow->size ().height (),
+					    priv->window->depth ());
+	if (textures.empty ())
+	{
+	    compLogMessage ("opengl", CompLogLevelInfo,
+			    "Couldn't bind redirected window 0x%x to "
+			    "texture\n", (int) priv->window->id ());
+	    return false;
+	}
+	else
+	{
+	    priv->textures = textures;
 	    priv->needsRebind = false;
-	    return true;
 	}
-	else
-	    return false;
-    }
-
-    GLTexture::List textures =
-	GLTexture::bindPixmapToTexture (priv->cWindow->pixmap (),
-					priv->cWindow->size ().width (),
-					priv->cWindow->size ().height (),
-					priv->window->depth ());
-    if (textures.empty ())
-    {
-	compLogMessage ("opengl", CompLogLevelInfo,
-			"Couldn't bind redirected window 0x%x to "
-			"texture\n", (int) priv->window->id ());
-	return false;
-    }
-    else
-    {
-	priv->textures = textures;
-	priv->needsRebind = false;
-    }
-
-    priv->updateState |= PrivateGLWindow::UpdateRegion | PrivateGLWindow::UpdateMatrix;
+
+	priv->updateState |= PrivateGLWindow::UpdateRegion | PrivateGLWindow::UpdateMatrix;
+
+	return true;
+    }
 
     return true;
 }
@@ -142,7 +147,8 @@
 void
 GLWindow::release ()
 {
-    priv->needsRebind = true;
+    if (!priv->cWindow->frozen ())
+	priv->needsRebind = true;
 }
 
 bool
@@ -202,8 +208,7 @@
 {
     window->resizeNotify (dx, dy, dwidth, dheight);
     updateState |= PrivateGLWindow::UpdateMatrix | PrivateGLWindow::UpdateRegion;
-    if (!window->hasUnmapReference ())
-	gWindow->release ();
+    gWindow->release ();
 }
 
 void
@@ -225,8 +230,7 @@
 	case CompWindowNotifyReparent:
 	case CompWindowNotifyUnreparent:
 	case CompWindowNotifyFrameUpdate:
-	    if (!window->hasUnmapReference ())
-		gWindow->release ();
+	    gWindow->release ();
 	    break;
 	default:
 	    break;

=== modified file 'src/window.cpp'
--- src/window.cpp	2012-05-27 04:32:55 +0000
+++ src/window.cpp	2012-06-22 05:29:36 +0000
@@ -1403,11 +1403,11 @@
 void
 CompWindow::unmap ()
 {
+    if (priv->mapNum)
+	priv->mapNum = 0;
+
     windowNotify (CompWindowNotifyBeforeUnmap);
 
-    if (priv->mapNum)
-	priv->mapNum = 0;
-
     /* Even though we're still keeping the backing
      * pixmap of the window around, it's safe to
      * unmap the frame window since there's no use
@@ -1461,9 +1461,7 @@
     priv->invisible = true;
 
     if (priv->shaded)
-    {
 	priv->updateFrameWindow ();
-    }
 
     screen->updateClientList ();