Bug 2807

Summary: doesn't handle text rendering modes
Product: poppler Reporter: Pablo Rodríguez <freedesktop>
Component: generalAssignee: poppler-bugs <poppler-bugs>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium CC: mpsuzuki, ondrejj, Thomas.Freitag
Version: unspecified   
Hardware: x86 (IA32)   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments: PDF with text rendering modes (on last page)
evince vs. acroread
acroread vs. poppler-0.14.0 rendering
Solves problem in SplashOutputDev
Correction
Consider render value when colorizing text
Patch for CairoOutputDev
The said pdf
Old and new rendering side by side
Patch for PSOutputDev
try to solve antialias of in PDF of comment 28
Use dynamic pattern
Implementation of dynamic axial pattern
Witt vector antialias

Description Pablo Rodríguez 2005-03-25 06:15:37 UTC
poppler seems to have problems to display fine the second line on page 5 of the
document located at http://project.ktug.or.kr/dvipdfmx/sample/omega/gentium.pdf
(see http://bugzilla.gnome.org/show_bug.cgi?id=156700 for more information).

By the way, I guess this bug belongs to poppler (and sorry if I'm wrong), but it
would be interesting that the website contains some guidelines on how identify
whether a given issue belongs to evince, cairo or poppler.
Comment 1 Kristian Høgsberg 2005-04-20 21:45:34 UTC
 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 :-)
Comment 2 Pablo Rodríguez 2005-04-21 10:45:02 UTC
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).
Comment 3 Kristian Høgsberg 2005-05-20 19:14:42 UTC
Created attachment 2730 [details]
evince vs. acroread 

Seems like a problem with uncolored tiling patters (PDF spec, 4.6.2)
Comment 4 Jeff Muizelaar 2005-12-28 05:42:49 UTC
This caused by not supporting patterned fills with text.
Comment 5 Jan ONDREJ (SAL) 2007-02-25 02:33:50 UTC
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?
Comment 6 Brad Hards 2007-12-24 19:16:45 UTC
Appears to still be a problem with current poppler.
Comment 7 Albert Astals Cid 2009-05-01 15:37:33 UTC
Page 5 is fixed by patch on bug 19670, the problem is that patch causes other regressions, so i'm not committing it immediately.
Comment 8 Albert Astals Cid 2009-06-02 14:03:31 UTC
Should be fixed in poppler >= 0.11.1 (once we release it)
Comment 9 Pablo Rodríguez 2009-07-05 05:06:57 UTC
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
Comment 10 Pablo Rodríguez 2010-06-13 11:28:28 UTC
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.
Comment 11 Carlos Garcia Campos 2010-06-16 06:27:39 UTC
(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
Comment 12 Thomas Freitag 2010-06-26 03:03:56 UTC
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
Comment 13 Thomas Freitag 2010-06-26 03:26:26 UTC
Created attachment 36513 [details] [review]
Correction

Made a mistake, this should be better
Comment 14 Thomas Freitag 2010-06-27 00:04:56 UTC
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.
Comment 15 Carlos Garcia Campos 2010-07-13 02:57:19 UTC
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?
Comment 16 Albert Astals Cid 2010-07-13 16:32:15 UTC
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.
Comment 17 Thomas Freitag 2010-07-18 10:49:37 UTC
(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.
Comment 18 Thomas Freitag 2010-07-18 11:27:56 UTC
(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...
Comment 19 Albert Astals Cid 2010-07-24 02:07:16 UTC
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?
Comment 20 Albert Astals Cid 2010-07-24 02:07:58 UTC
Created attachment 37347 [details]
The said pdf
Comment 21 Thomas Freitag 2010-07-25 10:05:37 UTC
(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.
Comment 22 Albert Astals Cid 2010-07-27 15:07:04 UTC
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?
Comment 23 Thomas Freitag 2010-07-31 04:26:21 UTC
(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?
Comment 24 Thomas Freitag 2010-08-01 02:13:32 UTC
(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.
Comment 25 Thomas Freitag 2010-08-01 03:02:38 UTC
(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
Comment 26 Thomas Freitag 2010-08-01 09:06:54 UTC
(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?
Comment 27 Thomas Freitag 2010-08-01 09:45:28 UTC
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.
Comment 28 Albert Astals Cid 2010-08-22 05:04:33 UTC
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.
Comment 29 Thomas Freitag 2010-09-10 02:25:31 UTC
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.
Comment 30 Albert Astals Cid 2010-09-12 14:09:58 UTC
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?
Comment 31 Thomas Freitag 2010-09-18 00:47:45 UTC
(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!
Comment 32 Thomas Freitag 2010-09-19 08:05:10 UTC
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.
Comment 33 Thomas Freitag 2010-09-22 09:13:17 UTC
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 ;.) ?
Comment 34 Thomas Freitag 2010-09-22 23:39:26 UTC
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?
Comment 35 Albert Astals Cid 2010-09-23 01:05:10 UTC
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.
Comment 36 Albert Astals Cid 2010-09-27 14:40:44 UTC
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?
Comment 37 Carlos Garcia Campos 2010-09-28 00:22:57 UTC
(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! :-)
Comment 38 Thomas Freitag 2010-09-28 01:04:10 UTC
(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
Comment 39 Albert Astals Cid 2010-09-28 12:35:44 UTC
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?
Comment 40 Thomas Freitag 2010-09-28 23:27:39 UTC
(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!

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.