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

[Andrea Canciani]

On Tue, Jan 25, 2011 at 11:38 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: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 am by no means doubting that your code is wrong, i am simply expressing that
> i prefer to go step by step. I don't want you to take this as any offense
> because it is not meant to be.
>
>>
>> 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).
>
> The cleanedup up patch can be regtested automatically, i just need to check
> that poppler with the cleaned up patch generates exactly the same png than
> with the non cleanup of patch for all the files. Testing yours will require me
> to spend again 4 hours looking at all the files that have an axial shading and
> making sure the changes are improvements and not regressions, i hope you can
> understand it's a different amount of work.

The axial code is not being modified, it is just being moved around.

The radial code should return the same images as Thomas patch
except for the RADIAL_EPSILON, which I set up as a "round"
number.

If you plan on performing automated regtesting, please apply 0003 as
well and define RADIAL_EPSILON to be the same as in Thomas
patch (1.192092896e-07F).
This should be sufficient to guarantee that the code generates
exactly the same images as the latest patch by Thomas (assuming
that SPLASH_RADIAL_USEBITMAP was not defined).

0003 obviously cannot be committed as-is, it is in bad need of a better
commit message.

Andrea