Merge lp:~compiz-team/compiz/gles2.scope_of_empty_vb_check into lp:~compiz-linaro-team/compiz/gles2

[Sam Spilsbury]

Make checking to see if the vertex buffer is empty the responsibility of
GLWindow::glDraw and not the clients of that function.

While this is technically slower (incurred wrapped function overhead), it
is better encapsulation, since clients of glDraw less are coupled with the
implementation of glDraw.

Put the check for an empty vertex buffer inside of glDraw.

The more expensive part of calling glDraw so much is the associated calls to glDrawArrays with an empty buffer - we can effectively cull out this part by putting the check for an empty buffer inside of glDrawTexture. This decouples the "necessary optimization" from the clients of glDrawTexture.

While this is technically more expensive due to the extra wrapped function call overhead, most of that is mitigated by plugins doing the Right Thing [tm] and not registering a wrapped function for glDrawTexture.

I compared benchmarks through manual testing and there wasn't any noticable impact.

[Daniel van Vugt (vanvugt)]

There are still a lot of calculations we wish to avoid if number of vertices is zero. So I'd like to keep the if statements:
  if (gWindow->vertexBuffer ()->countVertices ()) { /* stuff to avoid */ }

I think we just want the 2-line check added to glDrawTexture. It's redundant but an extra layer of performance-safety.

So keep lines 86-87 and drop everything else?

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

I feel like in that case this abstraction is totally wrong. The clients shouldn't be dependent upon the implementation detail of GLVertexBuffer. Perhaps GLVertexBuffer::end should return the number of vertices added?

[Daniel van Vugt (vanvugt)]

I was thinking along the same lines recently. Maybe end() should return a bool indicating whether you can or should render().

[Sam Spilsbury (smspillaz)]

On Wed, 8 Aug 2012, Daniel van Vugt wrote:

> I was thinking along the same lines recently. Maybe end() should return a bool indicating whether you can or should render().

Okay, how about this:

1) end () returns a bool which is essentially just countVertices () > 0
2) We add a new method to GLWindow called regenerateVertices (same
signature as glAddGeometry)) which
returns bool. This calls its internal GLVertexBuffer::begin (),
glAddGeometry () and GLVertexBuffer::end () and returns the result.

This would put the current code in line with the law-of-demeter.

Thoughts?

> --
> https://code.launchpad.net/~compiz-team/compiz/gles2.scope_of_empty_vb_check/+merge/117058
> Your team Compiz Maintainers is subscribed to branch lp:~compiz-team/compiz/gles2.scope_of_empty_vb_check.
>

[Daniel van Vugt (vanvugt)]

I don't understand #2. What's regenerateVertices for?

[Sam Spilsbury (smspillaz)]

The problem is that the clients know too much about GLWindow (eg, that it has a vertex buffer) and furthermore, they are free to manipulate this internal state. It would be far simpler, and far less repetitious if we just had this:

void
GLWindow::clearVertices ()
{
    priv->vertexBuffer->begin ();
}

bool
GLWindow::saveVertices ()
{
    return priv->vertexBuffer->end ();
}

(in usage)

gWindow->clearVertices ();
gWindow->glAddGeometry (...);
if (gWindow->saveVertices ())
    gWindow->glDrawTexture (...);

Turns out we can't have a single wrapper method like I first thought, as multiple calls to glAddGeometry are permissible.

[Daniel van Vugt (vanvugt)]

Nice idea, but you'd still end up with lots of ugly plugin code which does:

gWindow->clearVertices();
gWindow->vertexBuffer()->blah();
gWindow->vertexBuffer()->blah2();
gWindow->vertexBuffer()->blah3();
gWindow->saveVertices();

We cannot get away from the fact that plugins need direct access to a GLVertexBuffer; GLWindow::vertexBuffer(). In the same way that they use GLWindow::Geometry on trunk; GLWindow::geometry().

However this is not really an issue at all. If you want the plugin code to be nicer, just write it nicer:

GLVertexBuffer *vb = gWindow->vertexBuffer();
vb->begin();
vb->blah();
vb->blah2();
vb->blah3();
vb->end();

[Sam Spilsbury (smspillaz)]

On Wed, 8 Aug 2012, Daniel van Vugt wrote:

> Nice idea, but you'd still end up with lots of ugly plugin code which does:
>
> gWindow->clearVertices();
> gWindow->vertexBuffer()->blah();
> gWindow->vertexBuffer()->blah2();
> gWindow->vertexBuffer()->blah3();
> gWindow->saveVertices();
>
> We cannot get away from the fact that plugins need direct access to a GLVertexBuffer; GLWindow::vertexBuffer(). In the same way that they use GLWindow::Geometry on trunk; GLWindow::geometry().
>
> However this is not really an issue at all. If you want the plugin code to be nicer, just write it nicer:
>
> GLVertexBuffer *vb = gWindow->vertexBuffer();
> vb->begin();
> vb->blah();
> vb->blah2();
> vb->blah3();
> vb->end();
>

In fact, I checked the code and the main reason why the plugins are using
gWindow->vertexBuffer () are to call begin () and end (). The only other
place where it is used otherwise are in the plugin glAddGeometry calls.

I'd like to remove it entirely, so I think what might be better is is
plugins call this:

void
GLWindow::addNewGeometry (... (same as glAddGeometry))
{
     glAddGeometry (priv->vertexBuffer, ...);
}

That way, the plugins that do need to add stuff to the vertex buffer get a
pointer to it when they need to do it. In all other instances the vertex
buffer is effectively protected (as it should be).

Thoughts?

> --
> https://code.launchpad.net/~compiz-team/compiz/gles2.scope_of_empty_vb_check/+merge/117058
> Your team Compiz Maintainers is subscribed to branch lp:~compiz-team/compiz/gles2.scope_of_empty_vb_check.
>

[Daniel van Vugt (vanvugt)]

I like information hiding, but still suspect you will encounter lots of plugins where it won't work. Unless you expose the whole GLVertexBuffer interface through GLWindow, but that's just nasty.

If you have an idea for improving information hiding, test that it works and then propose it. So long as we don't have to touch vast amounts of plugin code...

As for now, I'll just do the "end() returns bool" thing.

[Sam Spilsbury (smspillaz)]

On Wed, 8 Aug 2012, Daniel van Vugt wrote:

> I like information hiding, but still suspect you will encounter lots of plugins where it won't work. Unless you expose the whole GLVertexBuffer interface through GLWindow, but that's just nasty.
>
> If you have an idea for improving information hiding, test that it works and then propose it. So long as we don't have to touch vast amounts of plugin code...
>
> As for now, I'll just do the "end() returns bool" thing.

Okay, I only have to touch two plugins and its very noninvasive. I will
propose it in the next few minutes

> --
> https://code.launchpad.net/~compiz-team/compiz/gles2.scope_of_empty_vb_check/+merge/117058
> Your team Compiz Maintainers is subscribed to branch lp:~compiz-team/compiz/gles2.scope_of_empty_vb_check.
>

Unmerged revisions

3300. By Sam Spilsbury on 2012-07-27

Make checking to see if the vertex buffer is empty the responsibility of
GLWindow::glDraw and not the clients of that function.

While this is technically slower (incurred wrapped function overhead), it
is better encapsulation, since clients of glDraw less coupled with the
implementation of glDraw.

=== modified file 'plugins/compiztoolbox/src/compiztoolbox.cpp'
--- plugins/compiztoolbox/src/compiztoolbox.cpp	2012-07-27 09:57:26 +0000
+++ plugins/compiztoolbox/src/compiztoolbox.cpp	2012-07-27 13:25:23 +0000
@@ -536,18 +536,15 @@
 
 	gWindow->vertexBuffer ()->end ();
 
-	if (gWindow->vertexBuffer ()->countVertices ())
-	{
-	    GLMatrix           wTransform (transform);
-
-	    wTransform.translate (g.x (), g.y (), 0.0f);
-	    wTransform.scale (sAttrib.xScale, sAttrib.yScale, 1.0f);
-	    wTransform.translate (sAttrib.xTranslate / sAttrib.xScale - g.x (),
-	                          sAttrib.yTranslate / sAttrib.yScale - g.y (),
-	                          0.0f);
-
-	    gWindow->glDrawTexture (icon, wTransform, sAttrib, mask);
-	}
+	GLMatrix           wTransform (transform);
+
+	wTransform.translate (g.x (), g.y (), 0.0f);
+	wTransform.scale (sAttrib.xScale, sAttrib.yScale, 1.0f);
+	wTransform.translate (sAttrib.xTranslate / sAttrib.xScale - g.x (),
+	                      sAttrib.yTranslate / sAttrib.yScale - g.y (),
+	                      0.0f);
+
+	gWindow->glDrawTexture (icon, wTransform, sAttrib, mask);
     }
 }
 

