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

[Pekka Paalanen]

On Wed, 02 Sep 2015 20:34:56 +0100
"Ben Avison" <bavison at riscosopen.org> wrote:

> 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.

Yes. My point is, you spell out the -16 in the code, but not the 32 -
1. Anyway, it doesn't matter, it is correct.


> >> +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.

Ah, yes, now I understand.

This is *not* for protecting against out-of-bounds access, but this is
allowing for the extreme cases where a cover path is still
*theoretically* possible. (Ignoring what the COVER flags actually mean
in Pixman implementation.)

> > 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

Right! Now I think I'm getting the hang of this. :-)

> 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)

Now I understand that what you want to do with the mentioned patch
series is to change the meaning of COVER_CLIP_BILINEAR flag, not only
to remove the useless fuzz margins.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20150903/a9b3653d/attachment.sig>