[Pixman] [PATCH] test: Add cover-test

Pekka Paalanen ppaalanen at gmail.com
Thu Jun 4 06:00:30 PDT 2015


On Tue, 26 May 2015 23:58:30 +0100
Ben Avison <bavison at riscosopen.org> wrote:

> This test aims to verify both numerical correctness and the honouring of
> array bounds for scaled plots (both nearest-neighbour and bilinear) at or
> close to the boundary conditions for applicability of "cover" type fast paths
> and iter fetch routines.
> 
> It has a secondary purpose: by setting the env var EXACT (to any value) it
> will only test plots that are exactly on the boundary condition. This makes
> it possible to ensure that "cover" routines are being used to the maximum,
> although this requires the use of a debugger or code instrumentation to
> verify.
> ---
> Note that this must be pushed after Pekka's fence-image patches.
> 
>  test/Makefile.sources |    1 +
>  test/cover-test.c     |  376 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 377 insertions(+), 0 deletions(-)
>  create mode 100644 test/cover-test.c

Hi Ben,

this was quite a task to review. :-)

Most of the comments are just analysis of the code, to make sure I got
it right. Requests for changes should be obviously phrased.

> diff --git a/test/Makefile.sources b/test/Makefile.sources
> index 14a3710..5b901db 100644
> --- a/test/Makefile.sources
> +++ b/test/Makefile.sources
> @@ -26,6 +26,7 @@ TESTPROGRAMS =			      \
>  	glyph-test		      \
>  	solid-test                    \
>  	stress-test		      \
> +	cover-test		      \
>  	blitters-test		      \
>  	affine-test		      \
>  	scaling-test		      \
> diff --git a/test/cover-test.c b/test/cover-test.c
> new file mode 100644
> index 0000000..fb173b2
> --- /dev/null
> +++ b/test/cover-test.c
> @@ -0,0 +1,376 @@
> +/*
> + * Copyright © 2015 RISC OS Open Ltd
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and its
> + * documentation for any purpose is hereby granted without fee, provided that
> + * the above copyright notice appear in all copies and that both that
> + * copyright notice and this permission notice appear in supporting
> + * documentation, and that the name of the copyright holders not be used in
> + * advertising or publicity pertaining to distribution of the software without
> + * specific, written prior permission.  The copyright holders make no
> + * representations about the suitability of this software for any purpose.  It
> + * is provided "as is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN
> + * AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING
> + * OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS
> + * SOFTWARE.
> + *
> + * Author:  Ben Avison (bavison at riscosopen.org)
> + *
> + */
> +
> +#include "utils.h"
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +/* Approximate limits for random scale factor generation - these ensure we can
> + * get at least 8x reduction and 8x enlargement.
> + */
> +#define LOG2_MAX_FACTOR (3)
> +
> +#define SQRT_0POINT5_0POINT32_FIXED (0xB504F334u)

Ok, I checked. Mathematically it is equivalent, but maybe a slightly
more obvious name would be INV_SQRT_2_...?

> +
> +#define MAX_INC (SQRT_0POINT5_0POINT32_FIXED >> (31 - 16 - LOG2_MAX_FACTOR))

This evaluates to 11.314 decimal I think. It does match 2^3.5 with an
error in the order of 3e-6, is what Octave tells me. log2(abs(error)) =
-18.321, so if I'm thinking right, it means around 18 fractional bits
being correct, which means pixman_fixed_t should be accurate to the
last bit? I suppose it should be obvious from the integer math, but I
just checked with Octave.

MAX_INC is a pixman_fixed_t value, right? Could use a comment for that,
or making it a static const variable.

What MAX_INC actually is, is sqrt(0.5) * 2 * 2^3 = 2^-0.5 * 2^4 =
2^(4 - 0.5) = 2^3.5. Just like it is in random_scale_factor().

It took a good while to decode this. Could use more commentary on where
this comes from. :-)

> +
> +/* Minimum source width (in pixels) based on a typical page size of 4K and
> + * maximum colour depth of 32bpp.
> + */
> +#define MIN_SRC_WIDTH (4096 / 4)

This could actually be anything you want, fence_image_create_bits()
will round it up as necessary, but yeah, this is the minimum width for
4 bytespp images. The a8 mask image will be extended to be 4096 pixels
wide, too. The size will depend on the page size.

> +
> +/* Derive the destination width so that at max increment we fit within source */
> +#define DST_WIDTH (MIN_SRC_WIDTH * pixman_fixed_1 / MAX_INC)

This assumes fence_image_create_bits() uses exactly MIN_SRC_WIDTH,
rather than taking it only as a minimum. Would be "more correct" to
compute DST_WIDTH from the size actually used by
fence_image_create_bits().

Hmm, but since src can only be equal or larger than MIN_SRC_WIDTH, I
think it works in any case.

> +
> +/* Calculate heights the other way round - no limits due to page alignment here */
> +#define DST_HEIGHT 3
> +#define SRC_HEIGHT ((DST_HEIGHT * MAX_INC + pixman_fixed_1 - 1) / pixman_fixed_1)

Multiply and round up, ok.

> +
> +/* At the time of writing, all the scaled fast paths use SRC, OVER or ADD
> + * Porter-Duff operators. XOR is included in the list to ensure good
> + * representation of iter scanline fetch routines.
> + */
> +static const pixman_op_t op_list[] = {
> +    PIXMAN_OP_SRC,
> +    PIXMAN_OP_OVER,
> +    PIXMAN_OP_ADD,
> +    PIXMAN_OP_XOR,
> +};
> +
> +/* At the time of writing, all the scaled fast paths use a8r8g8b8, x8r8g8b8
> + * or r5g6b5, or red-blue swapped versions of the same. When a mask channel is
> + * used, it is always a8 (and so implicitly not component alpha). a1r5g5b5 is
> + * included because it is the only other format to feature in any iters. */
> +static const pixman_format_code_t img_fmt_list[] = {
> +    PIXMAN_a8r8g8b8,
> +    PIXMAN_x8r8g8b8,
> +    PIXMAN_r5g6b5,
> +    PIXMAN_a1r5g5b5
> +};

I didn't actually check these lists, I assume they are ok.

If someone adds more scaled fast paths, we should remember to augment
the lists.

> +
> +/* This is a flag reflecting the environment variable EXACT. It can be used
> + * to ensure that source coordinates corresponding exactly to the "cover" limits
> + * are used, rather than any "near misses". This can, for example, be used in
> + * conjunction with a debugger to ensure that only COVER fast paths are used.
> + */
> +static int exact;

Right, so this would be the test tool after tightening the boundary
checks to make sure we actually use COVER paths. We likely won't be
using COVER paths before.

And the test itself, whether EXACT or not, makes sure that no fast path
that happens to get used will go off image boundaries or even to the
row padding.

I think it would be valuable to explain these high level goals in a
comment at the top, even when they are in the commit message.