=== modified file 'plugins/decor/src/decor.cpp'
--- plugins/decor/src/decor.cpp	2012-07-27 09:57:26 +0000
+++ plugins/decor/src/decor.cpp	2012-07-27 13:25:23 +0000
@@ -264,13 +264,10 @@
 
 	gWindow->vertexBuffer ()->end ();
 
-	if (gWindow->vertexBuffer ()->countVertices ())
-	{
-	    glEnable (GL_BLEND);
-	    gWindow->glDrawTexture (wd->decor->texture->textures[0], transform,
-	                            attrib, mask);
-	    glDisable (GL_BLEND);
-	}
+	glEnable (GL_BLEND);
+	gWindow->glDrawTexture (wd->decor->texture->textures[0], transform,
+	                        attrib, mask);
+	glDisable (GL_BLEND);
     }
     else if (wd && wd->decor->type == WINDOW_DECORATION_TYPE_WINDOW)
     {
@@ -293,9 +290,8 @@
 	    gWindow->glAddGeometry (ml, window->frameRegion (), region);
 	    gWindow->vertexBuffer ()->end ();
 
-	    if (gWindow->vertexBuffer ()->countVertices ())
-		gWindow->glDrawTexture (gWindow->textures ()[0], transform,
-		                        attrib, mask);
+	    gWindow->glDrawTexture (gWindow->textures ()[0], transform,
+	                            attrib, mask);
 	}
 	else
 	{
@@ -308,9 +304,8 @@
 		gWindow->glAddGeometry (ml, regions[i], region);
 		gWindow->vertexBuffer ()->end ();
 
-		if (gWindow->vertexBuffer ()->countVertices ())
-		    gWindow->glDrawTexture (gWindow->textures ()[i], transform,
-		                            attrib, mask);
+		gWindow->glDrawTexture (gWindow->textures ()[i], transform,
+		                        attrib, mask);
 	    }
 	}
 

=== modified file 'plugins/opengl/src/paint.cpp'
--- plugins/opengl/src/paint.cpp	2012-07-27 11:06:51 +0000
+++ plugins/opengl/src/paint.cpp	2012-07-27 13:25:23 +0000
@@ -1217,6 +1217,9 @@
 
     GLTexture::Filter filter;
 
+    if (!priv->vertexBuffer->countVertices ())
+	return;
+
     if (mask & PAINT_WINDOW_BLEND_MASK)
 	glEnable (GL_BLEND);
 
@@ -1295,8 +1298,7 @@
 	glAddGeometry (ml, priv->regions[i], reg);
 	priv->vertexBuffer->end ();
 
-	if (priv->vertexBuffer->countVertices())
-	    glDrawTexture (priv->textures[i], transform, attrib, mask);
+	glDrawTexture (priv->textures[i], transform, attrib, mask);
     }
 
     return true;

=== modified file 'plugins/ring/src/ring.cpp'
--- plugins/ring/src/ring.cpp	2012-07-27 09:57:26 +0000
+++ plugins/ring/src/ring.cpp	2012-07-27 13:25:23 +0000
@@ -375,26 +375,23 @@
 		gWindow->glAddGeometry (matricies, iconReg, iconReg);
 		gWindow->vertexBuffer ()->end ();
 
-		if (gWindow->vertexBuffer ()->countVertices ())
-		{
-		    GLWindowPaintAttrib wAttrib (sAttrib);
-		    GLMatrix            wTransform = transform;
-
-		    if (!pixmap)
-		        sAttrib.opacity = gWindow->paintAttrib ().opacity;
-
-		    if (mSlot)
-		        wAttrib.brightness = (float)wAttrib.brightness *
-		                             mSlot->depthBrightness;
-
-		    wTransform.translate (window->x (), window->y (), 0.0f);
-		    wTransform.scale (scale, scale, 1.0f);
-		    wTransform.translate ((x - window->x ()) / scale - window->x (),
-		                          (y - window->y ()) / scale - window->y (),
-		                          0.0f);
-
-		    gWindow->glDrawTexture (icon, wTransform, wAttrib, mask);
-		}
+		GLWindowPaintAttrib wAttrib (sAttrib);
+		GLMatrix            wTransform = transform;
+
+		if (!pixmap)
+		    sAttrib.opacity = gWindow->paintAttrib ().opacity;
+
+		if (mSlot)
+		    wAttrib.brightness = (float)wAttrib.brightness *
+		                                         mSlot->depthBrightness;
+
+		wTransform.translate (window->x (), window->y (), 0.0f);
+		wTransform.scale (scale, scale, 1.0f);
+		wTransform.translate ((x - window->x ()) / scale - window->x (),
+		                      (y - window->y ()) / scale - window->y (),
+		                      0.0f);
+
+		gWindow->glDrawTexture (icon, wTransform, wAttrib, mask);
 	    }
 	}
     }

