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

[Albert Astals Cid]

A Dimecres, 26 de gener de 2011, Albert Astals Cid va escriure:
> A Dimarts, 25 de gener de 2011, Andrea Canciani va escriure:
> > 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.
> 
> Ok, you got me, i'll regtest your patch. Please talk with Tomas to get a
> better understanding of why we need 0003 and what it does :-)

Your patch regresses at altona_visual_1v2a_x3.pdf [1]

The circles are not totally filled in their right border. I stopped having a 
look all at the other files in my regtest that change (so it might regress 
somewhere else) but makes no point having a look at all the other files since 
i'll have to do it anyway once you fix this regression.

Now my question is if I should commit cleaned Thomas patch or not.

Albert

[1] 
http://www.eci.org/lib/exe/fetch.php?id=de%3Adownloads&cache=cache&media=downloads:altona_test_suite:altona_visual_1v2a_x3.pdf

> 
> Albert
> 
> > 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