624699 - Incorrect landscape printing

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[u285178]

There are three problems with landscaping printing on Linux:

1) Printing a page with landscape orientation rotates the page 90 degrees clockwise instead of 90 degrees counterclockwise. The requirement to rotate user space 90 degrees counterclockwise when printing landscape is documented in the PostScript Language Reference Manual 3rd ed. page 401 "... rotates the default user space for landscape orientation 90 degrees counterclockwise with respect to that for portrait orientation.". This is to ensure that when the binding is on the long edge at the left the then viewed in landscape orientation the lower page number is at the top and pages are turned bottom to top.

2) When the Landscape PostScript output is viewed in a PS Viewer such as evince or gv the pages is shown in portrait orientation because it is missing the "%%Orientation: Landscape" DSC comment.

3) Landscape PDF pages are rotated so that when viewed in a PDF viewer the pages have portrait orientation. PS is the only format where pages need to be rotated for landscape printing.

Attached is a patch to fix these bugs.

Comment 1

[u285178]

Attachment: fix landscape bugs

This patch:

1) Only rotates print output for PostScript
2) Rotates the PS landscape pages the correct way
3) Inserts the "%%Orientation: Portrait" / "%%Orientation: Landscape" DSC comments depending on the orientation.

Comment 2

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

Comment on attachment 502777 [details] [diff] [review]
fix landscape bugs

+    PRBool  mRotate;

PRPackedBool, and call it mRotateForLandscape

But shouldn't SetRotateForLandscape/GetRotateForLandscape/mRotateForLandscape be in gfxPSSurface only? Seems like we could use a surface device offset to the the offset you need instead of changing nsSimplePageSequence.

Comment 3

[u285178]

Attachment: updated patch

Comment 4

[u285178]

(In reply to comment #2)

I found with the patch headers and footers were truncated in PDF landscape as I did not take into account the changes to nsPrintEngine.cpp to unswap the width/height when in landscape.

I've updated the patch with the following changes:

- Move all the GetRotateForLandscape() changes from gfxASurface into gfxPSSurface except for the virtual GetRotateForLandscape() function which returns false in gfxASurface.

- Move the width/height swap for landscape from nsDeviceContextSpecG into gfxPSSurface and only swap the arguments passed to cairo surface create. This allows the workaround in nsPrintEngine to be removed and fixes the header/footer truncation bug I mentioned above.

- Due to a bug in the version of cairo in mozilla (fixed in cairo 1.10.2), I can't use cairo_ps_surface_set_size() to change to a different surface size so instead of making SetRotateForLandscape() swap the width/height and calling cairo_ps_surface_set_size() I've replaced this function with an extra parameter to the gfxPSSurface constructor to specify the orientation so the correct surface size can be determined when the surface is created.

- I've moved the rotate page code from nsSimplePageSequence to gfxContext.

- I've replaced the Rotate(-M_PI/2) transformation with the equivalent matrix [0 -1  1  0  0,  0]. This avoids rounding errors in the transformation that were producing PS output like:

  0.000000000000000612 -10 10 0.000000000000000612 43.3 90 Tm

- I've rounded the width/height passed to cairo surface create to integers so it will be identical to the size returned by gfxPSSurface::GetSize(). This ensures the rotation in gfxContext will be using the exact same width as the cairo surface.

> Seems like we could use a surface device offset to the
> the offset you need instead of changing nsSimplePageSequence.

Rotating the page requires both a translate and rotate. Unfortunately the cairo API for modifying the device matrix only provides access to the tx and ty components of the matrix. Otherwise the rotation could have been applied in gfxPSSurface without needing to modify the user matrix.

Comment 5

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

Comment on attachment 503445 [details] [diff] [review]
updated patch

Review of attachment 503445 [details] [diff] [review]:
-----------------------------------------------------------------

Very, very sorry about the delay.

Comment 6

[u285178]

Is anything else required to get the patch committed? I've tested with the current mozilla-central and the patch still applies and works.

Comment 7

[Boris Zbarsky [:bzbarsky, bz on IRC]]

This somehow never ended up on anyone's radar to check in....

In general, adding the "checkin-needed" keyword to the bug will get it landed.

I'll get it checked in today.

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0ab660ee24ab

Comment 9

[:ehsan akhgari]

http://hg.mozilla.org/mozilla-central/rev/0ab660ee24ab