Merge lp:~smspillaz/compiz-core/compiz-core.fix_911530 into lp:compiz-core/0.9.5

[Sam Spilsbury]

PrivateGLScreen::paintBackground(): If background textures are empty,
fix the vertex array pointer to use the beginning of the vertex data
rather than (data + 2). Fix data allocation so only the necessary number
of vertices is allocated. Disable the unspecified texture coordinate
array in this case, since leaving it enabled may cause the GL driver to
segfault when attempting to dereference it.

[Daniel van Vugt (vanvugt)]

Duplicating the data allocation in 2 places not only makes the code bigger, but makes it harder to maintain (and potentially more buggy) in future. I suggest it should look more like:

    bool bgEmpty = backgroundTextures.empty ();
    int dataLen = nBox * (bgEmpty ? 8 : 16);
    data = new GLfloat [dataLen];
    if (!data)
        return;

    d = data;
    n = nBox;

    if (bgEmpty)

This should make the entire diff much smaller.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

FYI, the main fix for bug 911530, "fix the vertex array pointer to use the beginning of the vertex data
rather than (data + 2)" is not part of this proposal at all (see line 33). Although that bug is still present in oneiric, it was apparently fixed a long time ago already in compiz-core.

[Sam Spilsbury (smspillaz)]

    data = new GLfloat [dataLen];
    if (!data)
        return;

Passing a variable to array new [] I've found has resulted in std::bad_alloc exceptions, even when the length specified was completely valid. This happened to be about a year ago the last time I tried, so it could have been a compiler bug.

Doing the allocation in two places also struck me as a bit odd at first as well in the patch that was submitted.

[Alan Griffiths (alan-griffiths)]

I'd rather see data being of type boost::scoped_array<char> - avoiding the need for explicit deletes.

There's no need to check the return from new against 0 as operator new[] will throw bad_alloc if the allocation fails. So this code:

    data = new GLfloat [nBox * 8];
    if (!data)
        return;
...
    delete [] data;

becomes:

    data.reset(new GLfloat [nBox * 8]);

Note, the code currently only invokes C functions (which don't throw) so the there is no exception safety issue.

[Daniel van Vugt (vanvugt)]

This proposal is already present upstream in git. So whether or not we accept it here really depends on what the plan is for compiz in the future. Is git still upstream or is lp:compiz-core the main repo now?

[Daniel van Vugt (vanvugt)]

review: Needs Information

[lp:~smspillaz/compiz-core/compiz-core.fix_911530]

2899. By Sam Spilsbury on 2012-01-19

Use boost::scoped_array

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

=== modified file 'plugins/opengl/src/paint.cpp'
--- plugins/opengl/src/paint.cpp	2012-01-19 06:08:11 +0000
+++ plugins/opengl/src/paint.cpp	2012-01-19 14:56:54 +0000
@@ -33,6 +33,7 @@
 #include <math.h>
 
 #include <boost/foreach.hpp>
+#include <boost/scoped_array.hpp>
 #define foreach BOOST_FOREACH
 
 #include <opengl/opengl.h>
@@ -70,7 +71,9 @@
 {
     BoxPtr    pBox = const_cast <Region> (region.handle ())->rects;
     int	      n, nBox = const_cast <Region> (region.handle ())->numRects;
-    GLfloat   *d, *data;
+    GLfloat   *d;
+
+    boost::scoped_array <GLfloat> data;
 
     if (!nBox)
 	return;
@@ -94,15 +97,13 @@
 	backgroundLoaded = true;
     }
 
-    data = new GLfloat [nBox * 16];
-    if (!data)
-	return;
-
-    d = data;
-    n = nBox;
-
     if (backgroundTextures.empty ())
     {
+	data.reset (new GLfloat [nBox * 8]);
+
+	d = data.get ();
+	n = nBox;
+
 	while (n--)
 	{
 	    *d++ = pBox->x1;
@@ -120,14 +121,23 @@
 	    pBox++;
 	}
 
-	glVertexPointer (2, GL_FLOAT, sizeof (GLfloat) * 2, data);
+	glDisableClientState (GL_TEXTURE_COORD_ARRAY);
+
+	glVertexPointer (2, GL_FLOAT, sizeof (GLfloat) * 2, &data[0]);
 
 	glColor4us (0, 0, 0, std::numeric_limits<unsigned short>::max ());
 	glDrawArrays (GL_QUADS, 0, nBox * 4);
 	glColor4usv (defaultColor);
+
+	glEnableClientState (GL_TEXTURE_COORD_ARRAY);
     }
     else
     {
+	data.reset (new GLfloat [nBox * 16]);
+
+	d = data.get ();
+	n = nBox;
+
 	for (unsigned int i = 0; i < backgroundTextures.size (); i++)
 	{
 	    GLTexture *bg = backgroundTextures[i];
@@ -135,7 +145,7 @@
 
 	    pBox = const_cast <Region> (r.handle ())->rects;
 	    nBox = const_cast <Region> (r.handle ())->numRects;
-	    d = data;
+	    d = data.get ();
 	    n = nBox;
 
 	    while (n--)
@@ -167,8 +177,8 @@
 		pBox++;
 	    }
 
-	    glTexCoordPointer (2, GL_FLOAT, sizeof (GLfloat) * 4, data);
-	    glVertexPointer (2, GL_FLOAT, sizeof (GLfloat) * 4, data + 2);
+	    glTexCoordPointer (2, GL_FLOAT, sizeof (GLfloat) * 4, &data[0]);
+	    glVertexPointer (2, GL_FLOAT, sizeof (GLfloat) * 4, &data[2]);
 
 	    if (bg->name ())
 	    {
@@ -183,8 +193,6 @@
 	    }
 	}
     }
-
-    delete [] data;
 }