> +
> +static pixman_fixed_t
> +random_scale_factor(void)
> +{
> +    /* Get a random number with top bit set. */
> +    uint32_t f = prng_rand () | 0x80000000u;
> +

Presumably this is uniformly distributed between 2^31 and 2^32 - 1.
Right. It's not exactly uniform in the log2 space, which you can see in
the picture linked below. I think this is where the sawtooth pattern in
the final distribution comes from.

> +    /* In log(2) space, this is still approximately evenly spread between 31
> +     * and 32. Divide by sqrt(2) to centre the distribution on 2^31.
> +     */
> +    f = ((uint64_t) f * SQRT_0POINT5_0POINT32_FIXED) >> 32;

Ooh, I think I understand that. Ok. Centered around 31 in the log2
space, with spread of +/- 0.5.

> +
> +    /* Now shift right (ie divide by an integer power of 2) to spread the
> +     * distribution between centres at 2^(16 +/- LOG2_MAX_FACTOR).
> +     */
> +    f >>= 31 - 16 + prng_rand_n (2 * LOG2_MAX_FACTOR + 1) - LOG2_MAX_FACTOR;

This actually spreads it out to +/- 3.5 in log2 scale. The word
"approximate" would be in place here too.

Out of curiosity and because it's hard to reason about these things, I
did an experiment, and generated 10 million samples with this function.
The resulting distribution in log2 space is here:

https://people.collabora.com/~pq/pixman/random_scale_factor.png

I think it is close to uniform enough, right? And covers the expected
range.

Ok, now on the third or fourth day of looking at this, I finally
understand why the distribution is like it is. The first prng_rand()
produces the shape of one "tooth", and the prng__rand_n() then picks
one tooth out of the seven.

> +
> +    return f;
> +}
> +
> +static void
> +fence_image_randmemset (pixman_image_t *img)
> +{
> +    int rows = img->bits.height;
> +    uint32_t width = img->bits.width * PIXMAN_FORMAT_BPP (img->bits.format) / 8;
> +    uint32_t *bits = img->bits.bits;
> +    for (; rows > 0; --rows)
> +    {
> +        prng_randmemset (bits, width, 0);
> +        bits += img->bits.rowstride;
> +    }

Missing image_endian_swap(), perhaps? Oh, it's in the callers. Ok.

> +}
> +
> +static pixman_fixed_t
> +calc_translate (int            dst_size,
> +                int            src_size,
> +                pixman_fixed_t scale,
> +                pixman_bool_t  low_align,
> +                pixman_bool_t  bilinear)
> +{
> +    pixman_fixed_t ref_src, ref_dst, scaled_dst;
> +
> +    if (low_align)
> +    {
> +        ref_src = bilinear ? pixman_fixed_1 / 2 : pixman_fixed_e;

Why the bilinear case is not pixman_fixed_1 / 2 + pixman_fixed_e?
I have more questions of the same kind in check_transform().

> +        ref_dst = pixman_fixed_1 / 2;
> +    }
> +    else
> +    {
> +        ref_src = pixman_int_to_fixed (src_size) - bilinear * pixman_fixed_1 / 2;
> +        ref_dst = pixman_int_to_fixed (dst_size) - pixman_fixed_1 / 2;
> +    }
> +
> +    scaled_dst = ((uint64_t) ref_dst * scale + pixman_fixed_1 / 2) / pixman_fixed_1;

ref_dst is 16.16 fp, scale is 16.16 fp, so the product is 32.32 fp, but
adding pixman_fixed_1/2 to that actually means +0.5/65536, not +0.5.
And the final division then just scales it back to .16 fp.

Basically it's adding 0.5 * pixman_fixed_e... for rounding to nearest
representable value?

> +
> +    /* We need the translation to be set such that when ref_dst is fed through
> +     * the transformation matrix, we get ref_src as the result.
> +     */
> +    return ref_src - scaled_dst;

Alright!

> +}
> +
> +static pixman_fixed_t
> +random_offset (void)
> +{
> +    pixman_fixed_t offset = 0;
> +
> +    /* Ensure we test the exact case quite a lot */
> +    if (prng_rand_n (2))
> +        return offset;

50% of the cases are zero.

> +
> +    /* What happens when we are close to the edge of the first interpolation step? */
> +    if (prng_rand_n (2))
> +        offset += (pixman_fixed_1 >> BILINEAR_INTERPOLATION_BITS) - 16;

I had to draw this on paper before I finally got it. Right. Assuming I
guessed the meaning of INTERPOLATION_BITS right (number of most
significant fractional bits used in computations).

25% of the cases have this...

> +
> +    /* Try fine-grained variations */
> +    offset += prng_rand_n (32);

..with -16..15 adjustment and the other 25% has 0..31 adjustment.

> +
> +    /* Test in both directions */
> +    if (prng_rand_n (2))
> +        offset = -offset;

Right. Looks ok to me.

> +
> +    return offset;
> +}
> +
> +static void
> +check_transform (pixman_image_t     *dst_img,
> +                 pixman_image_t     *src_img,
> +                 pixman_transform_t *transform,
> +                 pixman_bool_t       bilinear)
> +{
> +    pixman_vector_t v1, v2;
> +
> +    v1.vector[0] = pixman_fixed_1 / 2;
> +    v1.vector[1] = pixman_fixed_1 / 2;
> +    v1.vector[2] = pixman_fixed_1;
> +    assert (pixman_transform_point (transform, &v1));

Take the center of the top-left corner pixel in destination and
transform it into source image coordinates. Ok.

> +
> +    v2.vector[0] = pixman_int_to_fixed (dst_img->bits.width) - pixman_fixed_1 / 2;
> +    v2.vector[1] = pixman_int_to_fixed (dst_img->bits.height) - pixman_fixed_1 / 2;
> +    v2.vector[2] = pixman_fixed_1;
> +    assert (pixman_transform_point (transform, &v2));

Similarly transform the bottom-right corner pixel center. Ok.

> +
> +    if (bilinear)
> +    {
> +        assert (v1.vector[0] >= pixman_fixed_1 / 2);
> +        assert (v1.vector[1] >= pixman_fixed_1 / 2);

Does this not need to be pixman_fixed_1 / 2 + pixman_fixed_e?

If the same cut-off rules apply, I'd assume exactly pixman_fixed_1/2 to
hit the previous pixel (with weight 0 maybe?).

> +        assert (v2.vector[0] <= pixman_int_to_fixed (src_img->bits.width) -
> +                                    pixman_fixed_1 / 2);
> +        assert (v2.vector[1] <= pixman_int_to_fixed (src_img->bits.height) -
> +                                    pixman_fixed_1 / 2);

This looks ok to me.

> +    }
> +    else
> +    {
> +        assert (v1.vector[0] >= pixman_fixed_e);
> +        assert (v1.vector[1] >= pixman_fixed_e);

Ensure src x,y are in the top-left corner pixel area. Exactly 0 would hit
the previous pixel outside of the image, so that's why need to have
greater or equal to eps. Did I get that right?

> +        assert (v2.vector[0] <= pixman_int_to_fixed (src_img->bits.width));
> +        assert (v2.vector[1] <= pixman_int_to_fixed (src_img->bits.height));

And here the same cut-off rules, on the right/bottom. Ok.

> +    }
> +}
> +
> +static uint32_t
> +test_cover (int testnum, int verbose)
> +{
> +    pixman_fixed_t         x_scale, y_scale;
> +    pixman_bool_t          left_align, top_align;
> +    pixman_bool_t          bilinear;
> +    pixman_op_t            op;
> +    size_t                 src_fmt_index;
> +    pixman_format_code_t   src_fmt, dst_fmt, mask_fmt;
> +    pixman_image_t        *src_img, *dst_img, *mask_img;
> +    pixman_transform_t     src_transform, mask_transform;
> +    pixman_fixed_t         fuzz[4];
> +    uint32_t               crc32;
> +    /* We allocate one fenced image for each pixel format up-front. This is to
> +     * avoid spending a lot of time on memory management rather than on testing
> +     * Pixman optimisations. We need one per thread because the transformation
> +     * matrices and filtering are properties of the source and mask images.
> +     */
> +    static pixman_image_t *src_imgs[ARRAY_LENGTH (img_fmt_list)];
> +    static pixman_image_t *mask_bits_img;
> +    static pixman_bool_t   fence_images_created;
> +#ifdef USE_OPENMP
> +#pragma omp threadprivate (src_imgs)
> +#pragma omp threadprivate (mask_bits_img)
> +#pragma omp threadprivate (fence_images_created)
> +#endif

This is the only reference to OpenMP here, but I see that
fuzzer_test_main() is the one having the omp parallel section, which
then calls here.

I was reading
http://stackoverflow.com/questions/18022133/difference-between-openmp-threadprivate-and-private
and if I understand right, this is correct.

They are never freed, but that's ok.

> +
> +    if (!fence_images_created)
> +    {
> +        int i;
> +
> +        prng_srand (0);

All threads need to init with the same seed, but the first testnum for
each thread will be different, hence 0 here. Ok.

> +
> +        for (i = 0; i < ARRAY_LENGTH (img_fmt_list); i++)
> +        {
> +            src_imgs[i] = fence_image_create_bits (img_fmt_list[i], MIN_SRC_WIDTH,
> +                                                   SRC_HEIGHT, TRUE);
> +            fence_image_randmemset (src_imgs[i]);
> +            image_endian_swap (src_imgs[i]);
> +        }
> +
> +        mask_bits_img = fence_image_create_bits (PIXMAN_a8, MIN_SRC_WIDTH,
> +                                                 SRC_HEIGHT, TRUE);
> +        fence_image_randmemset (mask_bits_img);
> +        image_endian_swap (mask_bits_img);
> +
> +        fence_images_created = TRUE;
> +    }
> +
> +    prng_srand (testnum);
> +
> +    x_scale = random_scale_factor ();
> +    y_scale = random_scale_factor ();
> +    left_align = prng_rand_n (2);
> +    top_align = prng_rand_n (2);
> +    bilinear = prng_rand_n (2);
> +
> +    op = op_list[prng_rand_n (ARRAY_LENGTH (op_list))];
> +
> +    dst_fmt = img_fmt_list[prng_rand_n (ARRAY_LENGTH (img_fmt_list))];
> +    dst_img = pixman_image_create_bits (dst_fmt, DST_WIDTH, DST_HEIGHT, NULL, 0);
> +    prng_randmemset (dst_img->bits.bits,
> +                     dst_img->bits.rowstride * DST_HEIGHT * sizeof (uint32_t), 0);
> +    image_endian_swap (dst_img);

Btw. just a random thought: seem like utils.h function for the
equivalent of fence_image_randmemset + image_endian_swap operating on a
pixman_image_t would be helpful. (Not asking it for this patch if not
for fixing the image_endian_swap() accessing padding.)

Now that I am looking at image_endian_swap() implementation in utils.c...

<offtopic>
- Does endianess really affect the order of bits in a byte in Pixman
  formats? How can that be meaningful? And it's only done for 1 and 4
  bits pp formats?

egads, the definition must really change meaning with endianess:
http://cgit.freedesktop.org/pixman/tree/pixman/pixman-access.c#n49
</offtopic>

- Looks like it processes the whole stride, not width * bytespp, which
  means this is going to explode on fenced images in the padding,
  right? Of course only on big-endian, because on little-endian the
  swap is a no-op.

> +
> +    src_fmt_index = prng_rand_n (ARRAY_LENGTH (img_fmt_list));
> +    src_fmt = img_fmt_list[src_fmt_index];
> +    src_img = src_imgs[src_fmt_index];
> +    pixman_image_set_filter (src_img,
> +                             bilinear ? PIXMAN_FILTER_BILINEAR : PIXMAN_FILTER_NEAREST,
> +                             NULL, 0);
> +    pixman_transform_init_scale (&src_transform, x_scale, y_scale);
> +    src_transform.matrix[0][2] = calc_translate (dst_img->bits.width,
> +                                                 src_img->bits.width,
> +                                                 x_scale, left_align, bilinear);
> +    src_transform.matrix[1][2] = calc_translate (dst_img->bits.height,
> +                                                 src_img->bits.height,
> +                                                 y_scale, top_align, bilinear);
> +
> +    if (prng_rand_n (2))
> +    {
> +        /* No mask */
> +        mask_fmt = PIXMAN_null;
> +        mask_img = NULL;
> +    }
> +    else if (prng_rand_n (2))
> +    {
> +        /* a8 bitmap mask */
> +        mask_fmt = PIXMAN_a8;
> +        mask_img = mask_bits_img;
> +        pixman_image_set_filter (mask_img,
> +                                 bilinear ? PIXMAN_FILTER_BILINEAR : PIXMAN_FILTER_NEAREST,
> +                                 NULL, 0);
> +        pixman_transform_init_scale (&mask_transform, x_scale, y_scale);
> +        mask_transform.matrix[0][2] = calc_translate (dst_img->bits.width,
> +                                                      mask_img->bits.width,
> +                                                      x_scale, left_align, bilinear);
> +        mask_transform.matrix[1][2] = calc_translate (dst_img->bits.height,
> +                                                      mask_img->bits.height,
> +                                                      y_scale, top_align, bilinear);

The mask image is likely a different size (4096 wide, not 1024 like
src), so the transformation for mask is quite different from src.
That's just fine, right?

Oh! Because image sizes depend on the value returned by getpagesize(),
doesn't this mean this test can only pass on 4096 B page size?
Should this test be skipped on other systems? Maybe with a plea to add
the crc from that system for their page size in main()?

(Return 77 from main() is the signal for SKIP.)

> +    }
> +    else
> +    {
> +        /* Solid mask */
> +        pixman_color_t color;
> +        memset (&color, 0xAA, sizeof color);
> +        mask_fmt = PIXMAN_solid;
> +        mask_img = pixman_image_create_solid_fill (&color);
> +    }
> +
> +    if (!exact)
> +    {
> +        int i = 0;
> +        while (i < 4)
> +            fuzz[i++] = random_offset ();
> +        src_transform.matrix[0][2] += fuzz[0];
> +        src_transform.matrix[1][2] += fuzz[1];
> +        mask_transform.matrix[0][2] += fuzz[2];
> +        mask_transform.matrix[1][2] += fuzz[3];
> +    }
> +
> +    pixman_image_set_transform (src_img, &src_transform);
> +    if (mask_fmt == PIXMAN_a8)
> +        pixman_image_set_transform (mask_img, &mask_transform);
> +
> +    if (verbose)
> +    {
> +        printf ("op=%s\n", operator_name (op));
> +        printf ("src_fmt=%s, dst_fmt=%s, mask_fmt=%s\n",
> +                format_name (src_fmt), format_name (dst_fmt),
> +                format_name (mask_fmt));

Printing the image widths might be good, it would show if they ever
change.

> +        printf ("x_scale=0x%08X, y_scale=0x%08X, align %s/%s, %s\n",
> +                x_scale, y_scale,
> +                left_align ? "left" : "right", top_align ? "top" : "bottom",
> +                bilinear ? "bilinear" : "nearest");
> +        if (!exact)
> +        {
> +            int i = 0;
> +            printf ("fuzz factors");
> +            while (i < 4)
> +                printf (" %d", fuzz[i++]);
> +            printf ("\n");
> +        }
> +    }
> +
> +    if (exact)
> +    {
> +        check_transform (dst_img, src_img, &src_transform, bilinear);
> +        if (mask_fmt == PIXMAN_a8)
> +            check_transform (dst_img, mask_img, &mask_transform, bilinear);
> +    }
> +
> +    pixman_image_composite (op, src_img, mask_img, dst_img,
> +                            0, 0, 0, 0, 0, 0,
> +                            dst_img->bits.width, dst_img->bits.height);
> +
> +    if (verbose)
> +        print_image (dst_img);
> +
> +    crc32 = compute_crc32_for_image (0, dst_img);
> +
> +    pixman_image_unref (dst_img);
> +    if (mask_fmt == PIXMAN_solid)
> +        pixman_image_unref (mask_img);
> +
> +    return crc32;
> +}
> +
> +int
> +main (int argc, const char *argv[])
> +{
> +    exact = getenv ("EXACT") != NULL;

Would be nice to print a message when EXACT is defined, so it's clear
it is there.

> +
> +    return fuzzer_test_main ("cover", 2000000,
> +                             exact ? 0xAEB36C31 : 0xF972EE43,
> +                             test_cover, argc, argv);
> +}

I didn't check coding style, it mostly looks good. I'd just sprinkle
some empty lines here and there. I can do that when merging this patch.

Ah, and I still have the few details to sort out in the fence image
patches.

I ran this test on x86_64, in both reference and optimized builds, with
and without EXACT, and it always passes.


This is how you described the tests we'd need:

On Thu, 14 May 2015 00:19:40 +0100
"Ben Avison" <bavison at riscosopen.org> wrote:

> What we need to test:
> * Both nearest-neighbour and bilinear scaling.

Done.

> * A range of scale factors, both reduction and enlargement. I remember
>    that I used a variety of routines for difference scales of reduction
>    (1..2x, 2..3x, 3..4x and so on) to avoid having to hold the integer
>    part of the increment in a register, so we need to test up to at least
>    8x reduction.

Done.

> * We need to test alignment with both the extreme minimum and extreme
>    maximum source coordinates permitted. Given that the scale factors
>    can't be assumed to be nice numbers, I think we need to test them
>    independently, using the translation offsets in the transformation
>    matrix to ensure they align as desired.

To do?

> * Possibly test "near miss" cases as well, though these would probably
>    want to be weighted so that positions at or closer to the limit are
>    checked most.

I think it does this with the fuzz-factors (random_offset).

> * Test both horizontal and vertical axes.

