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