Re: [PATCH] Support image filtering in xlib and xcb backends

On Tue, Aug 12, 2014 at 08:06:35PM -0700, Bill Spitzak wrote:
> _cairo_pattern_analyze_filter() will change GOOD to BILINEAR if
> it thinks bilinear is acceptable. Removed the equivalent test
> from _pixman_image_set_properties.
> 
> The xlib and xcb backends will use the image fallback for GOOD
> and BEST so that the convolution can be done.
> 
> Notes:
> 
> - I did not test the xcb changes.
> 
> - I am assuming other backends treat bilinear and good identically,
> at least for scales larger than 1/1.35.
> 
> - A CAIRO_RENDER option maybe should be added in case pixman ever
> supports GOOD/BEST.

Should the above notes be taken care of before this is merged?

Functionally this looks good, and I gather this is outcome from recent
discussions with Chris and others.  I'd appreciate a third set of eyes
on this just for sanity checking, but otherwise:

Reviewed-by: Bryce Harrington <[email protected]>

> ---
>  src/cairo-image-source.c       |   22 +++++++++-------------
>  src/cairo-pattern.c            |   32 ++++++++++++++++++++++++--------
>  src/cairo-xcb-surface-render.c |   12 ++++--------
>  src/cairo-xlib-source.c        |    5 +++++
>  4 files changed, 42 insertions(+), 29 deletions(-)
> 
> diff --git a/src/cairo-image-source.c b/src/cairo-image-source.c
> index b6b6b9f..9cb3533 100644
> --- a/src/cairo-image-source.c
> +++ b/src/cairo-image-source.c
> @@ -941,21 +941,17 @@ _pixman_image_set_properties (pixman_image_t *pixman_image,
>  	    pixman_filter = PIXMAN_FILTER_FAST;
>  	    break;
>  	case CAIRO_FILTER_GOOD:
> -	    pixman_filter = PIXMAN_FILTER_GOOD;
> -	    if (dx > 1.35 || dy > 1.35) {
> -		pixman_filter = PIXMAN_FILTER_SEPARABLE_CONVOLUTION;
> -		kernel = KERNEL_BOX;
> -		/* Clip the filter size to prevent extreme slowness. This
> -		   value could be raised if 2-pass filtering is done */
> -		if (dx > 16.0) dx = 16.0;
> -		/* Match the bilinear filter for dimension scaling up: */
> -		else if (dx < 1.0) dx = 1.0;
> -		if (dy > 16.0) dy = 16.0;
> -		else if (dy < 1.0) dy = 1.0;
> -	    }
> +	    pixman_filter = PIXMAN_FILTER_SEPARABLE_CONVOLUTION;
> +	    kernel = KERNEL_BOX;
> +	    /* Clip the filter size to prevent extreme slowness. This
> +	       value could be raised if 2-pass filtering is done */
> +	    if (dx > 16.0) dx = 16.0;
> +	    /* Match the bilinear filter for dimension scaling up: */
> +	    else if (dx < 1.35) dx = 1.0;
> +	    if (dy > 16.0) dy = 16.0;
> +	    else if (dy < 1.35) dy = 1.0;
>  	    break;
>  	case CAIRO_FILTER_BEST:
> -	    pixman_filter = PIXMAN_FILTER_BEST;
>  	    pixman_filter = PIXMAN_FILTER_SEPARABLE_CONVOLUTION;
>  	    kernel = KERNEL_CATMULL_ROM; /* LANCZOS3 is better but not much */
>  	    /* Clip the filter size to prevent extreme slowness. This
> diff --git a/src/cairo-pattern.c b/src/cairo-pattern.c
> index 6905e15..b40eaa1 100644
> --- a/src/cairo-pattern.c
> +++ b/src/cairo-pattern.c
> @@ -3371,15 +3371,31 @@ _cairo_pattern_analyze_filter (const cairo_pattern_t	*pattern,
>  	if (_cairo_matrix_is_pixel_exact (&pattern->matrix)) {
>  	    pad = 0.;
>  	    optimized_filter = CAIRO_FILTER_NEAREST;
> -	} else {
> -	    /* 0.5 is enough for a bilinear filter. It's possible we
> -	     * should defensively use more for CAIRO_FILTER_BEST, but
> -	     * without a single example, it's hard to know how much
> -	     * more would be defensive...
> -	     */
> -	    pad = 0.5;
> -	    optimized_filter = pattern->filter;
> +	    break;
> +	}
> +	optimized_filter = pattern->filter;
> +	/* Use BILINEAR for any scale greater than 1/1.35 instead
> +	 * of GOOD. For scales of 1 and larger this is identical,
> +	 * for the smaller sizes it was judged that the artifacts
> +	 * were not worse than the artifacts from a box filer.
> +	 */
> +	if (pattern->filter == CAIRO_FILTER_GOOD) {
> +	    double h;
> +	    h = pattern->matrix.xx * pattern->matrix.xx +
> +		pattern->matrix.xy * pattern->matrix.xy;
> +	    if (h < 1.35 * 1.35) {
> +		h = pattern->matrix.yx * pattern->matrix.yx +
> +		    pattern->matrix.yy * pattern->matrix.yy;
> +		if (h < 1.35 * 1.35)
> +		    optimized_filter = CAIRO_FILTER_BILINEAR;
> +	    }
>  	}
> +	/* 0.5 is enough for a bilinear filter. It's possible we
> +	 * should defensively use more for CAIRO_FILTER_BEST, but
> +	 * without a single example, it's hard to know how much
> +	 * more would be defensive...
> +	 */
> +	pad = 0.5;
>  	break;
>  
>      case CAIRO_FILTER_NEAREST:
> diff --git a/src/cairo-xcb-surface-render.c b/src/cairo-xcb-surface-render.c
> index a127000..64bd1c5 100644
> --- a/src/cairo-xcb-surface-render.c
> +++ b/src/cairo-xcb-surface-render.c
> @@ -411,17 +411,13 @@ _pattern_is_supported (uint32_t flags,
>      }
>  
>      if (pattern->type == CAIRO_PATTERN_TYPE_SURFACE) {
> -	cairo_filter_t filter;
> -
> -	filter = pattern->filter;
> -	if (_cairo_matrix_is_pixel_exact (&pattern->matrix))
> -	{
> -	    filter = CAIRO_FILTER_NEAREST;
> -	}
> -
> +	cairo_filter_t filter = pattern->filter;
>  	if (! (filter == CAIRO_FILTER_NEAREST || filter == CAIRO_FILTER_FAST)) {
>  	    if ((flags & CAIRO_XCB_RENDER_HAS_FILTERS) == 0)
>  		return FALSE;
> +            /* Filter matrix is not supported in XRender */
> +	    if (filter == CAIRO_FILTER_GOOD || filter == CAIRO_FILTER_BEST)
> +		return FALSE;
>  	}
>      } else { /* gradient */
>  	if ((flags & CAIRO_XCB_RENDER_HAS_GRADIENTS) == 0)
> diff --git a/src/cairo-xlib-source.c b/src/cairo-xlib-source.c
> index 8275da3..4326a6f 100644
> --- a/src/cairo-xlib-source.c
> +++ b/src/cairo-xlib-source.c
> @@ -1102,6 +1102,11 @@ pattern_is_supported (cairo_xlib_display_t *display,
>  	    /* No filters implies no transforms, so we optimise away BILINEAR */
>      }
>  
> +    /* Filter matrix is not supported in XRender */
> +    if (pattern->filter == CAIRO_FILTER_GOOD ||
> +	pattern->filter == CAIRO_FILTER_BEST)
> +	return FALSE;
> +
>      return TRUE;
>  }
>  cairo_surface_t *
> -- 
> 1.7.9.5
> 
> -- 
> cairo mailing list
> [email protected]
> http://lists.cairographics.org/mailman/listinfo/cairo
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo