Merge lp:~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos into lp:compiz/0.9.10

[MC Return]

Refactor screenshot code to make it clearer and also to allow us to take
screenshots in glPaintOutput as opposed to paint ().

Taking screenshots in paint () was probably correct pre-GLES, but isn't
really correct now - we want to be able to read off of the currently
bound scratch framebuffer where we last painted the frame. Reading off
the frontbuffer is an imprecise operation because the contents of both
buffers are really undefined after a swap. In the case where we are
not painting to a scratch framebuffer object, we'll just end up reading
from the backbuffer anyways, which would be correct to do mid-frame.

Also added the new static const GLenums
DRAW_FRAMEBUFFER_BINDING and
READ_FRAMEBUFFER_BINDING to opengl.h.

(LP: #771875)

\o/

I declare Screenshot to be 100% ready now.

@Sam: Thanks 4 the tip with glReadBuffer. The GL_FRONT version did the
trick. :) &&
thanks for the additional refactoring :)

[Sam Spilsbury (smspillaz)]

I don't think reading off the front buffer is correct. It should actually be the back buffer.

In any case I'm a little hesitant to ack this right now as I mentioned earlier. There are other plugins (namley unity) that assume that the current read buffer binding hasn't been touched. The only way they'd be able to know whether or not the binding should be switched is by querying OpenGL directly, and such queries are usually quite slow.

I'd much rather have a framework to track changes in the buffer bindings, a-la something like this: https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1040478/+merge/156465 before we do anything here.

So - we'll get back to this as soon as the right frameworks are in place to enable it properly. Sorry about that :/

[MC Return (mc-return)]

> I don't think reading off the front buffer is correct. It should actually be
> the back buffer.
>

Of course I tried the back buffer version first -> Did not work.
I also tried to flush, clear, clearcolor, forcing transparency to 1.0, but
nothing worked, if FBOs are enabled... :(

> In any case I'm a little hesitant to ack this right now as I mentioned
> earlier. There are other plugins (namley unity) that assume that the current
> read buffer binding hasn't been touched. The only way they'd be able to know
> whether or not the binding should be switched is by querying OpenGL directly,
> and such queries are usually quite slow.
>

I did try to optimize behaviour by just changing the buffer to front if FBOs are
enabled.
Would it help to set it to back buffer again, after glReadPixels has finished ?

> I'd much rather have a framework to track changes in the buffer bindings, a-la
> something like this: https://code.launchpad.net/~compiz-
> team/compiz/compiz.fix_1040478/+merge/156465 before we do anything here.
>

Okay, no problem -> this can wait. Folks now already have the ability to set the
opacity of the screenshot overlay manually, so they can workaround this nasty
bug in trunk version of Screenshot already.
So no need for any hurry here...

> So - we'll get back to this as soon as the right frameworks are in place to
> enable it properly. Sorry about that :/

No problem at all. This is just the point of the i.

[MC Return (mc-return)]

Set to WIP.

[Sam Spilsbury (smspillaz)]

Cheers. Will get on to this as soon as it makes sense to do so.

On Thu, Apr 25, 2013 at 3:53 PM, MC Return <email address hidden> wrote:
> Set to WIP.
> --
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos/+merge/160751
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos into lp:compiz.

--
Sam Spilsbury

[MC Return (mc-return)]

> Cheers. Will get on to this as soon as it makes sense to do so.
>
+1. No stress with that one. Every user can get it to work correctly already.

[MC Return (mc-return)]

Sam, just 2 questions regarding this matter:

1. Are you aware that glReadBuffer (GL_FRONT); will just be called once per screenshot and just in the case FBOs are enabled, that means if the user is veryveryvery fast, maximally 3 times per second ?

2. Would it help to call glReadBuffer (GL_BACK); after glReadPixels (...) if (GL::fboEnabled) to change it back, or is
this a stupid and useless idea ?

review: Needs Information

[Sam Spilsbury (smspillaz)]

Actually, I think you're probably right on this one. Though I think if
I remember correctly now the right approach is actually to read off of
the currently bound framebuffer object and not the frontbuffer. The
reason why the overlay persists is because the backbuffer is always
one-frame-behind. I'm not really sure why there is no overlay on the
frontbuffer, perhaps it is a race condition. In any case, those pixels
will be at least one frame out of date.

The way screenshot works is that it paints the overlay, and then upon
"deciding" to take the screenshot, damages that region and then calls
glReadPixels on the next paint pass. The pixels for that frame will be
painted into the currently bound framebuffer object.

So the correct approach would be:

GLuint readBufferBinding = 0;

if (GL::fboEnabled)
{
    glGetIntegerv (GL_READ_FRAMEBUFFER, &readBufferBinding);
    /* We want to read off of the last frame, so bind the backbuffer */
    GL::bindFramebuffer (GL_READ_FRAMEBUFFER, 0);
}

...

glReadPixels

...

if (GL::fboEnabled)
{
    /* Bind the last read buffer */
    GL::bindFramebuffer (GL_READ_FRAMEBUFFER, readBufferBinding);
}

You're right. Since this only happens in screenshot mode, I don't
think performance is a huge concern anways. glReadPixels is a far
slower operation than glBindFramebufferEXT anyways and we have to do
that.

On Fri, Apr 26, 2013 at 6:14 PM, MC Return <email address hidden> wrote:
> Review: Needs Information
>
> Sam, just 2 questions regarding this matter:
>
> 1. Are you aware that glReadBuffer (GL_FRONT); will just be called once per screenshot and just in the case FBOs are enabled, that means if the user is veryveryvery fast, maximally 3 times per second ?
>
> 2. Would it help to call glReadBuffer (GL_BACK); after glReadPixels (...) if (GL::fboEnabled) to change it back, or is
> this a stupid and useless idea ?
> --
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos/+merge/160751
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos into lp:compiz.

--
Sam Spilsbury

[MC Return (mc-return)]

> Actually, I think you're probably right on this one. Though I think if
> I remember correctly now the right approach is actually to read off of
> the currently bound framebuffer object and not the frontbuffer. The
> reason why the overlay persists is because the backbuffer is always
> one-frame-behind. I'm not really sure why there is no overlay on the
> frontbuffer, perhaps it is a race condition. In any case, those pixels
> will be at least one frame out of date.
>
> The way screenshot works is that it paints the overlay, and then upon
> "deciding" to take the screenshot, damages that region and then calls
> glReadPixels on the next paint pass. The pixels for that frame will be
> painted into the currently bound framebuffer object.
>
> So the correct approach would be:
>
> GLuint readBufferBinding = 0;
>
> if (GL::fboEnabled)
> {
> glGetIntegerv (GL_READ_FRAMEBUFFER, &readBufferBinding);
> /* We want to read off of the last frame, so bind the backbuffer */
> GL::bindFramebuffer (GL_READ_FRAMEBUFFER, 0);
> }
>
> ...
>
> glReadPixels
>
> ...
>
> if (GL::fboEnabled)
> {
> /* Bind the last read buffer */
> GL::bindFramebuffer (GL_READ_FRAMEBUFFER, readBufferBinding);
> }
>
> You're right. Since this only happens in screenshot mode, I don't
> think performance is a huge concern anways. glReadPixels is a far
> slower operation than glBindFramebufferEXT anyways and we have to do
> that.
>
Cool, I'll fix it that way then :)

[MC Return (mc-return)]

>
> So the correct approach would be:
>
> GLuint readBufferBinding = 0;
>
> if (GL::fboEnabled)
> {
> glGetIntegerv (GL_READ_FRAMEBUFFER, &readBufferBinding);
> /* We want to read off of the last frame, so bind the backbuffer */
> GL::bindFramebuffer (GL_READ_FRAMEBUFFER, 0);
> }
>
> ...
>
> glReadPixels
>
> ...
>
> if (GL::fboEnabled)
> {
> /* Bind the last read buffer */
> GL::bindFramebuffer (GL_READ_FRAMEBUFFER, readBufferBinding);
> }
>
This does not work:

/home/mcr2010/src/bzr/compiz/plugins/screenshot/src/screenshot.cpp: In member function ‘virtual void ShotScreen::paint(CompOutput::ptrList&, unsigned int)’:
/home/mcr2010/src/bzr/compiz/plugins/screenshot/src/screenshot.cpp:185:41: error: ‘readBufferBinding’ was not declared in this scope
    glGetIntegerv (GL_READ_FRAMEBUFFER, &readBufferBinding);
                                         ^
/home/mcr2010/src/bzr/compiz/plugins/screenshot/src/screenshot.cpp:196:46: error: ‘readBufferBinding’ was not declared in this scope
    GL::bindFramebuffer (GL_READ_FRAMEBUFFER, readBufferBinding);
                                              ^

[MC Return (mc-return)]

review: Needs Information

[Sam Spilsbury (smspillaz)]

Did you define the readBufferBinding variable? Compiler says that you didn't ;-)

On Fri, Apr 26, 2013 at 9:34 PM, MC Return <email address hidden> wrote:
>>
>> So the correct approach would be:
>>
>> GLuint readBufferBinding = 0;
>>
>> if (GL::fboEnabled)
>> {
>> glGetIntegerv (GL_READ_FRAMEBUFFER, &readBufferBinding);
>> /* We want to read off of the last frame, so bind the backbuffer */
>> GL::bindFramebuffer (GL_READ_FRAMEBUFFER, 0);
>> }
>>
>> ...
>>
>> glReadPixels
>>
>> ...
>>
>> if (GL::fboEnabled)
>> {
>> /* Bind the last read buffer */
>> GL::bindFramebuffer (GL_READ_FRAMEBUFFER, readBufferBinding);
>> }
>>
> This does not work:
>
> /home/mcr2010/src/bzr/compiz/plugins/screenshot/src/screenshot.cpp: In member function ‘virtual void ShotScreen::paint(CompOutput::ptrList&, unsigned int)’:
> /home/mcr2010/src/bzr/compiz/plugins/screenshot/src/screenshot.cpp:185:41: error: ‘readBufferBinding’ was not declared in this scope
> glGetIntegerv (GL_READ_FRAMEBUFFER, &readBufferBinding);
> ^
> /home/mcr2010/src/bzr/compiz/plugins/screenshot/src/screenshot.cpp:196:46: error: ‘readBufferBinding’ was not declared in this scope
> GL::bindFramebuffer (GL_READ_FRAMEBUFFER, readBufferBinding);
> ^
>
>
> --
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos/+merge/160751
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos into lp:compiz.

--
Sam Spilsbury

[MC Return (mc-return)]

urgh - sorry

[MC Return (mc-return)]

GLint readBufferBinding = 0;

instead of

GLuint readBufferBinding = 0;

now did compile -> testing now... :)

[MC Return (mc-return)]

The overlay is on the screenshot again :(

[MC Return (mc-return)]

I used this code:

      GLint readBufferBinding = 0;

      /* If we have fbos enabled we
       * need to clear the overlay
       * before reading the pixels */
      if (GL::fboEnabled)
      {
   glGetIntegerv (GL_READ_FRAMEBUFFER, &readBufferBinding);
   /* We want to read off of the last frame, so bind the backbuffer */
   GL::bindFramebuffer (GL_READ_FRAMEBUFFER, 0);
      }

      glReadPixels (x1, ::screen->height () - y2, w, h,
      GL_RGBA, GL_UNSIGNED_BYTE,
      (GLvoid *) buffer);

      if (GL::fboEnabled)
   /* Bind the last read buffer */
   GL::bindFramebuffer (GL_READ_FRAMEBUFFER, readBufferBinding);

[MC Return (mc-return)]

Okay, updated the commit message here.
The last thing missing here seems to be your +1, Sam ;)

[Sam Spilsbury (smspillaz)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'plugins/opengl/include/opengl/opengl.h'
--- plugins/opengl/include/opengl/opengl.h	2013-01-10 09:23:24 +0000
+++ plugins/opengl/include/opengl/opengl.h	2013-04-27 11:54:25 +0000
@@ -449,6 +449,8 @@
     /* OpenGL|ES does not support different draw/read framebuffers */
     static const GLenum 		    DRAW_FRAMEBUFFER = GL_FRAMEBUFFER;
     static const GLenum 		    READ_FRAMEBUFFER = GL_FRAMEBUFFER;
+    static const GLenum	                    DRAW_FRAMEBUFFER_BINDING = FRAMEBUFFER_BINDING;
+    static const GLenum	                    READ_FRAMEBUFFER_BINDING = FRAMEBUFFER_BINDING;
 
     static const GLenum 		    FRAMEBUFFER_COMPLETE = GL_FRAMEBUFFER_COMPLETE;
     static const GLenum 		    FRAMEBUFFER_UNDEFINED = 0;
@@ -484,6 +486,8 @@
 
     static const GLenum 		  DRAW_FRAMEBUFFER = GL_DRAW_FRAMEBUFFER_EXT;
     static const GLenum 		  READ_FRAMEBUFFER = GL_READ_FRAMEBUFFER_EXT;
+    static const GLenum	                  DRAW_FRAMEBUFFER_BINDING = GL_DRAW_FRAMEBUFFER_BINDING_EXT;
+    static const GLenum	                  READ_FRAMEBUFFER_BINDING = GL_READ_FRAMEBUFFER_BINDING_EXT;
     static const GLenum 		  FRAMEBUFFER_COMPLETE = GL_FRAMEBUFFER_COMPLETE_EXT;
     static const GLenum 		  FRAMEBUFFER_UNDEFINED = GL_FRAMEBUFFER_UNDEFINED;
     static const GLenum 		  FRAMEBUFFER_INCOMPLETE_ATTACHMENT = GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT_EXT;
 
=== modified file 'plugins/screenshot/src/screenshot.cpp'
--- plugins/screenshot/src/screenshot.cpp	2013-04-24 15:42:11 +0000
+++ plugins/screenshot/src/screenshot.cpp	2013-04-27 11:54:25 +0000
@@ -23,6 +23,9 @@
  * Author: David Reveman <[e-mail]>
  */
 
+#include <sstream>
+#include <boost/scoped_array.hpp>
+
 #include "screenshot.h"
 
 #include <dirent.h>
@@ -104,8 +107,6 @@
     action->setState (action->state () & ~(CompAction::StateTermKey |
 					   CompAction::StateTermButton));
 
-    gScreen->glPaintOutputSetEnabled (this, false);
-
     return false;
 }
 
@@ -148,79 +149,298 @@
 ShotScreen::paint (CompOutput::ptrList &outputs,
 		   unsigned int        mask)
 {
-    cScreen->paint (outputs, mask);
-
     if (mGrab)
     {
 	if (!mGrabIndex)
 	{
-	    int x1 = MIN (mX1, mX2);
-	    int y1 = MIN (mY1, mY2);
-	    int x2 = MAX (mX1, mX2);
-	    int y2 = MAX (mY1, mY2);
-
-	    int w = x2 - x1;
-	    int h = y2 - y1;
-
-	    if (w && h)
+	    /* Taking screenshot, enable full paint on
+	     * this frame */
+
+	    outputs.clear ();
+	    outputs.push_back (&screen->fullscreenOutput ());
+	}
+    }
+
+    cScreen->paint (outputs, mask);
+}
+
+namespace
+{
+    bool paintSelectionRectangleFill (const CompRect &rect,
+				      unsigned short *fillColor,
+				      GLVertexBuffer *streamingBuffer,
+				      const GLMatrix &transform)
+    {
+	GLfloat         vertexData[12];
+	GLushort        colorData[4];
+
+	int x1 = rect.x1 ();
+	int y1 = rect.y1 ();
+	int x2 = rect.x2 ();
+	int y2 = rect.y2 ();
+
+	const float MaxUShortFloat = std::numeric_limits <unsigned short>::max ();
+
+	/* draw filled rectangle */
+	float alpha = fillColor[3] / MaxUShortFloat;
+
+	colorData[0] = alpha * fillColor[0];
+	colorData[1] = alpha * fillColor[1];
+	colorData[2] = alpha * fillColor[2];
+	colorData[3] = alpha * MaxUShortFloat;
+
+	vertexData[0]  = x1;
+	vertexData[1]  = y1;
+	vertexData[2]  = 0.0f;
+	vertexData[3]  = x1;
+	vertexData[4]  = y2;
+	vertexData[5]  = 0.0f;
+	vertexData[6]  = x2;
+	vertexData[7]  = y1;
+	vertexData[8]  = 0.0f;
+	vertexData[9]  = x2;
+	vertexData[10] = y2;
+	vertexData[11] = 0.0f;
+
+	streamingBuffer->begin (GL_TRIANGLE_STRIP);
+
+	streamingBuffer->addColors (1, colorData);
+	streamingBuffer->addVertices (4, vertexData);
+
+	if (streamingBuffer->end ())
+	{
+	    glEnable (GL_BLEND);
+
+	    streamingBuffer->render (transform);
+
+	    glDisable (GL_BLEND);
+
+	    return true;
+	}
+
+	return false;
+    }
+
+    bool paintSelectionRectangleOutline (const CompRect &rect,
+					 unsigned short *outlineColor,
+					 GLVertexBuffer *streamingBuffer,
+					 const GLMatrix &transform)
+    {
+	GLfloat         vertexData[12];
+	GLushort        colorData[4];
+
+	int x1 = rect.x1 ();
+	int y1 = rect.y1 ();
+	int x2 = rect.x2 ();
+	int y2 = rect.y2 ();
+
+	const float MaxUShortFloat = std::numeric_limits <unsigned short>::max ();
+
+	/* draw outline */
+	float alpha = outlineColor[3] / MaxUShortFloat;
+
+	colorData[0] = alpha * outlineColor[0];
+	colorData[1] = alpha * outlineColor[1];
+	colorData[2] = alpha * outlineColor[2];
+	colorData[3] = alpha * MaxUShortFloat;
+
+	vertexData[0]  = x1;
+	vertexData[1]  = y1;
+	vertexData[2]  = 0.0f;
+	vertexData[3]  = x1;
+	vertexData[4]  = y2;
+	vertexData[5]  = 0.0f;
+	vertexData[6]  = x2;
+	vertexData[7]  = y2;
+	vertexData[8]  = 0.0f;
+	vertexData[9]  = x2;
+	vertexData[10] = y1;
+	vertexData[11] = 0.0f;
+
+	streamingBuffer->begin (GL_LINE_LOOP);
+
+	streamingBuffer->addColors (1, colorData);
+	streamingBuffer->addVertices (4, vertexData);
+
+	if (streamingBuffer->end ())
+	{
+	    glEnable (GL_BLEND);
+	    glLineWidth (2.0);
+
+	    streamingBuffer->render (transform);
+
+	    glDisable (GL_BLEND);
+
+	    return true;
+	}
+
+	return false;
+    }
+
+    void
+    ensureDirectoryForImage (CompString &directory)
+    {
+	/* If dir is empty, use user's desktop directory instead */
+	if (directory.length () == 0)
+	    directory = getXDGUserDir (XDGUserDirDesktop);
+    }
+
+    int
+    getImageNumberFromDirectory (const CompString &directory)
+    {
+	struct dirent **namelist;
+
+	int n = scandir (directory.c_str (), &namelist, shotFilter, shotSort);
+
+	if (n >= 0)
+	{
+	    int  number = 0;
+
+	    if (n > 0)
+		sscanf (namelist[n - 1]->d_name,
+			"screenshot%d.png",
+			&number);
+
+	    ++number;
+
+	    if (n)
+		free (namelist);
+
+	    return number;
+	}
+	else
+	{
+	    perror ("scandir");
+	    return 0;
+	}
+    }
+
+    CompString
+    getImageAbsolutePath (const CompString &directory,
+			  int              number)
+    {
+	std::stringstream ss;
+	ss << directory << "/screenshot" << number << ".png";
+	return ss.str ();
+    }
+
+    bool
+    saveBuffer (const boost::scoped_array <GLubyte> &buffer,
+		int                                 w,
+		int                                 h,
+		const CompString                    &path)
+    {
+	CompSize imageSize (w, h);
+
+	if (!::screen->writeImageToFile (const_cast <CompString &> (path),
+					 "png",
+					 imageSize,
+					 buffer.get ()))
+	{
+	    compLogMessage ("screenshot", CompLogLevelError,
+			    "failed to write screenshot image");
+	    return false;
+	}
+
+	return true;
+    }
+
+    bool
+    launchApplicationAndTakeScreenshot (const CompString &app,
+					const CompString &directory)
+    {
+	if (app.length () > 0)
+	{
+	    ::screen->runCommand (app + " " + directory);
+	    return true;
+	}
+
+	return false;
+    }
+
+    bool
+    readFromGPUBufferToCPUBuffer (const CompRect                &rect,
+				  boost::scoped_array <GLubyte> &buffer)
+    {
+	int x1 = rect.x1 ();
+	int y1 = rect.y1 ();
+	int x2 = rect.x2 ();
+	int y2 = rect.y2 ();
+
+	int w = x2 - x1;
+	int h = y2 - y1;
+
+	if (w && h)
+	{
+	    size_t size = w * h * 4;
+	    buffer.reset (new GLubyte[size]);
+
+	    if (buffer.get ())
 	    {
-		GLubyte *buffer;
-		CompString dir (optionGetDirectory ());
-
-		/* If dir is empty, use user's desktop directory instead */
-		if (dir.length () == 0)
-		    dir = getXDGUserDir (XDGUserDirDesktop);
-
-		buffer = (GLubyte *)malloc (sizeof (GLubyte) * w * h * 4);
-
-		if (buffer)
+		GLint drawBinding = 0;
+		GLint readBinding = 0;
+
+		/* Bind the currently bound draw framebuffer to
+		 * the read framebuffer and read from it */
+		if (GL::fboEnabled)
 		{
-		    struct dirent **namelist;
-
-		    glReadPixels (x1, ::screen->height () - y2, w, h,
-				  GL_RGBA, GL_UNSIGNED_BYTE,
-				  (GLvoid *) buffer);
-
-		    int n = scandir (dir.c_str (), &namelist, shotFilter, shotSort);
-
-		    if (n >= 0)
-		    {
-			char name[256];
-			int  number = 0;
-
-			if (n > 0)
-			    sscanf (namelist[n - 1]->d_name,
-				    "screenshot%d.png",
-				    &number);
-
-			++number;
-
-			if (n)
-			    free (namelist);
-
-			snprintf (name, 256, "screenshot%d.png", number);
-
-			CompString app (optionGetLaunchApp ());
-			CompString path (dir + "/" + name);
-			CompSize imageSize (w, h);
-
-			if (!::screen->writeImageToFile (path, "png",
-							 imageSize, buffer))
-			    compLogMessage ("screenshot", CompLogLevelError,
-					    "failed to write screenshot image");
-			else if (app.length () > 0)
-			    ::screen->runCommand (app + " " + path);
-		    }
-		    else
-			perror (dir.c_str ());
-
-		    free (buffer);
+		    glGetIntegerv (GL::DRAW_FRAMEBUFFER_BINDING, &drawBinding);
+		    glGetIntegerv (GL::READ_FRAMEBUFFER_BINDING, &readBinding);
+		    (GL::bindFramebuffer) (GL::READ_FRAMEBUFFER, drawBinding);
 		}
+
+		glGetError ();
+		glReadPixels (x1, ::screen->height () - y2, w, h,
+			      GL_RGBA, GL_UNSIGNED_BYTE,
+			      (GLvoid *) buffer.get ());
+
+		if (GL::fboEnabled)
+		    (GL::bindFramebuffer) (GL::READ_FRAMEBUFFER, readBinding);
+
+		if (glGetError () != GL_NO_ERROR)
+		    return false;
+
+		return true;
 	    }
-	    /* Disable screen capture */
-	    cScreen->paintSetEnabled (this, false);
-	    mGrab = false;
-	}
+	}
+
+	return false;
+    }
+
+    /* We need to take directory by copy because
+     * it may be modified later */
+    bool saveScreenshot (CompRect         rect,
+			 CompString       directory,
+			 const CompString &alternativeApplication)
+    {
+	ensureDirectoryForImage (directory);
+
+	int number = getImageNumberFromDirectory (directory);
+	CompString path = getImageAbsolutePath (directory, number);
+
+	boost::scoped_array <GLubyte> buffer;
+
+	bool success = readFromGPUBufferToCPUBuffer (rect,
+						     buffer);
+
+	if (success)
+	{
+	    success = saveBuffer (buffer,
+				  rect.width (),
+				  rect.height (), path);
+	}
+	else
+	{
+	    compLogMessage ("screenshot", CompLogLevelWarn, "glReadPixels failed");
+	}
+
+	if (!success)
+	    success =
+		launchApplicationAndTakeScreenshot (alternativeApplication,
+						    directory);
+
+	return success;
+
     }
 }
 
