Re: [PATCH] V5: Use NEAREST filter when possible

On Tue, Aug 12, 2014 at 03:48:04PM -0700, Bill Spitzak wrote:
> (changed to use determinant funciton and remove debug printf)
> 
> Modifies _cairo_matrix_has_unity_scale to return true for 90 degree rotations
> by allowing error caused by inaccuracy in trig functions.
> 
> This fails after 14 additions of M_PI_2 to itself as a float argument to
> cairo_rotate, but the failure is in the detection of the integer translate,
> not in the trig components. I believe this is due to the matrix inversion,
> which may need similar rounding.

If you've run this through make test, does it increase the number of test
failures substantially?  Do we have adequate test coverage for the
_cairo_matrix_has_unity_scale routine?

Other than that, the change itself looks good to me, merged.

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

Bryce

> ---
>  src/cairo-matrix.c             |   37 +++++++++++++++++++++++--------------
>  src/cairo-pattern.c            |    2 +-
>  src/cairo-xcb-surface-render.c |    7 ++-----
>  3 files changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/src/cairo-matrix.c b/src/cairo-matrix.c
> index ba975be..ae498f5 100644
> --- a/src/cairo-matrix.c
> +++ b/src/cairo-matrix.c
> @@ -748,23 +748,32 @@ _cairo_matrix_is_integer_translation (const cairo_matrix_t *matrix,
>      return FALSE;
>  }
>  
> +#define SCALING_EPSILON _cairo_fixed_to_double(1)
> +
> +/* This only returns true if the matrix is 90 degree rotations or
> + * flips. It appears calling code is relying on this. It will return
> + * false for other rotations even if the scale is one. Approximations
> + * are allowed to handle matricies filled in using trig functions
> + * such as sin(M_PI_2).
> + */
>  cairo_bool_t
>  _cairo_matrix_has_unity_scale (const cairo_matrix_t *matrix)
>  {
> -    if (matrix->xy == 0.0 && matrix->yx == 0.0) {
> -	if (! (matrix->xx == 1.0 || matrix->xx == -1.0))
> -	    return FALSE;
> -	if (! (matrix->yy == 1.0 || matrix->yy == -1.0))
> -	    return FALSE;
> -    } else if (matrix->xx == 0.0 && matrix->yy == 0.0) {
> -	if (! (matrix->xy == 1.0 || matrix->xy == -1.0))
> -	    return FALSE;
> -	if (! (matrix->yx == 1.0 || matrix->yx == -1.0))
> -	    return FALSE;
> -    } else
> -	return FALSE;
> -
> -    return TRUE;
> +    /* check that the determinant is near +/-1 */
> +    double det = _cairo_matrix_compute_determinant (matrix);
> +    if (fabs (det * det - 1.0) < SCALING_EPSILON) {
> +	/* check that one axis is close to zero */
> +	if (fabs (matrix->xy) < SCALING_EPSILON  &&
> +	    fabs (matrix->yx) < SCALING_EPSILON)
> +	    return TRUE;
> +	if (fabs (matrix->xx) < SCALING_EPSILON  &&
> +	    fabs (matrix->yy) < SCALING_EPSILON)
> +	    return TRUE;
> +	/* If rotations are allowed then it must instead test for
> +	 * orthogonality. This is xx*xy+yx*yy ~= 0.
> +	 */
> +    }
> +    return FALSE;
>  }
>  
>  /* By pixel exact here, we mean a matrix that is composed only of
> diff --git a/src/cairo-pattern.c b/src/cairo-pattern.c
> index e104440..6905e15 100644
> --- a/src/cairo-pattern.c
> +++ b/src/cairo-pattern.c
> @@ -3363,6 +3363,7 @@ _cairo_pattern_analyze_filter (const cairo_pattern_t	*pattern,
>      case CAIRO_FILTER_GOOD:
>      case CAIRO_FILTER_BEST:
>      case CAIRO_FILTER_BILINEAR:
> +    case CAIRO_FILTER_FAST:
>  	/* If source pixels map 1:1 onto destination pixels, we do
>  	 * not need to filter (and do not want to filter, since it
>  	 * will cause blurriness)
> @@ -3381,7 +3382,6 @@ _cairo_pattern_analyze_filter (const cairo_pattern_t	*pattern,
>  	}
>  	break;
>  
> -    case CAIRO_FILTER_FAST:
>      case CAIRO_FILTER_NEAREST:
>      case CAIRO_FILTER_GAUSSIAN:
>      default:
> diff --git a/src/cairo-xcb-surface-render.c b/src/cairo-xcb-surface-render.c
> index 3f2fc43..a127000 100644
> --- a/src/cairo-xcb-surface-render.c
> +++ b/src/cairo-xcb-surface-render.c
> @@ -414,8 +414,7 @@ _pattern_is_supported (uint32_t flags,
>  	cairo_filter_t filter;
>  
>  	filter = pattern->filter;
> -	if (_cairo_matrix_has_unity_scale (&pattern->matrix) &&
> -	    _cairo_matrix_is_integer_translation (&pattern->matrix, NULL, NULL))
> +	if (_cairo_matrix_is_pixel_exact (&pattern->matrix))
>  	{
>  	    filter = CAIRO_FILTER_NEAREST;
>  	}
> @@ -1033,9 +1032,7 @@ _cairo_xcb_surface_setup_surface_picture(cairo_xcb_picture_t *picture,
>  
>      filter = pattern->base.filter;
>      if (filter != CAIRO_FILTER_NEAREST &&
> -	_cairo_matrix_has_unity_scale (&pattern->base.matrix) &&
> -	_cairo_fixed_is_integer (_cairo_fixed_from_double (pattern->base.matrix.x0)) &&
> -	_cairo_fixed_is_integer (_cairo_fixed_from_double (pattern->base.matrix.y0)))
> +        _cairo_matrix_is_pixel_exact (&pattern->base.matrix))
>      {
>  	filter = CAIRO_FILTER_NEAREST;
>      }
> -- 
> 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