GNOME Bugzilla – Bug 628195
rounded corner aa
Last modified: 2011-08-26 17:07:37 UTC
Mutter should really support antialiased rounded window corners. The current situation really make windows decorations look dated.
yes please. it looks so bad that we're considering just using 90 degree angles for GNOME 3.
*** Bug 650115 has been marked as a duplicate of this bug. ***
Created attachment 191740 [details] [review] MetaShapedTexture: Use a proper stride, calculated by cairo This will help us when painting directly on to the mask texture with cairo, which is needed for rounded corner AA.
Created attachment 191741 [details] [review] frame: Add "get_corner_radiuses" chain
Created attachment 191742 [details] [review] Antialised corners
This is hosted on top of my invisible borders stuff in bug #644930.
Created attachment 192216 [details] [review] MetaShapedTexture: Use a proper stride, calculated by cairo This will help us when painting directly on to the mask texture with cairo, which is needed for rounded corner AA.
Created attachment 192217 [details] [review] frame: Add "get_corner_radiuses" chain Fix corner radius. Nicely spotted, Owen!
Created attachment 192218 [details] [review] Antialiased corners Fix a whole set of changes that actually gets it working properly!
Created attachment 192410 [details] [review] MetaShapedTexture: Use a proper stride, calculated by cairo This will help us when painting directly on to the mask texture with cairo, which is needed for rounded corner AA. Rebase on top of MetaTextureRectangle cogl changes.
Created attachment 192411 [details] [review] frame: Add "get_corner_radiuses" chain
Created attachment 192412 [details] [review] Antialiased corners Rebased on top of newer invisible border changes.
Review of attachment 192410 [details] [review]: Looks good
Review of attachment 192411 [details] [review]: ::: src/ui/frames.c @@ +828,3 @@ +meta_frames_get_corner_radiuses (MetaFrames *frames, + Window xwindow, + int *tl, your fingers won't fall off if you write these out as top_left, etc. @@ +841,3 @@ + + if (tl) + *tl = fgeom.top_left_corner_rounded_radius + sqrt(fgeom.top_left_corner_rounded_radius); A) You need to comment the weirdness B) We really want to get as close as possible to pixel exact with the old corners. The old corner code is using this value as a floating point value. Truncation to an int here can make you as much as 1 pixel off rouding would reduce that to 0.5 pixels, but I think it's best to pass this around as a float and draw with the same float value in Cairo when drawing the AA corners.
(In reply to comment #14) > Review of attachment 192411 [details] [review]: > > ::: src/ui/frames.c > @@ +828,3 @@ > +meta_frames_get_corner_radiuses (MetaFrames *frames, > + Window xwindow, > + int *tl, > > your fingers won't fall off if you write these out as top_left, etc. > > @@ +841,3 @@ > + > + if (tl) > + *tl = fgeom.top_left_corner_rounded_radius + > sqrt(fgeom.top_left_corner_rounded_radius); > > A) You need to comment the weirdness What should I say? > B) We really want to get as close as possible to pixel exact with the old > corners. The old corner code is using this value as a floating point value. > Truncation to an int here can make you as much as 1 pixel off rouding would > reduce that to 0.5 pixels, but I think it's best to pass this around as a float > and draw with the same float value in Cairo when drawing the AA corners. OK. I'll have to replace the region hack I did, because a region contains integer rectangles, unfortunately. Not like it matters too much, it was a big hack. I'll just have to create a new struct that contains four cairo_rectangle_t's.
Review of attachment 192412 [details] [review]: Definitely good to see progress towards this! Needs an actual commit message - describe the strategy being used here. ::: src/compositor/meta-shaped-texture.c @@ +73,3 @@ cairo_region_t *shape_region; + cairo_region_t *corners; + cairo_region_t *opaque_corners; opaque_corners is leaked @@ +189,3 @@ + /* Update opaque pixels */ + int x, y; + for (y = rect->y; y < rect->height; y ++) y < rect->height is buggy, and a stray space @@ +191,3 @@ + for (y = rect->y; y < rect->height; y ++) + { + for (x = rect->x; x < rect->width; x ++) same @@ +197,3 @@ + w ++; + + if (w > 0) w > x @@ +202,3 @@ + tmp.x = x; + tmp.y = y; + tmp.width = w; w - x @@ +220,3 @@ + MetaShapedTexturePrivate *priv = stex->priv; + int i; + gdouble x, y, radius; don't use gdouble in new code (especially if there is int usage right next to it) @@ +704,3 @@ +cairo_region_t * +meta_shaped_texture_get_opaque_corners (MetaShapedTexture *stex) how about docs? I don't think anybody would guess what this does from the name. @@ +706,3 @@ +meta_shaped_texture_get_opaque_corners (MetaShapedTexture *stex) +{ + return stex->priv->opaque_corners; should call ensure_mask() to avoid weirness where this is sometimes wrong @@ +717,3 @@ + * Set a #cairo_region_t containing four squares representing the area of corners + * to be painted over, in the order of "top left", "top right", "bottom left", + * "bottom right". The rectangles must be square. Bizarre interface. Just use four floats; if you are tired of passing four floats everywhere, you could add a structure (which would eliminate the relevance of ordering - which isn't exactly obvious) @@ +735,3 @@ + } + + priv->corners = corners; Needs to dirty the mask ::: src/compositor/meta-window-actor.c @@ +1671,3 @@ + region = meta_shaped_texture_get_opaque_corners (META_SHAPED_TEXTURE (priv->actor)); + if (region) + return region; How can it be right to be returning either this or shape_region, you want one or the other, right? It seems to me that meta_shaped_texture should just have a method that returns a region of guaranteed opaque pixels - what's "obscured" is determined by what the meta shaped texture draws.
(In reply to comment #16) > Review of attachment 192412 [details] [review]: > > Definitely good to see progress towards this! Needs an actual commit message - > describe the strategy being used here. > > ::: src/compositor/meta-shaped-texture.c > @@ +73,3 @@ > cairo_region_t *shape_region; > + cairo_region_t *corners; > + cairo_region_t *opaque_corners; > > opaque_corners is leaked > > @@ +189,3 @@ > + /* Update opaque pixels */ > + int x, y; > + for (y = rect->y; y < rect->height; y ++) > > y < rect->height is buggy, and a stray space > @@ +191,3 @@ > + for (y = rect->y; y < rect->height; y ++) > + { > + for (x = rect->x; x < rect->width; x ++) > > same > > @@ +197,3 @@ > + w ++; > + > + if (w > 0) > > w > x > > @@ +202,3 @@ > + tmp.x = x; > + tmp.y = y; > + tmp.width = w; > > w - x > > @@ +220,3 @@ > + MetaShapedTexturePrivate *priv = stex->priv; > + int i; > + gdouble x, y, radius; > > don't use gdouble in new code (especially if there is int usage right next to > it) > > @@ +704,3 @@ > > +cairo_region_t * > +meta_shaped_texture_get_opaque_corners (MetaShapedTexture *stex) > > how about docs? I don't think anybody would guess what this does from the name. > > @@ +706,3 @@ > +meta_shaped_texture_get_opaque_corners (MetaShapedTexture *stex) > +{ > + return stex->priv->opaque_corners; > > should call ensure_mask() to avoid weirness where this is sometimes wrong > > @@ +717,3 @@ > + * Set a #cairo_region_t containing four squares representing the area of > corners > + * to be painted over, in the order of "top left", "top right", "bottom left", > + * "bottom right". The rectangles must be square. > > Bizarre interface. Just use four floats; if you are tired of passing four > floats everywhere, you could add a structure (which would eliminate the > relevance of ordering - which isn't exactly obvious) It is an extremely bizarre interface. But because of the stex/mwa abstraction between them, and because 0,0 and tex_width/tex_height aren't realistic bounding points any more, I have to give it something to tell where the actual bounds of the window are so it can know where to paint corners. I've been leaning on passing it four cairo_rectangle_ts. > @@ +735,3 @@ > + } > + > + priv->corners = corners; > > Needs to dirty the mask > > ::: src/compositor/meta-window-actor.c > @@ +1671,3 @@ > + region = meta_shaped_texture_get_opaque_corners (META_SHAPED_TEXTURE > (priv->actor)); > + if (region) > + return region; > > How can it be right to be returning either this or shape_region, you want one > or the other, right? It seems to me that meta_shaped_texture should just have a > method that returns a region of guaranteed opaque pixels - what's "obscured" is > determined by what the meta shaped texture draws. OK.
> It is an extremely bizarre interface. But because of the stex/mwa abstraction > between them, and because 0,0 and tex_width/tex_height aren't realistic > bounding points any more, I have to give it something to tell where the actual > bounds of the window are so it can know where to paint corners. I've been > leaning on passing it four cairo_rectangle_ts. Hm, we could take the whole "corners" aspect out of meta-shaped-texture and instead give it an arbitrary clear region and cairo path. Some pyseudocode: def install_path(clear_region, path): # clear path cr.set_operator(CLEAR) for rect in clear_region: cr.rectangle(rect) cr.fill() # paint path cr.set_operator(SOURCE) cr.set_source(1, 1, 1, 1) cr.push_clip(clear_region) cr.append_path(path) cr.fill() cr.pop_clip() # update opaque region opaque_region = new_region() for rect in clear_region: scan_rect(mask_data, rect, opaque_region) and then the opaque_region gets unioned with the shape_region. If you think it will be fast enough, we can scan the entire mask.
Created attachment 192424 [details] [review] frame: Add "get_corner_radiuses" chain
Created attachment 192425 [details] [review] MetaShapedTexture: Allow for a "cairo overlay" A cairo overlay gives us a path to overlay the mask texture data, allowing for complex, anti-aliasing effects on the frame shape.
Created attachment 192426 [details] [review] Antialiased corners Use a specially constructed cairo overlay to give us fully anti-aliased corners on the mask texture.
Created attachment 192470 [details] [review] MetaShapedTexture: Allow for a "cairo overlay" A cairo overlay gives us a path to overlay the mask texture data, allowing for complex, anti-aliasing effects on the frame shape. Fix all embarrasing compile-time errors, and replace "opaque_region" with "visible_pixels_region". Through close xmag-ing, it turns out that frames.c:get_visible_region does the math correctly to cut out only all semi-transparent pixels. The "visible_pixels_region" contains all pixel values above 0.
Created attachment 192471 [details] [review] Antialiased corners Use a specially constructed cairo overlay to give us fully anti-aliased corners on the mask texture. Use the "visible_pixels_region" correctly.
Review of attachment 192424 [details] [review]: ::: src/ui/frames.c @@ +841,3 @@ + + if (top_left) + *top_left = fgeom.top_left_corner_rounded_radius + sqrt(fgeom.top_left_corner_rounded_radius); I think you can figure out what you need to say here - I don't need to provide it word for word. What you need to explain is *why* this is this way, since nobody could figure it out reading the code. And in this case, the *why* is that we need to preserve compatibility with old code that does it this way for unknown reasons.
Created attachment 193545 [details] [review] frame: Add "get_corner_radiuses" chain Oh, sure, that's easy enough.
Review of attachment 192470 [details] [review]: Looks line a fine approach ::: src/compositor/meta-shaped-texture.c @@ +214,3 @@ + cairo_clip (cr); + + cairo_set_operator (cr, CAIRO_OPERATOR_SOURCE); OVER may be just slightly more likely to be fully optimized for the same result. @@ +230,3 @@ + visible_pixels_region = cairo_region_create(); + + for (y = 0; y < tex_height; y ++) A) This shouldn't be in the same function - we create the mask, then we generate the visible region based on the mask. This function has only part of creating the mask, so it shouldn't create the whole visible region. B) I'd rather we were smart and didn't scan the whole mask - that took the shape region, subtracted off the overlay_region and then scanned each rectangle in the overlay_region individually and added the rectangles back. It's only slightly more complex. @@ +235,3 @@ + { + int w = x; + while (mask_data[y*stride+w] > 0 && w < tex_width) spaces around operators @@ +314,3 @@ } + if (priv->overlay_path != NULL && priv->overlay_region != NULL) I don't like the && here - either we require that they both be set or both NULL (g_return_if_fail() in the setter), or we should have an interpretation when they are mixed - it seems semi-obvious that if overlay path is NULL but overlay_region is not NULL we are supposed to just blank out the corners. @@ +663,3 @@ + + meta_shaped_texture_ensure_mask (stex); + return stex->priv->visible_pixels_region; right now, this is NULL if the overlay isn't set, which is wrong. @@ +677,3 @@ + */ +void +meta_shaped_texture_set_cairo_overlay (MetaShapedTexture *stex, I'd rather this was "overlay_path" or something - "cairo" seems like an irrelevant detail. @@ +703,3 @@ + + /* cairo_path_t does not have refcounting. */ + priv->overlay_path = overlay_path; You now need to dirty the mask
Review of attachment 192471 [details] [review]: Looks good except for bugs ::: src/compositor/meta-window-actor.c @@ +1679,3 @@ + region = meta_shaped_texture_get_visible_pixels_region (META_SHAPED_TEXTURE (priv->actor)); + if (region) + texture_clip_region = cairo_region_copy (region); Why should region be NULL? If the shaped texture knows what it is going to paint (it does), then it knows the correct region. @@ +1685,1 @@ texture_clip_region = cairo_region_copy (priv->bounding_region); This code (changed from 'else if') can't be correct, since it overwrites and leaks; in fact you overwrite and leak twice in a row. @@ +2011,3 @@ + + corner_rects[0].x = borders->invisible.left; + corner_rects[0].y = borders->invisible.top; You don't want to truncate here,you need to "expand" (this is usually best done x1/y1/x2/y2 form ... create a helper function)
Review of attachment 193545 [details] [review]: Looks good
Created attachment 193557 [details] [review] MetaShapedTexture: Allow for a "cairo overlay" A cairo overlay gives us a path to overlay the mask texture data, allowing for complex, anti-aliasing effects on the frame shape. (In reply to comment #26) > Review of attachment 192470 [details] [review]: > > Looks line a fine approach > > ::: src/compositor/meta-shaped-texture.c > @@ +214,3 @@ > + cairo_clip (cr); > + > + cairo_set_operator (cr, CAIRO_OPERATOR_SOURCE); > > OVER may be just slightly more likely to be fully optimized for the same > result. Seems unintuitive, but OK. > @@ +230,3 @@ > + visible_pixels_region = cairo_region_create(); > + > + for (y = 0; y < tex_height; y ++) > > A) This shouldn't be in the same function - we create the mask, then we > generate the visible region based on the mask. This function has only part of > creating the mask, so it shouldn't create the whole visible region. > > B) I'd rather we were smart and didn't scan the whole mask - that took the > shape region, subtracted off the overlay_region and then scanned each rectangle > in the overlay_region individually and added the rectangles back. It's only > slightly more complex. Hopefully this is more inline with what you want. > @@ +235,3 @@ > + { > + int w = x; > + while (mask_data[y*stride+w] > 0 && w < tex_width) > > spaces around operators > > @@ +314,3 @@ > } > > + if (priv->overlay_path != NULL && priv->overlay_region != NULL) > > I don't like the && here - either we require that they both be set or both NULL > (g_return_if_fail() in the setter), or we should have an interpretation when > they are mixed - it seems semi-obvious that if overlay path is NULL but > overlay_region is not NULL we are supposed to just blank out the corners. I can't really think of a situation where an overlay region without a path would be too useful, but it seemed to be the easier of the two choices. > @@ +663,3 @@ > + > + meta_shaped_texture_ensure_mask (stex); > + return stex->priv->visible_pixels_region; > > right now, this is NULL if the overlay isn't set, which is wrong. Because we're scanning the corners even if we have no overlay, I don't think this can ever be wrong now. > @@ +677,3 @@ > + */ > +void > +meta_shaped_texture_set_cairo_overlay (MetaShapedTexture *stex, > > I'd rather this was "overlay_path" or something - "cairo" seems like an > irrelevant detail. > > @@ +703,3 @@ > + > + /* cairo_path_t does not have refcounting. */ > + priv->overlay_path = overlay_path; > > You now need to dirty the mask Done.
Created attachment 193558 [details] [review] Antialiased corners Use a specially constructed cairo overlay to give us fully anti-aliased corners on the mask texture. (In reply to comment #27) > Review of attachment 192471 [details] [review]: > > Looks good except for bugs uhh... OK. > ::: src/compositor/meta-window-actor.c > @@ +1679,3 @@ > + region = meta_shaped_texture_get_visible_pixels_region (META_SHAPED_TEXTURE > (priv->actor)); > + if (region) > + texture_clip_region = cairo_region_copy (region); > > Why should region be NULL? If the shaped texture knows what it is going to > paint (it does), then it knows the correct region. In the case where there was no overlay, I said that the region was NULL, and it should be the same as the previous behavior. Since we fixed that, I can simplify this code a ton. > @@ +1685,1 @@ > texture_clip_region = cairo_region_copy (priv->bounding_region); > > This code (changed from 'else if') can't be correct, since it overwrites and > leaks; in fact you overwrite and leak twice in a row. I think this was leftover due to debugging. > @@ +2011,3 @@ > + > + corner_rects[0].x = borders->invisible.left; > + corner_rects[0].y = borders->invisible.top; > > You don't want to truncate here,you need to "expand" (this is usually best done > x1/y1/x2/y2 form ... create a helper function) Hopefully this is more of what you want.
Created attachment 193611 [details] [review] Antialiased corners Use a specially constructed cairo overlay to give us fully anti-aliased corners on the mask texture. - corner_rects[1].width = corner_rects[0].height = ceil(top_right); + corner_rects[1].width = corner_rects[1].height = ceil(top_right); A simple copy-paste bug that caused segfaults. Fixed.
Review of attachment 193611 [details] [review]: ::: src/compositor/meta-window-actor.c @@ +1707,3 @@ + texture_clip_region = meta_shaped_texture_get_visible_pixels_region (META_SHAPED_TEXTURE (priv->actor)); + + if (texture_clip_region) Just make meta_shaped_texture_get_visible_pixels_region() return the right thing always and never NULL. @@ +2041,3 @@ + corner_rects[0].x = floor(borders->invisible.left); + corner_rects[0].y = floor(borders->invisible.top); + corner_rects[0].width = corner_rects[0].height = ceil(top_left); This "coincidentally" works since borders->invisible.left is an integer, but it's not a correct way to determine the integral rectangle bounding a float-valued rectangle - e.g., the bounding rectangle of +0.9+0.9x1.2x1.2 is +0+0x3x3, not +0+0x2x2 so writing it in that form with the floor() doesn't make sense. What I meant was to add: void set_integral_bounding_rectangle (cairo_rectangle_int_t *rect, double x, double y, double width, double height) { rect->x = floor(x); rect->y = floor(y); rect->width = ceil(x + width) - rect->x; rect->height = ceil(y + height) - rect->y; } and then use that.
Review of attachment 193557 [details] [review]: couple of minor things left ::: src/compositor/meta-shaped-texture.c @@ +199,3 @@ + cairo_region_destroy (priv->visible_pixels_region); + + visible_pixels_region = cairo_region_copy (priv->shape_region); The code elsewhere in the file allows priv->shape_region to be NULL; ensure_mask() used to never be called when that was the case, but that's no longer the case with your current code where ensure_mask() is called from get_visible_pixels_region(). So you need to deal with ULL shape region here *and* you should make sure that ensure_mask() doesn't create a mask texture when it doesn't need to - it's better if a function like ensure_mask() "does the right thing" by itself, and doesn't count on the caller only calling it in appropriate circumstances. @@ +200,3 @@ + + visible_pixels_region = cairo_region_copy (priv->shape_region); + overlay_region = priv->overlay_region; Not really different, but I'd write: priv->visible_pixel_region = cairo_region_copy (priv->shape_region); overlay_region = priv->overlay_region; visible_pixel_region = priv->visible_pixel_region; to make it clear that you are just optimizing with local variables. (The delayed assigment in multiple code paths you have currently is confusing.) @@ +222,3 @@ + cairo_region_get_rectangle (overlay_region, i, &rect); + + for (y = rect.y; y < (rect.y + rect.height); y ++) extra space @@ +224,3 @@ + for (y = rect.y; y < (rect.y + rect.height); y ++) + { + for (x = rect.x; x < (rect.x + rect.width); x ++) and an extra space @@ +228,3 @@ + int w = x; + while (mask_data[y * stride + w] > 0 && w < (rect.x + rect.width)) + w ++; and an extra space @@ +696,2 @@ /** + * meta_shaped_texture_set_visible_pixels_region: get_visible_pixel_region @@ +699,3 @@ + * + * If this texture has a cairo overlay, return a region containing + * all visible pixels: those with values above 0. The meaning of this function has nothing to do with the cairo overlay - this function just returns a region that bounds the visible pixels of the shaped texture.
Created attachment 194752 [details] [review] MetaShapedTexture: Allow for a "cairo overlay" A cairo overlay gives us a path to overlay the mask texture data, allowing for complex, anti-aliasing effects on the frame shape.
Created attachment 194753 [details] [review] Antialiased corners Use a specially constructed cairo overlay to give us fully anti-aliased corners on the mask texture.
Review of attachment 194752 [details] [review]: Closer and closer ::: src/compositor/meta-shaped-texture.c @@ +331,3 @@ + * need to create a mask texture, so quit early. */ + if ((priv->shape_region == NULL || + cairo_region_num_rectangles (priv->shape_region) == 0) && Why would we be treating shape_region == NULL (shape is a rectangle) the same as num rectangles == 0 (window shaped away)? shape_region == NULL is currently handled with no mask, and a separate painting path. num_rectangles == 0 is handled with a completely empty mask, which I think is fine - it's not a normal case.) @@ +335,3 @@ + cairo_region_num_rectangles (priv->overlay_region) == 0)) + { + return; You still need to make the visible_region correctly even if not creating the mask; you can't just return.
Review of attachment 194753 [details] [review]: OK
Created attachment 194821 [details] [review] MetaShapedTexture: Allow for a "cairo overlay" A cairo overlay gives us a path to overlay the mask texture data, allowing for complex, anti-aliasing effects on the frame shape.
Review of attachment 194821 [details] [review]: ::: src/compositor/meta-shaped-texture.c @@ +337,3 @@ + * {0, 0, tex_width, tex_height}. */ + cairo_rectangle_int_t rect = { 0, 0, tex_width, tex_height }; + priv->visible_pixels_region = cairo_region_create_rectangle (&rect); You overwrite this region and leak the old one on every call to ensure_mask(); you could make dirty_mask() free and NULL the viisble pixels region and replace the priv->mask_texture == COGL_INVALID_HANDLE check with a visible_pixels_region == NULL check, I think
Created attachment 194849 [details] [review] MetaShapedTexture: Allow for a "cairo overlay" A cairo overlay gives us a path to overlay the mask texture data, allowing for complex, anti-aliasing effects on the frame shape.
Comment on attachment 194849 [details] [review] MetaShapedTexture: Allow for a "cairo overlay" whoops, attached the old verison
Created attachment 194850 [details] [review] MetaShapedTexture: Allow for a "cairo overlay" A cairo overlay gives us a path to overlay the mask texture data, allowing for complex, anti-aliasing effects on the frame shape.
Review of attachment 194850 [details] [review]: ::: src/compositor/meta-shaped-texture.c @@ +178,2 @@ cogl_handle_unref (priv->mask_texture); priv->mask_texture = COGL_INVALID_HANDLE; priv->mask_texture might be COGL_INVALID_HANDLE
Created attachment 194862 [details] [review] MetaShapedTexture: Allow for a "cairo overlay" A cairo overlay gives us a path to overlay the mask texture data, allowing for complex, anti-aliasing effects on the frame shape. This feels like an unfortunate game of "find the bug", now :( What's next? also, - if (priv->visible_pixels_region == NULL) + if (priv->visible_pixels_region != NULL) No idea how I did *that*.
Review of attachment 194862 [details] [review]: I'm not finding the bug :-) ... go ahead and commit!
Attachment 192410 [details] pushed as f83568f - MetaShapedTexture: Use a proper stride, calculated by cairo Attachment 194753 [details] pushed as dcfa698 - Antialiased corners Attachment 194850 [details] pushed as 49a3fd5 - MetaShapedTexture: Allow for a "cairo overlay" Attachment 194862 [details] pushed as 49a3fd5 - MetaShapedTexture: Allow for a "cairo overlay"