Re: [Pixman] [PATCH 1/4] test: static link pixman in matrix-test

2016-04-26 Thread Siarhei Siamashka 
Hello Emil,

On Sun, 24 Apr 2016 19:20:58 +0100
Emil Velikov  wrote:

> The test uses pixman internal symbols, which we currently export. To
> resolve that static link the pixman library. This is an internal only
> test ran at `make check' thus we are fine with the bloated binary and
> alike.
> 
> Signed-off-by: Emil Velikov 
> ---
> Afaics the Windows build already uses a static library thus things
> should be fine there. I have NOT tested it though.
> 
> If people are concerned with this approach, we can build the core files
> once and explicitly setup a pixman-static.la for the tests to use.
> ---
>  test/Makefile.am | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/test/Makefile.am b/test/Makefile.am
> index 88dc36d..f70440e 100644
> --- a/test/Makefile.am
> +++ b/test/Makefile.am
> @@ -5,6 +5,8 @@ AM_LDFLAGS = $(OPENMP_CFLAGS) $(TESTPROGS_EXTRA_LDFLAGS) 
> $(PTHREAD_LDFLAGS)
>  LDADD = libutils.la $(top_builddir)/pixman/libpixman-1.la -lm  $(PNG_LIBS) 
> $(PTHREAD_LIBS)
>  AM_CPPFLAGS = -I$(top_srcdir)/pixman -I$(top_builddir)/pixman $(PNG_CFLAGS)
>  
> +matrix_test_LDFLAGS = -static
> +
>  libutils_la_SOURCES = $(libutils_sources) $(libutils_headers)
>  
>  noinst_LTLIBRARIES = libutils.la

Well, I have only one question. Where is the profit and what are we
supposed to gain by applying patches from this series?

Right now if you are interested in compiling static test programs,
then you can use the --enable-static-testprogs configure option.
It is useful for running the pixman test suite under QEMU with the
help of binfmt_misc.

And we currently also can run the test suite against pixman shared
libraries, which are built and shipped as binary packages by various
Linux distributions. In the case if some distribution maintainers are
up to no good [1], this still allows us to confirm whether they are
shipping broken pixman binaries even without any need to figure out
how to use their build infrastructure.

If I understand it correctly, you are basically trying to go an
extra mile to remove the existing diagnostic interface from the
pixman shared library. Would we lose anything if we just keep
things working as they are?


[1] https://patchwork.freedesktop.org/patch/70676/

-- 
Best regards,
Siarhei Siamashka
___
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-26 Thread Bill Spitzak 
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.

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.



On Tue, Apr 26, 2016 at 10:59 AM, Petr Kobalíček 
wrote:

> Doesn't this check for NaNs?
>
> On Sun, Apr 24, 2016 at 8:22 PM, Emil Velikov 
> wrote:
>
>> With commit ed39992564b "Use pixman_transform_point_31_16() from
>> pixman_transform_point()" we added some strange hunks.
>>
>> Namely: we copy the data from the internal storage to the user vector
>> only to compare them immediately after.
>>
>> Cc: Siarhei Siamashka 
>> ---
>>
>> Siarhei, what is the intent with the original commit ? Any ideas why
>> things crash ?
>>
>> Seemingly this can be dropped/replaced with TRUE, yet it causes one of
>> the tests () to segfault in the optimised SSE2 codepath -
>> scaled_bilinear_scanline_sse2___SRC.
>>
>> BILINEAR_INTERPOLATE_ONE_PIXEL -> BILINEAR_INTERPOLATE_ONE_PIXEL_HELPER
>>
>> Regards,
>> Emil
>> ---
>>
>>  pixman/pixman-matrix.c | 8 ++--
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/pixman/pixman-matrix.c b/pixman/pixman-matrix.c
>> index 65b3d32..117015b 100644
>> --- a/pixman/pixman-matrix.c
>> +++ b/pixman/pixman-matrix.c
>> @@ -393,9 +393,7 @@ pixman_transform_point_3d (const struct
>> pixman_transform *transform,
>>  vector->vector[1] = tmp.v[1];
>>  vector->vector[2] = tmp.v[2];
>>
>> -return vector->vector[0] == tmp.v[0] &&
>> -   vector->vector[1] == tmp.v[1] &&
>> -   vector->vector[2] == tmp.v[2];
>> +return TRUE;
>>  }
>>
>>  PIXMAN_EXPORT pixman_bool_t
>> @@ -414,9 +412,7 @@ pixman_transform_point (const struct pixman_transform
>> *transform,
>>  vector->vector[1] = tmp.v[1];
>>  vector->vector[2] = tmp.v[2];
>>
>> -return vector->vector[0] == tmp.v[0] &&
>> -   vector->vector[1] == tmp.v[1] &&
>> -   vector->vector[2] == tmp.v[2];
>> +return TRUE;
>>  }
>>
>>  PIXMAN_EXPORT pixman_bool_t
>> --
>> 2.8.0
>>
>> ___
>> Pixman mailing list
>> Pixman@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/pixman
>>
>
>
> ___
> Pixman mailing list
> Pixman@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pixman
>
>
___
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-26 Thread Petr Kobalíček 
Doesn't this check for NaNs?

On Sun, Apr 24, 2016 at 8:22 PM, Emil Velikov 
wrote:

> With commit ed39992564b "Use pixman_transform_point_31_16() from
> pixman_transform_point()" we added some strange hunks.
>
> Namely: we copy the data from the internal storage to the user vector
> only to compare them immediately after.
>
> Cc: Siarhei Siamashka 
> ---
>
> Siarhei, what is the intent with the original commit ? Any ideas why
> things crash ?
>
> Seemingly this can be dropped/replaced with TRUE, yet it causes one of
> the tests () to segfault in the optimised SSE2 codepath -
> scaled_bilinear_scanline_sse2___SRC.
>
> BILINEAR_INTERPOLATE_ONE_PIXEL -> BILINEAR_INTERPOLATE_ONE_PIXEL_HELPER
>
> Regards,
> Emil
> ---
>
>  pixman/pixman-matrix.c | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/pixman/pixman-matrix.c b/pixman/pixman-matrix.c
> index 65b3d32..117015b 100644
> --- a/pixman/pixman-matrix.c
> +++ b/pixman/pixman-matrix.c
> @@ -393,9 +393,7 @@ pixman_transform_point_3d (const struct
> pixman_transform *transform,
>  vector->vector[1] = tmp.v[1];
>  vector->vector[2] = tmp.v[2];
>
> -return vector->vector[0] == tmp.v[0] &&
> -   vector->vector[1] == tmp.v[1] &&
> -   vector->vector[2] == tmp.v[2];
> +return TRUE;
>  }
>
>  PIXMAN_EXPORT pixman_bool_t
> @@ -414,9 +412,7 @@ pixman_transform_point (const struct pixman_transform
> *transform,
>  vector->vector[1] = tmp.v[1];
>  vector->vector[2] = tmp.v[2];
>
> -return vector->vector[0] == tmp.v[0] &&
> -   vector->vector[1] == tmp.v[1] &&
> -   vector->vector[2] == tmp.v[2];
> +return TRUE;
>  }
>
>  PIXMAN_EXPORT pixman_bool_t
> --
> 2.8.0
>
> ___
> Pixman mailing list
> Pixman@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pixman
>
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman