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

2015-09-16 Thread Oded Gabbay 
On Fri, Sep 11, 2015 at 12:06 PM, Pekka Paalanen  wrote:
> On Thu, 10 Sep 2015 16:11:07 +0100
> "Ben Avison"  wrote:
>
>> On Thu, 10 Sep 2015 13:16:22 +0100, Pekka Paalanen  
>> wrote:
>> > Changes in v5:
>> >  Skip if fenced memory is not supported.
>>
>> OK, I see how you've done that - fence_get_page_size() returning 0.
>>
>> I have no further comments. Is it appropriate for me to say I'll give a
>> Reviewed-by since I wrote the original version of the patch?
>
> Hi Ben,
>
> I think we can do it by adding this bunch of tags to the commit message:
>
> Signed-off-by: Ben Avison 
> Reviewed-by: Pekka Paalanen 
> [Pekka: changes in v4 and v5]
> Signed-off-by: Pekka Paalanen 
> Reviewed-by: Ben Avison 
>
> You will naturally be the patch author. This should convey exactly what
> we did.
>
> Oded wants to have a final look too, so I'll see about pushing this on
> Monday.
>
>
> Thanks,
> pq
>
> ___
> Pixman mailing list
> Pixman@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pixman
>

Hi,

Acked-by: Oded Gabbay 

   Oded
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

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

2015-09-16 Thread Pekka Paalanen 
On Wed, 16 Sep 2015 12:47:34 +0300
Oded Gabbay  wrote:

> On Fri, Sep 11, 2015 at 12:06 PM, Pekka Paalanen  wrote:
> > On Thu, 10 Sep 2015 16:11:07 +0100
> > "Ben Avison"  wrote:
> >
> >> On Thu, 10 Sep 2015 13:16:22 +0100, Pekka Paalanen  
> >> wrote:
> >> > Changes in v5:
> >> >  Skip if fenced memory is not supported.
> >>
> >> OK, I see how you've done that - fence_get_page_size() returning 0.
> >>
> >> I have no further comments. Is it appropriate for me to say I'll give a
> >> Reviewed-by since I wrote the original version of the patch?
> >
> > Hi Ben,
> >
> > I think we can do it by adding this bunch of tags to the commit message:
> >
> > Signed-off-by: Ben Avison 
> > Reviewed-by: Pekka Paalanen 
> > [Pekka: changes in v4 and v5]
> > Signed-off-by: Pekka Paalanen 
> > Reviewed-by: Ben Avison 
> >
> > You will naturally be the patch author. This should convey exactly what
> > we did.
> >
> > Oded wants to have a final look too, so I'll see about pushing this on
> > Monday.

> 
> Acked-by: Oded Gabbay 

Pushed:
   812c9c9..4c71f59  master -> master


Thanks,
pq


pgpDvHVwGIQwz.pgp
Description: OpenPGP digital signature
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

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

2015-09-11 Thread Pekka Paalanen 
On Thu, 10 Sep 2015 16:11:07 +0100
"Ben Avison"  wrote:

> On Thu, 10 Sep 2015 13:16:22 +0100, Pekka Paalanen  
> wrote:
> > Changes in v5:
> >  Skip if fenced memory is not supported.
> 
> OK, I see how you've done that - fence_get_page_size() returning 0.
> 
> I have no further comments. Is it appropriate for me to say I'll give a
> Reviewed-by since I wrote the original version of the patch?

Hi Ben,

I think we can do it by adding this bunch of tags to the commit message:

Signed-off-by: Ben Avison 
Reviewed-by: Pekka Paalanen 
[Pekka: changes in v4 and v5]
Signed-off-by: Pekka Paalanen 
Reviewed-by: Ben Avison 

You will naturally be the patch author. This should convey exactly what
we did.

Oded wants to have a final look too, so I'll see about pushing this on
Monday.


Thanks,
pq


pgpJs3xAV0QWD.pgp
Description: OpenPGP digital signature
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

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

2015-09-04 Thread Pekka Paalanen 
On Thu, 03 Sep 2015 13:59:07 +0100
"Ben Avison"  wrote:

> On Thu, 03 Sep 2015 11:13:25 +0100, Pekka Paalanen  
> wrote:
> > D'oh, of course, that's why Oded didn't see a segfault. But he did say
> > the CRC does not match on PPC64, neither LE nor BE.
> 
> Just to be clear, is that still an issue after I fixed the DST_HEIGHT
> SRC_HEIGHT bug?

He says he tested v3, so that should be fixed. I suppose the culprit
lies with page size.


Thanks,
pq


pgpKK60wbuOMB.pgp
Description: OpenPGP digital signature
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

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

2015-09-03 Thread Siarhei Siamashka 
On Thu, 03 Sep 2015 13:59:07 +0100
"Ben Avison"  wrote:

> On Thu, 03 Sep 2015 11:13:25 +0100, Pekka Paalanen  
> wrote:
> > Unless, you go and change the implementation meaning of Pixman's cover
> > flags which, reading your other reply, is what you are doing. I'm
> > finally starting to see it.
> 
> Glad the penny has dropped! The issue has been muddied somewhat by the
> 8*epsilon border. Perhaps I should have reposted the series as soon as it
> became clear what the history was in that respect.

Yes, sure. I'm also waiting for an updated patch set. Because dealing
with the 8*epsilon border separately is a rather major change,
affecting the way how the set is split into patches and the wording
of commit messages. We need a new patch set before starting a new
review round. Sorry for not communicating this explicitly enough.

> (from your other post on this thread)
> > 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.)
> 
> Yes. Pixman as a whole should be able to handle any coordinates without
> bounds violations. The COVER_CLIP flags just determine the threshold
> coordinates at which it chooses to use a cover path rather than the
> appropriate repeat path (PAD, NONE, NORMAL or REFLECT); while you would
> get the correct mathematical result even if always use the repeat path,
> the cover path is very likely to be faster because it will be implemented
> in a way that assumes it doesn't need to do bounds checking (because the
> COVER_CLIP calculation has effectively done the bounds check for it).

For BILINEAR scaling, the NONE and PAD repeats generally do not need
bounds checking either. So they should be as fast as COVER, except for
having a bit higher setup overhead.

And in the special edge case where bilinear weight coefficients are
zero for the pixels fetched from the outside of the source image
(your new interpretation of the COVER flag), we can also safely reduce
NORMAL and REFLECT repeats to NONE or PAD.

> Previously, the flag definition was overly cautious, resulting in some
> cases which could have been handled by cover paths instead being sent to
> repeat paths. I first encountered this myself with lowlevel-blt-bench,
> which routinely used repeat paths for its bilinear tests

Oh, this was actually intended. The bilinear scaling benchmark in
the lowlevel-blt-bench program tries to stress the use case, where
we are fetching pixels from the outside of the source image a little
bit. The reason is that this is quite typical for bilinear scaling
and that's how it is designed to work.

If this whole ordeal with the new COVER flag interpretation was just
a way to make it run fast in lowlevel-blt-bench, then this new
interpretation may not make much real practical sense. And why even
stop here? If we can prove that the scaling factor, used by
lowlevel-blt-bench, in fact behaves exactly as identity transform
for certain composite operations, then it probably can be optimized
too!

Well, maybe we need some improvements for the scaling benchmarks in
lowlevel-blt-bench. We can just measure performance numbers for
NONE, PAD, NORMAL, REFLECT and COVER cases and report them all. The
scaling factor needs to be adjusted, so that we hit these particular
code paths. When we get the performance numbers for different repeat
cases, then we can easily compare them. Like I said, NONE and PAD
performance should be normally roughly the same as COVER. Not all
the fast paths and iterators can handle NONE and PAD repeats
efficiently yet (SSSE3 is a good example), but we can and should
eventually get there.

> but cover paths for its nearest tests. However, we have to be careful
> when adjusting the flag calculations, as the cases where bounds violations
> occur may only be 1*epsilon wide, and so would have a high chance of not
> being detected by the previous test suite in a reasonable length of time
> - hence the need for a new test program.

For scaling, previously we had 'scaling-test', 'affine-test' and
'scaling-crash-test' programs, which are relying on the public
pixman API. And 'scaling-helpers-test' program as a unit test for
the internal 'bilinear_pad_repeat_get_scanline_bounds()' function,
which is also very important.

In the case of NEAREST scaling, potential reads outside of the
source image can be usually easily spotted, because this pixel
value contributes to the destination picture. This is not completely
reliable, but still reasonably efficient in practice.

But in the case of BILINEAR scaling, the new 'cover-test' is indeed
very useful and represents a nice addition to the test suite. Thanks
for doing this work.

-- 
Best regards,
Siarhei Siamashka
___
Pixman mailing list
Pixman@lists.freedesktop.org

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

2015-09-03 Thread Pekka Paalanen 
On Wed, 02 Sep 2015 20:15:26 +0100
"Ben Avison"  wrote:

> On Wed, 02 Sep 2015 12:56:40 +0100, Pekka Paalanen  
> wrote:
> > Right. I looked at fast_bilinear_cover_iter_init() and
> > fast_fetch_bilinear_cover() -> fetch_horizontal(), and yes, that really
> > is how it is implemented. The leftmost pixel is chosen essentially by
> > n = pixman_fixed_to_int(x - pixman_fixed_1 / 2).
> >
> > So, pixman_fixed_1 / 2 -> 0, not -1.
> >
> > The thing that confuses me is that with nearest, x=0 will sample pixel
> > n=-1. However with bilinear, x=0.5 will sample pixels n=0 and n=1, not
> > n=-1 and n=0.
> 
> When x=0.5, the numerical output of bilinear scaling only depends upon
> source pixel n=0. The stuff about whether you consider pixel n=+1 or n=-1
> to be involved but with a weighting factor of 0 is incidental to the
> numerical output. With nearest scaling, you don't have the option of
> sitting on the fence so you have to round one way or the other.

Yes, I have no problem with the weight zero.

My point is with the memory accesses. Bilinear fetchers always fetch
two pixels regardless of the coordinate, right? At least they must be
allowed to do that for efficient inner loop implementations, or so I've
understood the current design in Pixman.

Therefore when we are tightening the cover boundaries and relaxing the
conditions when cover is applicable, we must take that memory access
into account, even if the weight is zero. We must know whether all
paths sample on the left or the right the zero-weight pixel, when
sampling the exact center of a pixel.

Indeed, nearest does
n = pixman_fixed_to_int(x - pixman_pixed_e)
and bilinear does
n0 = pixman_fixed_to_int(x - pixman_fixed_1 / 2)
n1 = n0 + 1

Just like you explained, the epsilon offset applies only to nearest,
and that is what I find confusing. That's all. It is like nearest
"rounds" down, while bilinear "rounds" up by sampling n0 + 1 instead of
n0 - 1 when x is exactly half.

Anyway, that should be all settled now.

We just have to remember the "always sample two pixels" when we tighten
the cover boundaries for bilinear, since we must avoid accessing memory
outside of the row.

Unless, you go and change the implementation meaning of Pixman's cover
flags which, reading your other reply, is what you are doing. I'm
finally starting to see it.

> Of course, I've now changed cover-test so it doesn't do randmemset
> straight into fenced images anyway (due to needing to handle systems with
> different memory page sizes) so this is no longer an issue.

D'oh, of course, that's why Oded didn't see a segfault. But he did say
the CRC does not match on PPC64, neither LE nor BE.


Thanks,
pq


pgpyy7N6ZjHlb.pgp
Description: OpenPGP digital signature
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

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

2015-09-03 Thread Pekka Paalanen 
On Wed, 02 Sep 2015 20:34:56 +0100
"Ben Avison"  wrote:

> On Wed, 02 Sep 2015 14:03:01 +0100, Pekka Paalanen  
> 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


pgpc9Tx2IeJhu.pgp
Description: OpenPGP digital signature
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

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

2015-09-03 Thread Ben Avison 

On Thu, 03 Sep 2015 11:13:25 +0100, Pekka Paalanen  wrote:

Unless, you go and change the implementation meaning of Pixman's cover
flags which, reading your other reply, is what you are doing. I'm
finally starting to see it.


Glad the penny has dropped! The issue has been muddied somewhat by the
8*epsilon border. Perhaps I should have reposted the series as soon as it
became clear what the history was in that respect.

(from your other post on this thread)

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


Yes. Pixman as a whole should be able to handle any coordinates without
bounds violations. The COVER_CLIP flags just determine the threshold
coordinates at which it chooses to use a cover path rather than the
appropriate repeat path (PAD, NONE, NORMAL or REFLECT); while you would
get the correct mathematical result even if always use the repeat path,
the cover path is very likely to be faster because it will be implemented
in a way that assumes it doesn't need to do bounds checking (because the
COVER_CLIP calculation has effectively done the bounds check for it).

Previously, the flag definition was overly cautious, resulting in some
cases which could have been handled by cover paths instead being sent to
repeat paths. I first encountered this myself with lowlevel-blt-bench,
which routinely used repeat paths for its bilinear tests but cover paths
for its nearest tests. However, we have to be careful when adjusting the
flag calculations, as the cases where bounds violations occur may only be
1*epsilon wide, and so would have a high chance of not being detected by
the previous test suite in a reasonable length of time - hence the need
for a new test program.

When EXACT is set, cover-test only performs operations which are
theoretically achievable with a cover path (and in fact, they *are* all
achieved with a cover path if you apply all my patches). When it isn't
set, some operations will stray into territory that definitely needs a
repeat path. The size of the the fuzz factors that are applied to the
coordinates was set large enough, using knowledge of the 8*epsilon factor
in the old definition of the COVER_CLIP flags, that we should exercise
both cover and repeat paths right up to the coordinate limits at which
either old or new definition will allow them to be used.


Of course, I've now changed cover-test so it doesn't do randmemset
straight into fenced images anyway (due to needing to handle systems
with different memory page sizes) so this is no longer an issue.


D'oh, of course, that's why Oded didn't see a segfault. But he did say
the CRC does not match on PPC64, neither LE nor BE.


Just to be clear, is that still an issue after I fixed the DST_HEIGHT
SRC_HEIGHT bug?

Ben
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

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

2015-09-03 Thread Ben Avison 

On Thu, 03 Sep 2015 17:14:05 +0100, Siarhei Siamashka 
 wrote:

On Thu, 03 Sep 2015 13:59:07 +0100 "Ben Avison"  wrote:

Previously, the flag definition was overly cautious, resulting in some
cases which could have been handled by cover paths instead being sent
to repeat paths. I first encountered this myself with
lowlevel-blt-bench, which routinely used repeat paths for its bilinear
tests


Oh, this was actually intended. The bilinear scaling benchmark in
the lowlevel-blt-bench program tries to stress the use case, where
we are fetching pixels from the outside of the source image a little
bit. The reason is that this is quite typical for bilinear scaling
and that's how it is designed to work.

If this whole ordeal with the new COVER flag interpretation was just
a way to make it run fast in lowlevel-blt-bench, then this new
interpretation may not make much real practical sense.


That was just where it started... I should maybe explain that I
encountered it when I was writing bilinear fetchers for ARMv6, which (at
the time (*)) only handled the COVER case. This meant that
lowlevel-blt-bench was useless to me as it stood, because my COVER
fetchers were never selected, and it was important to me because I rely
on it to be able to compare different implementations (for example to
select the best prefetch distance). I was worried there might be
objections to changing lowlevel-blt-bench, and I also knew that I
followed different code paths for different scale factors (which are
fixed in lowlevel-blt-bench), many of which were so extreme that the
L1/L2/M/HT/VT/RT/R tests of lowlevel-blt-bench no longer made sense - so
that's how I came to write affine-bench.

But while writing affine-bench, it became apparent that it's really
complicated from a high level caller's point of view to construct a
transformation matrix that's guaranteed to hit a COVER path - and it
requires knowledge of details of the internal implementation of the guts
of Pixman, even after you deal with the obsolete 8*pixman_fixed_e
adjustment. This seemed wrong to me; I can imagine applications might
well want to use Pixman to scale between pixel centres rather than pixel
edges, and to expect such an application to know whether to exclude the
left pixel, or the right pixel, or both, or neither, and the same in the
vertical direction (for which the rules were often different) in order to
get the best performance is surely unreasonable.

Ben

(*) Actually, I've been revisiting this recently, and for various reasons
I've added repeat support to these fetchers. But now I'm getting a long
way ahead of myself...
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

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

2015-09-02 Thread Ben Avison 

On Wed, 02 Sep 2015 12:56:40 +0100, Pekka Paalanen  wrote:

Right. I looked at fast_bilinear_cover_iter_init() and
fast_fetch_bilinear_cover() -> fetch_horizontal(), and yes, that really
is how it is implemented. The leftmost pixel is chosen essentially by
n = pixman_fixed_to_int(x - pixman_fixed_1 / 2).

So, pixman_fixed_1 / 2 -> 0, not -1.

The thing that confuses me is that with nearest, x=0 will sample pixel
n=-1. However with bilinear, x=0.5 will sample pixels n=0 and n=1, not
n=-1 and n=0.


When x=0.5, the numerical output of bilinear scaling only depends upon
source pixel n=0. The stuff about whether you consider pixel n=+1 or n=-1
to be involved but with a weighting factor of 0 is incidental to the
numerical output. With nearest scaling, you don't have the option of
sitting on the fence so you have to round one way or the other.

If you look at the following ASCII art diagrams, the vertical axis is the
contribution from source pixel 0 (+) and source pixel 1 (x). Where these
coincide, I have used a *. Each column represents an increment of 0.125
(you'll have to read the labels on the horizontal axis vertically). Upper
diagram is nearest, lower diagram is bilinear.

   



  *

  -   -   +   +   +   +   +   +
  1   0   0   0   1   1   2   2   3
  .   .   .   .   .   .   .   .   .
  0   5   0   5   0   5   0   5   0

  +   x
+   +   x   x
  +   *   x
+   x   +   x
  *   *

See how the bilinear diagram is already symmetrical; no rounding is
needed of coordinates on the horizontal axis.

Of course, coordinates actually have 13 more bits of granularity than
this, but the same patterns hold.


> - Looks like it processes the whole stride, not width * bytespp,
>   which means this is going to explode on fenced images in the
>   padding, right?

Yes, looks like it is. Good spot. That probably does belong in a
separate patch; are you volunteering?


I should, shouldn't I? :-)
I'll do it.


Of course, I've now changed cover-test so it doesn't do randmemset
straight into fenced images anyway (due to needing to handle systems with
different memory page sizes) so this is no longer an issue.

Ben
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

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

2015-09-02 Thread Ben Avison 

On Wed, 02 Sep 2015 14:03:01 +0100, Pekka Paalanen  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
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

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

2015-09-02 Thread Pekka Paalanen 
Hi Ben,

here I'm only replying to the questions that are not yet completely
clear to me.

On Wed, 26 Aug 2015 21:26:57 +0100
"Ben Avison"  wrote:

> On Thu, 04 Jun 2015 14:00:30 +0100, Pekka Paalanen  
> wrote:
> 
> > On Tue, 26 May 2015 23:58:30 +0100
> > Ben Avison  wrote:
> >

> >> +static pixman_fixed_t
> >> +calc_translate (intdst_size,
> >> +intsrc_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?
> 
> At this point we're determining the translations required so that the
> first destination pixel, when transformed into source space, has exactly
> the lowest value it can have without straying into the repeat zones.
> 
> With bilinear scaling, a coordinate of pixman_fixed_1 / 2 has pixel value
> determined only by source pixel 0. A coordinate of pixman_fixed_1 * 3 / 2
> has pixel value determined only by source pixel 1. Any value in between
> has to be assumed to contain elements from both pixels 0 and 1.

> With nearest scaling, any coordinate between pixman_fixed_e and
> pixman_fixed_1 inclusive is determined by source pixel 0. Yes, this is
> slightly counter-intuitive, but it's the way Pixman has always done it.
> See for example in bits_image_fetch_nearest_affine() in
> pixman-fast-path.c where we have the lines
> 
>   x0 = pixman_fixed_to_int (x - pixman_fixed_e);
>   y0 = pixman_fixed_to_int (y - pixman_fixed_e);

Ok, I can take that as a given.

> There is no equivalent offset by pixman_fixed_e for bilinear scaling, for
> example in fast_bilinear_cover_iter_init() also in pixman-fast-path.c:
> 
>   info->x = v.vector[0] - pixman_fixed_1 / 2;
>   info->y = v.vector[1] - pixman_fixed_1 / 2;
> 
> All the other implementations follow suit (they have to, or they wouldn't
> pass the "make check" suite.)

Right. I looked at fast_bilinear_cover_iter_init() and
fast_fetch_bilinear_cover() -> fetch_horizontal(), and yes, that really
is how it is implemented. The leftmost pixel is chosen essentially by
n = pixman_fixed_to_int(x - pixman_fixed_1 / 2).

So, pixman_fixed_1 / 2 -> 0, not -1.

The thing that confuses me is that with nearest, x=0 will sample pixel
n=-1. However with bilinear, x=0.5 will sample pixels n=0 and n=1, not
n=-1 and n=0.

That alone is surprising, but I am further confused by the talk about
the sampling case n=-1 & n=0:
http://lists.freedesktop.org/archives/pixman/2015-August/003911.html
where you refer to the very end of:
http://lists.freedesktop.org/archives/pixman/2014-October/003457.html

It's exactly the question of whether to sample the next pixel to the
left or to the right of the sampling point.

Eh, maybe I'm trying too hard. I do believe your code is correct, but
in that case I have a follow-up question on check_transform() in my
following patch review email.

> > - Looks like it processes the whole stride, not width * bytespp, which
> >   means this is going to explode on fenced images in the padding,
> >   right?
> 
> Yes, looks like it is. Good spot. That probably does belong in a separate
> patch; are you volunteering?

I should, shouldn't I? :-)
I'll do it.

> >> * 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?
> 
> No - that's covered by supporting both left- and right-aligning (and top-
> and bottom-) in calc_translate(). Translation is done entirely using the
> transformation matrix because the src_x, src_y, mask_x and mask_y
> parameters to pixman_image_composite() only allow for integer positions
> in source/mask image coordinates and we need 16.16 fixed-point precision.

Ok, I think I misunderstood. I was thinking of the range of 16.16 fp
numbers in general, but that's going a bit off-topic here, too.


Thanks,
pq


pgp4FEcncW4o7.pgp
Description: OpenPGP digital signature
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

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

2015-09-02 Thread Pekka Paalanen 
On Thu, 27 Aug 2015 16:17:55 +0100
Ben Avison  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.
> ---
>  test/Makefile.sources |1 +
>  test/cover-test.c |  424 
> +
>  2 files changed, 425 insertions(+), 0 deletions(-)
>  create mode 100644 test/cover-test.c

Hi Ben

this is an incremental review compared to the previous version. I also
refer to things I wrote as a reply to your previous email (the reply to
my review comments).

> 
> diff --git a/test/Makefile.sources b/test/Makefile.sources
> index 5b9ae84..aece143 100644
> --- a/test/Makefile.sources
> +++ b/test/Makefile.sources
> @@ -27,6 +27,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 000..dbdf0f8
> --- /dev/null
> +++ b/test/cover-test.c
> @@ -0,0 +1,424 @@
> +/*
> + * 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 (bavi...@riscosopen.org)
> + *
> + */
> +
> +/*
> + * 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.
> + */

Very good.

> +
> +#include "utils.h"
> +#include 
> +#include 
> +
> +/* Approximate limits for random scale factor generation - these ensure we 
> can
> + * get at least 8x reduction and 8x enlargement.
> + */
> +#define LOG2_MAX_FACTOR (3)
> +
> +/* 1/sqrt(2) (or sqrt(0.5), or 2^-0.5) as a 0.32 fixed-point number */
> +#define INV_SQRT_2_0POINT32_FIXED (0xB504F334u)
> +
> +/* The largest increment that can be generated by random_scale_factor().
> + * This occurs when the "mantissa" part is 0x and the "exponent"
> + * part is -LOG2_MAX_FACTOR.
> + */
> +#define MAX_INC ((pixman_fixed_t)(INV_SQRT_2_0POINT32_FIXED >> (31 - 16 - 
> LOG2_MAX_FACTOR)))

Much better.

I am still a tiny bit struggling with why 31 and not 32, but it does
add up.

2^-0.5 is presented in 0.32, then an imaginary excercise:
- shift right by 32 bits to convert to 32.0 (ones)
- shift left by 1 bit to produce 2^0.5
- shift left by 16 bits to convert to 16.16
- shift left by LOG2_MAX_FACTOR, which produces 2^3.5

So we indeed get 2^3.5 presented in 16.16 fixed point.

All good here.

> +
> +/* 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)
> +
> +/* Derive the destination width so that at max increment we fit

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

2015-09-02 Thread Søren Sandmann 
On Wed, Sep 2, 2015 at 7:56 AM, Pekka Paalanen  wrote:


> Right. I looked at fast_bilinear_cover_iter_init() and
> fast_fetch_bilinear_cover() -> fetch_horizontal(), and yes, that really
> is how it is implemented. The leftmost pixel is chosen essentially by
> n = pixman_fixed_to_int(x - pixman_fixed_1 / 2).
>
> So, pixman_fixed_1 / 2 -> 0, not -1.
>
> The thing that confuses me is that with nearest, x=0 will sample pixel
> n=-1. However with bilinear, x=0.5 will sample pixels n=0 and n=1, not
> n=-1 and n=0.
>
>
Pixman's rounding rules can be confusing, but they do have an internal
logic:
The important thing to understand is that pixel 0 has coordinate 0.5, pixel
-1
has coordinate -0.5, pixel 1 has coordinate 1.5. In general, pixel n has
coordinate
n + 0.5.

Given that, and given that we always break ties by rounding down, we find
that,
for NEAREST: a coordinate of 0.5 is exactly at pixel 0, so we should use
that.
A coordinate of 0 is precisely midways between pixels -1 and 0, so since we
round
down, pixel -1 is the one to use.

For BILINEAR, x=0.5 is precisely at pixel 0, so that must be used with a
weight of 1.
The other pixel could be either -1 or 1 since it will have a weight of 0 in
either case.
But since we round down, -1 it is.

There are some more details about rounding in the file pixman/rounding.txt.



Søren
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

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

2015-08-27 Thread Ben Avison 
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.
---
 test/Makefile.sources |1 +
 test/cover-test.c |  424 +
 2 files changed, 425 insertions(+), 0 deletions(-)
 create mode 100644 test/cover-test.c

diff --git a/test/Makefile.sources b/test/Makefile.sources
index 5b9ae84..aece143 100644
--- a/test/Makefile.sources
+++ b/test/Makefile.sources
@@ -27,6 +27,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 000..dbdf0f8
--- /dev/null
+++ b/test/cover-test.c
@@ -0,0 +1,424 @@
+/*
+ * 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 (bavi...@riscosopen.org)
+ *
+ */
+
+/*
+ * 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.
+ */
+
+#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)
+
+/* 1/sqrt(2) (or sqrt(0.5), or 2^-0.5) as a 0.32 fixed-point number */
+#define INV_SQRT_2_0POINT32_FIXED (0xB504F334u)
+
+/* The largest increment that can be generated by random_scale_factor().
+ * This occurs when the mantissa part is 0x and the exponent
+ * part is -LOG2_MAX_FACTOR.
+ */
+#define MAX_INC ((pixman_fixed_t)(INV_SQRT_2_0POINT32_FIXED  (31 - 16 - 
LOG2_MAX_FACTOR)))
+
+/* 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)
+
+/* Derive the destination width so that at max increment we fit within source 
*/
+#define DST_WIDTH (MIN_SRC_WIDTH * pixman_fixed_1 / MAX_INC)
+
+/* 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)
+
+/* 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

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

2015-08-26 Thread Ben Avison 

On Thu, 04 Jun 2015 14:00:30 +0100, Pekka Paalanen ppaala...@gmail.com wrote:


On Tue, 26 May 2015 23:58:30 +0100
Ben Avison bavi...@riscosopen.org wrote:


+#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_...?


Fair enough, renamed (will repost when I've resolved everything else
below).


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


It's basically just the largest random number that can be generated by
random_scale_factor(), in other words if you start out with the largest
number and shift it right by the smallest number of places. I hope that
makes it clearer where it comes from - I'll update the comment.


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


Yes. I've a feeling some compilers might force a load and runtime
calculation of expressions if you use static const, so I'll stick a cast
in the definition (being careful because pixman_fixed_t is signed but we
need to use an unsigned shift to generate the number).


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.


I've duplicated the commit message in a comment.


+static pixman_fixed_t
+random_scale_factor(void)

[...]

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.


Nice diagram. For these purposes, I'd say it was close enough to uniform;
no particular value is more than 2x more or less likely than any other.
Without the shift step in the random number generator, what you'd have
instead would be a ramp with one end being soemthing like 128x more
likely than the other. You'd also get effects such as the probability of
a plot which was an enlargement in both X and Y being only 1 in 64, even
though that's quite a likely outcome in real life; this way it's 1 in 4.


+static pixman_fixed_t
+calc_translate (intdst_size,
+intsrc_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?


At this point we're determining the translations required so that the
first destination pixel, when transformed into source space, has exactly
the lowest value it can have without straying into the repeat zones.

With bilinear scaling, a coordinate of pixman_fixed_1 / 2 has pixel value
determined only by source pixel 0. A coordinate of pixman_fixed_1 * 3 / 2
has pixel value determined only by source pixel 1. Any value in between
has to be assumed to contain elements from both pixels 0 and 1.

With nearest scaling, any coordinate between pixman_fixed_e and
pixman_fixed_1 inclusive is determined by source pixel 0. Yes, this is
slightly counter-intuitive, but it's the way Pixman has always done it.
See for example in bits_image_fetch_nearest_affine() in
pixman-fast-path.c where we have the lines

x0 = pixman_fixed_to_int (x - pixman_fixed_e);
y0 = pixman_fixed_to_int (y - pixman_fixed_e);

There is no equivalent offset by pixman_fixed_e for bilinear scaling, for
example in fast_bilinear_cover_iter_init() also in pixman-fast-path.c:

 info-x = v.vector[0] - pixman_fixed_1 / 2;
 info-y = v.vector[1] - pixman_fixed_1 / 2;

All the other implementations follow suit (they have to, or they wouldn't
pass the make check suite.)


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


Yes, the transform coefficients, source and destination coordinates are
all 16.16 fixed point, and round-to-nearest is how Pixman handles this in
all sorts of places, for example in pixman_transform_point_31_16_3d().
It's worth noting that since we increment the destination coordinate by

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

2015-06-04 Thread Pekka Paalanen 
On Tue, 26 May 2015 23:58:30 +0100
Ben Avison bavi...@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 000..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 (bavi...@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

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

2015-05-26 Thread Matt Turner 
On Tue, May 26, 2015 at 3:58 PM, Ben Avison bavi...@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

 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\

Remember to add cover-test to .gitignore.
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

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

2015-05-26 Thread Ben Avison 
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

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 000..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 (bavi...@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)
+
+#define MAX_INC (SQRT_0POINT5_0POINT32_FIXED  (31 - 16 - LOG2_MAX_FACTOR))
+
+/* 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)
+
+/* Derive the destination width so that at max increment we fit within source 
*/
+#define DST_WIDTH (MIN_SRC_WIDTH * pixman_fixed_1 / MAX_INC)
+
+/* 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)
+
+/* 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
+};
+
+/* 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;
+
+static pixman_fixed_t
+random_scale_factor(void)
+{
+/* Get a random number with top bit set. */
+uint32_t f = prng_rand () | 0x8000u;
+
+/* In log(2) space, this is still approximately evenly spread between 31
+ * and 32. Divide by sqrt(2) to centre the