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

Andrea Canciani ranma42 at gmail.com
Wed Jan 26 02:48:05 PST 2011 

On Wed, Jan 26, 2011 at 10:45 AM, Thomas Freitag
<Thomas.Freitag at kabelmail.de> wrote:
> 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.

Yes, I didn't know about it.

This is not really a regression of the radial rasterization, it is a bug
in the existing code, already hit for axial shadings, which my patch
also triggers for radial shadings, because it shares the same code
path between them.

Thomas worked around it by using non-antialiasiased radial gradients,
which apparently extends the clipping region a little. This causes a
regression in my radial-gradent-* tests with Thomas patch (adjacent
shadings overlap a little, even if they should be disjoint).

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

The patch above restores aliasing/antialiasing behavior (i.e. works
around the existing clip bug for altona, breaks radial-gradient-*).

To me it looks like a hack. The right thing to do would be to fix the
clip computation and/or the antialias code. This would fix both radial
and axial shadings (which currently are probably broken as well,
even though nobody noticed).

I'm now reading the splash rasterization code, but the fix might not
be trivial (although I think that it's quite strange that a SplashClip
for a [0,0 - 0.5,0.5] rect results in an integer clipping range of
[0,0 - 0,0]).

Andrea

>
> 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
>>
>> .
>>
>
>
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler
>

More information about the poppler mailing list