GNOME Bugzilla – Bug 660941
Make MetaWindowActor a ClutterActor
Last modified: 2012-02-04 01:01:38 UTC
There were a lot of hacky parts of the code working around ClutterX11PixmapTexture problems. Port the stex to a ClutterTexture to avoid some of these.
Created attachment 198278 [details] [review] MetaShapedTexture: Junk old "notify" hack The bug in question hasn't existed for a long while now...
Created attachment 198279 [details] [review] MetaShapedTexture: Make into a ClutterTexture
ClutterTexture is 99% as bad as a base class. * It has a very poorly defined interaction between texture and pipeline, in terms of which one is set, that a subclass is going to * It has dozens of methods that aren't applicable to a specific subclass. I don't see any advantage of deriving from ClutterTexture compared to just deriving from ClutterActor.
(In reply to comment #3) > I don't see any advantage of deriving from ClutterTexture compared to just > deriving from ClutterActor. It just seemed we'd have to copy a lot of the things ClutterTexture has -- update_fbo, for instance.
(In reply to comment #4) > (In reply to comment #3) > > I don't see any advantage of deriving from ClutterTexture compared to just > > deriving from ClutterActor. > > It just seemed we'd have to copy a lot of the things ClutterTexture has -- > update_fbo, for instance. update_fbo() only matters for ClutterTexture created with clutter_texture_new_from_actor - exactly the kind of random cruft ClutterTexture() has that makes it a bad base class.
Created attachment 198355 [details] [review] MetaShapedTexture: Make into a ClutterActor ClutterTexture (and for that matter, ClutterTextureX11Pixmap) has a lot of cruft, as well magic for things that we, as a window manager, tend to handle ourselves. Additionally, ClutterTexture/ClutterTextureX11Pixmap use materials to paint that we just don't use.
Of note, ClutterX11Pixmap is inside Clutter so it has access to a special API that allows it to queue a clipped redraw. I just queue a regular redraw in this patch, but I filed bug #660997 to help fix this.
Created attachment 198374 [details] [review] MetaShapedTexture: Make into a ClutterActor ClutterTexture (and for that matter, ClutterTextureX11Pixmap) has a lot of cruft, as well magic for things that we, as a window manager, tend to handle ourselves. Additionally, ClutterTexture/ClutterTextureX11Pixmap use materials to paint that we just don't use. Remove an inadvertent "cogl_handle_ref" that caused memory leaks and criticals.
For reference, these patches make mutter/gnome-shell work with cogl from git. I would test with clutter from git, but clutter master is having a hard time finding glib for some reason
Review of attachment 198374 [details] [review]: Some comments on this patch, which apparently is still needed to build against the 3.4 stack. ::: src/compositor/meta-shaped-texture.c @@ +479,3 @@ MetaShapedTexturePrivate *priv = stex->priv; + if (clutter_actor_should_pick_paint (actor)) Why did you remove the (priv->shape_region == NULL) check? @@ +572,2 @@ if (create_mipmaps != priv->create_mipmaps) + meta_texture_tower_set_base_texture (priv->paint_tower, priv->texture); I think you're missing a priv->create_mipmaps = create_mipmaps; here. ::: src/compositor/meta-window-actor.c @@ +958,3 @@ + 0, + cogl_texture_get_width (texture), + cogl_texture_get_height (texture)); Why not clutter_actor_get_width/height here instead? This looks like the only place why you need meta_shaped_texture_get_texture() for, but you actually only need the width/height.
Review of attachment 198374 [details] [review]: ::: src/compositor/meta-shaped-texture.c @@ +479,3 @@ MetaShapedTexturePrivate *priv = stex->priv; + if (clutter_actor_should_pick_paint (actor)) I didn't think ClutterActor provided a default pick. Looks like it does, so I'll add it back. ::: src/compositor/meta-window-actor.c @@ +958,3 @@ + 0, + cogl_texture_get_width (texture), + cogl_texture_get_height (texture)); get_texture() is used in other places in JS, so we can't just remove it.
(In reply to comment #11) > ::: src/compositor/meta-window-actor.c > @@ +958,3 @@ > + 0, > + cogl_texture_get_width (texture), > + cogl_texture_get_height (texture)); > > get_texture() is used in other places in JS, so we can't just remove it. I don't understand...you are adding meta_shaped_texture_get_texture() with your patch, so how can it be used elsewhere?
(In reply to comment #12) > I don't understand...you are adding meta_shaped_texture_get_texture() with your > patch, so how can it be used elsewhere? Whoops, I was thinking of meta_window_actor_get_texture. Though we'll still need something to replace the use of clutter_texture_get_cogl_texture in shell_global_screenshot_area.
Bisected to cogl commit 4c3dadd35e4657a151025118814534d05091d4db
Created attachment 201096 [details] [review] Fix with cogl master The code here was always incorrect -- we were processing damage events for windows without having a texture. Before, this didn't matter, as cogl_texture_get_width silently returned for invalid handles. Cogl commit 4c3dadd35e4657a151025118814534d05091d4db changed this -- This is an alternate fix, one that's much less invasive. I'd like to get both in, because both strategies are valuable.
Created attachment 201097 [details] [review] meta-window-group: Fix types typo gboolean is a typedef for int, so it doesn't matter too much
Of note, after you apply these patches to mutter, you will need the fix in bug #663733 to keep the left side of your desktop from flashing all over the place.
Review of attachment 201097 [details] [review]: Sure
Review of attachment 201096 [details] [review]: Want to try and improve the subject and commit message? - Subject is entirely uninformative - Body should give some hint that the changes in the two places are not both required to fix the bug
Created attachment 201189 [details] [review] Fix a crash with the latest cogl The code here was always incorrect - we were processing damage events for windows without having a texture. Before, this didn't matter, as cogl_texture_get_width silently returned 0 for invalid handles. Cogl commit 4c3dadd35e4657a151025118814534d05091d4db changed this. The fix here involves two strategies. First, we try to guard MetaTextureTower from invalid textures. Second, we try not to go down the path that eventually calls meta_shaped_texture_update_area, by not handling damage events if the textures aren't prepared. I'm not good at writing good subjects. Is this better?
Comment on attachment 201097 [details] [review] meta-window-group: Fix types typo Attachment 201097 [details] pushed as 1596d1a - meta-window-group: Fix types typo
Review of attachment 201189 [details] [review]: s/textures aren't prepared/don't have a texture for the window/ change the subject to, mm: Fix cogl crash from updating non-existent texture
Comment on attachment 201189 [details] [review] Fix a crash with the latest cogl Amended and pushed. Thanks!
Created attachment 201192 [details] [review] MetaShapedTexture: Make into a ClutterActor ClutterTexture (and for that matter, ClutterTextureX11Pixmap) has a lot of cruft, as well magic for things that we, as a window manager, tend to handle ourselves. Additionally, ClutterTexture/ClutterTextureX11Pixmap use materials to paint that we just don't use. So, the crash is fixed, let's turn this back into what it was. Rebased the patch to master.
Created attachment 203593 [details] [review] MetaShapedTexture: Make into a ClutterActor ClutterTexture (and for that matter, ClutterTextureX11Pixmap) has a lot of cruft, as well magic for things that we, as a window manager, tend to handle ourselves. Additionally, ClutterTexture/ClutterTextureX11Pixmap use materials to paint that we just don't use. Now that Clutter has API to queue a clipped redraw, here's an updated patch.
Created attachment 203990 [details] [review] MetaShapedTexture: Make public and into a ClutterActor ClutterTexture (and for that matter, ClutterTextureX11Pixmap) has a lot of cruft, as well magic for things that we, as a window manager, tend to handle ourselves. Additionally, ClutterTexture/ClutterTextureX11Pixmap use materials to paint that we just don't use. Additionally, make it public. GNOME Shell was already assuming that any MetaShapedTexture was also a ClutterTexture, and we need to replace these bits with new API for GNOME Shell to use. Updated patch, made MetaShapedTexture public because the Shell needs to access internals.
Created attachment 203991 [details] [review] MetaShapedTexture: Add a new method to flatten the shaped texture into pixels This will be used by the Shell to implement a screenshot_window method. This will be used by the shell's screenshot_window method, as it needs to access the raw texture data of the stex.
Review of attachment 198278 [details] [review]: This patch leaves no code that calls meta_texture_tower_set_base_texture() with the appropriate texture, it could not possibly work. It is likely made irrelevant by later patches in the series can be squashed with them.
Review of attachment 203990 [details] [review]: In OO terminology MetaShapedTexture was already a ClutterActor "Make public and directly derive from ClutterActor" "as well magic" probably was meant to be "as well as magic", but I'd say something like: ClutterTexture has many features that we simply don't use and don't make sense for a subclass with custom drawing. Deriving directly from ClutterActor simplifies our code by avoiding workarounds and makes things more robust. Patch looks mostly good, but a few things: ::: src/compositor/meta-shaped-texture.c @@ -468,3 @@ - if (priv->shape_region == NULL) - CLUTTER_ACTOR_CLASS (meta_shaped_texture_parent_class) - ->pick (actor, color); The removal of this is unexpected and seems unnecessary, since ClutterTexture was just chaining up to ClutterActor for us. @@ +572,2 @@ if (create_mipmaps != priv->create_mipmaps) + meta_texture_tower_set_base_texture (priv->paint_tower, priv->texture); You need the old code here - you don't even change priv->create_mipmaps any more @@ +644,3 @@ + cogl_material_set_layer (priv->material_unshaped, 0, cogl_tex); + + if (G_LIKELY (cogl_tex != COGL_INVALID_HANDLE)) G_LIKELY/G_UNLIKELY should only be used in cases like g_return_if_fail() where the case is genuinely and provably unlikely. The cost of a wrong marking is much bigger than not having a marking. @@ +674,3 @@ + * @pixmap: The pixmap you want the stex to assume + * + * Returns: (transfer none): a #CoglHandle for the pixmap No, don't return a random value out of a setter just because one caller needs it, just add an accessor. @@ +790,3 @@ +{ + CoglHandle texture; + g_return_if_fail (META_IS_SHAPED_TEXTURE (stex)); blank lines around the g_return_if_fail() add another g_return_if_fail (stex->priv->texture != COGL_INVALID_HANDLE);
Review of attachment 203991 [details] [review]: ::: src/compositor/meta-shaped-texture.c @@ +802,3 @@ +dump_mask (guint8 *mask_data, + guint width, + guint height) Don't think this should be left in the patch - not that useful. @@ +832,3 @@ + * @data: A valid pointer to omit the texture data to. This pointer + * should be a memory block of at least width * height * 4, as this + * method uses %CLUTTER_CAIRO_FORMAT_ARGB32. Why don't we just return a cairo image surface: that will encapsulate all the rowstride, memory management, etc. @@ +846,3 @@ + + if (clip != NULL) + texture = cogl_texture_new_from_sub_texture (texture, You leak this and the corresponding subtexture for the mask @@ +851,3 @@ + clip->width, + clip->height); + cogl_flush(); Not necessary - cogl_flush() flushes drawing. @@ +869,3 @@ + + amount = cogl_texture_get_width (mask_texture) * cogl_texture_get_height (mask_texture); + mask_data = g_newa (guint8, amount); Was this a typo that compiled? You definitely don't want to use alloca() to arbitrarily large chunks of data. Switch to g_malloc and add a g_free @@ +876,3 @@ + for (i = 0; i < amount; i++) + { + data[i*4+0] *= mask_data[i] / 255.0; It's probably just leftover instinct, and perhaps irrelevant in the days of SSE2/SSE3 but floating math per pixel has traditionally been a bad idea. From st-node-drawing.c the macros: #define NORM(x) (t = (x) + 127, (t + (t >> 8)) >> 8) #define MULT(c,a) NORM(c*a) Do the math you want. (with a 'guint t' variable) (http://people.redhat.com/otaylor/pixel-converting.html) Or you could do this in cairo - use a DEST_IN operator if I recall correctly.
Created attachment 206712 [details] [review] MetaShapedTexture: Make public and directly derive from ClutterActor ClutterTexture has many features that we simply don't use and don't make sense for a subclass with custom drawing. Deriving directly from ClutterActor simplifies our code by avoiding workarounds and makes things more robust. Additionally, make it public. GNOME Shell was already assuming that any MetaShapedTexture was also a ClutterTexture, and we need to replace these bits with new API for GNOME Shell to use. OK, I think I've fixed most of your issues here.
Created attachment 206713 [details] [review] MetaShapedTexture: Add a new method to flatten the shaped texture into pixels This will be used by the Shell to implement a screenshot_window method. (In reply to comment #30) > Review of attachment 203991 [details] [review]: > > ::: src/compositor/meta-shaped-texture.c > @@ +802,3 @@ > +dump_mask (guint8 *mask_data, > + guint width, > + guint height) > > Don't think this should be left in the patch - not that useful. OK. Scrapped. > @@ +832,3 @@ > + * @data: A valid pointer to omit the texture data to. This pointer > + * should be a memory block of at least width * height * 4, as this > + * method uses %CLUTTER_CAIRO_FORMAT_ARGB32. > > Why don't we just return a cairo image surface: that will encapsulate all the > rowstride, memory management, etc. That makes a lot more sense. > @@ +846,3 @@ > + > + if (clip != NULL) > + texture = cogl_texture_new_from_sub_texture (texture, > > You leak this and the corresponding subtexture for the mask I thought about various ways to clean this up, but this is the cleanest that I could come up with. I'm not too proud of it. > @@ +851,3 @@ > + clip->width, > + clip->height); > + cogl_flush(); > > Not necessary - cogl_flush() flushes drawing. This was copied from the shell's screenshot methods. I assumed it was necessary, but drago01 said it wasn't necessary there either. > @@ +869,3 @@ > + > + amount = cogl_texture_get_width (mask_texture) * cogl_texture_get_height > (mask_texture); > + mask_data = g_newa (guint8, amount); > > Was this a typo that compiled? You definitely don't want to use alloca() to > arbitrarily large chunks of data. Switch to g_malloc and add a g_free I switched to cairo instead. > @@ +876,3 @@ > + for (i = 0; i < amount; i++) > + { > + data[i*4+0] *= mask_data[i] / 255.0; > > It's probably just leftover instinct, and perhaps irrelevant in the days of > SSE2/SSE3 but floating math per pixel has traditionally been a bad idea. From > st-node-drawing.c the macros: > > #define NORM(x) (t = (x) + 127, (t + (t >> 8)) >> 8) > #define MULT(c,a) NORM(c*a) > > Do the math you want. (with a 'guint t' variable) > > (http://people.redhat.com/otaylor/pixel-converting.html) > > Or you could do this in cairo - use a DEST_IN operator if I recall correctly. cairo is probably the best way to do this. Using DEST_IN worked perfectly, thanks!
Created attachment 206717 [details] [review] MetaShapedTexture: Add a new method to flatten the shaped texture into pixels This will be used by the Shell to implement a screenshot_window method. I forgot to remove the line 'clip = NULL;', which was used for debugging purposes.
Review of attachment 206712 [details] [review]: Looks good ::: src/compositor/meta-window-actor.c @@ +1838,3 @@ if (priv->back_pixmap == None) { + CoglHandle tex; just write out texture
Review of attachment 206717 [details] [review]: Looking close ::: src/compositor/meta-shaped-texture.c @@ +798,3 @@ + +/** + * meta_shaped_texture_as_flattened_data: this is a overly complex and obscure function name. I'd just call it get_image() or something like that. @@ +800,3 @@ + * meta_shaped_texture_as_flattened_data: + * @stex: A #MetaShapedTexture + * @clip: A clipping rectangle, to help prevent extra processing describe this in sufficient detail - what happens if this rectangle is partly or entirely outside of the bounds of the shaped texture? Note that I'm pretty sure that meta_window_get_outer_rect() is *NOT* synchronized with the size of the texture pixmap at all times - so something like the code currently in gnome-shell isn't guaranteed to have clip contained in the texture pixmap - so it's almost certainly better to do something reasonable than to just define it as undefined behavior that might crash. @@ +803,3 @@ + * + * Returns: (transfer full): a new cairo surface to be freed with + * cairo_surface_destroy(). You don't actually document the function, you just have some brief notes about the function parameters. @@ +827,3 @@ + cogl_texture_get_height (texture)); + + cogl_texture_get_data (texture, CLUTTER_CAIRO_FORMAT_ARGB32, 0, fix the stride here too, not just for the mask
Created attachment 206725 [details] [review] MetaShapedTexture: Add a new method to flatten the shaped texture into pixels This will be used by the Shell to implement a screenshot_window method. This should be everything.
Review of attachment 206725 [details] [review]: ::: src/compositor/meta-shaped-texture.c @@ +893,3 @@ + * In the case that the clipping rectangle is partially or fully + * outside the bounds of the texture, this function has undefined + * behaviour. Are you saying the caller needs to call meta_shaped_texture_get_texture() first and get its size, and then clip the clip rectangle? If so, you need to say it. If not, I think you need to rethink the undefined behavior.
Created attachment 206730 [details] [review] MetaShapedTexture: Add a new method to flatten the shaped texture into pixels This will be used by the Shell to implement a screenshot_window method. Here.
Review of attachment 206730 [details] [review]: Looks good!
(git-bz was having some trouble, this is fixed)