Support printing page with different rotation

[xlc]

This PR fixed a printing issue on document that contains page with different orientation.

Some page may print upside down, but rotate a printed paper sheet shouldn't be a problem.

@Rob--W This is the PR you and @Hengjie discussed in #5857

Some test pdfs:

https://drive.google.com/a/notable.ac/file/d/0B6YhVF9p-y2AbUJlcEI5ZGZ5Z2c/view?usp=sharing
https://drive.google.com/a/notable.ac/file/d/0B6YhVF9p-y2AQTNxMVVOekJYeFE/view?usp=sharing

[Rob--W]

Thanks for the PR. I'll put reviewing it on my to-do list.

[timvandermeij]

/botio-linux preview

[pdfjsbot]

From: Bot.io (Linux)

---

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/60608bee397dc6f/output.txt

[pdfjsbot]

From: Bot.io (Linux)

---

Success

Full output at http://107.21.233.14:8877/60608bee397dc6f/output.txt

Total script time: 0.70 mins

Published

• Viewer: http://107.21.233.14:8877/60608bee397dc6f/web/viewer.html
• B2G Viewer: http://107.21.233.14:8877/60608bee397dc6f/extensions/b2g/content/web/viewer.html
• Extension: http://107.21.233.14:8877/60608bee397dc6f/extensions/firefox/pdf.js.xpi

[yurydelendik]

Looks okay. I'm still not sure about isFirstPagePortrait-parameter-logic usage. Shall we make that decision before printing and use it e.g. aspageOrientationMode parameter instead?

[Rob--W]

@yurydelendik The purpose of this patch is to allow pages with varying orientation levels to be printed correctly.

For that specific purpose and the given test cases, the patch looks OK to me, but I'm not completely certain whether this special case is always warranted in its current form. For instance, if a PDF is approximately a square (e.g. presentation slides), then flipping the pages because e.g. 501 > 500 doesn't make a lot of sense to me. I'd like to see a stricter logic to address this issue (e.g. changing pageHeight > pageWidth to something like pageHeight > SOME_MINIMUM_ASPECT_RATIO * pageWidth || pageHeight > pageWidth + SOME_MINIMAL_DIFFERENCE).

[yurydelendik]

The purpose of this patch is to allow pages with varying orientation levels to be printed correctly.

I see -- I was confused about the parameter name, ignore my concern :)

[Hengjie]

Anything preventing this from being merged?

[Rob--W]

@Hengjie Yes, the question whether unconditionally rotating a page makes sense. I can obviously see that the patch has the desired effect for the provided test PDF files, but I'm not convinced whether it makes sense to unconditionally rotate all pages whose orientation is slightly different (see my previous comment, 6103#issuecomment-117242076).

Another example is a set of pictures that are so small that it doesn't matter whether the page is rotated when printed. Well, after applying your logic, the images will always be rotated, unconditionally.

Do you have any data on distribution of page sizes/orientations for PDF files?

[Rob--W]

@Hengjie Ping for an answer to my last question (July, 24th) and implementation of my suggestion from 30th of June.

[xlc]

I don't have any data about PDF file page sizes but I am unable to see the disadvantage of rotate page with size of 500x501? The result won't be much different and I can't think any case when user doesn't want the auto rotate behaviour. Even there are some edge cases, this PR still fixes more than it breaks so I think the trade-off is worth it.

[Rob--W]

@xlc With my given example, if I print the slides of a presentation that perfectly fit on a page, it would be odd to have them rotated.

It might be a good idea to add a new preference to disable auto-rotating, to allow users / embedders to choose whether to rotate a page.

[xlc]

@Rob--W Sorry I still can't see why auto-rotate is bad.
If all pages are same size, only different orientation, we definitely want auto-rotate.
If some pages have non-standard size, those pages can't be print perfectly fit, rotate will reduce the aspect-ratio difference.
In some edge case, given pages with sizes of 500 * 501, 501* 500, rotate may not improve the print quality but I don't think such kind of pdf exists. Why you need to rotate almost square presentation slides?

But if you insist the change is necessary, I can still do it. Just give me the value of SOME_MINIMUM_ASPECT_RATIO and SOME_MINIMAL_DIFFERENCE.

[Rob--W]

Why you need to rotate almost square presentation slides?

I do not want to rotate the slides when not needed (e.g. when PDFs with irregular pages are printed with a printer that automatically puts a staple on the result). But maybe I'm worrying about nothing, so let's not use those magic numbers for now and see how it goes.

The patch looks good to me.
Are you going to add a preference to allow users to disable auto rotate?

[xlc]

@Rob--W Updated

[timvandermeij]

@xlc Could you squash the commits into one commit? See https://github.com/mozilla/pdf.js/wiki/Squashing-Commits if you're not familiar with that.

[2Owy]

Hi guys ! So when is this going to be live ?

[Rob--W]

I don't know why this patch was not merged. @timvandermeij After your comment this PR was updated but not merged, why?

(the PR needs to be rebased now due to bit rot)

[2Owy]

Did someone forget to confirm a merge request ?
I kinda need this fix.
Thank you guys for your hard work.

[timvandermeij]

I think I just forgot about this after asking about squashing since it was assigned to @Rob--W. Sorry for that. Now that I look at this again, I'm not entirely sure if we need to make this a preference, i.e., why would one want to disable this? If we do keep this a preference, we should be able to remove the hash parameter change as we don't use them anymore.

I think this patch is good to go after a rebase, simplification and testing to ensure it still works as intended after the rebase (since the printing code changed quite a bit after this patch).

[Rob--W]

I'm not entirely sure if we need to make this a preference, i.e., why would one want to disable this?

If pages are irregularly shaped, then it does not make sense to rotate pages. PDFs are supposed to closely match the printed result, so I believe that a change as significant as rotating a page should be customizable.

[timvandermeij]

Good point. Let's keep the preference, but only remove https://github.com/mozilla/pdf.js/pull/6103/files#diff-dd42e9b2d2a652b8e312efa567698aa6R1454 since we don't really use the hash parameters anymore.

[xlc]

Sorry guys I won't be able to maintain this PR anymore. I haven't worked with pdf.js for long time and a quick rebase shows that there too many changes in the code structure and I don't have time to deal with it. Feel free to take whatever code you need from this PR to make a new one.
@timvandermeij @Rob--W

[2Owy]

So, as a conclusion, is PDF.js going to support printing a PDF with different orientations ?

[timvandermeij]

It will if someone has time to redo this patch. Until then, the issue is tracked in #6696.