Bug 30436 - Add antialiasing to the splash backend shadings
Summary: Add antialiasing to the splash backend shadings
Status: RESOLVED FIXED
Alias: None
Product: poppler
Classification: Unclassified
Component: splash backend (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Thomas Freitag
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-28 12:33 UTC by Albert Astals Cid
Modified: 2011-07-07 00:30 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Implement axial shading with dynamic pattern (8.21 KB, patch)
2010-10-02 09:18 UTC, Thomas Freitag
Details | Splinter Review
Axial shading extend false (26.59 KB, application/pdf)
2010-10-03 01:36 UTC, Thomas Freitag
Details
Axial shading extend false (10.24 KB, patch)
2010-10-03 01:38 UTC, Thomas Freitag
Details | Splinter Review
another sample for radial shading (68.80 KB, application/pdf)
2010-10-09 07:37 UTC, Thomas Freitag
Details
last sample for radial shading (41.29 KB, application/pdf)
2010-10-09 07:38 UTC, Thomas Freitag
Details
Includes Christian's gouraud shading improvements (39.16 KB, patch)
2010-10-16 09:46 UTC, Thomas Freitag
Details | Splinter Review
Screenshot of the problem (66.37 KB, image/png)
2010-10-18 14:34 UTC, Albert Astals Cid
Details
Compromise for axial shading (39.09 KB, patch)
2010-10-24 04:15 UTC, Thomas Freitag
Details | Splinter Review
correct behaviour if aaBuf is uninitialized (39.52 KB, patch)
2010-11-02 01:32 UTC, Thomas Freitag
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Albert Astals Cid 2010-09-28 12:33:56 UTC
As seen in https://bugs.freedesktop.org/show_bug.cgi?id=2807 there seems be a problem with the "default" Gfx implementation for shading when using antialias, seems the better suggestion is the splash outputdev implementing the shadings by itself.
Comment 1 Albert Astals Cid 2010-09-30 14:44:28 UTC
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?
Comment 2 Thomas Freitag 2010-09-30 23:51:11 UTC
(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
Comment 3 Thomas Freitag 2010-10-02 09:18:57 UTC
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
Comment 4 Thomas Freitag 2010-10-03 01:36:15 UTC
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)
Comment 5 Thomas Freitag 2010-10-03 01:38:45 UTC
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.
Comment 6 Thomas Freitag 2010-10-09 07:37:51 UTC
Created attachment 39304 [details]
another sample for radial shading
Comment 7 Thomas Freitag 2010-10-09 07:38:53 UTC
Created attachment 39305 [details]
last sample for radial shading
Comment 8 Thomas Freitag 2010-10-09 07:45:38 UTC
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.
Comment 9 Thomas Freitag 2010-10-16 09:46:59 UTC
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
Comment 10 Albert Astals Cid 2010-10-18 14:32:34 UTC
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)
Comment 11 Albert Astals Cid 2010-10-18 14:34:33 UTC
Created attachment 39515 [details]
Screenshot of the problem
Comment 12 Thomas Freitag 2010-10-19 01:25:31 UTC
(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
	}
Comment 13 Thomas Freitag 2010-10-19 01:33:32 UTC
(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!
Comment 14 Albert Astals Cid 2010-10-21 15:11:09 UTC
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
Comment 15 Thomas Freitag 2010-10-22 02:47:24 UTC
(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?
Comment 16 Thomas Freitag 2010-10-24 04:15:38 UTC
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.
Comment 17 Albert Astals Cid 2010-10-30 04:38:24 UTC
I just started the regtest, will tell you something as soon as i have some mroe info
Comment 18 Albert Astals Cid 2010-10-31 17:54:54 UTC
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?
Comment 19 Thomas Freitag 2010-11-01 04:13:50 UTC
(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?
Comment 20 Albert Astals Cid 2010-11-01 16:24:08 UTC
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?
Comment 21 Thomas Freitag 2010-11-02 01:32:25 UTC
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.
Comment 22 Albert Astals Cid 2010-11-02 17:33:15 UTC
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.
Comment 23 Thomas Freitag 2010-11-03 01:53:35 UTC
(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.
Comment 24 Albert Astals Cid 2010-11-03 17:05:53 UTC
Wops, you're right, sorry
Comment 25 Albert Astals Cid 2010-11-04 14:01:15 UTC
I've commited it, thanks :-)
Comment 26 Albert Astals Cid 2011-07-06 14:19:40 UTC
Errr, all of this was commited right?
Comment 27 Thomas Freitag 2011-07-07 00:22:36 UTC
(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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.