Skip to content

Improve pre-rendering at the start/end of the document #14158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 24, 2021

Conversation

Snuffleupagus
Copy link
Collaborator

This is a very old "issue", which has existed since essentially forever, and it affects all of the available scrollModes. However, in the recently added Page-mode it's particularily noticeable since we use a simulated scroll direction there.

When deciding what page(s) to pre-render, we only consider the current scroll direction. This works well in most cases, but can break down at the start/end of the document by trying to pre-render a page outside of the existing ones. To improve this, we'll thus force the scroll direction at the start/end of the document.

Steps to reproduce:

  1. Open the viewer, e.g. https://mozilla.github.io/pdf.js/web/viewer.html
  2. Enable vertical scrolling.
  3. Press the End key.
  4. Open the devtools and, using the DOM Inspector, notice how page 13 is not being pre-rendered.

This is a very old "issue", which has existed since essentially forever, and it affects all of the available scrollModes. However, in the recently added Page-mode it's particularily noticeable since we use a *simulated* scroll direction there.

When deciding what page(s) to pre-render, we only consider the current scroll direction. This works well in most cases, but can break down at the start/end of the document by trying to pre-render a page *outside* of the existing ones. To improve this, we'll thus *force* the scroll direction at the start/end of the document.

*Steps to reproduce:*

 0. Open the viewer, e.g. https://mozilla.github.io/pdf.js/web/viewer.html
 1. Enable vertical scrolling.
 2. Press the <kbd>End</kbd> key.
 3. Open the devtools and, using the DOM Inspector, notice how page 13 is *not* being pre-rendered.
…les" (PR 14131 follow-up)

This was a stupid oversight on my part, since the first/last visible pages have obviously already been rendered at the point when we're checking for any potential "holes" in the page layout.
While this will obviously not have any measurable effect on performance, we should nonetheless avoid doing completely unnecessary checks here.
@Snuffleupagus Snuffleupagus marked this pull request as ready for review October 24, 2021 07:24
@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

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/f0f99c93287349b/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/30aeb28440053bb/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

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

Total script time: 3.83 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/30aeb28440053bb/output.txt

Total script time: 6.60 mins

  • Integration Tests: Passed

@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

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/44b984850ecde12/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/44b984850ecde12/output.txt

Total script time: 4.65 mins

Published

@timvandermeij timvandermeij merged commit 1ab9a6e into mozilla:master Oct 24, 2021
@timvandermeij
Copy link
Contributor

Thanks!

@Snuffleupagus Snuffleupagus deleted the preRender-doc-limits branch October 24, 2021 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants