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

[Sam Spilsbury]

Fix broken mipmap support. Mipmap support was broken in the following ways:

1. glGenerateMipmapEXT is called multiple times for tfp textures
2. glGenerateMipmapEXT is called after glTexParameteri with GL_LINEAR_MIPMAP_LINEAR as the filter mode, and sometimes not called at all, which is undefined
3. We check for GL::fbo support before actually setting it from the extension lookup after glXCreateContext is called. As such, when we lookup an optimal pixmap fbconfig, we never find one that supports mipmaps, and it is merely "luck of the draw" as to whether or not we actually get one that does
4. We generate mipmaps for tfp textures even though mipmaps aren't actually supported on the config target, because we don't check priv->mipmapSupport

This generally results in white windows when mipmapping is enabled. See (LP: #1016364)

As such the following measures were implemented:
1. Virtual method to generate a mipmap, different for tfp textures as
   mipmap generation has to happen on damage there
2. Always generate mipmaps before setting texture filter
3. Do not check for GL::fbo support when checking fbconfigs for mimap support
4. Use GLTexture::mipmap () to determine if we actually can do mipmaps - this
   checks if the fbconfig supports mipmaps, that the texture is not
   an NV_TEXTURE_RECTANGLE_EXT, checks for fbo support and NPOT texture
   support

== Problem ==

1. glGenerateMipmapEXT is called multiple times for tfp textures
2. glGenerateMipmapEXT is called after glTexParameteri with GL_LINEAR_MIPMAP_LINEAR as the filter mode, and sometimes not called at all, which is undefined
3. We check for GL::fbo support before actually setting it from the extension lookup after glXCreateContext is called. As such, when we lookup an optimal pixmap fbconfig, we never find one that supports mipmaps, and it is merely "luck of the draw" as to whether or not we actually get one that does
4. We generate mipmaps for tfp textures even though mipmaps aren't actually supported on the config target, because we don't check priv->mipmapSupport

This generally results in white windows when mipmapping is enabled. See (LP: #1016364)

== Solution ==

1. Virtual method to generate a mipmap, different for tfp textures as
   mipmap generation has to happen on damage there
2. Always generate mipmaps before setting texture filter
3. Do not check for GL::fbo support when checking fbconfigs for mimap support
4. Use GLTexture::mipmap () to determine if we actually can do mipmaps - this
   checks if the fbconfig supports mipmaps, that the texture is not
   an NV_TEXTURE_RECTANGLE_EXT, checks for fbo support and NPOT texture
   support

== Test ==

None yet, will add autotests when appropriate to do so (eg, after this fix lands or shortly after, since its a higher priority than normal).

Can be verified at least by turning on mipmap support and observing that scaled down windows don't go white.

[Daniel van Vugt (vanvugt)]

Corrected bug IDs.

[Daniel van Vugt (vanvugt)]

Mipmaps don't work at all any more!

Try switcher (Alt+Tab) or enable mipmaps in expo. In both cases, mipmaps don't work at all any more using this branch. They do however work using trunk. Tested with intel.

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

That's suprising, it works fine here (nouveau + nvidia).

I don't think intel ever supported mipmapping on tfp textures. You can check by reading the value of config->mipmap in GLTexture::bindPixmapToTexture.

What might be different is that it maybe doesn't fall back to bilinear filtering when mipmaps are not available. Can you check that in GLTexture::enable ? (I don't have any intel hardware to test on, and won't for another month)

[Sam Spilsbury (smspillaz)]

Oh weird, it isn't working for me anymore either!

That's very strange, I'll need to give it a closer look, since I know they were working for sure when I wrote this.

[Sam Spilsbury (smspillaz)]

Did some more investigation into this.

It seems like at least on nouveau, mipmap support is not to be found in any of the available fbconfigs:

found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0
found fbconfig with mipmap: 0

Here is an inline patch to get a similar kind of output.

=== modified file 'plugins/opengl/src/screen.cpp'
--- plugins/opengl/src/screen.cpp 2012-06-22 04:32:12 +0000
+++ plugins/opengl/src/screen.cpp 2012-06-26 12:09:05 +0000
@@ -523,6 +523,8 @@

      mipmap = value;

+ printf ("found fbconfig with mipmap: %i\n", mipmap);
+
      (*GL::getFBConfigAttrib) (dpy, fbConfigs[j],
           GLX_Y_INVERTED_EXT, &value);

=== modified file 'plugins/opengl/src/texture.cpp'
--- plugins/opengl/src/texture.cpp 2012-06-22 04:32:12 +0000
+++ plugins/opengl/src/texture.cpp 2012-06-26 12:06:09 +0000
@@ -150,6 +150,11 @@
 bool
 GLTexture::mipmap () const
 {
+ printf ("mipmap: %i support: %i npot %i fb %i\n", priv->mipmap,
+ priv->mipmapSupport,
+ GL::textureNonPowerOfTwo,
+ GL::fbo);
+
     return priv->mipmap &&
     priv->mipmapSupport &&
     GL::textureNonPowerOfTwo &&
@@ -202,6 +207,7 @@
  {
      if (mipmap ())
      {
+ printf ("use mipmaps\n");
   generateMipmap ();
   glTexParameteri (priv->target,
      GL_TEXTURE_MIN_FILTER,
@@ -327,6 +333,8 @@
  mipmap = false;
     }

+ printf ("bind regular texture: mipmap support: %i\n", mipmap);
+
     t->setData (target, matrix, mipmap);
     t->setGeometry (0, 0, width, height);

@@ -571,6 +579,8 @@
      return GLTexture::List ();
     }

+ printf ("bind tfp texture: mipmap support: %i fbconfig: %i\n", mipmap, config->mipmap);
+
     tex = new TfpTexture ();
     tex->setData (texTarget, matrix, mipmap);
     tex->setGeometry (0, 0, width, height);

If it also gives you a similar output, then the driver is lying about its support for mipmaps, saying they aren't supported when they are, in which case we will have to add a workaround to the workarounds plugin to force mipmaps even if the driver says it doesn't support them on the glxPixmap fbconfigs.

I don't have time to try it on nvidia right now (the sun is rising :( ), but I am pretty certain it worked there. In any case, we should always check the driver to see if it supports mipmaps, drivers have probably b...

[Sam Spilsbury (smspillaz)]

fbconfigm: 1
fbconfigm: 1
fbconfigm: 1
fbconfigm: 1
fbconfigm: 1
fbconfigm: 1
fbconfigm: 1
fbconfigm: 1
fbconfigm: 1
fbconfigm: 1
fbconfigm: 1
fbconfigm: 1
fbconfigm: 1

Yeah, nvidia says it supports mipmapped glx pixmaps bound to textures.

I call driver bug in mesa. Should we add a workaround, or just leave it disabled where its not supposed to be supported.

[Sam Spilsbury (smspillaz)]

First is with mipmapping, second is without (nvidia):

http://imgur.com/abbZQ,LS9wu

[Daniel van Vugt (vanvugt)]

Sorry, still broken.

Using trunk I get nice bilinear-filtered mipmaps. Using this branch, I get no mipmaps (and hence no bilinear filtering).

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

> Sorry, still broken.
>
> Using trunk I get nice bilinear-filtered mipmaps. Using this branch, I get no
> mipmaps (and hence no bilinear filtering).

You actually still get bilinear filtering (or at least should ... I can double check). Note that you don't actually need mipmapping support to get bilinear filtering, you only need it for /trilinear/ filtering (eg GL_LINEAR_MIPMAP_LINEAR).

Like I just explained, there's a driver bug at work here (spoke to RAOF about it yesterday). That leaves us with three options, and I'd like some guidance as to which one to take.

a) Fix mesa to correctly report whether it supports mipmapping on glx pixmap configs (should take me 2-3 days)
b) Add a workaround to force mipmapping even if the driver doesn't say it supports it
c) "revert" the code that detects whether mipmapping is supported correctly and truly break other drivers where mipmapping really isn't supported.
c)

[Daniel van Vugt (vanvugt)]

Add a workaround, methinks.

[Sam Spilsbury (smspillaz)]

I am moving this to WIP for now. There are several things which we will need to do independently for this to work correctly because of the driver bug in mesa:

1) We need to add a framework to the workarounds plugin to detect which driver you are running and apply workarounds based on that
2) We need to re-use the fbconfig choosing logic to override the core fbconfigs, which means that we need to abstract that code and get it under test
3) As part of 2), we've resolved to move to glXChooseFBConfig to simplify the code, that's another big chunk of work.

review: Needs Resubmitting

[Sam Spilsbury (smspillaz)]

Resubmit, because its WIP and after our call today, we have far more pressing things.

review: Needs Resubmitting

[MC Return (mc-return)]

Is this still a valid fix ?

Unmerged revisions

3257. By Sam Spilsbury on 2012-06-22

Fix broken mipmap support. Mipmap support was broken in the following ways:

1. glGenerateMipmapEXT is called multiple times for tfp textures
2. glGenerateMipmapEXT is called after glTexParameteri with GL_LINEAR_MIPMAP_LINEAR as the filter mode, and sometimes not called at all, which is undefined
3. We check for GL::fbo support before actually setting it from the extension lookup after glXCreateContext is called. As such, when we lookup an optimal pixmap fbconfig, we never find one that supports mipmaps, and it is merely "luck of the draw" as to whether or not we actually get one that does
4. We generate mipmaps for tfp textures even though mipmaps aren't actually supported on the config target, because we don't check priv->mipmapSupport

This generally results in white windows when mipmapping is enabled. See (LP: #1006216)

As such the following measures were implemented:
1. Virtual method to generate a mipmap, different for tfp textures as
   mipmap generation has to happen on damage there
2. Always generate mipmaps before setting texture filter
3. Do not check for GL::fbo support when checking fbconfigs for mimap support
4. Use GLTexture::mipmap () to determine if we actually can do mipmaps - this
   checks if the fbconfig supports mipmaps, that the texture is not
   an NV_TEXTURE_RECTANGLE_EXT, checks for fbo support and NPOT texture
   support

(LP: #1016364)

=== modified file 'plugins/opengl/include/opengl/opengl.h'
--- plugins/opengl/include/opengl/opengl.h	2012-01-20 09:05:56 +0000
+++ plugins/opengl/include/opengl/opengl.h	2012-06-22 05:27:21 +0000
@@ -35,7 +35,7 @@
 #include <opengl/texture.h>
 #include <opengl/fragment.h>
 
-#define COMPIZ_OPENGL_ABI 4
+#define COMPIZ_OPENGL_ABI 5
 
 #include <core/pluginclasshandler.h>
 

=== modified file 'plugins/opengl/include/opengl/texture.h'
--- plugins/opengl/include/opengl/texture.h	2012-01-18 16:26:45 +0000
+++ plugins/opengl/include/opengl/texture.h	2012-06-22 05:27:21 +0000
@@ -141,6 +141,11 @@
 	virtual void disable ();
 
 	/**
+	 * Generates mipmap levels
+	 */
+	virtual bool generateMipmap ();
+
+	/**
 	 * Returns true if this texture is MipMapped
 	 */
 	bool mipmap () const;

=== modified file 'plugins/opengl/src/privatetexture.h'
--- plugins/opengl/src/privatetexture.h	2012-01-18 16:26:45 +0000
+++ plugins/opengl/src/privatetexture.h	2012-06-22 05:27:21 +0000
@@ -68,6 +68,7 @@
 	~TfpTexture ();
 
 	void enable (Filter filter);
+	bool generateMipmap ();
 
 	static List bindPixmapToTexture (Pixmap pixmap,
 					 int width,

=== modified file 'plugins/opengl/src/screen.cpp'
--- plugins/opengl/src/screen.cpp	2012-04-20 07:35:20 +0000
+++ plugins/opengl/src/screen.cpp	2012-06-22 05:27:21 +0000
@@ -515,16 +515,13 @@
 
 	    depth = value;
 
-	    if (GL::fbo)
-	    {
-		(*GL::getFBConfigAttrib) (dpy, fbConfigs[j],
-					  GLX_BIND_TO_MIPMAP_TEXTURE_EXT,
-					  &value);
-		if (value < mipmap)
-		    continue;
+	    (*GL::getFBConfigAttrib) (dpy, fbConfigs[j],
+				      GLX_BIND_TO_MIPMAP_TEXTURE_EXT,
+				      &value);
+	    if (value < mipmap)
+		continue;
 
-		mipmap = value;
-	    }
+	    mipmap = value;
 
 	    (*GL::getFBConfigAttrib) (dpy, fbConfigs[j],
 				      GLX_Y_INVERTED_EXT, &value);
@@ -551,6 +548,7 @@
 			"this isn't going to work.");
 	screen->handleCompizEvent ("opengl", "fatal_fallback", o);
 	setFailed ();
+	return;
     }
 
     if (!glInitContext (visinfo))

=== modified file 'plugins/opengl/src/texture.cpp'
--- plugins/opengl/src/texture.cpp	2012-06-05 09:31:34 +0000
+++ plugins/opengl/src/texture.cpp	2012-06-22 05:27:21 +0000
@@ -150,7 +150,10 @@
 bool
 GLTexture::mipmap () const
 {
-    return priv->mipmap & priv->mipmapSupport;
+    return priv->mipmap &&
+	   priv->mipmapSupport &&
+	   GL::textureNonPowerOfTwo &&
+	   GL::fbo;
 }
 
 GLenum
@@ -159,6 +162,19 @@
     return priv->filter;
 }
 
+bool
+GLTexture::generateMipmap ()
+{
+    if (priv->initial)
+    {
+	(*GL::generateMipmap) (priv->target);
+	priv->initial = false;
+	return true;
+    }
+
+    return false;
+}
+
 void
 GLTexture::enable (GLTexture::Filter filter)
 {
@@ -184,8 +200,9 @@
     {
 	if (gs->textureFilter () == GL_LINEAR_MIPMAP_LINEAR)
 	{
-	    if (GL::textureNonPowerOfTwo && GL::fbo && priv->mipmap)
+	    if (mipmap ())
 	    {
+		generateMipmap ();
 		glTexParameteri (priv->target,
 				 GL_TEXTURE_MIN_FILTER,
 				 GL_LINEAR_MIPMAP_LINEAR);
@@ -221,15 +238,6 @@
 	    priv->filter = gs->textureFilter ();
 	}
     }
-
-    if (priv->filter == GL_LINEAR_MIPMAP_LINEAR)
-    {
-	if (priv->initial)
-	{
-	    (*GL::generateMipmap) (priv->target);
-	    priv->initial = false;
-	}
-    }
 }
 
 void
@@ -587,6 +595,20 @@
     return rv;
 }
 
+bool
+TfpTexture::generateMipmap ()
+{
+    if (updateMipMap)
+    {
+	(*GL::generateMipmap) (target ());
+	updateMipMap = false;
+
+	return true;
+    }
+
+    return false;
+}
+
 void
 TfpTexture::enable (GLTexture::Filter filter)
 {
@@ -604,10 +626,5 @@
     if (damaged)
 	updateMipMap = true;
 
-    if (this->filter () == GL_LINEAR_MIPMAP_LINEAR && updateMipMap)
-    {
-	(*GL::generateMipmap) (target ());
-	updateMipMap = false;
-    }
     damaged = false;
 }