[Pixman] [PATCH 07/15] pixman-filter: Speed up the BOX+BOX filter

[Pekka Paalanen]

On Thu, 7 Jan 2016 10:35:57 -0800
Bill Spitzak <spitzak at gmail.com> wrote:

> You are right, I ran the tests on the earlier version of this code but not
> on the current one. There were very few tests of the separable filters,
> however, which is probably why it passed. Adding some tests would be nice,
> but only if we can prove that the resulting images are accurate (maybe by
> calculating them with a math program or something).

There are two different ways to test.

One is to hand-craft tests where you know what the correct result is,
and compare to the known correct answer. This takes a lot of work to
cover many parameter combinations.

Another is to test that the results do not change. Pixman uses this
method extensively with fuzzing: generate lots of pseudo-random
(repeatable) test cases and checksum over them all. When creating such
a test, you assume that the current implementation is correct and just
record the checksum as the correct result. It does not matter whether
it actually is correct or not: applications are already using Pixman and
perhaps relying on it, and you only care about finding changes. It is a
very good guard against regressions, but this method is not always
applicable.

The latter method is not applicable if one must allow some inaccuracy
in the results, like often with floating point operations. You could
still hand-craft some tests to ensure results don't change by assuming
that the current implementation is correct, saving what you get, and
comparing to that in the test. Proving correctness becomes a factor
only when you think the current code is not giving correct results or
you introduce a new feature.

I don't know what the case is here, but if it is possible to write
fuzzing tests, I would very much recommend that since it is pretty
easy. If not, then testing is more work.

The major users of Pixman that I know of are Xorg and Cairo. You are
familiar with Cairo at least, so if Xorg does not use these filtering
operations, there is probably less to worry about. I don't know what
Xorg uses, though.

Please keep in mind that the filters GOOD and BEST have been as is for
a long long time, AFAIU, so changing their behaviour now is likely not
a good idea. They are no longer "a good filter" and "whatever best
filter", but "the specific filter called GOOD" and "the specific filter
called BEST", in lack of documentation saying otherwise.


Thanks,
pq

> On Thu, Jan 7, 2016 at 12:09 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 
> > On Wed, 23 Dec 2015 10:08:01 +0200
> > Oded Gabbay <oded.gabbay at gmail.com> wrote:
> >  
> > > On Tue, Dec 22, 2015 at 10:53 PM, Bill Spitzak <spitzak at gmail.com>  
> > wrote:  
> > > > I think the improvement is obvious if you check how much code is run  
> > to do  
> > > > the Simpson's integration. BOX.BOX will literally be the filter used  
> > almost  
> > > > always once this is finally fixed.
> > > >  
> > >
> > > I understand the logic behind this, and I can agree with it, but the
> > > standard in pixman, AFAIK, is to show actual performance improvements
> > > using benchmarks/real-world cases.
> > >
> > > I'm not against this patch, and I think it will be eventually merged.
> > > I'm just saying I think we need to defer it until:
> > >
> > > 1. The changes you talk about them are actually merged into pixman &  
> > cairo  
> > > 2. We can show the (hopefully major) improvement using cairo  
> > benchmarking.
> >
> > Hi,
> >
> > some general comments:
> >
> > Not to forget, that any piece of existing code you replace should
> > already have a test excercising it in place, so that when you actually
> > change the code, the test ensures the effective result is still the
> > same. (The same does not necessarily imply correct, though. It's just
> > about catching unintended changes.)
> >
> > That should be pretty easy to check by deliberately breaking that piece
> > of code and seeing that 'make check' fails. If necessary, one can use
> > any of the PIXMAN_DISABLE options to force the use of generic code.
> >
> > Maybe this is already done and tested, but it would be nice to mention
> > it, so that the new people who don't know the test suite by heart are
> > assured it has been considered.
> >
> > If you find out that the test suite does not cover the code in
> > question, you may need to extend the test suite first.
> >
> > This policy would also highlight the cases where old code was actually
> > wrong and the tests ensure that it stays the same rather than correct.
> > Fixing this needs even more care as you want to check the new results
> > really are correct before fixing the test. One might also have to
> > consider if the old results are relied upon by the users like the Xorg
> > server, and the fix would break users. In that case the fix cannot land.
> >
> > Btw. the above is also a warning about changing the meaning of (the
> > numerical results produced by) existing filter flags. If e.g. Xorg
> > depends on some flag producing some exact result, you probably can't
> > change the result of that flag. This would call for looking into Xorg
> > and X11 test suites.
> >
> > And Cairo, of course.
> >
> >
> > Thanks,
> > pq
> >  

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20160108/91511191/attachment.sig>