[poppler] Followup Bug 32349 & Poppler: More shading fun ;-)
Andrea Canciani
ranma42 at gmail.com
Wed Jan 26 06:01:29 PST 2011
On Wed, Jan 26, 2011 at 11:48 AM, Andrea Canciani <ranma42 at gmail.com> wrote:
> 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()
This should not be a problem anymore with backward rasterization
>> 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)
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.
>>>>
>>>> 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
More information about the poppler
mailing list