Bug 2807

 I couldn't download the PDF you linked to - if it's not too big (< 1MB) could
you attach it to the bug?  Thanks.

As for identifying wether a bugs is in evince, poppler or cairo; yeah, that's
not always obvious, but UI bugs are typically evince and buggy PDF rendering is
typically poppler.  You might hit a cairo bug, but I'd says that's uncommon. 
Anyway, as long as you file against what you think is the right component,
that's helpful.  We can always pass the hot potato around :-)

Created attachment 2496 [details]
PDF with text rendering modes (on last page)

Here you have the PDF document attached. Compare last page display from
acroread and evince (or poppler).

Created attachment 2730 [details]
evince vs. acroread 

Seems like a problem with uncolored tiling patters (PDF spec, 4.6.2)

This caused by not supporting patterned fills with text.

I think I have similiar problems with some PDF files. They are using similiar effect to display prices with a border. Using poppler aprox. 1/3 of page is black and I can't view page properly. "gv" (ghostview?) displays this PDF properly. Is it possible to backport this feature?

Appears to still be a problem with current poppler.

Page 5 is fixed by patch on bug 19670, the problem is that patch causes other regressions, so i'm not committing it immediately.

Should be fixed in poppler >= 0.11.1 (once we release it)

I'm afraid this is not fixed yet, since text rendering modes are fine, but the external line is missing in poppler.

Would you be so kind to fix it also?

BTW rendering speed is much slower in with poppler than with acroread9. But this is just a comment.

Thanks for your help,


Pablo

Created attachment 36253 [details]
acroread vs. poppler-0.14.0 rendering

The external outline is missing in poppler.

BTW, gtk-cairo-test from poppler-0.14.0 refuses to load the file. I don't know why.

