Seems correct and gets more cases than my code did (I was assuming nobody
would use bilinear for scales less than 1/2 so I did not check for it).
Minor comments:
On Mon, Mar 14, 2016 at 11:19 PM, Søren Sandmann Pedersen <
soren.sandm...@gmail.com> wrote:
> Generalize and simplify the code that reduces BILINEAR to NEAREST so
> that all the reduction happens for all affine transformations where
> t00..t22 are integers and (t00 + t01) and (t10 + t11) are both
> odd. This is a sufficient condition for the resulting transformed
> coordinates being exactly at the center of a pixel so that BILINEAR
> becomes identical to NEAREST.
>
Actually the last row (t20, t21, t22) has to be 0,0,1 (or 0,0,w where you
then make a new matrix that divides everything by w and then it is 0,0,1).
This is already tested for by the test for AFFINE, but this comment is a
bit incorrect.
>
> Signed-off-by: Søren Sandmann <soren.sandm...@gmail.com>
> ---
> pixman/pixman-image.c | 69
> ++++++++++++++++++++++++++++++---------------------
> 1 file changed, 41 insertions(+), 28 deletions(-)
>
> diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
> index 1ff1a49..cff8a30 100644
> --- a/pixman/pixman-image.c
> +++ b/pixman/pixman-image.c
> @@ -335,37 +335,50 @@ 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 ]
> + * [ 120, t21, t22 ]
>
typo 120->t20. I would change the last row to just read [0,0,1] as you are
assuming this and not using these values in the following equation.
> + *
> + * 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
>
I would use u,v rather than tx,ty for the source coordinates. More complex
analysis of transforms gets very hard to read unless the variables are kept
to one letter.
+ * 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]) == 0
> &&
> + pixman_fixed_frac (t[0][1]) == 0
> &&
> + pixman_fixed_frac (t[0][2]) == 0
> &&
> + ((t[0][0] + t[0][1]) & pixman_fixed_1) == pixman_fixed_1
> &&
> + pixman_fixed_frac (t[1][0]) == 0
> &&
> + pixman_fixed_frac (t[1][1]) == 0
> &&
> + pixman_fixed_frac (t[1][2]) == 0
> &&
> + ((t[1][0] + t[1][1]) & pixman_fixed_1) == pixman_fixed_1)
>
You can or all the numbers together to test they are all integers in one
step.
> {
> - 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.
> + */
> + 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)
> + {
> + flags |= FAST_PATH_NEAREST_FILTER;
> + }
>
Can this be tested to see if the bug still exists?
The comment should be reworded to point out that the bug is in the BILINEAR
code, not the NEAREST code, but this patch is needed to not have the
NEAREST case "fix" the ones that are broken and thus make the fast path
output different.
> }
> }
> break;
> --
> 1.7.11.7
>
> _______________________________________________
> Pixman mailing list
> Pixman@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pixman
>
_______________________________________________
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman
