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

[Ben Avison]

On Wed, 02 Sep 2015 14:03:01 +0100, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> I am still a tiny bit struggling with why 31 and not 32, but it does
> add up.

Maybe the reasoning in the comments in random_scale_factor() make more
sense? There, it only talks about the numbers as integers, avoiding the
additional confusion caused by MAX_INC being 0.32 fixed-point and the
return value being 16.16 fixed-point.

If you still want to think of it as real numbers, then:

* random_scale_factor() generates a scale factor between
   2^(-LOG2_MAX_FACTOR-0.5) and 2^(LOG2_MAX_FACTOR+0.5)
* INV_SQRT_2_0POINT32_FIXED represents 2^-0.5
* to get from 2^-0.5 to 2^(LOG2_MAX_FACTOR+0.5) we must multiply by
   2^(LOG2_MAX_FACTOR+1)
* but to convert from 0.32 to 16.16 fixed point, we multiply by 2^(-32+16)
* so the total factor is 2^(LOG2_MAX_FACTOR+1-32+16)
* but this is < 1, so is actually a shift right by
   -LOG2_MAX_FACTOR-1+32-16, which can trivially be rearranged to match
   the macro definition.

>> +static pixman_image_t *
>> +create_src_image (pixman_format_code_t fmt)
>> +{
[...]
>> +    prng_randmemset (tmp_img->bits.bits,
>> +                     tmp_img->bits.rowstride * DST_HEIGHT * sizeof (uint32_t), 0);
>
> DST_HEIGHT? Should that not be SRC_HEIGHT?

I think you're right, yes - looks like careless copy-and-paste from the
code to initialise the destination image. That means part of the image
was uninitialised, yet it never showed up as a difference on subsequent
runs. The checksums will need updating too. I'll repost shortly.

>> +static void
>> +check_transform (pixman_image_t     *dst_img,
>> +                 pixman_image_t     *src_img,
>> +                 pixman_transform_t *transform,
>> +                 pixman_bool_t       bilinear)
>> +{
[...]
>> +    if (bilinear)
>> +    {
>> +        assert (v1.vector[0] >= pixman_fixed_1 / 2);
>> +        assert (v1.vector[1] >= pixman_fixed_1 / 2);
>> +        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);
>
> Since we had that discussion about bilinear filtering sampling pixels at
> n=0,1 instead of n=-1,0, should the last two asserts not have < instead
> of <=?

No. calc_translate() specifically ensures that (depending upon low_align)
either the lower or upper coordinate exactly corresponds to the lowest or
highest value that contains no non-zero-weighted contribution from any
pixel beyond the source image (and the image sizes are chosen so that the
opposite coordinate should always fall within the source image too, at
the largest possible increment). These are in fact the very cases that
are most likely to trigger a fast path or fetcher to perform an out-of
bounds access, so they were deliberately included.

> I mean, wouldn't sampling for x=width-0.5 cause reads on pixels
> n=width-1,width, where the latter would be out of bounds (even if
> weight zero)?

My recent 7-patch series deals with precisely this case:
http://lists.freedesktop.org/archives/pixman/2015-August/003878.html

Before this patch series, the conditions under which the
FAST_PATH_SAMPLES_COVER_CLIP_BILINEAR flag is set mean that this case
never gets passed to a "cover" fast path, even though such fast paths can
actually handle them by at least these three methods:

* check the weight and don't load the upper sample if it's zero (it turns
   out this can be achieved for free with ARM fetchers)
* invert the increments so that when x=width-0.5 it reads samples
   n=width-2,width-1 and applies a weight of 0 to the former (I used this
   for pixman-fast-path.c)
* detect the case in the C binding function and handle it separately (I
   used this for the existing armv7/mips/sse2 fast paths)

The fuzz factor added into the non-EXACT case means that even with the
old rules for the cover clip flag, we should end up exercising both cover
fast paths and repeat fast paths at least some of the time, right up to
the limits of their applicability.

Ben