[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 <oreaus at gmail.com> 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, 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);

This function call will result in a copy of the image, possibly
multiple times but at least once.

If we simply removed the exact size check here and guarantee a copy,
then we no longer need the image user data destruction that was set up
to free(reply), because we know we can always free the reply before
returning. Removing the old fix as unnecessary would be a follow-up
patch.

> +
>  	if (window->frame)
>  		frame_set_icon(window->frame, new_surface);
>  	else /* We don’t have a frame yet */


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180329/1346848e/attachment.sig>