[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 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?

Albert

> 
> Andrea
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler