A Dimecres, 26 de gener de 2011, Andrea Canciani va escriure: > On Wed, Jan 26, 2011 at 8:28 PM, Albert Astals Cid <[email protected]> wrote: > > A Dimecres, 26 de gener de 2011, Andrea Canciani va escriure: > >> On Wed, Jan 26, 2011 at 3:45 PM, Thomas Freitag > >> > >> <[email protected]> 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? _______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