Done.

> * We should survey which Porter-Duff operations have scaled fast paths
>    (across all platforms) and make sure we test each of those, plus at
>    least one other which will guarantee that we're exercising the fetchers
>    - XOR is probably safe for this.

Done, AFAIK.

> 
> For nearest-neighbour scaling, the acceptable criteria should be:
> * Low end: centre of first output pixel coincides with source coordinate
>    pixman_fixed_e
> * High end: centre of last pixel output coincides with source coordinate
>    pixman_fixed_1*width

Done.

> 
> For bilinear scaling:
> * Low end: centre of first output pixel coincides with source coordinate
>    pixman_fixed_1/2 (i.e. centre of first source pixel)

This one I had some questions about, but it is in the code, so done.

> * High end: I'm slightly torn on this one. You could say
>    pixman_fixed_1*width - pixman_fixed_1/2
>    But it's also true that BILINEAR_INTERPOLATION_BITS comes into play,
>    meaning that some slightly higher coordinates should also be achievable
>    without requiring any input from the out-of-bounds pixel. For
>    BILINEAR_INTERPOLATION_BITS=7, anything up to and including 0x0.01FF
>    higher than this should be indistinguishable. And yes, that does mean
>    that there is a very slight bias because coordinates are truncated by
>    9 bits, rounding towards zero.

This is there too, I think.


Thanks,
pq


More information about the Pixman mailing list