(In reply to comment #10)
> Created an attachment (id=36253) [details]
> acroread vs. poppler-0.14.0 rendering
> 
> The external outline is missing in poppler.

Yes, this is because when using a pattern to draw text we are always assuming the text rendering mode is 0 (fill text) and doPatternFill is called. In this case the rendering mode is 2 (fill and stroke). We are indeed saving the rendering mode before changing it to 7, but we are not using it after text has been added to the path for clipping. 

Thomas, could you take a look at it, please?

> BTW, gtk-cairo-test from poppler-0.14.0 refuses to load the file. I don't know
> why.

It works for me, remember you need to use file:// with gtk-cairo-test

Created attachment 36512 [details] [review]
Solves problem in SplashOutputDev

This patch should solve the regression. Can You please regtest it, if it works I will make simular changes in PSOutputDev

Created attachment 36513 [details] [review]
Correction

Made a mistake, this should be better

Created attachment 36528 [details] [review]
Consider render value when colorizing text

Thinking a little bit more about this PDF I recognized that I should consider the actual text render mode:
If (render & 4) filling text is already done by PDF commands and should not be done by manually filling it with the pattern colorspace.
Only if !(render & 4) and actual colorspace is pattern colorspace we should fill it manually.
That also means that changing the rendering mode should change this behaviour. Therefore I redesigned the code a little bit. This now should work in a more general way.
Once again: If this change will run through Your regtest, I'll have a look at PSOutputDev again.

Created attachment 36989 [details] [review]
Patch for CairoOutputDev

Thomas' patch works for me with cairo backend after applying this patch. Albert, could you run the regressions test, please?

Yes, i need to run the regtest for both splash and cairo outputdevices but i will be on holiday until next tuesday so it'll have to wait, sorry about it.

(In reply to comment #16)
> Yes, i need to run the regtest for both splash and cairo outputdevices but i
> will be on holiday until next tuesday so it'll have to wait, sorry about it.

In the meantime I made a patch for PSOutputDev, too. But gs 8.71 crashes with this page with segmentation fault. It crashes also in the former version, and it crashes also if I remove the text in pattern colorspace from the output. The reason seems to be the pattern colorspace itself, which is also used for drawing the background. If I remove that background, too, it is displayed. I'll ask a collegue of mine, when he comes back from holiday, if he can help.

(In reply to comment #17)
If I redefine useTilingPatternFill in PSOutputDev.h, i.e.
  virtual GBool useTilingPatternFill() { return gFalse; }
my patch works with this PDF, i.e. gs 8.71 displays it correctly. So its definitely a problem how tilingPatternFill is defined in PSOutputDev.cc in combination with gs 8.71. Has anyone of You an idea, what's going wrong? I mean that I can remember there were problems with using of cache devices in PostScript...

I found a small regression in the pdf that i will attach, the characters in the lower right e.g. "VOGL WEST" seem to have lost antialiasing. Can you have a look?

Created attachment 37347 [details]
The said pdf

(In reply to comment #19)
Ups, one of the PDF I upload myself several month ago. Yes, I can see the problem, and yes, the whole text block in the lower right incl. the phone numbers are drawn in pattern colorspace. So I tried to go back to the former version, but I can't see any difference in the output. And as far as I can see it is a problem of the way I draw text in pattern colorspace: I make a path and then fill it, but filling is not able to do antialiasing, is it? In short words, I have no really good idea to solve this regression.

Created attachment 37416 [details]
Old and new rendering side by side

You mean you don see the difference in rendering in the red highlighted areas between the stable version and the version with your patch applied?

(In reply to comment #22)
> Created an attachment (id=37416) [details]
> Old and new rendering side by side
> 
> You mean you don see the difference in rendering in the red highlighted areas
> between the stable version and the version with your patch applied?

I wanted to do the changes step by step to find out, where the regression comes from. So I fetched the actual version with git, but NOW the complete text block lower right is painted white. So I give up for the moment. Can You send me a link to the version which You use to show me the differences?

(In reply to comment #22)
Forget it, Albert. When I went back to old source, I still used SPLASH_CMYK. Now I change it to SPLASH_RGB, and the output is like Yours. Reason for this strange behaviour is a bug in the old source interpreting render mode:
1. The text should be rendered in pattern colorspace.
2. After begin text, there is a "0 Tr", which changes rendering mode from 7 (caused by pattern colorspace) back to 0.
3. Now drawing characters is done again with splash->fillChar instead of setting a clipping path, and this uses color 0, therefore in SPLASH_CMYK it become white, but in SPLAH_RGB black, but with text antialising, which looks much better.

The actual patch now correct this, so drawing characters still is done by setting clipping pathes and later fill it with the pattern colorspace, but unfortunately here we have, as I already mentioned, no antialising, so the output looks worser, even if the behaviour now is correct. 

You would have seen that, if the pattern colorspace uses for example red as base color, without the patch the text still would be paint black, with the patch it would be paint red, but looses the antialising.

So what I have to do (I'm not sure, if I'm really able to do it correctly) is to use antialising when filling text clip pathes with pattern colorspace.

(In reply to comment #22)
After encountered the problem, s. commet 24, it was quite easy to find the reason during debugging in Gfx::doShadingPatternFill:

#if 1 //~tmp: turn off anti-aliasing temporarily
  GBool vaa = out->getVectorAntialias();
  if (vaa) {
    out->setVectorAntialias(gFalse);
  }
#endif

Do You know, why this was done? If I remove this lines and the corresspondig later turn on, everything works fine. So do You have any idea, where this comes from? If it is still needed, I could implement an additional parameter which turn anti-aliasing only off if it is not a text path fill. Otherwise we should remove it or chnage it to #if 0

(In reply to comment #25)
If Your are not sure if it is possible to remove it, and to avoid other regressions, just do the following:

#if 1//~tmp: turn off anti-aliasing temporarily
  GBool vaa = out->getVectorAntialias();
  if (vaa && !textHaveCSPattern ) {
    out->setVectorAntialias(gFalse);
  }
#endif

I tested it with several PDF with text in pattern colospace, and it seems to work! Because its only a minor change, I don't upload a new patch for the moment, should I?

Created attachment 37504 [details] [review]
Patch for PSOutputDev

Even my collegue hasn't yet the time to have a look at it, and because I already thought that the gs crash is caused by the use of setcachedevice, I had again a look into the code and see, that there was already an alternative in using setcharwidth. So I commented out the lines which uses setcachedevice and saw, that it works. So I decide to upload a patch for it now.

Unfortunately enabling antialias causes weird artifacts in page 2 of http://bugsfiles.kde.org/attachment.cgi?id=8847

So we have four options here:
 1) Not accept the patch
 2) Accept the patch without the antialias fix
 3) Accept the patch with the antialias fix
 4) Fix the artifacts when antialias is enabled

Obviously the best thing would be 4) but not sure it can be easily done.

Created attachment 38596 [details] [review]
try to solve antialias of in PDF of comment 28

With this patch I now try to fix the artifacts when antialias is enabled. At least I found the code, where the weird artifacts came from:
Reason for this is that antialising is done on the clipping path, and when filling is done in small pieces, we got small clipping pathes but a lot of them. Therefore the "inside" lines of the small clipping pathes should not be antialiased, only the outer lines from the clipping path of the complete fill area. I think, that was the reason to introduce the lines of comment 25: the person who insert them wanted to get rid of the misbehaviour for the "inner" clipping pathes and approved the loss of antialising of the "outer" clipping pathes.
I myself have only an idea, how antialising is really done in splash, so I made a hack for the last line of the fill area in case of filling text: instead of using the antialiased last line (which is wrong in this case), I duplicated the line before last. This is not really correct, and perhaps someone else with a deeper knowledge can do it in a better way in future, but it was the easiest way to solve it without making to much changes, and in my opinion the firefox text looks much better than the one which was produced without antialiasing, and also the other PDF with text in pattern colorspace I tested are almost looking better than before.

Still causes some regressions, for example the "Segurança no internet" text in https://bugzilla.gnome.org/attachment.cgi?id=148574

We are stuck between a rock and a hard place :-/

I think we could go with "Accept the patch without the antialias fix" and leave for the future fixing antialias shadings in the splash backend.

What you guys think?

(In reply to comment #30)
> Still causes some regressions, for example the "Segurança no internet" text in
> https://bugzilla.gnome.org/attachment.cgi?id=148574
> 
> We are stuck between a rock and a hard place :-/
> 
> I think we could go with "Accept the patch without the antialias fix" and leave
> for the future fixing antialias shadings in the splash backend.
> 
> What you guys think?

"Segurança no internet" uses shading from left to right, the bug fix I implemented considers always shading from top to bottom, therefore it doesn't work for this PDF. The patch without the antialias fix works for PSOutputDev and CairoOutputDev because these devs implement the shading routines themselves. Therefore I think that You should accept the patch without the antialias fix first.
As I figured out, SplashOutputDev is the only device which doesn't implement the shading routines and use the Gfx algorithm with switching antialias off. In filling the area in small pieces, it is hard to decide wether a clipping line is a "inside" line without antialias or if it is an outer line where antialias should be done. Therefore I think the clearest and understandable way is to implement the shading routines in SplashOutputDev too.
This could be done in three steps:
1. Implement the shading algorithms in SplashOutputDev, too, by copying the algorithms from Gfx without antialising.
2. Implement the shading routine used by the PDFs of this bug with antialias in SplashOutputDev. It is easier to decide if it is outer or inner line in SplashOutputDev when we look at the complete clipping path.
3. Implement the other two shading routines with antialias in SplashOutputDev, too.
Without starting the work I can't estimate if implementing this steps will take only a few hours or some days, but I would think that doing step 1 should be quite easy.
So if You agree, accept the patch without the antialias fix and open an new bug and assign it to me. Then I'll start to implement it in my free time!

Created attachment 38797 [details]
Use dynamic pattern

I started in some more investigation, and found some interesting code fragments in splash: there is some so called dynamic pattern, where when filling with such a pattern the actual color at a coordinate is fetched from this pattern with getColor(), and if such a pattern for i.e. axial shading exists, the whole including antialias could be done with a simple splash->fillWithPattern().
So the only effort to do is to implement dynamic patterns (which were prepared, but never implemented, so there exists no sample) for i.e. axial shading and "convert" the GfxShading to a SplashShadingPattern.
I spent some hours to start with it, and implement an shading pattern which assumes that shading is done from left to right and starts at the upper left corner and ends at upper right corner, so the result is not exact what I want, but it shows that it works and antialias is done correctly. So I attach the result, knowing I need some more days to convert at least a GfxAxialShading to the new implemented SplashAxialPattern.

Created attachment 38879 [details] [review]
Implementation of dynamic axial pattern

This is now the first implementation of a dynamic pattern to do axial shading, so it does step 1 and 2 of comment 31.
For functionShadedFill and radialShadedFill I just copied the routines from Gfx and made smaller changes so that we can compile it. It would be better to implement dynamic pattern for them, too, but because I have no samples to test this, I skip that for the moment.
Of course this could cause some new regression (Indeed, I hope that: then You can send me the PDFs and I could implement the dynamic pattern for them and test it)!

What do You think? Are we still stuck between a rock and a hard place? Or do we cut it ;.) ?

Created attachment 38894 [details]
Witt vector antialias

I insert a new line in SplashOutputDev::axialShadedFill, just at the beginning:
setVectorAntialias(gTrue);
Then also the non text element with axial shaded fills are antialiased, see for example the black shaded circle. It looks nicer, isn't it?

It looks real good.

At the moment i'm running regression testing against the cairo and ps outputdev patches and if they work i'll commit the old splash patch (the one that did not use antialias) with the cairo and ps outputdev patch.

After that iĺl have a look + start regressions at the good work you are doing with the antialias in the splash outputdev.

I've commited splash, cairo and ps outputdev changes to master i think we should close this bug and open a separate one for the new changes Thomas is working on.

Do you guys agree?

(In reply to comment #36)
> I've commited splash, cairo and ps outputdev changes to master i think we
> should close this bug and open a separate one for the new changes Thomas is
> working on.
> 
> Do you guys agree?

Sure, great work Thomas! :-)

(In reply to comment #36)
> I've commited splash, cairo and ps outputdev changes to master i think we
> should close this bug and open a separate one for the new changes Thomas is
> working on.
> 
> Do you guys agree?

Just send me the new bug id. I'll insert then the new implemented axial shading and will try to implement functionShadedFill and radialShadedFill in SplashOutputDev, too

https://bugs.freedesktop.org/show_bug.cgi?id=30436

Do you want me to start a regression test against https://bugs.freedesktop.org/attachment.cgi?id=38879 or better wait?

(In reply to comment #39)
> https://bugs.freedesktop.org/show_bug.cgi?id=30436
> 
> Do you want me to start a regression test against
> https://bugs.freedesktop.org/attachment.cgi?id=38879 or better wait?

I'll first checkout the commited sources, then I'll make a new patch, propbably with already implemented functionShadedFill and radialShadedFill. Let us continue this in Bug 30436!