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

[Albert Astals Cid]

A Dimecres, 26 de gener de 2011, Andrea Canciani va escriure:
> On Wed, Jan 26, 2011 at 8:28 PM, Albert Astals Cid <aacid at kde.org> wrote:
> > A Dimecres, 26 de gener de 2011, Andrea Canciani va escriure:
> >> On Wed, Jan 26, 2011 at 3:45 PM, Thomas Freitag
> >> 
> >> <Thomas.Freitag at kabelmail.de> wrote:
> >> > 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.
> >> 
> >> I see, your patch makes sure that none of the pixels of the transparency
> >> layer is outside the target bitmap. After your explanation I read the
> >> context of that code and I understood it. It is trying to intersect the
> >> bitmap rect with the smallest
> >> integer rectangle containing the input extents. It also seems to require
> >> the output rectangle to be non-empty. I believe that the correct fix
> >> would then be:
> >> 
> >> diff --git a/poppler/SplashOutputDev.cc b/poppler/SplashOutputDev.cc
> >> index 2ab8f6d..6c744b0 100644
> >> --- a/poppler/SplashOutputDev.cc
> >> +++ b/poppler/SplashOutputDev.cc
> >> @@ -3144,23 +3144,23 @@ void
> >> SplashOutputDev::beginTransparencyGroup(GfxState *state, double *bbox,
> >>    tx = (int)floor(xMin);
> >>    if (tx < 0) {
> >>      tx = 0;
> >> -  } else if (tx > bitmap->getWidth()) {
> >> -    tx = bitmap->getWidth();
> >> +  } else if (tx > bitmap->getWidth() - 1) {
> >> +    tx = bitmap->getWidth() - 1;
> >>    }
> >>    ty = (int)floor(yMin);
> >>    if (ty < 0) {
> >>      ty = 0;
> >> -  } else if (ty > bitmap->getHeight()) {
> >> -    ty = bitmap->getHeight();
> >> +  } else if (ty > bitmap->getHeight() - 1) {
> >> +    ty = bitmap->getHeight() - 1;
> >>    }
> >> -  w = (int)ceil(xMax) - tx + 1;
> >> +  w = (int)ceil(xMax) - tx;
> >>    if (tx + w > bitmap->getWidth()) {
> >>      w = bitmap->getWidth() - tx;
> >>    }
> >>    if (w < 1) {
> >>      w = 1;
> >>    }
> >> -  h = (int)ceil(yMax) - ty + 1;
> >> +  h = (int)ceil(yMax) - ty;
> >>    if (ty + h > bitmap->getHeight()) {
> >>      h = bitmap->getHeight() - ty;
> >>    }
> >> 
> >> Your patch guarantees that the extents are within the "bitmap" extents,
> >> this patch also guarantees that they are minimal.
> >> 
> >> (The code to transform a bbox by a matrix is duplicated in
> >> multiple places in poppler :( )
> >> 
> >> >>>> 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.
> >> 
> >> Disabling antialiasing if the pattern has a bbox looks quite arbitrary.
> >> Worse, it looks like your antialias/non-antialias policy is not
> >> sufficient to always get the correct behavior.
> >> 
> >> Albert, can you confirm if the glitch you noticed in altona with radial
> >> shadings also happens with axial shadings?
> >> 
> >> >> 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 :-)
> >> 
> >> I agree, but the issue seems to be that the existing poppler bugs are
> >> stopping the progress of the radial shading improvements.
> >> 
> >> I think that right now the best option is to leave antialiasing enabled
> >> for both axial and radial, push the new radial implementation and file
> >> a bug to keep track of the progress for the clipping problem.
> >> If we need a temporary workaround for altona, we might clip to the
> >> integer coordinate rect as in the patch in my previous email, but my
> >> understanding of the problem is that this would just make the bug not
> >> visible for (some?) axial/radial shadings, but it would not actually fix
> >> it.
> >> 
> >> Another option would be to disable antialiasing for splash shadings,
> >> but this would obviously worsen their quality.
> > 
> > Got a little confused here with messages coming to my private mail
> > address and to here. You want me to apply the patch that you sent here
> > or the one you sent to me only before running the regtest again?
> 
> The two patches are unrelated.
> 
> I believe that the attached patchset should do quite well in the regtest.
> 
> Unfortunately 0005 is just a work around, but I think it is to be
> preferred to disabling
> antialiasing because it provides higher quality.
> 
> I'd love to know other regressions, but please notice that most of
> them are unlikely
> to be caused by the radial shading implementation.

The altona file shows a vertical line that wasn't present before.

Albert

> 
> Andrea
> 
> PS: did you manage to reproduce the problem with axial shadings on master?