Flicker free pdf reload

[James-Yu]

This is a continuation of #4281, authored by @quoc-ho

I've changed the strategy a bit: instead of saving and redrawing the canvas, the viewer now delays page load until all visible pages are rendered. Still not down to 3 changes, but all the important changes happen in only one function setDocument.

Note that the patch also restores the scale and the scroll position within the setDocument function. This is to make sure that no weird flicker happens due to scrolling to the top. This might also be relevant for reducing flicker in the new trimming implementation.

... Essentially, the only change is that I moved the scale setting from refresh.ts into setDocument inside viewer.mjs and then commented out the relevant line in _resetView(), also in viewer.mjs.

[James-Yu]

Thats a great piece of work @quoc-ho ! I have an issue with the current implementation: it seems that all PDFs will start from showing the second page instead of the first one. Any idea?

[quoc-ho]

Fixed in the newest commit. The reason was that the line

this._currentScale = _ui_utils_js__WEBPACK_IMPORTED_MODULE_1__.UNKNOWN_SCALE;

in _resetView() is also responsible for resetting the scroll position. So I uncommented it and added 2 lines in setDocument to save and restore this value.

[James-Yu]

I'm playing around with the patch, and have two proposed modifications. May you kindly help check if the changes won't affect the performance?

• Near L11833, the original this._firstPageCapability.resolve(firstPdfPage); was commented out. Can this promise still being resolved here, instead of later in the new for-loop? Looks like still flicker-free but not sure.
• Near L11863, the scale was set to the original value. Here also I tried to use the original const scale = this.currentScale; and notice no difference. Am I right?
• As I'm trying to extract the new re-rendering logic, does it make a difference if near L11909, the value of getPagesLeft is still pagesCount - 1;?

[quoc-ho]

• Near L11833, the original this._firstPageCapability.resolve(firstPdfPage); was commented out. Can this promise still being resolved here, instead of later in the new for-loop? Looks like still flicker-free but not sure.

It should be ok to do it as you suggested. The only flicker-inducing thing was page removal and scrolling before page rendered.

• As I'm trying to extract the new re-rendering logic, does it make a difference if near L11909, the value of getPagesLeft is still pagesCount - 1;?

Good catch. My implementation there wasn't correct. Fixed in the latest commit.

[James-Yu]

Seems that I do not have permission to access your branch. Can you grant me the permission, so I can make some final clean-ups?

[James-Yu]

Added clean-up logic and dev edit scripts. Can you help check if the current state is good with flicker-resolution? @quoc-ho we should be quite near to merge.

edit: no need to check the editviewer.py or the diff file.

[quoc-ho]

Everything seems to work great!

One note for the future: when the number of pages is large (>200 pages), and there is a mixture of landscape and portrait pages, then flicker still happens a little bit because scrolling from the old position to the new position is somehow animated (?). This doesn't happen for very large documents (>500 pages) when all pages are of the same size and orientation.

[James-Yu]

This patch and PR are purely awesome!

[quoc-ho]

Awesome!!! I've been bothered by the flicker for so many years :D.

[James-Yu]

In some rare cases (maybe scrolling during refresh), there is an error says

viewer.mjs:10482 Unable to initialize viewer TypeError: Failed to execute 'removeChild' on 'Node': parameter 1 is not of type 'Node'.
    at globalThis.lwRenderSync (latexworkshop.js:337:26)
    at async viewer.mjs:10413:7

originates from

        for (let i = 1; i <= oldPageCount; i++) {
            pdfViewer.viewer.removeChild(pdfViewer.viewer.firstChild)
        }

Any idea on that? @quoc-ho

May be related to #4300

[quoc-ho]

Let me first note here one way to trigger this error.

1. Make a relatively long document (~30 pages), compile it, and view its pdf.
2. Assign a shortcut to Refresh all pdf viewers
3. Hold the shortcut for a couple of seconds

It seems that when the pdf is rapidly refreshed, the following line

oldPageCount = pdfViewer.viewer.children.length

gives the wrong number since it might have counted pages that were added from a couple of iterations ago. So I changed that line to

oldPageCount = pdfViewer.pdfDocument ? pdfViewer.pdfDocument.numPages : 0

and that error disappeared.

I work from the tag v10 rather than the latest commit. In particular, no setTimeout is needed.

[quoc-ho]

L78 in refresh.ts also sometimes causes some errors on very rapid refresh

PDFViewerApplication.pdfViewer.currentPageNumber = prevState.page

That line doesn't seem to be necessary since the scroll update in the patch already took care of it. With that line removed, I could hold the refresh shortcut key for around 10 seconds without any error. After that, there were still some errors. But these were different ones inside viewer.mjs at L10448 where pageView happens to be null. This also happens without the patch though, so it's fine.

[quoc-ho]

Maybe we can make it even more robust by rate-limiting the refresh? For example, in L65 of refresh.ts, can we make sure that the previous pages DOM has been loaded before calling load? Or we can use a time-based approach.

[James-Yu]

Maybe we can make it even more robust by rate-limiting the refresh? For example, in L65 of refresh.ts, can we make sure that the previous pages DOM has been loaded before calling load? Or we can use a time-based approach.

A great idea! 782cf32632bdbd87baa031f4e11fb5884414f4fb and fcab0fa implemented a rate-limit by temporarily disabling new refreshes when there's an ongoing one, and queue the new one to run after current. Together with oldPageCount and removal of setting page number, this issue should be resolved.

[quoc-ho]

There seems to be a bug in the current implementation of rate-limited reload. If I make rapid refreshes, after a couple of seconds, the viewer refuses to refresh altogether. So in particular, compiling doesn't even update the viewer.

[quoc-ho]

I tried changing

export function doneRefresh() {
    if (shouldRefreshAgain) {
        shouldRefreshAgain = false
        void refresh()
    } else {
        refreshing = false
    }
}

to

export function doneRefresh() {
    refreshing = false
    if (shouldRefreshAgain) {
        shouldRefreshAgain = false
        void refresh()
    }
}

and it works. The point is that refreshing should only be true when the pdf viewer is actually loading (and not just preparing to load again).

[James-Yu]

I've reverted the refresh queueing in 8f38eb3 as it seems that queue the very fast refreshing seems not quite necessary. Now it's more robust I suppose?

[quoc-ho]

But now, what happens when a compile just finishes when the previous render wasn't done yet? Like some pages are very complicated to render. That would be very rare; I certainly don't have anything that complicated myself. But in this case, I think queueing the rerender would be more robust.

[James-Yu]

It's valid, yet I need to think of a more robust way of implementing this.

[quoc-ho]

I think the suggested change I made above is robust. If you want to be ultrasafe, maybe use mutex?

[quoc-ho]

Another bug is when the pdf fails to reload since it's corrupted for some reason. In this case, doneRefresh() is never called, and hence, refreshing stays true. So we need to sprinkle these lines with some try/catch/finally.

[James-Yu]

What about 922d393219672ab59ebfe87d0759160c880f973e ? Should work fine with some very preliminary tests.

[quoc-ho]

It should be fine now :).