[Pixman] [PATCH 03/14] More general BILINEAR=>NEAREST reduction

[Bill Spitzak]

Only minor comments on the large nearest patch at the bottom

On 04/11/2016 07:36 PM, Søren Sandmann Pedersen wrote:
> Generalize and simplify the code that reduces BILINEAR to NEAREST so
> that the reduction happens for all affine transformations where
> t00...t12 are integers and (t00 + t01) and (t10 + t11) are both
> odd. This is a sufficient condition for the resulting transformed
> coordinates to be exactly at the center of a pixel so that BILINEAR
> becomes identical to NEAREST.
>
> V2: Address some comments by Bill Spitzak
>
> Signed-off-by: Søren Sandmann <soren.sandmann at gmail.com>
> ---
>   pixman/pixman-image.c | 66 +++++++++++++++++++++++++++++----------------------
>   1 file changed, 38 insertions(+), 28 deletions(-)
>
> diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
> index 1ff1a49..681864e 100644
> --- a/pixman/pixman-image.c
> +++ b/pixman/pixman-image.c
> @@ -335,37 +335,47 @@ compute_image_info (pixman_image_t *image)
>   	{
>   	    flags |= FAST_PATH_NEAREST_FILTER;
>   	}
> -	else if (
> -	    /* affine and integer translation components in matrix ... */
> -	    ((flags & FAST_PATH_AFFINE_TRANSFORM) &&
> -	     !pixman_fixed_frac (image->common.transform->matrix[0][2] |
> -				 image->common.transform->matrix[1][2])) &&
> -	    (
> -		/* ... combined with a simple rotation */
> -		(flags & (FAST_PATH_ROTATE_90_TRANSFORM |
> -			  FAST_PATH_ROTATE_180_TRANSFORM |
> -			  FAST_PATH_ROTATE_270_TRANSFORM)) ||
> -		/* ... or combined with a simple non-rotated translation */
> -		(image->common.transform->matrix[0][0] == pixman_fixed_1 &&
> -		 image->common.transform->matrix[1][1] == pixman_fixed_1 &&
> -		 image->common.transform->matrix[0][1] == 0 &&
> -		 image->common.transform->matrix[1][0] == 0)
> -		)
> -	    )
> +	else if (flags & FAST_PATH_AFFINE_TRANSFORM)
>   	{
> -	    /* FIXME: there are some affine-test failures, showing that
> -	     * handling of BILINEAR and NEAREST filter is not quite
> -	     * equivalent when getting close to 32K for the translation
> -	     * components of the matrix. That's likely some bug, but for
> -	     * now just skip BILINEAR->NEAREST optimization in this case.
> +	    /* Suppose the transform is
> +	     *
> +	     *    [ t00, t01, t02 ]
> +	     *    [ t10, t11, t12 ]
> +	     *    [   0,   0,   1 ]
> +	     *
> +	     * and the destination coordinates are (n + 0.5, m + 0.5). Then
> +	     * the transformed x coordinate is:
> +	     *
> +	     *     tx = t00 * (n + 0.5) + t01 * (m + 0.5) + t02
> +	     *        = t00 * n + t01 * m + t02 + (t00 + t01) * 0.5
> +	     *
> +	     * which implies that if t00, t01 and t02 are all integers
> +	     * and (t00 + t01) is odd, then tx will be an integer plus 0.5,
> +	     * which means a BILINEAR filter will reduce to NEAREST. The same
> +	     * applies in the y direction
>   	     */
> -	    pixman_fixed_t magic_limit = pixman_int_to_fixed (30000);
> -	    if (image->common.transform->matrix[0][2] <= magic_limit  &&
> -	        image->common.transform->matrix[1][2] <= magic_limit  &&
> -	        image->common.transform->matrix[0][2] >= -magic_limit &&
> -	        image->common.transform->matrix[1][2] >= -magic_limit)
> +	    pixman_fixed_t (*t)[3] = image->common.transform->matrix;
> +
> +	    if ((pixman_fixed_frac (
> +		     t[0][0] | t[0][1] | t[0][2] |
> +		     t[1][0] | t[1][1] | t[1][2]) == 0)			&&
> +		(pixman_fixed_to_int (
> +		    (t[0][0] + t[0][1]) & (t[1][0] + t[1][1])) % 2) == 1)
>   	    {
> -		flags |= FAST_PATH_NEAREST_FILTER;
> +		/* FIXME: there are some affine-test failures, showing that
> +		 * handling of BILINEAR and NEAREST filter is not quite
> +		 * equivalent when getting close to 32K for the translation
> +		 * components of the matrix. That's likely some bug, but for
> +		 * now just skip BILINEAR->NEAREST optimization in this case.
> +		 */

May be a good idea to point out that the bug is (probably) in BILINEAR, 
not in NEAREST. Also you think this may have been fixed so this could be 
removed, but that would be a different patch.

> +		pixman_fixed_t magic_limit = pixman_int_to_fixed (30000);
> +		if (image->common.transform->matrix[0][2] <= magic_limit  &&
> +		    image->common.transform->matrix[1][2] <= magic_limit  &&
> +		    image->common.transform->matrix[0][2] >= -magic_limit &&
> +		    image->common.transform->matrix[1][2] >= -magic_limit)

I would change these to use 't'.

> +		{
> +		    flags |= FAST_PATH_NEAREST_FILTER;
> +		}
>   	    }
>   	}
>   	break;
>