Created attachment 206473 [details] [review]
ShellScreenGrabber: grab the screen using pixel buffers

For the Intel drivers, using glReadPixels() to read into client-memory
directly from the frame buffer is much slower than creating a pixel
buffer, copying into that, and then mapping that for reading. On other
drivers, the two approaches are likely to be similar in speed. Create
a ShellScreenGrabber abstraction that uses pixel buffers if available.
Use that for screenshots and screen recording.

Review of attachment 206473 [details] [review]:

Looks good to me and provides a *significant* speed boost on my GM45 based laptop (when recording).
The usage of pack_invert without checking breaks NVIDIA (and possibly fglrx) though.

::: src/shell-screen-grabber.c
@@ +133,3 @@
+      glPixelStorei (GL_PACK_SKIP_ROWS, 0);
+      glPixelStorei (GL_PACK_ALIGNMENT, 1);
+      glPixelStorei (GL_PACK_ALIGNMENT, 1);

You are setting this one twice.

@@ +134,3 @@
+      glPixelStorei (GL_PACK_ALIGNMENT, 1);
+      glPixelStorei (GL_PACK_ALIGNMENT, 1);
+      glPixelStorei (GL_PACK_INVERT_MESA, FALSE);

Should be GL_FALSE (both have the same value but still).
What is more important though you should check whether the GL_MESA_pack_invert is supported at all.
NVIDIA does not support it resulting into tons of:
Cogl-WARNING **: ./cogl-buffer.c:358: GL error (1280): Invalid enumeration value

(In reply to comment #2)
> Review of attachment 206473 [details] [review]:
> 
> Looks good to me and provides a *significant* speed boost on my GM45 based
> laptop (when recording).
> The usage of pack_invert without checking breaks NVIDIA (and possibly fglrx)
> though.

Well "breaks" it just floods warning but the recording actually works.

Created attachment 206525 [details] [review]
ShellScreenGrabber: grab the screen using pixel buffers

With fixes - thanks for the review.

Review of attachment 206525 [details] [review]:

Looks good.

Attachment 206525 [details] pushed as 4e89a5e - ShellScreenGrabber: grab the screen using pixel buffers

Created attachment 208430 [details]
Test case for timing glReadPixels

For what it's worth, I had a quick look at trying to get Mesa to have a faster read pixels and I filed this bug here:

https://bugs.freedesktop.org/show_bug.cgi?id=46631

It looks like the fallback code in Mesa already has a fast path for the case where the format of the framebuffer and the destination buffer are the same so it can just do a memcpy. However it's over cautious with the format checking so it effectively wouldn't hit it unless you are using a floating point format to read into. The non-fast path case converts every pixel to an array of four floats and then does some floating point arithmetic to convert it back so it's no wonder it's slow.

I'm attaching a little standalone test case for timing glReadPixels. Without the patch and just using glReadPixels directly into normal memory it takes 10.319 seconds. With the patch it takes 0.832 seconds. If I change it to use a PBO with a map and memcpy to normal memory then it takes 0.805.

It seems kind of suprising that the blit/map/memcpy case is slightly faster because it implies copying the memory twice (although presumably the blit can be done faster than a memcpy). However both cases are very much in the same ballpark so I don't think it would be worthwhile using PBOs if we can use the fast path patch instead. It would be nice to try and avoid any direct GL usage in Gnome Shell because it is a layering violation. The more apps that do this the more limited we become with how we can change Cogl.

You're probably fed up of me going on about the screen recorder by now, but just to let you know anyway, I've added a patch to Cogl to do the same workaround by reading into a temporary PBO when it detects it's running with Mesa on an Intel GPU. If Gnome Shell ever starts requiring a recent enough version of Cogl it might be worth dropping this patch to just let Cogl do it.

http://lists.freedesktop.org/archives/cogl/2012-April/000091.html