80190 - [ps] Poor Postscript Rendering

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Michael B. Allen]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.01; Windows NT; SYSCERT001)
BuildID:    2001050521

When printing this page: http://users.erols.com/mballen/bugzilla/out.html which 
uses the css:
http://users.erols.com/mballen/bugzilla/style.css I get this: 
http://users.erols.com/mballen/bugzilla/out.ps some of the vertical lines 
between cells are missing.

Reproducible: Always
Steps to Reproduce:
1. Go to: http://users.erols.com/mballen/bugzilla/out.html
2. Print it to a printer or to a ps file.
3. Look closely at the vertical lines inbetween cells; some are missing.

Actual Results:  As you can see some of the vertical lines within cells are not 
showing. I know this is nit-picking but I suspect it's just an artifact of 
something bigger.

Expected Results:  All lines between cells should be rendered properly.

NOTE: In http://users.erols.com/mballen/bugzilla/style.css only the right and 
bottom cell line weights are specified(Specifying a weight for the right wall 
of a cell and specifying a line weight for the left wall of its adjacent cell 
would be redundant). I bet this has something to do with it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is probably just a rounding error somewhere... 

Comment 2

[Jim Crumley]

I can confirm this with build 2001071722 - under Sparc Solaris.

Take a look at attachment
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=42756 on 
bug 84149 (an unrelated printing bug) for another example of this
behavior.  That attachment is printed from
http://ham.space.umn.edu/crumley/mozilla/frame.html .

Notice that besides some inter-cell lines being missing, the lines that
are supposed to surround the entire table are missing.

Comment 3

[Keyser Sose]

Marking NEW.

Comment 4

[Roland Mainz]

Reporter:
Your examples are "gone" - can you please attach them (or new ones) to this bug,
please ?

Comment 5

[Michael B. Allen]

I have created new examples. The out.ps example was generated with Mozilla 
0.9.9 Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.9) Gecko/20020314. All 
links should work but for your convenience I have embedded what is in style.css 
directly into out.html.

I can see there has been some improvement but the generated PostScript is 
clearly using a "brute force" technique rather than taking advantage of the 
Gecko's rendering. Is it not feasible to switch the underlying drawing 
implementation to one that accommodates the drawing primitives required by 
Gecko with PostScript spewing equivalents?

Comment 6

[Roland Mainz]

Michael B. Allen wrote:
> Is it not feasible to switch the underlying drawing 
> implementation to one that accommodates the drawing primitives required by 
> Gecko with PostScript spewing equivalents?

Uh, uhm... we have such a thing already... it's called "Xprint" ... :)

Comment 7

[Roland Mainz]

Details how to install/use Xprint are available under http://xprint.mozdev.org/
or http://puck.informatik.med.uni-giessen.de/people/gisburn/work/xprint/

Comment 8

[Roland Mainz]

Reporter:
The test case URL (http://users.erols.com/mballen/bugzilla/out.ps) is a
PostScript file... can you replace it with the HTML "source" URL, please ?

Comment 9

[Michael B. Allen]

See the links in my original report: 
http://users.erols.com/mballen/bugzilla/out.html 

Comment 10

[Roland Mainz]

Attachment: http://users.erols.com/mballen/bugzilla/out.html printed with 2002-05-08-08-trunk + Xprint module (DIN A4, 300 DPI)

Reporter:
Does this PostScript job print OK for you ?

Comment 11

[Michael B. Allen]

Oh, I just noticed you said with Xprint. Yes, I experimented with Xprint after 
our private dialog and the printouts do look considerably better than the 
default PostScript module. I prepared a page with samples here:

  http://users.erols.com/mballen/xprint/

Comment 12

[Michael B. Allen]

Err, I thought I answered this but I guess the commit wasn't successfull. Maybe 
I hit reset instead.

Anyway, regarding Comment #10; yes, I am able to print ok with RC1 using the 
default PostScript module. Some of the tops of characters get a little cut off 
though an the output isn't what I would call "great". But it does print all the 
cell borders properly unlike the original issue. The padding might be off 
though. Here are samples of that RC2 Letter 300 DPI output:

http://users.erols.com/mballen/bugzilla/outrc2.ps
http://users.erols.com/mballen/bugzilla/outrc2.pdf

which I think is pretty much identical to the 0.9 out.ps submitted earlier. 
Incedentally I got these 'NO FONT WAS FOUND' messages when I did this:

[miallen@miallen3 miallen]$ export XPSERVERLIST=
[miallen@miallen3 miallen]$ /usr/local/mozilla/mozilla
 NO FONT WAS FOUND 
 NO FONT WAS FOUND

Maybe it doesn't like the Verdana I have in the stype sheet.

Also, here's the equivalent Xprint output just to compare as long as we're on 
that topic:

http://users.erols.com/mballen/bugzilla/outrc2xp.ps
http://users.erols.com/mballen/bugzilla/outrc2xp.pdf

Comment 13

[Pete Zha]

Attachment: output from ps module of mozilla1.3a (.gz file)

Reporter, I don't see any different between the output and the html page. Can
you check this output and finger out the problem?

Comment 14

[Peter Brueckner]

Attachment: patch for the bug

the poor rendering is a result of rounding errors from
PAGE->DISPLAYPIXEL->POINTS->DEZIPOINTS->POINTS specially the calculation
with DISPLAYPIXEL (Transform) is useless i think. The result was: all
coordinates were integers (or .9). The solution is to calculate everything
with float. (Why float instead of double wherever i look?)

Comment 15

[tor]

Attachment: update Peter Brueckner's patch to tip

Thanks for this patch - I've updated it to the current trunk.

Comment 16

[Kenneth Herron]

Rather than just converting all of these operations to floating point, how about
changing the document's coordinate space to the nscoord (twip) scale? It seems
this would allow removing most of the coordinate conversions from mozilla, which
would make the code much simpler to follow.

Comment 17

[Kenneth Herron]

I'll take this. I have a patch almost ready for review.

Comment 18

[Kenneth Herron]

Attachment: Generate PS using layout's native coordinate system

This patch removes most of the coordinate scaling and transformations from the
PS module. The generated postscript uses the same coordinate system as layout,
i.e. scaled to twips (1/20th of a point) with the origin in the upper left
corner.

I've tested the sample URL with this patch and the rendering problems are
fixed. The new PS also renders much faster, at least using ghostview. I also
check-tested bug 195206, another pixel-rounding bug; it seems to be fixed as
well.

I've also included a fix for bug 194830 and bug 211259 (clipping rectangle is
visible). Finally, I've removed some unused code and added a few comments.

Comment 19

[Boris Zbarsky [:bzbarsky]]

How come you removed RenderPostScriptDataFragment()?  Won't that break plug-in
printing?

Comment 20

[Kenneth Herron]

Nothing in mozilla calls RenderPostScriptDataFragment(). I didn't research well
enough and assumed it was dead code that could be removed. I can update the
patch in a day or so.

I must say that it seems the description for RenderPostScriptDataFragment().
could stand improvement. It mentions setting up a clipping region first, but in
fact the caller has to do something like:

    PushState();
    SetClipRect(...);
    RenderPostScriptDataFragment();
    PopState(...);

Otherwise, the new clipping rectangle and the coordinate system changes will
affect the rest of the page. Even so, the caller can alter the interpreter's
current state (e.g. insert new functions into the dictionary). Beyond that, it's
up to the caller to know what part of the page it's supposed to be drawing on. I
suppose it gets that information from layout somehow. But in all, this seems
difficult to use properly.

PS has a standard for fragments that can be embedded in other pages, called ESP
(encapsulated postscript), which addresses issues like these. How mature is the
existing API? Could we rip it out and replace it with EPS support?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Nothing in mozilla calls RenderPostScriptDataFragment()

nsObjectFrame::Paint() does... The basic idea is to allow a plugin to render
itself as postscript and just dump into the print output.  See bug 198515 for
details.

Comment 22

[Kenneth Herron]

Attachment: Native coordinate system v2

This restores the RenderPostScriptDataFragment() function I incorrectly
removed. 

I've also attempted to clarify the header documentation for this function,
based on my understanding of how the function may be used safely and the kind
of PS environment that it provides.

Comment 23

[Kenneth Herron]

Attachment: Native coordinate system v3

Hmm, the image-printing functions had some out-of-date coordinate handling. It
still ended up with the corrected result, but it was very misleading.

Comment 24

[Kenneth Herron]

Comment on attachment 132521 [details] [diff] [review]
Native coordinate system v3

I"m withdrawing this. The line-drawing routines still aren't placing the lines
where layout expects them to be, which is the heart of this particular bug.

Comment 25

[Kenneth Herron]

Attachment: Twip-based coordinate system v4

This corrects the fundamental problems, of which there were two:

1) The line-drawing function takes two x,y endpoints and renders a line between
them. PS was drawing the actual line centered over the geometric line, where it
should have drawn the line to the right (vertical) or below (horizontal) the
geometric line. It was also drawing the line two twips wide where it should
have been one scaled pixel, or more like 14-16 twips.

2) When PS converts a rendered object from the document's coordinate system
("user space") to the output device's actual pixel grid ("device space"), it
colors any pixel touched by the object, no matter how small the overlap. So two
objects can be next to each other in user space but overlap in device space. In
fact, layout will sometimes draw the black lines around a table cell, then draw
a white rectangle painting the interior of the cell. Due to coordinate system
conversion the white rectangle will take out some of the black pixels.

I've introduced a postscript routine for drawing rectangles. It transforms the
rectangle's coordinates into device space, aligns them to pixel boundaries,
then transforms them back into user space for drawing purposes. Horizontal and
vertical lines are also drawn as filled rectangles (instead of stroked lines)
so they're subject to the same process.

This seems to correct the line-drawing problems. Thin rectangles, such as link
underlines, look inconsistent when viewing a document using ghostview, but this
is due to the low resolution.

This patch also contains some other improvements:

1) Because of the inverted coordinate system, it's necessary to invert fonts as
well. Previous versions of this patch did this at font-scaling time using the
makefont operator. This version inverts the font once at the beginning of the
document.

2) Previous behavior was to set up fonts using the level one findfont,
scalefont, and setfont operators. This patch makes uses of the level two
selectfont operator, which is much more efficient. Selectfont is emulated if
it's not available so there should be no compatibility impact.

3) I've removed about 70 lines of unused PS boilerplate.

4) The level 2 PS automatic stroke adjustment feature is enabled if it's
available.

Comment 26

[Kenneth Herron]

Nominating for 1.6a. This is a major overhaul to the postscript printing
subsystem which fixes several bugs. It would be nice to get some widespread
testing as early as possible.

Comment 27

[tor]

Looks good.  A couple minor things:

  * grayimage and color image should probably be unified, since they
      duplicate much of their code.  Mainly noticed this due to your
      addition of the zero scale code in both places.  Not a blocker
      for this patch, but a future cleanup.

  * if SetClipRect falls through into the error case, it will try
      setting the clip with a null path.  Experimenting in gs, this
      seems to mean everything will be clipped.  Is that the behavior
      we want?

  * the "// doesn't it seem..." comment in DrawScaledImage no longer
      applies.

Comment 28

[Asa Dotzler [:asa]]

Time is short for 1.6a. We freeze tonight at midnight and hope to have a release
in the next few days.  This needs to land quickly if it's going to make it. 

Comment 29

[Kenneth Herron]

Attachment: Twip-based coordinate system v5

* Return a suitable error for unrecognized aClipCombine values.

* Removed the "doesn't it seem" comment.

I'd rather clean up the image functions another time; this bug contains enough
out of scope work as it is.

Comment 30

[Kenneth Herron]

Comment on attachment 133792 [details] [diff] [review]
Twip-based coordinate system v5

asa: I'm having difficulty finding someone to sr this (much less check it
in...)

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 133792 [details] [diff] [review]
Twip-based coordinate system v5

Looks good, although I'm not much of a Postscript wizard. Also 1.6a approval
granted. I will check this in tonight.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in. This is very nice work, thanks!!!

Comment 33

[Andreas Lange]

Very nice work!

A possible regression filed in bug 223551 though.

Comment 34

[Kenneth Herron]

Another regression -- landscape mode no longer works. See bug 223625.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I checked in the following attempted fix for IRIX bustage:

Index: nsAFMObject.cpp
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/ps/nsAFMObject.cpp,v
retrieving revision 1.25
retrieving revision 1.26
diff -p -u -0 -r1.25 -r1.26
--- nsAFMObject.cpp     23 Oct 2003 22:43:04 -0000      1.25
+++ nsAFMObject.cpp     22 Feb 2004 07:06:55 -0000      1.26
@@ -263,2 +263,2 @@ nsAutoString    psfontname;
-      score = abs(aFont.weight-gSubstituteFonts[i].mWeight);
-      score+= abs(aFont.style-gSubstituteFonts[i].mStyle);
+      score = abs(PRInt32(aFont.weight)-PRInt32(gSubstituteFonts[i].mWeight));
+      score+= abs(PRInt32(aFont.style)-PRInt32(gSubstituteFonts[i].mStyle));

On the assumption that the compiler had a valid reason to include C++ standard
library headers instead of just C ones and to have a |using std::abs|, the error
it was reporting seems likely to be correct (although I'm not sure).  The C++
standard provides multiple versions of std::abs, but only for signed types, so
providing it with an unsigned type might (I'd have to double-check the standard
on this, though) lead to equivalent conversion sequences leading to ambiguous
overload resolution.