=== modified file 'plugins/scale/src/scale.cpp'
--- plugins/scale/src/scale.cpp	2012-07-27 09:57:26 +0000
+++ plugins/scale/src/scale.cpp	2012-07-27 13:25:23 +0000
@@ -233,15 +233,12 @@
 
 	    priv->gWindow->vertexBuffer ()->end ();
 
-	    if (priv->gWindow->vertexBuffer ()->countVertices ())
-	    {
-		GLMatrix           wTransform (transform);
-
-		wTransform.scale (scale, scale, 1.0f);
-		wTransform.translate (x / scale, y / scale, 0.0f);
-
-		priv->gWindow->glDrawTexture (icon, wTransform, sAttrib, mask);
-	    }
+	    GLMatrix           wTransform (transform);
+
+	    wTransform.scale (scale, scale, 1.0f);
+	    wTransform.translate (x / scale, y / scale, 0.0f);
+
+	    priv->gWindow->glDrawTexture (icon, wTransform, sAttrib, mask);
 	}
     }
 }

=== modified file 'plugins/shift/src/shift.cpp'
--- plugins/shift/src/shift.cpp	2012-07-27 09:57:26 +0000
+++ plugins/shift/src/shift.cpp	2012-07-27 13:25:23 +0000
@@ -421,45 +421,42 @@
 
 		gWindow->vertexBuffer ()->end ();
 
-		if (gWindow->vertexBuffer ()->countVertices ())
-		{
-		    GLWindowPaintAttrib wAttrib (sAttrib);
-		    GLMatrix            wTransform (transform);
-
-		    if (gWindow->textures ().empty ())
-		        sAttrib.opacity = gWindow->paintAttrib ().opacity;
-
-		    wAttrib = GLWindowPaintAttrib (sAttrib);
-
-		    wAttrib.opacity = (float)wAttrib.opacity * sopacity;
-		    wAttrib.brightness = (float)wAttrib.brightness *
-		                         ss->mReflectBrightness;
-
-		    wTransform.translate (sx, sy, sz);
-
-		    wTransform.translate (window->x () +
-		                            (window->width ()  * sscale / 2),
-		                          window->y () +
-		                            (window->height ()  * sscale / 2.0),
-		                          0.0f);
-
-		    wTransform.scale (ss->mOutput->width (),
-		                      -ss->mOutput->height (),
-		                      1.0f);
-
-		    wTransform.rotate (srot, 0.0, 1.0, 0.0);
-
-		    wTransform.scale (1.0f  / ss->mOutput->width (),
-		                      -1.0f / ss->mOutput->height (),
-		                      1.0f);
-
-		    wTransform.translate (x - (window->width () * sscale / 2),
-		                          y - (window->height () * sscale / 2.0),
-		                          0.0f);
-		    wTransform.scale (scale, scale, 1.0f);
-
-		    gWindow->glDrawTexture (icon, wTransform, wAttrib, mask);
-		}
+		GLWindowPaintAttrib wAttrib (sAttrib);
+		GLMatrix            wTransform (transform);
+
+		if (gWindow->textures ().empty ())
+		    sAttrib.opacity = gWindow->paintAttrib ().opacity;
+
+		wAttrib = GLWindowPaintAttrib (sAttrib);
+
+		wAttrib.opacity = (float)wAttrib.opacity * sopacity;
+		wAttrib.brightness = (float)wAttrib.brightness *
+		                                        ss->mReflectBrightness;
+
+		wTransform.translate (sx, sy, sz);
+
+		wTransform.translate (window->x () +
+		                              (window->width ()  * sscale / 2),
+		                      window->y () +
+		                           (window->height ()  * sscale / 2.0),
+		                      0.0f);
+
+		wTransform.scale (ss->mOutput->width (),
+		                  -ss->mOutput->height (),
+		                  1.0f);
+
+		wTransform.rotate (srot, 0.0, 1.0, 0.0);
+
+		wTransform.scale (1.0f  / ss->mOutput->width (),
+		                  -1.0f / ss->mOutput->height (),
+		                  1.0f);
+
+		wTransform.translate (x - (window->width () * sscale / 2),
+		                      y - (window->height () * sscale / 2.0),
+		                      0.0f);
+		wTransform.scale (scale, scale, 1.0f);
+
+		gWindow->glDrawTexture (icon, wTransform, wAttrib, mask);
 	    }
 	}
     }