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

[Albert Astals Cid]

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.

Albert