Created attachment 191910 [details] [review]
Use a utility function to create GL_ARB_texture_rectangles

meta-texture-rectangle and meta-shaped-texture both create textures
with GL_TEXTURE_RECTANGLE_ARB as the target using direct GL
calls. This patch moves that code into a shared utility function in a
separate file instead. The function resolves the required GL symbols
dynamically instead of linking to them directly so that if Clutter
eventually stops linking to -lGL mutter will continue to build. The
function also splits the texture creation into a separate texture
creation and data upload stage so that it can use
cogl_texture_set_region to upload the data. That way it can avoid
clobbering the glPixelStore state and it can let Cogl do any necessary
format conversion. The code preserves the old value of the rectangle
texture binding instead of clobbering it because Cogl expects to be
able to cache this value to avoid redundant glBindTexture
calls. Finally, the function uses cogl_object_set_data to
automatically destroy the GL texture when the Cogl texture is
destroyed. This avoids having to have special code to destroy the cogl
texture.

Review of attachment 191910 [details] [review]:

Not that I believe quite in running Mutter with either GLX or GLES by runtime switch :-) ... but this looks fine to commit other than a few style nits.

::: src/compositor/meta-texture-rectangle.c
@@ +31,3 @@
+
+static void
+(* pf_glGetIntegerv) (GLenum pname, GLint *params);

Would prefer these on one line

@@ +50,3 @@
+rectangle_texture_destroy_cb (void *user_data)
+{
+#ifdef GL_TEXTURE_RECTANGLE_ARB

The #ifdef needs to be outside, or this won't compile when the symbol is missing because of warnings for an unused function

@@ +90,3 @@
+                   internal_gl_format, width, height,
+                   0, internal_gl_format,
+                   GL_UNSIGNED_BYTE, NULL);

This is the only potential problem I see with this patch - for indirect GLX, using NULL for glTextImage2D and setting the data separately is a big lose, because there is no GLX protocol for it - it sends a bunch of zeros over the wire. But indirect GLX really isn't a target.

@@ +120,3 @@
+                             data);
+
+#endif

For any #ifdef of more than a few lines, I like to have

#endif /* GL_TEXTURE_RECTANGLE_ARGB */

Created attachment 192235 [details] [review]
Use a utility function to create GL_ARB_texture_rectangles

Here is the patch again with the suggested tweaks, apart from fixing
using NULL for upload. Can we just hope that anyone using indirect GLX
will also have NPOT texture support instead of using rectangle
textures? :) In the long run we want to expose a
cogl_texture_rectangle_new function instead of having to create a
foreign texture so we could eventually get rid of this.

Review of attachment 192235 [details] [review]:

Wasn't really suggesting doing anything about the NULL texture over indirect GLX issue.

> In the long run we want to expose a
> cogl_texture_rectangle_new function instead of having to create a
> foreign texture so we could eventually get rid of this.

Hmm :-) ... the creating the foreign rectangle thing was a compromise we made because earlier there was resistance to adding public rectangle texture support to COGL

Thanks, I've pushed it as fccd626

Yeah, ok, so we've sort of changed our minds. However I think originally we didn't want to make it automatically use rectangle textures when cogl_texture_new is called because they don't have the same semantics as NPOT textures (ie, no mipmapping or repeats). We still want to keep it that way but we want to add API to explicitly create cogl textures for all of the primitive types (eg we'd even add a cogl_texture_2d_new). That way you can be sure to get a direct texture with whatever limitations your driver has on it, but if you just create a texture with cogl_texture_new you'll get the same magic texture which may be sliced or atlased or whatever.