Created attachment 147148 [details] [review]
Nice looking scaledown with mipmap emulation

Add MutterTextureTower, an abstraction for getting a image with
the right level of detail for rendering at a particular scale,
by manually scaling down by powers of two.

This results in much better looking scaled window images when
mipmaps can't be used with texture_from_pixmap (which is the
typical case for current GL drivers.)

When framebuffer objects are available, they are used to do
the scaledown using the GPU without having to pull the data
back from video memory. A software codepath is also available
for the case when FBO's are not present, though performance
will suffer

Tried this out on my 965.  Looks great.  A huge usability improvement.  Noticed two problems though.  Though I can't be completely sure they are a result of this.

 1. Most of the time when I take a screenshot in the overview the screenshot save dialog doesn't appear in the window picker - but the icon for the window does.
 2. When I created a new workspace some of the existing windows in the picker turned bright yellow.

Attaching screenshots.

Created attachment 147150 [details]
screenshot of missing window

Created attachment 147151 [details]
screenshot of yellow windows

Created attachment 147316 [details] [review]
Fix crash with 0 scale (SQUASH)

Fix for crasher when rendering at a scale of 0

Created attachment 147318 [details] [review]
Fix problems with newly created windows (SQUASH)

Relying on update_area to catch changes to the base texture
didn't work, do it a different way.

Created attachment 147390 [details] [review]
Fix some errors in invalid region computation (SQUASH)

