Bug 90570

Created attachment 115961 [details]
incorrect output with diagonal line artifacts

output from
pdftoppm -png 203170.pdf 203170
The png is 1/10 the size of the ppm and shows the problem also.
The black diagonal lines show in all output formats, so the problem is in the splash rasterization and not in the final image file generation.
The lines seem to be one pixel in the image, no matter what I use as the resolution, so maybe a fill or crop calculation is not symmetrical and two adjacent regions end up with an unfilled gap between them.
xpdf-3.04 works correctly, but the splash code has diverged enough that it is hard to compare.  vectorAntialias is used mostly in Splash.cc.
This PDF uses Splash::blitImageClipped, Splash::fillWithPattern and Splash::gouraudTriangleShadedFill.

The problem happens only when I build with -DSPLASH_CMYK=1
Enabling vector antialias without SPLASH_CMYK or enabling SPLASH_CMYK without vector antialias is not sufficient.  Both must be enabled to show the problem.
colorMode is splashModeRGB8, so only a small amount of CYMK code like SplashOutputDev::setOverprintMask() is active.

I did a bisect. The first bad commit is fa7c41cb9c52ecd3d7c574455b1258a3021b8c75 Improvements to the splash backend

Reviewing the changes in the commit identified by git bisect, I found that I could fix the problem by commenting out the lines
  if (out->useShadedFills( shading->getType())) {
    if (out->gouraudTriangleShadedFill( state, shading))
      return;
  }
in Gfx::doGouraudTriangleShFill() in Gfx.cc.

That is not a good solution because gouraudTriangleShadedFill() is an important optimization, but at least it narrows down the problem.

The image is a rectangle with a shaded fill background that is light in the center and dark at the edges.
gouraudTriangleShadedFill() processes it as four triangles.
Each triangle has the center as a vertex and one of the edges as a side.
The artifacts in the image correspond to the sides of the triangles from the vertex to each of the corners.

The loop through the triangles in Splash::gouraudTriangleShadedFill() is the problem.  The loop does the calculation correctly and then calls shading->getParameterizedColor() to write the color into bitmapData[].

SplashGouraudPattern::getParameterizedColor() in SplashOutputDev.cc calls convertGfxColor().

splashColorCopy() in SplashTypes.h always copies 8 bytes if SPLASH_CMYK is enabled.  Since the bitmap in my test file is RGB8, it should copy only 3 bytes.
Since each row of the triangle is filled left to right, the overwrite is not a problem because any overwrite in data[X] is fixed in the next time around the loop for data[X+1].  But any overwrites by the rightmost cell of the last triangle do not get fixed by later data so the overwrite leaves 0's in some of the color values.  Those 0's create the artifacts.

Created attachment 116004 [details] [review]
patch to fix the problem

This patch creates an alternate version of convertGfxColor() called convertGfxShortColor() that copies only the necessary color bytes according to the colorMode.

SplashGouraudPattern::getParameterizedColor() now uses convertGfxShortColor() instead of convertGfxColor() to avoid corrupting bitmap data.

This fixes the bug and also makes splash slightly faster because it now copies only the necessary bytes instead of always copying 8 bytes and it now writes the bytes directly to the destination instead of writing them to a buffer and then copying the buffer with splashColorCopy().

Splash::gouraudTriangleShadedFill() in Splash.cc has a lot of tests to ensure the processing order and an uncharacteristically large number of assertions.
Is it possible that the author of that procedure hit the bug in SplashGouraudPattern::getParameterizedColor() + convertGfxColor() but thought that the problem was in gouraudTriangleShadedFill() and discovered that forcing a left-to-right processing order worked?
Are all of the swaps necessary for the algorithm, especially the swap of X limits with the comment "FIXME I'm sure there is a more efficient way to check this."?

(In reply to William Bader from comment #5)
> Created attachment 116004 [details] [review] [review]
> patch to fix the problem
> 

Looks good for me. And it solves also a potential memory corruption in this area.

(In reply to William Bader from comment #6)
> Splash::gouraudTriangleShadedFill() in Splash.cc has a lot of tests to
> ensure the processing order and an uncharacteristically large number of
> assertions.
> Is it possible that the author of that procedure hit the bug in
> SplashGouraudPattern::getParameterizedColor() + convertGfxColor() but
> thought that the problem was in gouraudTriangleShadedFill() and discovered
> that forcing a left-to-right processing order worked?
> Are all of the swaps necessary for the algorithm, especially the swap of X
> limits with the comment "FIXME I'm sure there is a more efficient way to
> check this."?

The author of gouraudTriangleShadedFill() was Chistian Feuersänger, cfeuersaenger@googlemail.com, but I'm not sure if this email address still is correct. Last time Christian answered us was 2012....

Thanks for looking at the patch.

I experimented with the fill a few days ago, and I now think that changing the calculation of the limits would not help because the inner loop for X becomes more complicated if it has to deal with going in either direction.

Is it worth removing the asserts from the inner loop or is there a configure option to disable them?
  assert(fabs(colorinterp - (scanColorMap[0] * X + scanColorMap[1])) < 1e-10);
  assert(bitmapOff == Y * rowSize + colorComps * X && scanLineOff == Y * rowSize);

William

Now that 0.34 is closed, can you consider adding this patch?
It fixes a memory overwrite that can cause bad output.

I can't reproduce the output with the diagonal lines, it just renders fine.

Any idea why that may be?

The problem happens only when you build with cmyk support because splashColorCopy() in splash/SplashTypes.h always copies 8 bytes even if the current color mode in convertGfxColor() in poppler/SplashOutputDev.cc has fewer than 8 bytes per pixel.

Without cmyk support, it still always copies 3 bytes, so it might show the problem on color modes with only 1 byte per pixel, for example "pdftoppm -gray 203170.pdf x".

Pushed.