@@ -231,12 +451,6 @@
 			   CompOutput                *output,
 			   unsigned int               mask)
 {
-    GLVertexBuffer *streamingBuffer = GLVertexBuffer::streamingBuffer ();
-    GLMatrix        transform (matrix);
-    GLfloat         vertexData[12];
-    GLushort        colorData[4];
-    GLushort        *color;
-
     bool status = gScreen->glPaintOutput (attrib, matrix, region, output, mask);
 
     if (status && mGrab)
@@ -245,87 +459,42 @@
 	 * we are grabbed, the size has changed and the CCSM
 	 * option to draw it is enabled. */
 
+	CompRect selectionRect (std::min (mX1, mX2),
+				std::min (mY1, mY2),
+				std::abs (mX2 - mX1),
+				std::abs (mY2 - mY1));
+
 	if (mGrabIndex &&
 	    selectionSizeChanged &&
 	    optionGetDrawSelectionIndicator ())
 	{
-	    int x1 = MIN (mX1, mX2);
-	    int y1 = MIN (mY1, mY2);
-	    int x2 = MAX (mX1, mX2);
-	    int y2 = MAX (mY1, mY2);
-
-	    const float MaxUShortFloat = std::numeric_limits <unsigned short>::max ();
-
-	    /* draw filled rectangle */
-	    float alpha = optionGetSelectionFillColorAlpha () / MaxUShortFloat;
-	    color = optionGetSelectionFillColor ();
-
-	    colorData[0] = alpha * color[0];
-	    colorData[1] = alpha * color[1];
-	    colorData[2] = alpha * color[2];
-	    colorData[3] = alpha * MaxUShortFloat;
-
-	    vertexData[0]  = x1;
-	    vertexData[1]  = y1;
-	    vertexData[2]  = 0.0f;
-	    vertexData[3]  = x1;
-	    vertexData[4]  = y2;
-	    vertexData[5]  = 0.0f;
-	    vertexData[6]  = x2;
-	    vertexData[7]  = y1;
-	    vertexData[8]  = 0.0f;
-	    vertexData[9]  = x2;
-	    vertexData[10] = y2;
-	    vertexData[11] = 0.0f;
-
-	    transform.translate (-0.5f, -0.5f, -DEFAULT_Z_CAMERA);
-	    transform.scale (1.0f / output->width (),
-			     -1.0f / output->height (),
-			     1.0f);
-	    transform.translate (-output->region ()->extents.x1,
-				 -output->region ()->extents.y2,
-				 0.0f);
-
-	    glEnable (GL_BLEND);
-
-	    streamingBuffer->begin (GL_TRIANGLE_STRIP);
-
-	    streamingBuffer->addColors (1, colorData);
-	    streamingBuffer->addVertices (4, vertexData);
-
-	    streamingBuffer->end ();
-	    streamingBuffer->render (transform);
-
-	    /* draw outline */
-	    alpha = optionGetSelectionOutlineColorAlpha () / MaxUShortFloat;
-	    color = optionGetSelectionOutlineColor ();
-
-	    colorData[0] = alpha * color[0];
-	    colorData[1] = alpha * color[1];
-	    colorData[2] = alpha * color[2];
-	    colorData[3] = alpha * MaxUShortFloat;
-
-	    vertexData[6]  = x2;
-	    vertexData[7]  = y2;
-	    vertexData[9]  = x2;
-	    vertexData[10] = y1;
-
-	    glLineWidth (2.0);
-
-	    streamingBuffer->begin (GL_LINE_LOOP);
-
-	    streamingBuffer->addColors (1, colorData);
-	    streamingBuffer->addVertices (4, vertexData);
-
-	    streamingBuffer->end ();
-	    streamingBuffer->render (transform);
-
-	    glDisable (GL_BLEND);
+	    GLMatrix        transform (matrix);
+	    GLVertexBuffer *streamingBuffer (GLVertexBuffer::streamingBuffer ());
+	    transform.toScreenSpace (output, -DEFAULT_Z_CAMERA);
+
+	    paintSelectionRectangleFill (selectionRect,
+					 optionGetSelectionFillColor (),
+					 streamingBuffer,
+					 transform);
+
+	    paintSelectionRectangleOutline (selectionRect,
+					    optionGetSelectionOutlineColor (),
+					    streamingBuffer,
+					    transform);
 
 	    /* we finished painting the selection box,
 	     * reset selectionSizeChanged now */
 	    selectionSizeChanged = false;
 	}
+	else if (!mGrabIndex)
+	{
+	    /* Taking a screenshot */
+	    saveScreenshot (selectionRect,
+			    optionGetDirectory (),
+			    optionGetLaunchApp ());
+	    cScreen->paintSetEnabled (this, false);
+	    gScreen->glPaintOutputSetEnabled (this, false);
+	}
     }
 
     return status;