(In reply to comment #0)
> Generally I think this is in good shape. Two notes:
> 
> - The main thing not handled here is detecting when mipmaps
>   *are* supported with texture_from_pixmap - right now, I think
>   the Clutter code will cause them to be generated and we'll
>   ignore them and do the maual scale-down anyways.
> 
>   That needs investigation/fixing before this lands.

we will probably add a CoglFeature for mipmapping support on tfp textures

Created attachment 147596 [details] [review]
Nice looking scaledown with mipmap emulation

- Squashes together all the fixes
 - Handles the case of mipmaps being supported by dectecting the
   behavior of ClutterGLXTexturePixmap in the two cases
 - Removes the debug drawing of a yellow background on the window
   before updating it

I think it's good to go at this point.

(In reply to comment #8)
> (In reply to comment #0)
> > Generally I think this is in good shape. Two notes:
> > 
> > - The main thing not handled here is detecting when mipmaps
> >   *are* supported with texture_from_pixmap - right now, I think
> >   the Clutter code will cause them to be generated and we'll
> >   ignore them and do the maual scale-down anyways.
> > 
> >   That needs investigation/fixing before this lands.
> 
> we will probably add a CoglFeature for mipmapping support on tfp textures

I wonder if perhaps the right way would be to add ClutterX11TexturePixmap API to query a particular actor for support?

The thing is it isn't really a global yes/no kind of feature. Ultimately it's an attribute of fbconfigs that determines if mipmapping tfps is supported, and the fbconfig is currently owned by ClutterGLXTexturePixmap.

Eventually Cogl will own the fbconfig when we add CoglTexturePixmap support, and I expect we'll add API at the same time to check for mipmapping support. Actually since the question is also interesting for other texture types, I wonder if we should add a function like cogl_texture_supports_mipmapping()? The problem here is that until we migrate the GLX tfp down from Clutter to Cogl we will probably just return FALSE always, since the current Cogl texture is just a foreign GL texture.

Created attachment 147683 [details] [review]
[x11-texture-pixmap] support querying mipmapping support

Many drivers don't support mipmapping with GLX_texture_from_pixmap so to
allow applications to provide fallbacks for poor scaling quality they can
query if a given ClutterX11TexturePixmap actor supports mipmapping.

This is done on a per actor basis instead of as a feature flag because
support depends on the underlying fbconfig that is owned by the actor.  The
constraints used to choose an fbconfig may change according to actor
specific settings and so even if we know *some* fbconfigs support tfp
mipmapping; until we apply the final constraints we can't know for sure.

The API is a bit awkward to use because it's documented that the actor must
be realized and have an associated Pixmap, but I can't see a way around
that.

Review of attachment 147596 [details] [review]:

::: src/compositor/mutter-texture-tower.c
@@ +445,3 @@
+texture_tower_revalidate_fbo (MutterTextureTower *tower,
+                              int                 level)
+{

This is a bit fragile because it has to make some assumptions about what state needs to be saved/restored, which may change in latter versions of Clutter, but it's probably better than trying to work around the broken fbo code in 1.0.

As a stop-gap solution it should hopefully work, though I think it may break for Clutter 1.2. If you want some overlap with early Clutter 1.2 before everyone hopefully switches to your code below I think you should also save/restore shader state. Damien has been working on implementing CoglMaterial with ARBfp for better performance on programmable GPUs so even if you know you don't use the Cogl shader API this could cause incorrect results.

Assuming you should bind framebuffer 0 at the end seems a bit fragile too given that Clutter 1.2 now does framebuffer binding lazily when you come to draw, but I think you're fairly likely to get away with it.

@@ +582,3 @@
+/* This code is approximately right for Clutter-1.2, but hasn't been tested */
+static gboolean
+texture_tower_revalidate_fbo (MutterTextureTower *tower,

Cogl defines the default model space (I.e. with identity matrices loaded) in the same way as OpenGL, with an origin in the center of the screen, x+ going right, y+ going up, and z+ coming out towards the viewer with coordinates ranging from from -1 to 1 on each axis across the current viewport. When you create a new offscreen draw buffer it will have this coordinate space. We don't yet provide a utility function that makes it easy to setup a clutter style model space with a top left origin, so you currently need to duplicate the logic in _cogl_setup_viewport. Hopefully I'll add something before we release Clutter 1.2 though.

Also to possibly be usable in more situations it might be good to use cogl_push_draw_buffer and cogl_pop_draw_buffer instead of explicitly restoring to the window buffer.

Review of attachment 147683 [details] [review]:

Looks like the API would work to me, implementation looks good, except that it doesn't seem ABI compat.

Also, see:

 http://bugzilla.openedhand.com/show_bug.cgi?id=1877

the RFE I filed for this feature.

::: clutter/x11/clutter-x11-texture-pixmap.c
@@ +1404,3 @@
+ * mipmaps.
+ *
+ * Return value: TRUE if the actor supports mipmapping the texture-from-pixmap

As a nitpack, should use %TRUE and %FALSE in a gtk-doc comment

::: clutter/x11/clutter-x11-texture-pixmap.h
@@ +54,3 @@
                                            gint                     width,
                                            gint                     height);
+  gboolean      (*supports_mipmapping)    (ClutterX11TexturePixmap *texture);

This is a binary compatibility break :-( - Mutter compiled before this change won't work with Clutter compiled after this change. 

I guess it would be possible to say that Clutter-1.2 is only bin-compatible if you aren't doing crazy stuff like deriving from ClutterX11TexturePixmap.

You could move the can_mipmap flag to the base class and add a setter that ClutterGLXTexturePixmap uses. Or you could just make this ClutterGLXTexturePixmap API.

Created attachment 148140 [details] [review]
Nice looking scaledown with mipmap emulation

Here's a new version of the patch with a tested Clutter-1.2 code path.

Hitches I hit:

 * I'd like a way to be able to create a cogl_offscreen without
   a stencil buffer. Right now I'm saving the fbo's around so
   I can do updates without recreating them, but with stencil
   buffers attached there's a real cost to that. And there is
   also a real cost to recreating them since the stencil buffers
   will need syscall traffic to allocate.

 * In cogl_offscreen_new_to_texture, I needed:

-  if (tex_gl_target != GL_TEXTURE_2D)
+  if (tex_gl_target != GL_TEXTURE_2D && tex_gl_target != GL_TEXTURE_RECTANGLE_ARB)
     return COGL_INVALID_HANDLE;

 * I was suprised to have to pass different parameters to cogl_ortho() than
   the pure GL version passed to glOrtho() (reversing the top and bottom
   parameters.) If there's a discrepancy there it possibly should be highlighted
   in the docs. (cogl_ortho() doesn't have useful docs. glOrtho() is behaving
   as documented.)

   Might be something I'm missing about how I'm using it.

 * I saw some drawing glitches I didn't see with the pure-GL version;
   like occasionally having the window-buffer drawing flash to blue
   for a frame. This might be the Cogl code triggering Radeon driver
   bugs, or it might be bugs in the Cogl code. Didn't investigate.

I looked into adding shader state save/restore a bit, but it looked
painful (it's not covered by glPushAttr(), so you'd have to fetch individual
attributes for both GLSL and arb_fragment_program and save/restore yourself),
so I decided to skip it.

Comment on attachment 148140 [details] [review]
Nice looking scaledown with mipmap emulation

I'm happy with the patche myself, and no problems have
showed up in testing of the latest version by myself and Jon, so
while more review might be nice (I appreciate Robert going through 
the cogl/GL portions), I'm going to go ahead and push.

The patch isn't getting any better by sitting here.

Attachment 148140 [details] pushed as 47af6a0 - Nice looking scaledown with mipmap emulation

Can we move the discussion of the clutter patch to the bugzilla.openedhand.com bug and close this one?

(In reply to comment #14)

I was away last week so wasn't able to follow up, but before we close this bug I'll quickly reply:

> Created an attachment (id=148140) [details] [review]
> Nice looking scaledown with mipmap emulation
> 
> Here's a new version of the patch with a tested Clutter-1.2 code path.
> 
> Hitches I hit:
> 
>  * I'd like a way to be able to create a cogl_offscreen without
>    a stencil buffer. Right now I'm saving the fbo's around so
>    I can do updates without recreating them, but with stencil
>    buffers attached there's a real cost to that. And there is
>    also a real cost to recreating them since the stencil buffers
>    will need syscall traffic to allocate.
CoglDrawBuffers (or CoglFramebuffers if renamed) will eventually provide complete control over the ancillary buffers so you can avoid having a stencil buffer, but that feature is unlikely to arrive for Clutter 1.2.

> 
>  * In cogl_offscreen_new_to_texture, I needed:
> 
> -  if (tex_gl_target != GL_TEXTURE_2D)
> +  if (tex_gl_target != GL_TEXTURE_2D && tex_gl_target !=
> GL_TEXTURE_RECTANGLE_ARB)
>      return COGL_INVALID_HANDLE;
I've just pushed this fix to master; thanks.

> 
>  * I was suprised to have to pass different parameters to cogl_ortho() than
>    the pure GL version passed to glOrtho() (reversing the top and bottom
>    parameters.) If there's a discrepancy there it possibly should be
> highlighted
>    in the docs. (cogl_ortho() doesn't have useful docs. glOrtho() is behaving
>    as documented.)
> 
>    Might be something I'm missing about how I'm using it.
cogl_ortho *should* behave like glOrtho and it should be documented. On quick code inspection I didn't see a trivial muddle up of the top/bottom arguments, but there is probably a silly bug in Cogl.

> 
>  * I saw some drawing glitches I didn't see with the pure-GL version;
>    like occasionally having the window-buffer drawing flash to blue
>    for a frame. This might be the Cogl code triggering Radeon driver
>    bugs, or it might be bugs in the Cogl code. Didn't investigate.

I'm not sure about the artifacts either. It's quite possible there's a bug in Cogl. I suppose it would be good to know if this is reproducible with the swrast driver and if so a standalone unit test would be ideal. I'll wait for more details before investigating this.

> 
> I looked into adding shader state save/restore a bit, but it looked
> painful (it's not covered by glPushAttr(), so you'd have to fetch individual
> attributes for both GLSL and arb_fragment_program and save/restore yourself),
> so I decided to skip it.
right. You may want to keep an eye on Damien's wip/arbfp-combiners branch. We'd like to get this working for Clutter 1.2. If we do I think you'll have to switch to the cogl offscreen based code path.

This has been fixed since forever. It's now broken again due to Clutter changes (see bug 627087, http://bugzilla.clutter-project.org/show_bug.cgi?id=1877), but no point in keeping it open.

And if cogl_ortho() has coordinates wrong, presumably it will have to stay that way for compat at this point. [ Not going the spend the time right now to figure out all the different GL matrices and try to confirm or deny that it's different from glOrtho() as my experience indicated. ]