[poppler] Followup Bug 32349 & Poppler: More shading fun ;-)
Thomas Freitag
Thomas.Freitag at kabelmail.de
Wed Jan 26 01:45:06 PST 2011
Am 26.01.2011 09:57, schrieb 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.
I think, You mean 0005, isn't it? Here the reason for the changes
a) SplashOutputDev::beginTransparencyGroup
The wrong setup of tx and ty caused segmentation faults in
Splash::blitTransparent()
b) Splash::pipeRun
If getColor return gFalse, we need to increment ALL pointers, use now
the already existing method pipeIncX
c) Splash::shadedFill
shadedFill is now used by axial and radial shading, axial supports
antialiasing, radial not. Therefore we need to support either case in it.
>> 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]
Okay, that's obviously. This PDF was the reason, why I removed
antialiasing again in radial shading. Andrea hadn't known that, and
therefore probably missed it.
> 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.
If You agree, give Andrea's patch a final chance (I had a lot of chances
:- ) ). Unfortunately I'm not able to make this small patch pieces
today, so here just the correction at end of SplashOutputDev.cc:
>>> code snippet start
GBool SplashOutputDev::univariateShadedFill(GfxState *state,
SplashUnivariatePattern *pattern, double tMin, double tMax) {
double xMin, yMin, xMax, yMax;
SplashPath *path;
GBool retVal = gFalse;
// get the clip region bbox
state->getUserClipBBox(&xMin, &yMin, &xMax, &yMax);
// fill the region
state->moveTo(xMin, yMin);
state->lineTo(xMax, yMin);
state->lineTo(xMax, yMax);
state->lineTo(xMin, yMax);
state->closePath();
path = convertPath(state, state->getPath());
retVal = (splash->shadedFill(path,
pattern->getShading()->getHasBBox(), pattern) == splashOk);
state->clearPath();
delete path;
return retVal;
}
GBool SplashOutputDev::axialShadedFill(GfxState *state, GfxAxialShading
*shading, double tMin, double tMax) {
GBool vaa = getVectorAntialias();
// restore vector antialias because we support it here
setVectorAntialias(gTrue);
SplashAxialPattern *pattern = new SplashAxialPattern(colorMode,
state, shading);
GBool retVal = univariateShadedFill(state, pattern, tMin, tMax);
setVectorAntialias(vaa);
delete pattern;
return retVal;
}
GBool SplashOutputDev::radialShadedFill(GfxState *state,
GfxRadialShading *shading, double tMin, double tMax) {
SplashRadialPattern *pattern = new SplashRadialPattern(colorMode,
state, shading);
GBool retVal = univariateShadedFill(state, pattern, tMin, tMax);
delete pattern;
return retVal;
}
<<< code snippet end.
Thomas
> 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
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler
>
> .
>
More information about the poppler
mailing list