[cairo] pixman "Simplify clipping rule" problems

[Siarhei Siamashka]

On Sunday 28 June 2009, Kalle Vahlman wrote:
> 2009/6/28 Siarhei Siamashka <siarhei.siamashka at gmail.com>
>
> > The one thing that worries me a bit is that apparently (almost) nobody
> > else is using 'scaling-test' program. As limited as it is, it could have
> > prevented a handful of broken commits already. And you can't rely on me
> > monitoring this mailing list forever, effectively working as a trained
> > monkey who just runs the test program occasionally and complains when
> > something gets obviously broken.
>
> I'm sure nobody is asking you to do that?-)

You probably misunderstood me. I'm not whining or complaining.

The case is quite clear here. If nobody is using scaling-test program in
practice, then it is a dead weight in pixman repository. It may have one of
the following futures:
1. be removed
2. be put into real use
3. eventually become unmaintained and obsolete, confusing everyone who takes a 
look at it

Which one do you prefer?

> > Seriously, should we add some 'make test' target with an easily
> > interpretable result and ask commiters to reject any patches which make
> > it fail unless this failure is investigated and properly explained?
>
> That would be nice, but at least it seems that scaling-test as it is
> can't be used in such target.

It can be easily used as such target. It only needs some return code depending
on whether it was successful or not (to automatically check it in the scripts
or makefiles) and fix for the big endian systems to provide exactly the same
deterministic result as on little endian systems.

> A quick look in it tells me to just run it, but when I do it says that
> it fails. On x86 with and without all optimizations. I tried with
> master, 0.15.14, 0.15.12 and 0.14.0. Of these the two first ones say
> that it fails and the two others crash.

For master and 0.15.14, crc32 value just has to be updated, have a look
at the patch attached to the post you were replying:
http://lists.cairographics.org/archives/cairo/2009-June/017502.html
I don't expect this crc32 value to be changed very often because it is
really a change in pixman behavior which can also affect any pixman users.
The description of changes in pixman have been provided by Soeren and
discussed in this mailing list. *If* ensuring that scaling-test is passed
was a part of pixman release process, then the reference crc32 value in this
test would have been updated too by now. Also introducing bug in 0.15.12
could have been avoided.

Version 0.15.12 is really buggy, so no surprise that it is crashing, that's
actually what this whole thread is about. The fix was committed in
http://cgit.freedesktop.org/pixman/commit/?id=895a8da63370635b05ffb91d3d670c6627d8b2ab

Versions 0.15.10 and 0.15.8 pass the test just fine.

Version 0.15.6 passes the test when MMX and SSE2 optimizations are disabled,
but fails with them. The fixes for these problems have been committed in
http://cgit.freedesktop.org/pixman/commit/?id=da9f3266fd00a5634fd2fb8a9cffbf24d668aaab
http://cgit.freedesktop.org/pixman/commit/?id=53ce8838254d436b6a4d527aacdece7dba7ceacd

Version 0.14.0 did not have scaling-test yet. But if you take scaling-test 
from the current pixman sources and run it with 0.14.0, it really reveals a
presence of a crasher bug in PIXMAN_REPEAT_REFLECT implementation. The fix for
this bug has been committed in:
http://cgit.freedesktop.org/pixman/commit/?id=2d9c7cd84b276ebe2ff72d03c34a2d7f4f98b9f9


As you can see, every case of scaling-test failure is associated with one
pixman bug or another.

> A longer look tells me that I should compile static binaries and run
> the ruby script that figures out which test failed. If I actually had
> some code to contribute, this is probably the point where rather than
> installing ruby and building static binaries I'd just submit the patch
> and hope for the best.

With all the respect, I strongly disagree here.

Using scaling-test, you are able to get two answers:
1. is the code buggy? (just compile pixman, run 'test/scaling-test' and you
will get the answer)
2. where exactly the problem is? (this involves installing ruby and the other 
steps).

If you don't like ruby or the proposed way to narrow down the problem, you
have to find and fix the bug yourself in some other way. But blindly
committing broken code is not a good option.

> This is of course a sort of blind-testing, I didn't take time to
> actually find out how the tool works or anything. Which is the way I'd
> expect most contributors to behave, so maybe that's why the tool is
> not used more widely.

The point here is to make running regression tests a part of pixman release
process, properly document it and ask (or enforce) contributors to use them.

> To make the scaling test useful for such reviewing purposes, it
> probably needs first to work without reference binaries (make test
> recompiling the library twice sounds like a difficult thing to
> achieve), maybe by just accepting the CRC on the command line? That
> way you could just run the reference once and store the CRC (unless I
> don't understand how the tool works).

It works exactly this way. The reference crc32 value is stored in the source.

But in the case of failure, we need to identify which one of the millions of
variants of different pixman calls has failed. For this identification, two
binaries are needed. An alternative would be to store a huge list of crc32
values, which is nonpractical.

It is technically possible to implement disabling/enabling fastpath functions
at runtime and have only a single binary. But this will only compare general
path vs. fastpath.

> Also, the ruby script would be nice to rewrite in sh or some other
> more widespread language (it looks trivial enough anyway).

Sure, we could use a help of someone with sh scripting knowledge to do this.

> And to make it git-bisect friendly (when/if it can be expected to just
> be run and to give a meaningful result), it should return with a
> non-zero value on failures.

Agreed.

-- 
Best regards,
Siarhei Siamashka