Bug 10942

Created attachment 9961 [details]
pdf file showing spurious stripes

Created attachment 10483 [details]
presentation pdf with stripes on gradients

I also see this on two separate setups: 
- using evince 0.8.1 and libpoppler 0.5.4, on Ubuntu Feisty.
- using evince 0.9.1 and libpoppler 0.5.9, on Ubuntu Gutsy.

For me the procedure is:

1. Create a presentation with openoffice
2. Change the background to a gradient
3. Export as PDF
4. Open in evince

Test case file is attached.

This may be related to bug#11165

I'm seeing the same bug in PDF generated by latest cairo.

*** Bug 11165 has been marked as a duplicate of this bug. ***

The problem is that the builtin splash rendering backend doesn't support gradient patterns.  The higher level poppler rendering logic breaks a gradient filled polygon into a series of solid color polygons corresponding to the color bands in the gradient.  When these polygons are rendered in several passes with anti-aliasing, seams occur between the polygons.

The bug is also present with poppler 0.6 on Ubuntu Gutsy. A bit annoying for Beamer presentations...

The patch in bug 14408 may help with this.

I can reproduce this by creating a PDF with a gradient that has transparency in it using Scribus 1.3.3.11 running on Kubuntu Hardy and viewing it in either Okular 0.7 or KPDF 0.5.9.

It seems it adds one stripe for each span between gradient points that are transparent or something.

I'm attaching stripes-from-scribus-gradient.pdf which shows the issue.

Created attachment 18078 [details]
Rect with gradient from Scribus 1.3.3.11

*** Bug 19831 has been marked as a duplicate of this bug. ***

*** Bug 18025 has been marked as a duplicate of this bug. ***

*** Bug 16235 has been marked as a duplicate of this bug. ***

*** Bug 15501 has been marked as a duplicate of this bug. ***

Created attachment 26402 [details] [review]
Implement axialShadedFill() in cairo backend using cairo linear gradient patterns

 poppler/CairoOutputDev.cc |  297 +++++++++++++++++++++++++++++++++++++++++++++                                                                              
 poppler/CairoOutputDev.h  |    6 +                                                                                                                          
 2 files changed, 303 insertions(+), 0 deletions(-)

This patch implements linear gradients in cairo backend. I'm not a cairo guru so I'm not sure it's completely right, it fixes the problem with the first attachment.

