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

[Bill Spitzak]

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.
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

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

[Pekka Paalanen]

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?


Thanks,
pq


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

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

[Emil Velikov]

Hello Siarhei,

On 26 April 2016 at 19:32, Siarhei Siamashka
 wrote:
> 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?
>
Precisely - it aims to remove the unneeded exports.

Exporting symbols only to call them "internal only" does seem a bit
wrong. It also opens the door for (in)intentional abuse. Let's not
forget that one of them does not even have the "internal only" tag in
its name.

While I can see your point about running the tests against system
pixman, I'm not sure how much in reality that is an actual thing.
Afaict everyone building pixman must run `make check' (without -k of
course) to ensure that they have a properly working binary. That's the
point of `make check', isn't it ? If they don't ... then they are just
asking for trouble.

Obviously the size of the final binary was (slightly) improved, but
I'm not sure how much you're going to buy that as an argument.

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