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

[Bill Spitzak]

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).

Furthermore I have determined I made some mistakes in this newest version.
I tried reducing the number of Simpson's integration steps and discovered
that the recursion for the linear filters does make a difference, and
should be restored. It then looks like Simpsons integration could be
reduced to 2 or 4 with no visible change to the graphs.

This testing also showed that my replacement for BOX.BOX is vastly more
accurate than even the highest Simpsons integration (this should be obvious
as the function has sharp corners on the trapazoid).

Other problem is with figuring out the minimum level of subsampling for the
good/best filters. The technique I am doing is to use the zone plate image
with a rotation of about 30 degrees at various scales, and adjusting the
subsampling to the point where there is no visible change when it is
increased. The current algorithm seems to match the good filter, but is
actually 1 too large for the best, except at 1/8 scale where it is 1 too
low and 1/2 scale where it is correct. It is probably worth it to fix this
as the filter is generated on every transform (something I hope a future
version that keeps a set of filters can avoid).

A third problem is that my comment about the filter types is not accurate.
LINEAR.LINEAR is not a cubic apparently.

So I think there will have to be another version of this patch, will try to
get it asap.


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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20160107/0b086788/attachment.html>