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

[Thomas Freitag]

Am 26.01.2011 15:01, schrieb Andrea Canciani:
>
>>> 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()
> This should not be a problem anymore with backward rasterization
You're not true: This is a basic bug in SplashOutputDev and it does't 
depend on radial shading, so it should be corrected anyway.
>>> b) Splash::pipeRun
>>> If getColor return gFalse, we need to increment ALL pointers, use now the
>>> already existing method pipeIncX
> Cleanup is good, I agree.
>
>>> 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.
> Where does this difference come from?
> Axial and radial shadings work the same way (the formula for the parameter
> is different, but that doesn't affect the ability to antialias the
> gradient, because
> in splash antialiasing is performed by supersampling)
This is history, Gfx turns off vector antialiasing in every shading, the 
only device which supports turning off antialiasing, is SplashOutputDev. 
And probably it was introduced because of the implementation of 
antialiasing in Splash. It was already so in xpdf, the ancestor of 
poppler. When I implemented colorizing text in pattern colorspace I run 
into this problem: when a font with a quite small font weight should be 
filled with a pattern colorspace, I need vector antialiasing, otherwise 
it looks awful. That was one of the reasons why I implemented axial 
shading in Splash, where I switched it on again, s. 
https://bugs.freedesktop.org/show_bug.cgi?id=30436
When I did that in first quarter last year, I also planned to implement 
radial shading in Splash, but I hadn't a good idea how to do that and 
not really time enough. So I started that during my xmas vacation. And 
of course first what I did was turning antialiasing on again. But this 
run into a problem with the PDF Albert mailed today (BTW, we had also in 
some cases problems with antialiasing and axial shading, therefore I 
switched it off if that kind of shading has a bounding box). But here I 
found no parameter in the shading where I can decide whether to let it 
on or not (other than in axial shading), so I decided to switch if off 
again in case of radial shading. You can find that mail in the archives, 
sent on 28th of december.
> AFAICT disabling antialias on radial shadings is just a workaround to some
> cornercases Albert hit. The problem exists with axial shadings as well, so
> we should fix it for both of them instead of working around it for radial and
> leaving it broken for axial.
I agree. But the main idea of my patch was not to solve the antialiasing 
problems but to solve the problems with radial shadings. So let us do it 
step for step instead of rewriting poppler completly :-) . And for me, 
as propably for the most people here, poppler is a private pleasure, so 
I can only help from time to time, when my family agrees :-)

Thomas
>>>>> 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-*).
> I managed to reproduce the same problem with axial shadings in master,
> so it's a problem in the existing code, rather than in radial patches.
>
> This patch solves it for me on the altona test and preserves antialiased
> shadings, which I would prefer it over disabling the antialiasing:
>
> diff --git a/poppler/SplashOutputDev.cc b/poppler/SplashOutputDev.cc
> index 0669f98..2ab8f6d 100644
> --- a/poppler/SplashOutputDev.cc
> +++ b/poppler/SplashOutputDev.cc
> @@ -3495,7 +3495,35 @@ GBool
> SplashOutputDev::univariateShadedFill(GfxState *state,
> SplashUnivariatePat
>    // restore vector antialias because we support it here
>    setVectorAntialias(gTrue);
>    // get the clip region bbox
> -  state->getUserClipBBox(&xMin,&yMin,&xMax,&yMax);
> +  state->getClipBBox(&xMin,&yMin,&xMax,&yMax);
> +
> +  xMin = floor (xMin);
> +  yMin = floor (yMin);
> +  xMax = ceil (xMax);
> +  yMax = ceil (yMax);
> +
> +  {
> +    Matrix ctm, ictm;
> +    double x[4], y[4];
> +    int i;
> +
> +    state->getCTM(&ctm);
> +    ctm.invertTo(&ictm);
> +
> +    ictm.transform(xMin, yMin,&x[0],&y[0]);
> +    ictm.transform(xMax, yMin,&x[1],&y[1]);
> +    ictm.transform(xMin, yMax,&x[2],&y[2]);
> +    ictm.transform(xMax, yMax,&x[3],&y[3]);
> +
> +    xMin = xMax = x[0];
> +    yMin = yMax = y[0];
> +    for (i = 1; i<  4; i++) {
> +      xMin = std::min<double>(xMin, x[i]);
> +      yMin = std::min<double>(yMin, y[i]);
> +      xMax = std::max<double>(xMax, x[i]);
> +      yMax = std::max<double>(yMax, y[i]);
> +    }
> +  }
>
>    // fill the region
>    state->moveTo(xMin, yMin);
>
>> 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 not sure if my patch is an hack as well or if it is the right thing to do.
> It changes the clip computation by ensuring that it is pixel-aligned, but
> there is probably some problem in the splash antialiasing/clipping code.
>
> This patch regresses my radial-gradient just like the patch proposed by
> Thomas (introducing again the overlap between adjacent tiles), but
> preserves antialiasing for all shading types.
>
> Andrea
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler
>
> .
>