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

[Andrea Canciani]

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.

Andrea