Re: [Pixman] [PATCH 02/14] Add new test of filter reduction from BILINEAR to NEAREST

2016-04-29 Thread Bill Spitzak 
On Fri, Apr 29, 2016 at 4:55 AM, Oded Gabbay  wrote:

> On Tue, Apr 12, 2016 at 5:36 AM, Søren Sandmann Pedersen
>  wrote:
> > This new test tests a bunch of bilinear downscalings, where many have
> > a transformation such that the BILINEAR filter can be reduced to
> > NEAREST (and many don't).
> >
> > A CRC32 is computed for all the resulting images and compared to a
> > known-good value for both 4-bit and 7-bit interpolation.
> >
> > V2: Remove leftover comment, some minor formatting fixes, use a
> > timestamp as the PRNG seed.
> >
> > Signed-off-by: Søren Sandmann 
> > ---
> >  test/Makefile.sources|   1 +
> >  test/filter-reduction-test.c | 112
> +++
> >  2 files changed, 113 insertions(+)
> >  create mode 100644 test/filter-reduction-test.c
> >
> > diff --git a/test/Makefile.sources b/test/Makefile.sources
> > index 5d55e67..0a56231 100644
> > --- a/test/Makefile.sources
> > +++ b/test/Makefile.sources
> > @@ -21,6 +21,7 @@ TESTPROGRAMS =  \
> > gradient-crash-test   \
> > pixel-test\
> > matrix-test   \
> > +   filter-reduction-test \
> > composite-traps-test  \
> > region-contains-test  \
> > glyph-test\
> > diff --git a/test/filter-reduction-test.c b/test/filter-reduction-test.c
> > new file mode 100644
> > index 000..705fa4b
> > --- /dev/null
> > +++ b/test/filter-reduction-test.c
> > @@ -0,0 +1,112 @@
> > +#include 
> > +#include 
> > +#include "utils.h"
> > +
> > +static const pixman_fixed_t entries[] =
> > +{
> > +pixman_double_to_fixed (-1.0),
> > +pixman_double_to_fixed (-0.5),
> > +pixman_double_to_fixed (-1/3.0),
> > +pixman_double_to_fixed (0.0),
> > +pixman_double_to_fixed (0.5),
> > +pixman_double_to_fixed (1.0),
> > +pixman_double_to_fixed (1.5),
> > +pixman_double_to_fixed (2.0),
> > +pixman_double_to_fixed (3.0),
> > +};
> > +
> > +#define SIZE 12
> > +
> > +static uint32_t
> > +test_scale (const pixman_transform_t *xform, uint32_t crc)
> > +{
> > +uint32_t *srcbuf, *dstbuf;
> > +pixman_image_t *src, *dest;
> > +
> > +srcbuf = malloc (SIZE * SIZE * 4);
> I admit I didn't look at other tests, but it really caught my eye we
> don't check for memory allocation failure here.
>
> > +prng_randmemset (srcbuf, SIZE * SIZE * 4, 0);
> > +src = pixman_image_create_bits (
> > +   PIXMAN_a8r8g8b8, SIZE, SIZE, srcbuf, SIZE * 4);
> > +
> > +dstbuf = malloc (SIZE * SIZE * 4);
> same comment
>

I think it is ok to not look for that in unit tests. They will either crash
or fail.


>
> > +prng_randmemset (dstbuf, SIZE * SIZE * 4, 0);
> > +dest = pixman_image_create_bits (
> > +   PIXMAN_a8r8g8b8, SIZE, SIZE, dstbuf, SIZE * 4);
> > +
> > +pixman_image_set_transform (src, xform);
> > +pixman_image_set_repeat (src, PIXMAN_REPEAT_NORMAL);
> > +pixman_image_set_filter (src, PIXMAN_FILTER_BILINEAR, NULL, 0);
> > +
> > +image_endian_swap (src);
> > +image_endian_swap (dest);
> > +
> > +pixman_image_composite (PIXMAN_OP_SRC,
> > +   src, NULL, dest,
> > +   0, 0, 0, 0, 0, 0,
> > +   SIZE, SIZE);
> > +
> > +crc = compute_crc32_for_image (crc, dest);
> > +
> > +pixman_image_unref (src);
> > +pixman_image_unref (dest);
> > +
> > +free (srcbuf);
> > +free (dstbuf);
> > +
> > +return crc;
> > +}
> > +
> > +#if BILINEAR_INTERPOLATION_BITS == 7
> > +#define CHECKSUM 0x02169677
> > +#elif BILINEAR_INTERPOLATION_BITS == 4
> > +#define CHECKSUM 0xE44B29AC
> > +#else
> > +#define CHECKSUM 0x
> > +#endif
> > +
> > +int
> > +main (int argc, const char *argv[])
> > +{
> > +const pixman_fixed_t *end = entries + ARRAY_LENGTH (entries);
> > +const pixman_fixed_t *t0, *t1, *t2, *t3, *t4, *t5;
> > +uint32_t crc = 0;
> > +
> > +prng_srand (0x56EA1DBD);
> > +
> > +for (t0 = entries; t0 < end; ++t0)
> > +{
> > +   for (t1 = entries; t1 < end; ++t1)
> > +   {
> > +   for (t2 = entries; t2 < end; ++t2)
> > +   {
> > +   for (t3 = entries; t3 < end; ++t3)
> > +   {
> > +   for (t4 = entries; t4 < end; ++t4)
> > +   {
> > +   for (t5 = entries; t5 < end; ++t5)
> > +   {
> > +   pixman_transform_t xform = {
> > +   { { *t0, *t1, *t2 },
> > + { *t3, *t4, *t5 },
> > + { 0, 0, pixman_fixed_1 } }
> > +   };
> > +
> > +   crc = test_scale (, crc);
> > +   }
> > +   }
> > +   }
> > +   }
> > +

Re: [Pixman] [RFC 1/2] Remove seemingly unneeded comparison(s)

2016-04-29 Thread Bill Spitzak 
If the comparison fails, the returned values are modulus fixed_16_16,
rather than clamped. I think that gives a lot less possibilities for the
calling code to recover from or simulate the results of the out-of-range
value.


On Fri, Apr 29, 2016 at 6:33 AM, Emil Velikov 
wrote:

> On 29 April 2016 at 11:35, Pekka Paalanen  wrote:
> > On Fri, 29 Apr 2016 10:15:37 +0100
> > Emil Velikov  wrote:
> >
> >> On 27 April 2016 at 18:46, Bill Spitzak  wrote:
> >> > On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen 
> wrote:
> >> >>
> >> >> On Wed, 27 Apr 2016 09:56:44 +0100
> >> >> Emil Velikov  wrote:
> >> >>
> >> >> > On 26 April 2016 at 19:12, Bill Spitzak  wrote:
> >> >> > > The old code is comparing pixman_fixed_48_16_t values to
> >> >> > > pixman_fixed_16_16_t values, thus it is checking for truncation
> of
> >> >> > > overflow
> >> >> > > values.
> >> >> > >
> >> >> > Indeed it does. I'll grep more before asking silly questions ;-)
> >> >> >
> >> >> > > It would probably be better to clamp these overflowed values,
> like
> >> >> > > pixman_transform_point_31_16 is doing to clamp to the
> >> >> > > pixman_fixed_48_16
> >> >> > > result. Right now the result is an odd mix of clamping and
> modulus. A
> >> >> > > rewrite to go directly to clamped pixman_fixed_16_16 values
> would be
> >> >> > > even
> >> >> > > better.
> >> >> > >
> >> >> > Sounds like a plan. Sadly I doubt I'll get to it any time soon.
> >> >>
> >> >> Wasn't the point of the boolean return from these functions to tell
> the
> >> >> caller to drop what it is doing because it cannot be done properly?
> >> >
> >> >
> >> > Dropping a fill is a lot worse than trying to simulate it using the
> clamped
> >> > path. It will produce a wrong result if one of the edges connected to
> a
> >> > clamped point passes through the clip, but this often does not
> happen, and
> >> > the transition to the wrong result is gradual as the point moves
> outside the
> >> > clamped region.
> >> >
> >> > More importantly the caller cannot do anything with the return values
> right
> >> > now, as they are modulus MAX_16_16+1. Even the direction they are in
> is
> >> > lost.
> >> >
> >> I think that keeping the user provided memory as-is when the function
> >> does not succeed is a good idea.
> >> Afaics currently the contents get overwritten regardless of the result.
> >>
> >> This is what you guys were on about, right ? Or perhaps you're
> >> thinking about spinning v2 of the function with different
> >> signature/behaviour ?
> >
> > Hi Emil,
> >
> > I think the conclusion was that the comparisons are not useless, and
> > this patch should be dropped. You noted it yourself that this patch
> > causes a regression in the test suite.
> >
> Fully agree on both points. Just trying to understand what you and
> Bill are talking about and suggest that if one changes things, would
> be nice to avoid "feeding garbage" back to the user [on error].
>
> Thanks
> Emil
>
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [RFC 1/2] Remove seemingly unneeded comparison(s)

2016-04-29 Thread Emil Velikov 
On 29 April 2016 at 11:35, Pekka Paalanen  wrote:
> On Fri, 29 Apr 2016 10:15:37 +0100
> Emil Velikov  wrote:
>
>> On 27 April 2016 at 18:46, Bill Spitzak  wrote:
>> > On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen  
>> > wrote:
>> >>
>> >> On Wed, 27 Apr 2016 09:56:44 +0100
>> >> Emil Velikov  wrote:
>> >>
>> >> > On 26 April 2016 at 19:12, Bill Spitzak  wrote:
>> >> > > The old code is comparing pixman_fixed_48_16_t values to
>> >> > > pixman_fixed_16_16_t values, thus it is checking for truncation of
>> >> > > overflow
>> >> > > values.
>> >> > >
>> >> > Indeed it does. I'll grep more before asking silly questions ;-)
>> >> >
>> >> > > It would probably be better to clamp these overflowed values, like
>> >> > > pixman_transform_point_31_16 is doing to clamp to the
>> >> > > pixman_fixed_48_16
>> >> > > result. Right now the result is an odd mix of clamping and modulus. A
>> >> > > rewrite to go directly to clamped pixman_fixed_16_16 values would be
>> >> > > even
>> >> > > better.
>> >> > >
>> >> > Sounds like a plan. Sadly I doubt I'll get to it any time soon.
>> >>
>> >> Wasn't the point of the boolean return from these functions to tell the
>> >> caller to drop what it is doing because it cannot be done properly?
>> >
>> >
>> > Dropping a fill is a lot worse than trying to simulate it using the clamped
>> > path. It will produce a wrong result if one of the edges connected to a
>> > clamped point passes through the clip, but this often does not happen, and
>> > the transition to the wrong result is gradual as the point moves outside 
>> > the
>> > clamped region.
>> >
>> > More importantly the caller cannot do anything with the return values right
>> > now, as they are modulus MAX_16_16+1. Even the direction they are in is
>> > lost.
>> >
>> I think that keeping the user provided memory as-is when the function
>> does not succeed is a good idea.
>> Afaics currently the contents get overwritten regardless of the result.
>>
>> This is what you guys were on about, right ? Or perhaps you're
>> thinking about spinning v2 of the function with different
>> signature/behaviour ?
>
> Hi Emil,
>
> I think the conclusion was that the comparisons are not useless, and
> this patch should be dropped. You noted it yourself that this patch
> causes a regression in the test suite.
>
Fully agree on both points. Just trying to understand what you and
Bill are talking about and suggest that if one changes things, would
be nice to avoid "feeding garbage" back to the user [on error].

Thanks
Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 03/14] More general BILINEAR=>NEAREST reduction

2016-04-29 Thread Oded Gabbay 
On Tue, Apr 12, 2016 at 7:03 PM, Bill Spitzak  wrote:
> Only minor comments on the large nearest patch at the bottom
>
>
> On 04/11/2016 07:36 PM, Søren Sandmann Pedersen wrote:
>>
>> Generalize and simplify the code that reduces BILINEAR to NEAREST so
>> that the reduction happens for all affine transformations where
>> t00...t12 are integers and (t00 + t01) and (t10 + t11) are both
>> odd. This is a sufficient condition for the resulting transformed
>> coordinates to be exactly at the center of a pixel so that BILINEAR
>> becomes identical to NEAREST.
>>
>> V2: Address some comments by Bill Spitzak
>>
>> Signed-off-by: Søren Sandmann 
>> ---
>>   pixman/pixman-image.c | 66
>> +--
>>   1 file changed, 38 insertions(+), 28 deletions(-)
>>
>> diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
>> index 1ff1a49..681864e 100644
>> --- a/pixman/pixman-image.c
>> +++ b/pixman/pixman-image.c
>> @@ -335,37 +335,47 @@ compute_image_info (pixman_image_t *image)
>> {
>> flags |= FAST_PATH_NEAREST_FILTER;
>> }
>> -   else if (
>> -   /* affine and integer translation components in matrix ... */
>> -   ((flags & FAST_PATH_AFFINE_TRANSFORM) &&
>> -!pixman_fixed_frac (image->common.transform->matrix[0][2] |
>> -image->common.transform->matrix[1][2]))
>> &&
>> -   (
>> -   /* ... combined with a simple rotation */
>> -   (flags & (FAST_PATH_ROTATE_90_TRANSFORM |
>> - FAST_PATH_ROTATE_180_TRANSFORM |
>> - FAST_PATH_ROTATE_270_TRANSFORM)) ||
>> -   /* ... or combined with a simple non-rotated translation
>> */
>> -   (image->common.transform->matrix[0][0] == pixman_fixed_1
>> &&
>> -image->common.transform->matrix[1][1] == pixman_fixed_1
>> &&
>> -image->common.transform->matrix[0][1] == 0 &&
>> -image->common.transform->matrix[1][0] == 0)
>> -   )
>> -   )
>> +   else if (flags & FAST_PATH_AFFINE_TRANSFORM)
>> {
>> -   /* FIXME: there are some affine-test failures, showing that
>> -* handling of BILINEAR and NEAREST filter is not quite
>> -* equivalent when getting close to 32K for the translation
>> -* components of the matrix. That's likely some bug, but for
>> -* now just skip BILINEAR->NEAREST optimization in this case.
>> +   /* Suppose the transform is
>> +*
>> +*[ t00, t01, t02 ]
>> +*[ t10, t11, t12 ]
>> +*[   0,   0,   1 ]
>> +*
>> +* and the destination coordinates are (n + 0.5, m + 0.5).
>> Then
>> +* the transformed x coordinate is:
>> +*
>> +* tx = t00 * (n + 0.5) + t01 * (m + 0.5) + t02
>> +*= t00 * n + t01 * m + t02 + (t00 + t01) * 0.5
>> +*
>> +* which implies that if t00, t01 and t02 are all integers
>> +* and (t00 + t01) is odd, then tx will be an integer plus
>> 0.5,
>> +* which means a BILINEAR filter will reduce to NEAREST. The
>> same
>> +* applies in the y direction
>>  */
>> -   pixman_fixed_t magic_limit = pixman_int_to_fixed (3);
>> -   if (image->common.transform->matrix[0][2] <= magic_limit  &&
>> -   image->common.transform->matrix[1][2] <= magic_limit  &&
>> -   image->common.transform->matrix[0][2] >= -magic_limit &&
>> -   image->common.transform->matrix[1][2] >= -magic_limit)
>> +   pixman_fixed_t (*t)[3] = image->common.transform->matrix;
>> +
>> +   if ((pixman_fixed_frac (
>> +t[0][0] | t[0][1] | t[0][2] |
>> +t[1][0] | t[1][1] | t[1][2]) == 0) &&
>> +   (pixman_fixed_to_int (
>> +   (t[0][0] + t[0][1]) & (t[1][0] + t[1][1])) % 2) == 1)
>> {
>> -   flags |= FAST_PATH_NEAREST_FILTER;
>> +   /* FIXME: there are some affine-test failures, showing
>> that
>> +* handling of BILINEAR and NEAREST filter is not quite
>> +* equivalent when getting close to 32K for the
>> translation
>> +* components of the matrix. That's likely some bug, but
>> for
>> +* now just skip BILINEAR->NEAREST optimization in this
>> case.
>> +*/
>
>
> May be a good idea to point out that the bug is (probably) in BILINEAR, not
> in NEAREST. Also you think this may have been fixed so this could be
> removed, but that would be a different patch.
>
>> +   pixman_fixed_t magic_limit = pixman_int_to_fixed (3);
>> +   if (image->common.transform->matrix[0][2] <= magic_limit
>> &&
>> +

Re: [Pixman] [PATCH 05/14] demos/scale: fix blank subsamples spin box

2016-04-29 Thread Oded Gabbay 
On Tue, Apr 12, 2016 at 5:36 AM, Søren Sandmann Pedersen
 wrote:
> From: Bill Spitzak 
>
> It now shows the initial value of 4 when the demo is started
>
> Signed-off-by: Bill Spitzak 
> Reviewed-by: Søren Sandmann 
> ---
>  demos/scale.ui | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/demos/scale.ui b/demos/scale.ui
> index ee985dd..f6f6e89 100644
> --- a/demos/scale.ui
> +++ b/demos/scale.ui
> @@ -301,6 +301,7 @@
> id="subsample_spin_button">
>  True
>  name="adjustment">subsample_adjustment
> +   4
>
>
>  1
> --
> 1.7.11.7
>
> ___
> Pixman mailing list
> Pixman@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pixman

This patch is:
Reviewed-by: Oded Gabbay 
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 06/14] demos/scale: Default to locked axis

2016-04-29 Thread Oded Gabbay 
On Tue, Apr 12, 2016 at 5:36 AM, Søren Sandmann Pedersen
 wrote:
> From: Bill Spitzak 
>
> Signed-off-by: Bill Spitzak 
> Reviewed-by: Søren Sandmann 
> ---
>  demos/scale.ui | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/demos/scale.ui b/demos/scale.ui
> index f6f6e89..d498d26 100644
> --- a/demos/scale.ui
> +++ b/demos/scale.ui
> @@ -177,6 +177,7 @@
>   id="lock_checkbutton">
> Lock X and Y 
> Dimensions
> 0.0
> +   True
>   
>
>  False
> --
> 1.7.11.7
>
> ___
> Pixman mailing list
> Pixman@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pixman

This patch is:
Reviewed-by: Oded Gabbay 
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 11/14] pixman-filter: Speed up BOX/BOX filter

2016-04-29 Thread Oded Gabbay 
On Tue, Apr 12, 2016 at 5:36 AM, Søren Sandmann Pedersen
 wrote:
> The convolution of two BOX filters is simply the length of the
> interval where both are non-zero, so we can simply return width from
> the integral() function because the integration region has already
> been restricted to be such that both functions are non-zero on it.
>
> This is both faster and more accurate than doing numerical integration.
>
> This patch is based on one by Bill Spitzak
>
> https://lists.freedesktop.org/archives/pixman/2016-March/004446.html
>
> with these changes:
>
> - Rebased to not assume any changes in the arguments to integral().
>
> - Dropped the multiplication by scale
>
> - Added more details in the commit message.
>
> Signed-off-by: Søren Sandmann 
> ---
>  pixman/pixman-filter.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c
> index c868723..32aaa9a 100644
> --- a/pixman/pixman-filter.c
> +++ b/pixman/pixman-filter.c
> @@ -160,11 +160,15 @@ integral (pixman_kernel_t kernel1, double x1,
>   pixman_kernel_t kernel2, double scale, double x2,
>   double width)
>  {
> +if (kernel1 == PIXMAN_KERNEL_BOX && kernel2 == PIXMAN_KERNEL_BOX)
> +{
> +   return width;
> +}
>  /* The LINEAR filter is not differentiable at 0, so if the
>   * integration interval crosses zero, break it into two
>   * separate integrals.
>   */
> -if (kernel1 == PIXMAN_KERNEL_LINEAR && x1 < 0 && x1 + width > 0)
> +else if (kernel1 == PIXMAN_KERNEL_LINEAR && x1 < 0 && x1 + width > 0)
>  {
> return
> integral (kernel1, x1, kernel2, scale, x2, - x1) +
> --
> 1.7.11.7
>
> ___
> Pixman mailing list
> Pixman@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pixman

This patch is:
Acked-by: Oded Gabbay 
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [RFC 1/2] Remove seemingly unneeded comparison(s)

2016-04-29 Thread Pekka Paalanen 
On Fri, 29 Apr 2016 10:15:37 +0100
Emil Velikov  wrote:

> On 27 April 2016 at 18:46, Bill Spitzak  wrote:
> > On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen  
> > wrote:  
> >>
> >> On Wed, 27 Apr 2016 09:56:44 +0100
> >> Emil Velikov  wrote:
> >>  
> >> > On 26 April 2016 at 19:12, Bill Spitzak  wrote:  
> >> > > The old code is comparing pixman_fixed_48_16_t values to
> >> > > pixman_fixed_16_16_t values, thus it is checking for truncation of
> >> > > overflow
> >> > > values.
> >> > >  
> >> > Indeed it does. I'll grep more before asking silly questions ;-)
> >> >  
> >> > > It would probably be better to clamp these overflowed values, like
> >> > > pixman_transform_point_31_16 is doing to clamp to the
> >> > > pixman_fixed_48_16
> >> > > result. Right now the result is an odd mix of clamping and modulus. A
> >> > > rewrite to go directly to clamped pixman_fixed_16_16 values would be
> >> > > even
> >> > > better.
> >> > >  
> >> > Sounds like a plan. Sadly I doubt I'll get to it any time soon.  
> >>
> >> Wasn't the point of the boolean return from these functions to tell the
> >> caller to drop what it is doing because it cannot be done properly?  
> >
> >
> > Dropping a fill is a lot worse than trying to simulate it using the clamped
> > path. It will produce a wrong result if one of the edges connected to a
> > clamped point passes through the clip, but this often does not happen, and
> > the transition to the wrong result is gradual as the point moves outside the
> > clamped region.
> >
> > More importantly the caller cannot do anything with the return values right
> > now, as they are modulus MAX_16_16+1. Even the direction they are in is
> > lost.
> >  
> I think that keeping the user provided memory as-is when the function
> does not succeed is a good idea.
> Afaics currently the contents get overwritten regardless of the result.
> 
> This is what you guys were on about, right ? Or perhaps you're
> thinking about spinning v2 of the function with different
> signature/behaviour ?

Hi Emil,

I think the conclusion was that the comparisons are not useless, and
this patch should be dropped. You noted it yourself that this patch
causes a regression in the test suite.


Thanks,
pq


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

Re: [Pixman] [RFC 1/2] Remove seemingly unneeded comparison(s)

2016-04-29 Thread Emil Velikov 
On 27 April 2016 at 18:46, Bill Spitzak  wrote:
> On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen  wrote:
>>
>> On Wed, 27 Apr 2016 09:56:44 +0100
>> Emil Velikov  wrote:
>>
>> > On 26 April 2016 at 19:12, Bill Spitzak  wrote:
>> > > The old code is comparing pixman_fixed_48_16_t values to
>> > > pixman_fixed_16_16_t values, thus it is checking for truncation of
>> > > overflow
>> > > values.
>> > >
>> > Indeed it does. I'll grep more before asking silly questions ;-)
>> >
>> > > It would probably be better to clamp these overflowed values, like
>> > > pixman_transform_point_31_16 is doing to clamp to the
>> > > pixman_fixed_48_16
>> > > result. Right now the result is an odd mix of clamping and modulus. A
>> > > rewrite to go directly to clamped pixman_fixed_16_16 values would be
>> > > even
>> > > better.
>> > >
>> > Sounds like a plan. Sadly I doubt I'll get to it any time soon.
>>
>> Wasn't the point of the boolean return from these functions to tell the
>> caller to drop what it is doing because it cannot be done properly?
>
>
> Dropping a fill is a lot worse than trying to simulate it using the clamped
> path. It will produce a wrong result if one of the edges connected to a
> clamped point passes through the clip, but this often does not happen, and
> the transition to the wrong result is gradual as the point moves outside the
> clamped region.
>
> More importantly the caller cannot do anything with the return values right
> now, as they are modulus MAX_16_16+1. Even the direction they are in is
> lost.
>
I think that keeping the user provided memory as-is when the function
does not succeed is a good idea.
Afaics currently the contents get overwritten regardless of the result.

This is what you guys were on about, right ? Or perhaps you're
thinking about spinning v2 of the function with different
signature/behaviour ?

-Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman