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

[Oded Gabbay]

On Tue, Apr 12, 2016 at 7:03 PM, Bill Spitzak <spitzak at gmail.com> wrote:
> 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'.

Søren,
Is it OK if I will change those to 't' and push the patch with that change ?

Oded

>
>> +               {
>> +                   flags |= FAST_PATH_NEAREST_FILTER;
>> +               }
>>             }
>>         }
>>         break;
>>
>
> _______________________________________________
> Pixman mailing list
> Pixman at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pixman

With that change, this patch is:

Acked-by: Oded Gabbay <oded.gabbay at gmail.com>