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

[Andrea Canciani]

On Tue, Jan 25, 2011 at 10:54 PM, Albert Astals Cid <aacid at kde.org> wrote:
> A Dimarts, 25 de gener de 2011, vàreu escriure:
>> On Tue, Jan 25, 2011 at 10:25 PM, Albert Astals Cid <aacid at kde.org> wrote:
>> > A Dimarts, 25 de gener de 2011, Andrea Canciani va escriure:
>> >> On Tue, Jan 25, 2011 at 9:00 PM, Albert Astals Cid <aacid at kde.org> wrote:
>> >> > ...
>> >> >
>> >> >> Finished successfully, i'll have a look at the code tomorrow and if i
>> >> >> don't find anything obviously wrong will commit it to master :-)
>> >> >
>> >> > Had a look at the code and it is too untidy, please remove all the
>> >> > ifdefs of unused code and remove the T_EDGE and T_CORNER defines and
>> >> > i'll commit it.
>> >>
>> >> 0001 and 0002 replace most of Thomas's patch, the exception being
>> >> attachment 0003. I don't understand what is the purpose of that change,
>> >> maybe Thomas can explain it and suggest a better commit message.
>> >>
>> >> I think 0001 and 0002 should be quite clean and commit ready.
>> >> Please review them, I'll fix any remaining problem as soon as possible.
>> >
>> > Hmm, sincerely i prefer to commit Thomas patches first. It has taken us
>> > lots of regtesting iterations to get to something that gives
>> > improvements and no regressions. Once that is in we can start with your
>> > patches :-)
>>
>> These patches replace it, they implement the same change.
>> I think it's quite pointless to commit it just to revert it immediately,
>> but if you like it better...
>
> I know they implement the same feature, but can you guarantee 100% that they
> will not have any regression that Thomas patches we know do not have?

No, I can't guarantee at 100% (I wouldn't do it even for Thomas's patch, even
if it has been tested throughly), but I'm quite confident they are as correct,
because they performs the very same computation, which is also the same
as the one performed in pixman and in cairo-gl (yes, I've already implemented
radial gradients multiple times...)

I did not test them as extensively as the current Thomas's patch has been
tested, but I think that the same objection would be apply to the cleaned
up patch (which would also require additional work).

These patch should be small enough to be reviewed easily. Moreover they
can render correctly the cairo-generated gradients, which should trigger all
the possible paths in the rasterizer.

If you don't notice anything weird while reviewing them, I'll give
a 99.9% guarantee of correctness ;)

Andrea

PS: FWIW the backward rasterization technique is simple, so it should
be even easier to trust it.
Bresenham continued to show some minor glitches even after multiple
test&fix rounds, while backward rasterization was immediately correct.

PPS: If you want to, I should be able to create pdf files where both
Thomas's and this implementation fail at rendering the correct result,
because of numerical instability problems.
If you care about that, computations need to be performed in arbitrary
size floating points, because for intermediate computations might require
up to 4 times the input accuracy (i.e. if you use double with 53 bits of
mantissa, you might need up to 212 bits), but this can only happen in
almost-degenerate and not very interesting cases.