[poppler] Followup Bug 32349 & Poppler: More shading fun ;-)

[Andrea Canciani]

On Tue, Jan 25, 2011 at 11:36 AM, Thomas Freitag
<Thomas.Freitag at kabelmail.de> wrote:
> Am 24.01.2011 21:44, schrieb Andrea Canciani:
>
> On Sun, Jan 23, 2011 at 9:36 PM, Albert Astals Cid <aacid at kde.org> wrote:
>
> A Diumenge, 23 de gener de 2011, Thomas Freitag va escriure:
>
> Am 23.01.2011 18:12, schrieb Albert Astals Cid:
>
> A Dissabte, 15 de gener de 2011, Thomas Freitag va escriure
>
> The memory problems were coming still from the wrong calculation of
> shading extension which I took from Gfx.cc and tried to correct, but it
> still leaded to huge circles. Andrea gave me a hint of a correct
> calculation of radial shading extension in cairo, and I adapated that
> code piece now successfully to Splash. It seems, as if with this the
> memory problems are now gone, at least I could render every PDF You sent
> me in the past, and I couldn't find any regressions in the rendering
> results.
>
> I got a new regression, will send you privately the pdf file.
>
> The question now is, you want me to try the other newest patch? Or you
> want to fix this "old" one?
>
> Please test the newest patch. It has a completely other algorithm and as
> I already tested, it has not this regression and hopefully no other. I
> 'll answer the other mails in a few seconds.
>
> Ok, i'll run the regtest on that.
>
> If that passes the regtest, this patchset should pass it as well.
>
> 0001, 0002 and 0003 are fixes and cleanups for poppler Functions.
>
> 0004 abstracts a common interface from axial and radial shadings
>
> 0005 is not completely clear to me. I think it forces antialiasing to be
> applied on shadings.
>
> In 0006 I cleaned up the radial implementation by Thomas. In particular
> I simplified the code which chooses one of the two solutions.
>
> 0007 const-ifies Matrix methods and adds norm() and determinant().
>
> 0008 adds simple caching (with uniform sampling) with a number of
> samples which depends on the shading extents. It guarantees correct
> rendering and provides much better performance than an hardcoded
> constant (for example 65536 caused performance regressions with
> respect to no caching at all in the ducks&roses pdf).
> I left in some debugging code which is helpful while testing the
> performance, because it prints the cache size extimate and if the cache
> was activated or not.
>
> 0009 improves the parameter range estimate (in 0007 it is assumed to be
> the full range). This might provide a speedup in some very particular
> pdf files, but so far I haven't found any in which it actually matters.
>
> Fantastic!!!!!!!!!!
> I just applied the patches because I was full of curiosity. Two examples
> a) ducks and roses is now rendered in less than half of time in comparison
> without any radial shading patches, and in less than a quarter of the time I
> needed with my patch.

This is consistent with about 75% of the time being spent in color function
evaluation prior to the color caching, assuming that the color function caching
is very effective.

> b) rendering the non reduced PDF where acrobat gives up is done now in a few
> seconds, and in 2/3 I need with the radial patch.
> So the speed optimization is really great!

For this case we can even get some additional speed boost (with non-uniform
sampling).

>
> I had only a small problem compiling it with visual studio under windows:
> the c-compiler has a problem with using std::min or std::max, s. i.e.
> http://heifner.blogspot.com/2008/02/stdmin-and-stdmax.html, so I defined
>
> #undef MIN
> #define MIN(a, b) ((a) < (b) ? (a) : (b))
> #undef MAX
> #define MAX(a, b) ((a) > (b) ? (a) : (b))
>
> and used MIN and MAX instead of std::min and std::max. And You should remove
> the fprintf's in GfxState.cc ....

Sorry, I didn't know about the problem with std::min/max.
It's quite easy to fix, as suggested in the blog you linked to (and by
Pino Toscano as well).

The fprintf are in the code only for debug purposes. I will obviously remove
that when the patchset will be ready to be committed.

>
> But as You already seen, Albert will probably commit my patch to master
> soon, so it would be very nice, if You can create a new patch after he will
> commit it. Because the basic method (calculation the color given the
> x,y-position) is the same, a regtest with that patch then will propably not
> have such a lot differences and Albert has less work to compare the results.

The computation is essentially the same. I just used a mathematical property
of the solutions of the equation to clean up the code which chooses which
one is the valid solution. I added a comment about it in the code.

> And on the other hand I would like to have my patch now in and don't like to
> stop that having a additional ping pong game with patches.

Although I agree I want radial gradients to be fixed ASAP, I'd like your patch
to be cleaned up a little before going into master, especially if we plan to
drop the Bresenham code anyway.

For example I'm not sure if I want something like 0009 in master, because
it adds a huge amount of nontrivial code and it only provides speedups
in very particular cases.

Andrea

>
> Thomas
>
> Commit messages and authorship are to be fixed. I haven't implemented
> the non-uniform sampling because I had some problem with clipping
> to the domain and range, but it can be solved and I hope to do it soon.
>
> I'd like to get a confirmation that the cache is working (i.e. it speeds up
> the documents which the radial gradient patch slows down and causes
> no rendering regressions).
> Please remember that this patchset also affects axial shadings, which
> should become faster as well.
>
> Andrea
>
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler
>
>
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler
>
>