(In reply to comment #15)
> Created an attachment (id=26402) [details]
> Implement axialShadedFill() in cairo backend using cairo linear gradient
> patterns
> 
>  poppler/CairoOutputDev.cc |  297 +++++++++++++++++++++++++++++++++++++++++++++ 
>  poppler/CairoOutputDev.h  |    6 +                                             
>  2 files changed, 303 insertions(+), 0 deletions(-)
> 
> This patch implements linear gradients in cairo backend. I'm not a cairo guru
> so I'm not sure it's completely right, it fixes the problem with the first
> attachment. 
> 

The use of cairo looks fine. Would it be better to avoid duplicating the bisection code from Gfx and instead add a new axial shading OutputDev function that provides the coordinates of each stop?

Yeah man, duplicating the code is BAD

Why the current Gfx code is not enough for the Cairo backend?

(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=26402) [details] [details]
> > Implement axialShadedFill() in cairo backend using cairo linear gradient
> > patterns
> > 
> >  poppler/CairoOutputDev.cc |  297 +++++++++++++++++++++++++++++++++++++++++++++ 
> >  poppler/CairoOutputDev.h  |    6 +                                             
> >  2 files changed, 303 insertions(+), 0 deletions(-)
> > 
> > This patch implements linear gradients in cairo backend. I'm not a cairo guru
> > so I'm not sure it's completely right, it fixes the problem with the first
> > attachment. 
> > 
> 
> The use of cairo looks fine. Would it be better to avoid duplicating the
> bisection code from Gfx and instead add a new axial shading OutputDev function
> that provides the coordinates of each stop?
> 

On closer inspection the use of cairo is less than ideal. It would be better to use the actual stops in the shading in the case of linear interpolation (Type 2 Exponential functions with N=1) and fallback to bisection for other cases. This would allow cairo to do the linear interpolation where possible.

(In reply to comment #16)
> 
> The use of cairo looks fine. Would it be better to avoid duplicating the
> bisection code from Gfx and instead add a new axial shading OutputDev function
> that provides the coordinates of each stop?
> 

yes, sure, this patch is just a quick hack to see whether it works. I'll rework the patch and try to implement radial gradients too. 

(In reply to comment #17)
> Why the current Gfx code is not enough for the Cairo backend?
> 

I don't know exactly, but the thing is that it doesn't work, see comment #6. Here is a screenshot showing the difference:

http://people.freedesktop.org/~carlosgc/poppler-cairo-patterns.png

The evince window is using poppler without the patch while the glib demo is using the patch. 

(In reply to comment #17)
> Yeah man, duplicating the code is BAD
> 
> Why the current Gfx code is not enough for the Cairo backend?
> 

Cairo does a poor job of drawing adjacent polygons due to the antialising causing visible seams. I am not aware of any easy fix.

It is a useful optimization for all backends that can natively draw gradients to be able to get a list of gradient stops to draw the gradient instead of drawing a very large number of solid fills.

Created attachment 26432 [details] [review]
Implement axial/radial ShadedFill in cairo backend using cairo gradients

 poppler/CairoOutputDev.cc |   34 ++++++++++++++++++                                                                                                         
 poppler/CairoOutputDev.h  |    8 ++++                                                                                                                       
 poppler/Gfx.cc            |   86 ++++++++++++++++++++++++++++++++-------------                                                                              
 poppler/OutputDev.h       |    1 +                                                                                                                          
 4 files changed, 104 insertions(+), 25 deletions(-)

This patch removes duplicated code and implements radial gradients too.

*** Bug 4330 has been marked as a duplicate of this bug. ***

A couple of PDFs that don't render correctly with the patch:

http://people.freedesktop.org/~ajohnson/linear.pdf
http://people.freedesktop.org/~ajohnson/radial.pdf

You mean they fail with new patch or failed with old patch work with new one?

Both work on Splash BTW

Also 

@@ -2751,12 +2763,13 @@ void Gfx::doRadialShFill(GfxRadialShading *shading) {
   ya = y0 + sa * (y1 - y0);
   ra = r0 + sa * (r1 - r0);
   if (ta < t0) {
-    shading->getColor(t0, &colorA);
+    tt = t0;
   } else if (ta > t1) {
-    shading->getColor(t1, &colorA);
+    tt = t1;
   } else {
-    shading->getColor(ta, &colorA);
+    tt = ta;
   }
+  shading->getColor(tt, &colorA);
 
   // fill the circles
   while (ia < radialMaxSplits) {

looks like an unneeded change

(In reply to comment #25)
> You mean they fail with new patch or failed with old patch work with new one?

yes, they don't work with the patch. 

> Both work on Splash BTW

sure

> Also 
> 
> @@ -2751,12 +2763,13 @@ void Gfx::doRadialShFill(GfxRadialShading *shading) {
>    ya = y0 + sa * (y1 - y0);
>    ra = r0 + sa * (r1 - r0);
>    if (ta < t0) {
> -    shading->getColor(t0, &colorA);
> +    tt = t0;
>    } else if (ta > t1) {
> -    shading->getColor(t1, &colorA);
> +    tt = t1;
>    } else {
> -    shading->getColor(ta, &colorA);
> +    tt = ta;
>    }
> +  shading->getColor(tt, &colorA);
> 
>    // fill the circles
>    while (ia < radialMaxSplits) {
> 
> looks like an unneeded change
> 

tt is used again below, without this change I would have to check again if ta < t1 or ta > t1, and so on. 

I think the problem with the patch is the offset calculation for cairo_pattern_add_color_stop but I don't know how to do it right yet. 

Created attachment 26583 [details] [review]
Implement axialShadedFill in cairo backend using cairo gradients

I only managed to fix linear patterns (thanks to adrianj), so this patch contains only the implementation of axialShadedFill. 

Albert, ok to push this one?

Created attachment 26589 [details] [review]
Attempt to implement radial patterns

Following the same approach than previous patch I've tried to implement radial patterns, but it still doesn't work with the test case provided by Adrian

I'd say this can break PSOutputDev, PSOutputDev::axialShadedFill also returns false in some cases and i'd bet it needs the calls to updateFillColor, fill etc. in this cases to get a correct output, but that would not happen in your case.

Can you please check?

sure, I thought no other backed implemented shaded fills :-( I forgot PSOutputdev. 

Created attachment 27495 [details] [review]
Implement axialShadedFill in cairo backend using cairo gradients

I've added a method useFillColorStop() specific to decide whether to use updateFillColorStop or not, instead of using useShadedFills().

(In reply to comment #31)
> Created an attachment (id=27495) [details]
> Implement axialShadedFill in cairo backend using cairo gradients

applied to git master :-)

Created attachment 28167 [details] [review]
Radial gradients using cairo

This is an updated patch to implement radial gradients using cairo.

Forgot to update radialShadedFill in PSOutputDev 

Fixed in git master.