Only redraw after zooming is finished (bug 1661253)

[calixteman]

Right now, the visible pages are redrawn for each scale change. Consequently, zooming with mouse wheel or in pinching can be pretty janky (even on a desktop machine but with a hdpi screen). So the main idea in this patch is to draw the visible pages only once zooming is finished.

[Snuffleupagus]

Another, and very severe effect, of these changes (that I just realized) is that this may end up slowing down parsing/rendering a fair bit in some cases. Note that if the page is currently parsing/rendering when zooming happens, we'll cancel both main-thread rendering and any worker-thread parsing via

pdf.js/web/pdf_page_view.js

Lines 611 to 614 in 0fdac9b

	if (this.paintTask) {
	this.paintTask.cancel();
	this.paintTask = null;
	}

Note that PR #11106 was implemented to slightly delay the worker-thread cancelling of getOperatorList, such that the viewer triggering cancel and then immediate re-render (e.g. on zooming) wouldn't force us to needlessly "throw away" work.
It seems to me that this patch will just break that (and simply increasing the referenced API-delay isn't really a good idea since that could make it useless, as outlined there).

[Snuffleupagus]

I'm not sure how I feel about it, and it's not really a complete patch yet, but we might be able work-around #15812 (comment) with something along the following lines[1]: master...Snuffleupagus:pdf.js:cancel-extraDelay

---

[1] Where we e.g. use the postpone-parameter as an argument when calling paintTask.cancel() in the PDFPageView code.

[calixteman]

I'm not sure how I feel about it, and it's not really a complete patch yet, but we might be able work-around #15812 (comment) with something along the following lines[1]: master...Snuffleupagus:pdf.js:cancel-extraDelay

[1] Where we e.g. use the postpone-parameter as an argument when calling paintTask.cancel() in the PDFPageView code.

After having thought about that, my feeling is that your patch is not that bad.
Right now, once zooming is finished we almost wait for 100ms to to continue or really cancel the getOperatorList thing because we expect that a draw could happen just once the zooming is finished.
So I'd say that we should delay the cancellation by AppOptions.get("defaultZoomDelay") + 100ms maybe.
In reading your comment here:
#11106 (comment)
I think it makes sense to have two distinct values w.r.t scrolling/zooming.

[Snuffleupagus]

Right now, once zooming is finished we almost wait for 100ms to to continue or really cancel the getOperatorList thing because we expect that a draw could happen just once the zooming is finished.

Please note that in the normal case, when parsing/rendering actually finishes successfully, there's nothing that needs to be aborted on the worker-thread and the _abortOperatorList-code isn't actually invoked at all.
The only case where we want to abort on the worker-thread, but with a small delay to handle repeated drawing, is when cancelling a partially parsed/rendered page.

So I'd say that we should delay the cancellation by AppOptions.get("defaultZoomDelay") + 100ms maybe.

It sounds like you're suggesting that the RenderTask.cancel-method, in the official API, should just accept a custom extra-delay. If so that feels like it could be a pretty big foot-gun, given the things that third-party users sometimes do, and we'd probably need to place an upper limit to prevent things from breaking; so how about the updated patch?
master...Snuffleupagus:pdf.js:cancel-extraDelay

[calixteman]

It sounds like you're suggesting that the RenderTask.cancel-method, in the official API, should just accept a custom extra-delay. If so that feels like it could be a pretty big foot-gun, given the things that third-party users sometimes do, and we'd probably need to place an upper limit to prevent things from breaking; so how about the updated patch?
master...Snuffleupagus:pdf.js:cancel-extraDelay

Could we just immediately cancel when we're scrolling and just have this computed delay when zooming/rotating ?
When scrolling is there any advantages to not cancel right now ?
I'm thinking about my own usage of the pdf viewer on my phone, and sometimes I quickly scroll the pdf to go to the page I'm looking for and in such a case I've the feeling that this delay would be a penalty.
About zooming, what do you think about postponing the cancel only when we've one page to render ? I'm wondering if it could be a penalty in general to postpone with several rendering pages.
That said, these different points could be addressed or not, in some follow-ups.
And your patch looks good for me.

[Snuffleupagus]

Could we just immediately cancel when we're scrolling and just have this computed delay when zooming/rotating ?
When scrolling is there any advantages to not cancel right now ?

Yes, if you quickly change scroll direction it may still make sense to slightly delay cancelling. E.g. consider a case where you scroll just past the page(s) you wanted to see and then scroll back quickly.

About zooming, what do you think about postponing the cancel only when we've one page to render ? I'm wondering if it could be a penalty in general to postpone with several rendering pages.

I'm not sure how much of a problem that is, and anyway if there's multiple visible (or even cached) pages we really don't want to cancel their worker-thread parsing right away since that could easily regress performance (and it's somewhat difficult to imagine every possible edge-case here).
In general, I very much prefer that we don't complicate the API side of things (or even the viewer) too much regarding the cancelling. Especially given the how much things are now "moving" in this code, with the potential risk of breakage.

That said, these different points could be addressed or not, in some follow-ups.

We could possibly try to slightly reduce the default cancel-delay, to e.g. 50 ms instead, however with so many other related changes that's not really something that I think we want to do right now.

And your patch looks good for me.

I'll submit a separate PR, however probably not today since I'm currently sick with a cold.

[calixteman]

I'll submit a separate PR, however probably not today since I'm currently sick with a cold.

Take care of yourself.

[Snuffleupagus]

This is looking much better now, but I've got a few more small suggestions/questions here.

[Snuffleupagus]

Submitted twice by mistake.

[Snuffleupagus]

/botio integrationtest

[pdfjsbot]

From: Bot.io (Windows)

---

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/aee10a23f9f7b59/output.txt

[pdfjsbot]

From: Bot.io (Linux m4)

---

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/84b1c408dbec547/output.txt

[pdfjsbot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.241.84.105:8877/84b1c408dbec547/output.txt

Total script time: 4.31 mins

• Integration Tests: Passed

[pdfjsbot]

From: Bot.io (Windows)

---

Failed

Full output at http://54.193.163.58:8877/aee10a23f9f7b59/output.txt

Total script time: 13.81 mins

• Integration Tests: FAILED

[Snuffleupagus]

/botio-linux preview

[pdfjsbot]

From: Bot.io (Linux m4)

---

Received

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

Live output at: http://54.241.84.105:8877/1746d4cb643f676/output.txt

[pdfjsbot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.241.84.105:8877/1746d4cb643f676/output.txt

Total script time: 1.26 mins

Published

• Viewer: http://54.241.84.105:8877/1746d4cb643f676/web/viewer.html
• Viewer (legacy): http://54.241.84.105:8877/1746d4cb643f676/legacy/web/viewer.html

[Snuffleupagus]

This is so close to done now, however I found a few small issue in the latest update (see the inline comments).

[Snuffleupagus]

Sorry about the delay in getting through this, but between being sick (multiple times) and the size/scope of the changes this was quite challenging to review.

Hopefully I've not missed anything obvious here, since I've spent quite a lot of time thinking about the "surrounding" code. However with the new preference we have a simple way to disable this functionality, should that ever become necessary.

r=me, with a couple of final comments; thank you!

[calixteman]

/botio integrationtest

[pdfjsbot]

From: Bot.io (Windows)

---

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/fae67d57f4cc7a7/output.txt

[pdfjsbot]

From: Bot.io (Linux m4)

---

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/0d417b6d5d1470e/output.txt

[pdfjsbot]

From: Bot.io (Linux m4)

---

Failed

Full output at http://54.241.84.105:8877/0d417b6d5d1470e/output.txt

Total script time: 4.44 mins

• Integration Tests: FAILED

[calixteman]

/botio-linux integrationtest

[pdfjsbot]

From: Bot.io (Linux m4)

---

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/fad1739a42c581e/output.txt

[pdfjsbot]

From: Bot.io (Windows)

---

Failed

Full output at http://54.193.163.58:8877/fae67d57f4cc7a7/output.txt

Total script time: 12.35 mins

• Integration Tests: FAILED

[pdfjsbot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.241.84.105:8877/fad1739a42c581e/output.txt

Total script time: 4.23 mins

• Integration Tests: Passed