[Pixman] [PATCH 1/7] Refactor calculation of cover flags

[Siarhei Siamashka]

On Mon, 24 Aug 2015 21:42:00 +0100
Ben Avison <bavison at riscosopen.org> wrote:

> This patch has no effect upon the way the flags are calculated, but splitting
> it out into a separate patch like this means we can consider the bilinear and
> nearest calculations independently.
> ---
>  pixman/pixman.c |   25 ++++++++-----------------
>  1 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/pixman/pixman.c b/pixman/pixman.c
> index a07c577..982c820 100644
> --- a/pixman/pixman.c
> +++ b/pixman/pixman.c
> @@ -497,29 +497,20 @@ analyze_extent (pixman_image_t       *image,
>      if (!compute_transformed_extents (transform, extents, &transformed))
>  	return FALSE;
>  
> -    /* Expand the source area by a tiny bit so account of different rounding that
> -     * may happen during sampling. Note that (8 * pixman_fixed_e) is very far from
> -     * 0.5 so this won't cause the area computed to be overly pessimistic.
> -     */
> -    transformed.x1 -= 8 * pixman_fixed_e;
> -    transformed.y1 -= 8 * pixman_fixed_e;
> -    transformed.x2 += 8 * pixman_fixed_e;
> -    transformed.y2 += 8 * pixman_fixed_e;

Thanks for spotting this! I think that this code was used to compensate
matrix multiplication accuracy problems in older versions of pixman.
But we have already fixed these accuracy problems some time ago:

    http://cgit.freedesktop.org/pixman/commit/?id=c3deb8334a71998b986a7b8d5b74bedf26cc23aa
    http://cgit.freedesktop.org/pixman/commit/?id=5a78d74ccccba2aeb473f04ade44512d2f6c0613
    http://cgit.freedesktop.org/pixman/commit/?id=ed39992564beefe6b12f81e842caba11aff98a9c

Basically, the old pixman matrix multiplication code did the following
calculations:

    "((a + 0x8000) >> 16) + ((b + 0x8000) >> 16) + ((c + 0x8000) >> 16)

They were replaced with a better implementation, which avoids doing
any intermediate rounding:

    "(a + b + c + 0x8000) >> 16"

And this is where the magic "8 * pixman_fixed_e" comes from. The
accumulated rounding errors could not be larger than (0.5 * 3),
so 8 was clearly a safe choice, even somewhat larger than necessary.

Now we can drop this "8 * pixman_fixed_e" adjustment because there
is no accuracy loss in the matrix multiplication for affine
transformations. And it is probably better to just do this simple fix
with a single patch. There is no need to spread it across multiple
patches.

The commit message should just explain that for affine transformations
we are guaranteed to get the same end result by any of the following:
1) transforming the coordinate of the leftmost destination pixel and
   then walking (width - 1) steps in the source space.
2) transforming the coordinate of the rightmost destination pixel.

>      if (image->common.type == BITS)
>      {
> -	if (pixman_fixed_to_int (transformed.x1) >= 0			&&
> -	    pixman_fixed_to_int (transformed.y1) >= 0			&&
> -	    pixman_fixed_to_int (transformed.x2) < image->bits.width	&&
> -	    pixman_fixed_to_int (transformed.y2) < image->bits.height)
> +	if (pixman_fixed_to_int (transformed.x1 - 8 * pixman_fixed_e) >= 0                &&
> +	    pixman_fixed_to_int (transformed.y1 - 8 * pixman_fixed_e) >= 0                &&
> +	    pixman_fixed_to_int (transformed.x2 + 8 * pixman_fixed_e) < image->bits.width &&
> +	    pixman_fixed_to_int (transformed.y2 + 8 * pixman_fixed_e) < image->bits.height)

Just as you do it in
    http://lists.freedesktop.org/archives/pixman/2015-August/003877.html
this part becomes:

if (pixman_fixed_to_int (transformed.x1 - pixman_fixed_e) >= 0                &&
    pixman_fixed_to_int (transformed.y1 - pixman_fixed_e) >= 0                &&
    pixman_fixed_to_int (transformed.x2 - pixman_fixed_e) < image->bits.width &&
    pixman_fixed_to_int (transformed.y2 - pixman_fixed_e) < image->bits.height)

>  	{
>  	    *flags |= FAST_PATH_SAMPLES_COVER_CLIP_NEAREST;
>  	}
>  
> -	if (pixman_fixed_to_int (transformed.x1 - pixman_fixed_1 / 2) >= 0		  &&
> -	    pixman_fixed_to_int (transformed.y1 - pixman_fixed_1 / 2) >= 0		  &&
> -	    pixman_fixed_to_int (transformed.x2 + pixman_fixed_1 / 2) < image->bits.width &&
> -	    pixman_fixed_to_int (transformed.y2 + pixman_fixed_1 / 2) < image->bits.height)
> +	if (pixman_fixed_to_int (transformed.x1 - pixman_fixed_1 / 2 - 8 * pixman_fixed_e) >= 0                &&
> +	    pixman_fixed_to_int (transformed.y1 - pixman_fixed_1 / 2 - 8 * pixman_fixed_e) >= 0                &&
> +	    pixman_fixed_to_int (transformed.x2 + pixman_fixed_1 / 2 + 8 * pixman_fixed_e) < image->bits.width &&
> +	    pixman_fixed_to_int (transformed.y2 + pixman_fixed_1 / 2 + 8 * pixman_fixed_e) < image->bits.height)

And this part does not need any changes. At least as far as dropping
"8 * pixman_fixed_e" is concerned.

>  	{
>  	    *flags |= FAST_PATH_SAMPLES_COVER_CLIP_BILINEAR;
>  	}

I believe that neither of FAST_PATH_SAMPLES_COVER_CLIP_NEAREST and
FAST_PATH_SAMPLES_COVER_CLIP_BILINEAR flags matters for projective
transformations, but this can be additionally checked just in case.

-- 
Best regards,
Siarhei Siamashka