Bug 30436

Thomas i'm not sure if you are subscribed to the poppler mailing list (you should if you are not), but it seems to me that Christian is trying to do something similar to what you mention in http://lists.freedesktop.org/archives/poppler/2010-July/006104.html

Can you have a look?

(In reply to comment #1)
> Thomas i'm not sure if you are subscribed to the poppler mailing list (you
> should if you are not), but it seems to me that Christian is trying to do
> something similar to what you mention in
> http://lists.freedesktop.org/archives/poppler/2010-July/006104.html
> 
> Can you have a look?

Okay, I subscribed to the mailing list with my private e-mail address and had a look at the patch Christian works on: as far as I can see, Christian is working on GouraudTriangleShFill, (type 4 and 5), I'm working on axialShadedFill, functionShadedFill and radialShadedFill (type 1, 2 and 3), so there should be no overlap. But to be sure, let Christian have a look in https://bugs.freedesktop.org/show_bug.cgi?id=2807#c33, too

Created attachment 39119 [details] [review]
Implement axial shading with dynamic pattern

This is a refactoring patch of https://bugs.freedesktop.org/show_bug.cgi?id=2807#c33. It should solve every axial shading filling with antialising. Please regtest it.
Regarding the outstanding functionShadedFill and radialShadedFill with antialising: At least for radial shading it could be solved like axial shading, and I've already an idea how it could be done. But I found no samples for it, so is impossible for me to test a possible implementation. Do You have any samples? If You're not sure, just change the return values for functionShadedFill and radialShadedFill of SplashOutputDev in my patch to gTrue and run the regtest again. Every radial and function based filling should them be skipped and therefore cause a regression

Created attachment 39129 [details]
Axial shading extend false

The yesterday uploaded patch causes an regression with the attached PDF, where the shading extend is set to false (shading 2)

Created attachment 39130 [details] [review]
Axial shading extend false

This patch solves the regression of comment 3. This sample PDF uses also radial shading. So I can continue with implement radial shading using dynamic pattern.

Created attachment 39304 [details]
another sample for radial shading

Created attachment 39305 [details]
last sample for radial shading

I tried to implement radial shading filling with dynamic patterns, but I found no easy and more important fast way to figure out, starting wit a x/y position, in which circles this point are part of and find the innermost circle and its color. So I test the latest patch with the samples of comment 6 and comment 7, too, and think that the antialiasing also for radial shadings is good enough. So if the regression test runs through with the patch, I would like to stop my work on it.

Created attachment 39480 [details] [review]
Includes Christian's gouraud shading improvements

I now included Christian's gouraud shading improvements, This patch solves bug 30887, too

There is a problem with the XRGB8 conversions, i get reds instead of blues (i will attach a screenshot). Also on pdf from https://bugs.kde.org/attachment.cgi?id=12625 i get vertical empty lines in the title bar (seems like we are off by one pixel somewhere)

Created attachment 39515 [details]
Screenshot of the problem

(In reply to comment #10)
> There is a problem with the XRGB8 conversions, i get reds instead of blues (i
> will attach a screenshot). Also on pdf from
> https://bugs.kde.org/attachment.cgi?id=12625 i get vertical empty lines in the
> title bar (seems like we are off by one pixel somewhere)

Regarding exchange of red & blue, just change the switch statement in the new function convertGfxColor in SplashOutputDev.cc to following:

	switch (colorMode) {
  case splashModeMono1:
  case splashModeMono8:
	  colorSpace->getGray(src, &gray);
	  color[0] = colToByte(gray);
	  break;
  case splashModeXBGR8:
	  color[3] = 255;
  case splashModeBGR8:
  case splashModeRGB8:
	  colorSpace->getRGB(src, &rgb);
	  color[0] = colToByte(rgb.r);
	  color[1] = colToByte(rgb.g);
	  color[2] = colToByte(rgb.b);
	  break;
#if SPLASH_CMYK
  case splashModeCMYK8:
	  colorSpace->getCMYK(src, &cmyk);
	  color[0] = colToByte(cmyk.c);
	  color[1] = colToByte(cmyk.m);
	  color[2] = colToByte(cmyk.y);
	  color[3] = colToByte(cmyk.k);
	  break;
#endif
	}

(In reply to comment #10)
> There is a problem with the XRGB8 conversions, i get reds instead of blues (i
> will attach a screenshot). Also on pdf from
> https://bugs.kde.org/attachment.cgi?id=12625 i get vertical empty lines in the
> title bar (seems like we are off by one pixel somewhere)

Regarding the vertical empty lines: I'm not sure if I can solve that. It's a general and often seen problem in Splash. Reason for this "new" empty vertical lines is, that the top border is not painted by on square with an axial shading, but it is painted by a lot of smaller squares, every with an axial shading. And every time we hit exactly a pixel, we get now the antialiased line from the left square and continue the the right vertical line from the next square.
You can see that if You change i.e. the resolution to 300 dpi: the empty lines are than sometimes away, sometimes move to another place!

But I'll try to have an additional look at it next sunday. Please continue the regtest to see how often this happens!

Going thorugh the differences, there are lots of files with different (that is better because it's antialiased) rendering.

For the moment i only found another one that gets worse (though i still have 90 files to go :S) https://bugs.freedesktop.org/attachment.cgi?id=21129

(In reply to comment #14)
> Going thorugh the differences, there are lots of files with different (that is
> better because it's antialiased) rendering.
> 
> For the moment i only found another one that gets worse (though i still have 90
> files to go :S) https://bugs.freedesktop.org/attachment.cgi?id=21129

Regarding this second one: It has another reason than the first one, it comes from my first try to implement anti aliasing in gouraud shading. I will have a look at it, too, but if I can't solve this case, I'll propably disable anti aliasing for gouraud shading.
Once again regarding the first PDF: I think about to disable anti aliasing for vertical and horizonzal lines only (and let it for diagonal or curves) in axial shading, but if the first PDF is the only one which has disadvantages with the new behaviour, we should probably commit the patch and think a little bit more about the details. What are You thinking about this?

Created attachment 39664 [details] [review]
Compromise for axial shading

I'd now a deeper look into the PDFs which causes problems. Regarding the first one, I think I found a good compromise: if the shading has a bounding box, I do no antialiasing, otherwise, I do. At least with all my sample PDFs it wotks fine: I got antialiaing where I desired it, and I have no antialiasing if not needed. And even more interesting: the results are looking all the same as if I open them in acrobat reader. Seems, as if the reader does it the same way :-).
I fear, You need to have once again a look at a lot of PDFs after a new regression test, and I can't say, if it is easier to compare differences to my old patch results or if it is easier to compare them with the results without the patch, so sorry for the inconvenience, but hopefully this patch is worth the effort.
Regarding the second, the triangle PDF: there I made a mistake in my patch: I haven't considered the fall back scenarios, so I change that now.
Even if the changes are really small in comparison with the old patch, I attach a complete new patch, because I made changes at several places.

I just started the regtest, will tell you something as soon as i have some mroe info

Regtest seems to be good so far, but i just discovered that Splash::shadedFill uses aaBuf unconditionally and thus crashes if Splash was created without vectorAntialias.

You think you can fix this?

(In reply to comment #18)
> Regtest seems to be good so far, but i just discovered that Splash::shadedFill
> uses aaBuf unconditionally and thus crashes if Splash was created without
> vectorAntialias.
> 
> You think you can fix this?

Great news so far! For my interest, in how many PDF files do You need to look?
Regarding aaBuf, to make a secure fix: just insert into Splash::shadedFill at the very beginning:

  if (aaBuf == NULL) {	// should not happen, but to be secure
    aaBuf = new SplashBitmap(splashAASize * bitmap->width, splashAASize,
			     1, splashModeMono1, gFalse);
    for (x0 = 0; x0 <= splashAASize * splashAASize; ++x0) {
      aaGamma[x0] = splashPow((SplashCoord)x0 /
			       (SplashCoord)(splashAASize * splashAASize),
			     1.5);
    }
  }

and to be sure in Splash::gouraudTriangleShadedFill, please replace 

  if (vectorAntialias) {
    drawAAPixelInit();
  }

through 

  if (vectorAntialias) {
	  if (aaBuf == NULL) {	// should not happen, but to be secure
		aaBuf = new SplashBitmap(splashAASize * bitmap->width, splashAASize,
					 1, splashModeMono1, gFalse);
		for (i = 0; i <= splashAASize * splashAASize; ++i) {
		  aaGamma[i] = splashPow((SplashCoord)i /
					   (SplashCoord)(splashAASize * splashAASize),
					 1.5);
		}
	  }
    drawAAPixelInit();
  }

Or should I upload a new patch?

131 files. All of them show improvements or 1pixel-width changes in rendering (which is acceptable to me). There is a very minor regression in a file (that is 130 MB so i can't attach it, tell me if you want me to send it) but i think there is a net win.

About the aaBuf thing, i think it's the wrong fix, i mean if people did not enable vectorAntialias is because they don't want antialias, so we should not be creating aaBuf, no?

Created attachment 39974 [details] [review]
correct behaviour if aaBuf is uninitialized

Gotcha! I didn't want to upload a patch the nth time, because this shows my shallow work before :-)
Regarding the PDF causes a regression. Do You have a HFER where I can download it? Otherwise, try to mail it to my private e-mail acount, thomas.freitag@kabelmail.de.

I guess if instead of doing

+         GBool vaa = getVectorAntialias();
+         GBool retVal = gFalse;
+         setVectorAntialias(gTrue);
+         retVal = splash->gouraudTriangleShadedFill(splashShading);
+         setVectorAntialias(vaa);
+         return retVal;

we should do something like

GBool retVal = gFalse;
if (getVectorAntialias())
  retVal = splash->gouraudTriangleShadedFill(splashShading);
return retVal;

So if you don't have antialias you get the "old" rendering, bad luck for you.

(In reply to comment #22)
> I guess if instead of doing
> 
> +         GBool vaa = getVectorAntialias();
> +         GBool retVal = gFalse;
> +         setVectorAntialias(gTrue);
> +         retVal = splash->gouraudTriangleShadedFill(splashShading);
> +         setVectorAntialias(vaa);
> +         return retVal;
> 
> we should do something like
> 
> GBool retVal = gFalse;
> if (getVectorAntialias())
>   retVal = splash->gouraudTriangleShadedFill(splashShading);
> return retVal;
> 
> So if you don't have antialias you get the "old" rendering, bad luck for you.

I don't agree: If we do it this way, we can remove the new functionality completely, because before running in this routines in Gfx.cc 

 GBool vaa = out->getVectorAntialias();

  if (vaa) {

    out->setVectorAntialias(gFalse);

  }

is called, so getVerctorAntialias() is always false here. But my change should work: if the calling routines don't want antialias, they create Splash without it, so aaBuf is NULL and so splash->gouraudTriangleShadedFill will return false, we fall back in the old routines.

Wops, you're right, sorry

I've commited it, thanks :-)

Errr, all of this was commited right?

(In reply to comment #26)
> Errr, all of this was commited right?

Yes, of course, and even if not, a new implementation based on it was checked in together with the implementation for radial shading and antialiasing.