Re: [PATCH v4 2/2] xwm: Scale icon if size is not the target size

[Pekka Paalanen]

On Fri, 23 Mar 2018 13:47:33 -0600
Scott Moreau  wrote:

> This scales the icon cairo surface for the titlebar if it isn't the
> target size.
> 
> shared/cairo-util: Add surface resizing function to be used for this
> case and other potential cases.
> ---
> 
> Changed in v2:
> 
> - Rebase to [PATCH 1/1 v3] xwm: Choose icon closest to target size
> 
> Changed in v3:
> 
> - No changes
> 
> Changed in v4:
> 
> - Fixed whitespace problems
> - Renamed cairo_resize_surface() to resize_cairo_surface()
> 
>  shared/cairo-util.c   | 63 
> +++
>  shared/cairo-util.h   |  4 +++
>  xwayland/window-manager.c |  4 +++
>  3 files changed, 71 insertions(+)
> 

Hi Scott,

I appreciate the effort and this is a feature I would like to have in
Weston. I have come questions below, though.

> diff --git a/shared/cairo-util.c b/shared/cairo-util.c
> index d71e0ed..442182b 100644
> --- a/shared/cairo-util.c
> +++ b/shared/cairo-util.c
> @@ -365,6 +365,69 @@ load_cairo_surface(const char *filename)
>  width, height, stride);
>  }
>  
> +static cairo_surface_t *
> +scale_surface(cairo_surface_t *source, cairo_filter_t filter,
> + double width, double height)

Double seems an inappropriate type here, a surface size must always be
an integer.

The second line's first word "double" should be aligned to the previous
line's 'c' right after the opening parenthesis.

> +{
> + cairo_surface_t *dest;
> + cairo_t *cr;
> + int old_width, old_height;
> +
> + old_width = cairo_image_surface_get_width(source);
> + old_height = cairo_image_surface_get_height(source);
> +
> + dest = cairo_surface_create_similar(source,
> + CAIRO_CONTENT_COLOR_ALPHA,
> + width, height);
> + cr = cairo_create (dest);
> +
> + cairo_scale (cr, width / old_width, height / old_height);

We don't use a space between the function name and the opening
parenthesis.

> + cairo_set_source_surface (cr, source, 0, 0);
> +
> + cairo_pattern_set_extend (cairo_get_source(cr),
> + CAIRO_EXTEND_REFLECT);

Is there no need to set the filter as well? Looks like unused argument.

> +
> + cairo_set_operator (cr, CAIRO_OPERATOR_SOURCE);
> +
> + cairo_paint (cr);
> +
> + cairo_destroy (cr);
> +
> + cairo_surface_destroy(source);
> +
> + return dest;
> +}
> +
> +cairo_surface_t *
> +resize_cairo_surface(cairo_surface_t *source, cairo_filter_t filter,
> + int width, int height)
> +{
> + if (!filter)
> + filter = CAIRO_FILTER_BEST;
> +
> + while((cairo_image_surface_get_width(source) / 2.0f) > width)

Needs a space between a C keyword and an opening parenthesis.

> + source = scale_surface(source, filter,
> + cairo_image_surface_get_width(source) / 2.0f,
> + cairo_image_surface_get_height(source));
> +
> + while((cairo_image_surface_get_height(source) / 2.0f) > height)
> + source = scale_surface(source, filter,
> + cairo_image_surface_get_width(source),
> + cairo_image_surface_get_height(source) / 2.0f);
> +
> + while((cairo_image_surface_get_width(source) * 2.0f) < width)
> + source = scale_surface(source, filter,
> + cairo_image_surface_get_width(source) * 2.0f,
> + cairo_image_surface_get_height(source));
> +
> + while((cairo_image_surface_get_height(source) * 2.0f) < height)
> + source = scale_surface(source, filter,
> + cairo_image_surface_get_width(source),
> + cairo_image_surface_get_height(source) * 2.0f);

Why bother with all this? The answer should be in the commit message,
and "other potential cases" is not it if you cannot actually name a
case that will be used in Weston (e.g. you know you will write or have
already written a patch to use it).

I understand that it is to compute a good quality minification by doing
the convolution in factor of two steps, but do we really need that?
Especially when we pick a size closest to the wanted size, the code
complexity here seems unwarranted. I think the very least it requires a
comment explaining why this is done like it is.

I don't deny that there could be a good reason for this, I just want
you to explicitly record it.

For magnification, does this make any difference to a single-step
scaling?

> +
> + return scale_surface(source, filter, width, height);

I would have thought this last statement is all this function actually
needs to do, but I could accept a rationale for the step-wise
minification e.g. if you say it really actually looks bad if done in a
single step.

> +}
> +
>  void
>  theme_set_background_source(struct theme *t, 

Re: [PATCH v4 2/2] xwm: Scale icon if size is not the target size

[Scott Moreau]

On Fri, Mar 23, 2018 at 2:15 PM, Derek Foreman 
wrote:

> On 2018-03-23 02:47 PM, Scott Moreau wrote:
> > This scales the icon cairo surface for the titlebar if it isn't the
> > target size.
> >
> > shared/cairo-util: Add surface resizing function to be used for this
> > case and other potential cases.
> > ---
> >
> > Changed in v2:
> >
> > - Rebase to [PATCH 1/1 v3] xwm: Choose icon closest to target size
> >
> > Changed in v3:
> >
> > - No changes
> >
> > Changed in v4:
> >
> > - Fixed whitespace problems
> > - Renamed cairo_resize_surface() to resize_cairo_surface()
> >
> >  shared/cairo-util.c   | 63 ++
> +
> >  shared/cairo-util.h   |  4 +++
> >  xwayland/window-manager.c |  4 +++
> >  3 files changed, 71 insertions(+)
> >
> > diff --git a/shared/cairo-util.c b/shared/cairo-util.c
> > index d71e0ed..442182b 100644
> > --- a/shared/cairo-util.c
> > +++ b/shared/cairo-util.c
> > @@ -365,6 +365,69 @@ load_cairo_surface(const char *filename)
> >  width, height, stride);
> >  }
> >
> > +static cairo_surface_t *
> > +scale_surface(cairo_surface_t *source, cairo_filter_t filter,
> > + double width, double height)
>
> Still have some whitespace issues here.
>

Hm, I have 5 * 8 wide tabs here and it seems to appear correct in my editor.
I'll wait for other potential comments/reviews before submitting another
patch.


>
> Otherwise, this is
> Reviewed-by: Derek Foreman 
>
> Does this + your leak fix close all known bugs in the icon code?
>

Well now that you mention it, I did notice one other thing.
weston_wm_handle_icon() can be called multiple times and sets
window->icon_surface from a newly created surface without destroying the
old one first, in the case where there isn't a frame yet. I haven't
submitted a patch for this at all.


>
> Thanks,
> Derek
>

Thanks,
Scott


>
> > +{
> > + cairo_surface_t *dest;
> > + cairo_t *cr;
> > + int old_width, old_height;
> > +
> > + old_width = cairo_image_surface_get_width(source);
> > + old_height = cairo_image_surface_get_height(source);
> > +
> > + dest = cairo_surface_create_similar(source,
> > + CAIRO_CONTENT_COLOR_ALPHA,
> > + width, height);
> > + cr = cairo_create (dest);
> > +
> > + cairo_scale (cr, width / old_width, height / old_height);
> > + cairo_set_source_surface (cr, source, 0, 0);
> > +
> > + cairo_pattern_set_extend (cairo_get_source(cr),
> > + CAIRO_EXTEND_REFLECT);
> > +
> > + cairo_set_operator (cr, CAIRO_OPERATOR_SOURCE);
> > +
> > + cairo_paint (cr);
> > +
> > + cairo_destroy (cr);
> > +
> > + cairo_surface_destroy(source);
> > +
> > + return dest;
> > +}
> > +
> > +cairo_surface_t *
> > +resize_cairo_surface(cairo_surface_t *source, cairo_filter_t filter,
> > + int width, int height)
> > +{
> > + if (!filter)
> > + filter = CAIRO_FILTER_BEST;
> > +
> > + while((cairo_image_surface_get_width(source) / 2.0f) > width)
> > + source = scale_surface(source, filter,
> > + cairo_image_surface_get_width(source) / 2.0f,
> > + cairo_image_surface_get_height(source));
> > +
> > + while((cairo_image_surface_get_height(source) / 2.0f) > height)
> > + source = scale_surface(source, filter,
> > + cairo_image_surface_get_width(source),
> > + cairo_image_surface_get_height(source) / 2.0f);
> > +
> > + while((cairo_image_surface_get_width(source) * 2.0f) < width)
> > + source = scale_surface(source, filter,
> > + cairo_image_surface_get_width(source) * 2.0f,
> > + cairo_image_surface_get_height(source));
> > +
> > + while((cairo_image_surface_get_height(source) * 2.0f) < height)
> > + source = scale_surface(source, filter,
> > + cairo_image_surface_get_width(source),
> > + cairo_image_surface_get_height(source) * 2.0f);
> > +
> > + return scale_surface(source, filter, width, height);
> > +}
> > +
> >  void
> >  theme_set_background_source(struct theme *t, cairo_t *cr, uint32_t
> flags)
> >  {
> > diff --git a/shared/cairo-util.h b/shared/cairo-util.h
> > index 6fd11f6..1c58f3b 100644
> > --- a/shared/cairo-util.h
> > +++ b/shared/cairo-util.h
> > @@ -49,6 +49,10 @@ rounded_rect(cairo_t *cr, int x0, int y0, int x1, int
> y1, int radius);
> >  cairo_surface_t *
> >  load_cairo_surface(const char *filename);
> >
> > +cairo_surface_t *
> > +resize_cairo_surface(cairo_surface_t *source, cairo_filter_t filter,
> > + int width, int height);
> > +
> >  struct theme {
> >   cairo_surface_t 

Re: [PATCH v4 2/2] xwm: Scale icon if size is not the target size

[Derek Foreman]

On 2018-03-23 02:47 PM, Scott Moreau wrote:
> This scales the icon cairo surface for the titlebar if it isn't the
> target size.
> 
> shared/cairo-util: Add surface resizing function to be used for this
> case and other potential cases.
> ---
> 
> Changed in v2:
> 
> - Rebase to [PATCH 1/1 v3] xwm: Choose icon closest to target size
> 
> Changed in v3:
> 
> - No changes
> 
> Changed in v4:
> 
> - Fixed whitespace problems
> - Renamed cairo_resize_surface() to resize_cairo_surface()
> 
>  shared/cairo-util.c   | 63 
> +++
>  shared/cairo-util.h   |  4 +++
>  xwayland/window-manager.c |  4 +++
>  3 files changed, 71 insertions(+)
> 
> diff --git a/shared/cairo-util.c b/shared/cairo-util.c
> index d71e0ed..442182b 100644
> --- a/shared/cairo-util.c
> +++ b/shared/cairo-util.c
> @@ -365,6 +365,69 @@ load_cairo_surface(const char *filename)
>  width, height, stride);
>  }
>  
> +static cairo_surface_t *
> +scale_surface(cairo_surface_t *source, cairo_filter_t filter,
> + double width, double height)

Still have some whitespace issues here.

Otherwise, this is
Reviewed-by: Derek Foreman 

Does this + your leak fix close all known bugs in the icon code?

Thanks,
Derek

> +{
> + cairo_surface_t *dest;
> + cairo_t *cr;
> + int old_width, old_height;
> +
> + old_width = cairo_image_surface_get_width(source);
> + old_height = cairo_image_surface_get_height(source);
> +
> + dest = cairo_surface_create_similar(source,
> + CAIRO_CONTENT_COLOR_ALPHA,
> + width, height);
> + cr = cairo_create (dest);
> +
> + cairo_scale (cr, width / old_width, height / old_height);
> + cairo_set_source_surface (cr, source, 0, 0);
> +
> + cairo_pattern_set_extend (cairo_get_source(cr),
> + CAIRO_EXTEND_REFLECT);
> +
> + cairo_set_operator (cr, CAIRO_OPERATOR_SOURCE);
> +
> + cairo_paint (cr);
> +
> + cairo_destroy (cr);
> +
> + cairo_surface_destroy(source);
> +
> + return dest;
> +}
> +
> +cairo_surface_t *
> +resize_cairo_surface(cairo_surface_t *source, cairo_filter_t filter,
> + int width, int height)
> +{
> + if (!filter)
> + filter = CAIRO_FILTER_BEST;
> +
> + while((cairo_image_surface_get_width(source) / 2.0f) > width)
> + source = scale_surface(source, filter,
> + cairo_image_surface_get_width(source) / 2.0f,
> + cairo_image_surface_get_height(source));
> +
> + while((cairo_image_surface_get_height(source) / 2.0f) > height)
> + source = scale_surface(source, filter,
> + cairo_image_surface_get_width(source),
> + cairo_image_surface_get_height(source) / 2.0f);
> +
> + while((cairo_image_surface_get_width(source) * 2.0f) < width)
> + source = scale_surface(source, filter,
> + cairo_image_surface_get_width(source) * 2.0f,
> + cairo_image_surface_get_height(source));
> +
> + while((cairo_image_surface_get_height(source) * 2.0f) < height)
> + source = scale_surface(source, filter,
> + cairo_image_surface_get_width(source),
> + cairo_image_surface_get_height(source) * 2.0f);
> +
> + return scale_surface(source, filter, width, height);
> +}
> +
>  void
>  theme_set_background_source(struct theme *t, cairo_t *cr, uint32_t flags)
>  {
> diff --git a/shared/cairo-util.h b/shared/cairo-util.h
> index 6fd11f6..1c58f3b 100644
> --- a/shared/cairo-util.h
> +++ b/shared/cairo-util.h
> @@ -49,6 +49,10 @@ rounded_rect(cairo_t *cr, int x0, int y0, int x1, int y1, 
> int radius);
>  cairo_surface_t *
>  load_cairo_surface(const char *filename);
>  
> +cairo_surface_t *
> +resize_cairo_surface(cairo_surface_t *source, cairo_filter_t filter,
> + int width, int height);
> +
>  struct theme {
>   cairo_surface_t *active_frame;
>   cairo_surface_t *inactive_frame;
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index 5fb41bf..50c5855 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -1463,6 +1463,10 @@ weston_wm_handle_icon(struct weston_wm *wm, struct 
> weston_wm_window *window)
>   return;
>   }
>  
> + if (width != XWM_ICON_SIZE || height != XWM_ICON_SIZE)
> + new_surface = resize_cairo_surface(new_surface, 0,
> + XWM_ICON_SIZE, XWM_ICON_SIZE);
> +
>   if (window->frame)
>   frame_set_icon(window->frame, new_surface);
>   else /* We don’t have a frame yet */
> 

___
wayland-devel mailing list

[PATCH v4 2/2] xwm: Scale icon if size is not the target size

[Scott Moreau]

This scales the icon cairo surface for the titlebar if it isn't the
target size.

shared/cairo-util: Add surface resizing function to be used for this
case and other potential cases.
---

Changed in v2:

- Rebase to [PATCH 1/1 v3] xwm: Choose icon closest to target size

Changed in v3:

- No changes

Changed in v4:

- Fixed whitespace problems
- Renamed cairo_resize_surface() to resize_cairo_surface()

 shared/cairo-util.c   | 63 +++
 shared/cairo-util.h   |  4 +++
 xwayland/window-manager.c |  4 +++
 3 files changed, 71 insertions(+)

diff --git a/shared/cairo-util.c b/shared/cairo-util.c
index d71e0ed..442182b 100644
--- a/shared/cairo-util.c
+++ b/shared/cairo-util.c
@@ -365,6 +365,69 @@ load_cairo_surface(const char *filename)
   width, height, stride);
 }
 
+static cairo_surface_t *
+scale_surface(cairo_surface_t *source, cairo_filter_t filter,
+   double width, double height)
+{
+   cairo_surface_t *dest;
+   cairo_t *cr;
+   int old_width, old_height;
+
+   old_width = cairo_image_surface_get_width(source);
+   old_height = cairo_image_surface_get_height(source);
+
+   dest = cairo_surface_create_similar(source,
+   CAIRO_CONTENT_COLOR_ALPHA,
+   width, height);
+   cr = cairo_create (dest);
+
+   cairo_scale (cr, width / old_width, height / old_height);
+   cairo_set_source_surface (cr, source, 0, 0);
+
+   cairo_pattern_set_extend (cairo_get_source(cr),
+   CAIRO_EXTEND_REFLECT);
+
+   cairo_set_operator (cr, CAIRO_OPERATOR_SOURCE);
+
+   cairo_paint (cr);
+
+   cairo_destroy (cr);
+
+   cairo_surface_destroy(source);
+
+   return dest;
+}
+
+cairo_surface_t *
+resize_cairo_surface(cairo_surface_t *source, cairo_filter_t filter,
+   int width, int height)
+{
+   if (!filter)
+   filter = CAIRO_FILTER_BEST;
+
+   while((cairo_image_surface_get_width(source) / 2.0f) > width)
+   source = scale_surface(source, filter,
+   cairo_image_surface_get_width(source) / 2.0f,
+   cairo_image_surface_get_height(source));
+
+   while((cairo_image_surface_get_height(source) / 2.0f) > height)
+   source = scale_surface(source, filter,
+   cairo_image_surface_get_width(source),
+   cairo_image_surface_get_height(source) / 2.0f);
+
+   while((cairo_image_surface_get_width(source) * 2.0f) < width)
+   source = scale_surface(source, filter,
+   cairo_image_surface_get_width(source) * 2.0f,
+   cairo_image_surface_get_height(source));
+
+   while((cairo_image_surface_get_height(source) * 2.0f) < height)
+   source = scale_surface(source, filter,
+   cairo_image_surface_get_width(source),
+   cairo_image_surface_get_height(source) * 2.0f);
+
+   return scale_surface(source, filter, width, height);
+}
+
 void
 theme_set_background_source(struct theme *t, cairo_t *cr, uint32_t flags)
 {
diff --git a/shared/cairo-util.h b/shared/cairo-util.h
index 6fd11f6..1c58f3b 100644
--- a/shared/cairo-util.h
+++ b/shared/cairo-util.h
@@ -49,6 +49,10 @@ rounded_rect(cairo_t *cr, int x0, int y0, int x1, int y1, 
int radius);
 cairo_surface_t *
 load_cairo_surface(const char *filename);
 
+cairo_surface_t *
+resize_cairo_surface(cairo_surface_t *source, cairo_filter_t filter,
+   int width, int height);
+
 struct theme {
cairo_surface_t *active_frame;
cairo_surface_t *inactive_frame;
diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
index 5fb41bf..50c5855 100644
--- a/xwayland/window-manager.c
+++ b/xwayland/window-manager.c
@@ -1463,6 +1463,10 @@ weston_wm_handle_icon(struct weston_wm *wm, struct 
weston_wm_window *window)
return;
}
 
+   if (width != XWM_ICON_SIZE || height != XWM_ICON_SIZE)
+   new_surface = resize_cairo_surface(new_surface, 0,
+   XWM_ICON_SIZE, XWM_ICON_SIZE);
+
if (window->frame)
frame_set_icon(window->frame, new_surface);
else /* We don’t have a frame yet */
-- 
2.7.4

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel