James-Yu / LaTeX-Workshop

Boost LaTeX typesetting efficiency with preview, compile, autocomplete, colorize, and more.
MIT License
10.62k stars 529 forks source link

Seamless and flicker-free pdf reload #4281

Closed quoc-ho closed 3 months ago

quoc-ho commented 3 months ago

Pre-checks

The Missed

After each compilation, the PDF viewer reloads the PDF with a flicker. Namely, instead of displaying the content of the new PDF seamlessly, a blank page briefly appears before being replaced by the new content.

It would be great to remove the flicker.

The Solution*

The PDF viewer of Overleaf-Workshop is also built on top of pdf.js but doesn't flicker when reloading the PDF. I think the main difference between what they do and LaTeX-Workshop is that when reloading, pdf.js object is not recreated (though I'm not familiar with the code base of LaTeX-Workshop enough to say). Moreover, they also read the content of the PDF file into a byte array before handing it to pdf.js, which might have reduced the latency.

Anything Else?

The amount of code related to PDF viewing in Overleaf-Workshop is quite small. So probably/hopefully, the amount of work needed to implement this in LaTeX-Workshop is also small. I tried to look into this myself, but I found the PDF viewing code in LaTeX-Workshop a bit complicated for me to do it quickly since I'm not very familiar with the codebase.

Even if the maintainers of LaTeX-Workshop decide that it's not worth the trouble, having this available here might help others who want to do this themselves.

Thank you!

quoc-ho commented 3 months ago

I wanted to add that the popular vscode-pdf also does not flicker when the pdf reloads. See this commit https://github.com/tomoki1207/vscode-pdfviewer/commit/6c4c96d5f380e8c8130b9e58138867d6036f291f.

This snippet in the commit seems to do it:

async function reload() {
  // Prevents flickering of page when PDF is reloaded
  const oldResetView = PDFViewerApplication.pdfViewer._resetView
  PDFViewerApplication.pdfViewer._resetView = function () {
    this._firstPageCapability = (0, pdfjsLib.createPromiseCapability)()
    this._onePageRenderedCapability = (0, pdfjsLib.createPromiseCapability)()
    this._pagesCapability = (0, pdfjsLib.createPromiseCapability)()

    this.viewer.textContent = ""
  }

  // Changing the fingerprint fools pdf.js into keeping scroll position
  const doc = await pdfjsLib.getDocument(config.path).promise
  doc._pdfInfo.fingerprints = [config.path]
  PDFViewerApplication.load(doc)

  PDFViewerApplication.pdfViewer._resetView = oldResetView
}
James-Yu commented 3 months ago

Both are using PDFViewerApplication.load(doc), while we are using PDFViewerApplication.open(url).

However, I have been playing with the API for a while. Switching to load() seems not resolving the flicking in this extension. As this request is primarily aesthetic and seems need indeed much work. I will leave it here and hopefully there will be community contribution.


edit: seems not caused by the difference here. I have no idea on how flicker-free is achieved.

quoc-ho commented 3 months ago

Very interesting. And thanks for leaving it here.

Is there a particular reason why PDFViewerApplication.open(url) rather than PDFViewerApplication.load(doc) is used in LaTeX-Workshop? Also, the snippet I copied above does a little more than just calling PDFViewerApplication.load(doc). However, you still think that it wouldn't do anything for our extension?

James-Yu commented 3 months ago

Is there a particular reason why PDFViewerApplication.open(url) rather than PDFViewerApplication.load(doc) is used in LaTeX-Workshop?

No. open has been there since the start of this extension. load is a recent one. Former is indeed a wrapper of latter.

Also, the snippet I copied above does a little more than just calling PDFViewerApplication.load(doc). However, you still think that it wouldn't do anything for our extension?

Tried and seems no.

quoc-ho commented 3 months ago

Thanks for trying so quickly! I might take a look into this at some point.

James-Yu commented 3 months ago

Also note that the PDF.js version used by vscode-pdf is quite old. During the time, API changed a log, e.g., pdfjsLib.createPromiseCapability => Promise.withResolvers (https://github.com/mozilla/pdf.js/commit/e4d0e84802098251bd72ffa6ef28907d1ce50f2a)

quoc-ho commented 3 months ago

It seems that commenting out this line reduces flickering quite a bit without having any side-effect so far. Later on, setDocument is called with the current pdf document anyway. The flicker is still there though.

https://github.com/James-Yu/LaTeX-Workshop/blob/d89a9e5883268f22797400af1b9792fe637cd964/viewer/viewer.mjs#L2593

quoc-ho commented 3 months ago

This is great: the last commit reduced the flicker by quite a bit, around the same amount as commenting out the line in the previous comment! For a one-page document, sometimes, there's no flicker even.

quoc-ho commented 3 months ago

So it seems that I've managed to patch viewer.mjs a little bit to remove all flicker (almost, see comments below). See the screen recording :)

https://github.com/James-Yu/LaTeX-Workshop/assets/10014608/1c4b2216-51ad-4229-a5d9-dd7d9ce53ba7


The full commit is here https://github.com/quoc-ho/LaTeX-Workshop/commit/51fdbe25f15c02144258e55308c3d431f612c358. The main points are


Now, I said "almost" because there is still some issue with zoom/trim level that people here probably know better how to handle.

  1. Without trimming, it's working fine now, but I needed the following hack which sets the size of the new canvas to be that of the old one: https://github.com/quoc-ho/LaTeX-Workshop/blob/51fdbe25f15c02144258e55308c3d431f612c358/viewer/viewer.mjs#L9307-L9310

I've tested with scrolling, resizing using the mouse, and changing zoom level, etc., and there seems to be no adverse effect.

This hack is necessary because for some reason, the size of the canvas, as calculated in the snippet below, is different from the final size https://github.com/James-Yu/LaTeX-Workshop/blob/989b9941a9c4316785bf94e010cfeb32ecb1c100/viewer/viewer.mjs#L9284-L9290 This probably has to do with zoom, but I don't know how to fix this more elegantly.

  1. With trimming turned on, flicker still happens around 50% of the time, but now, it's caused by the page being displayed in the wrong size briefly before getting corrected. So it's definitely related to the above also.

It would be great if someone more knowledgeable could chime in/help out :).

James-Yu commented 3 months ago

Thank you very much for such in-depth discussion. A bit of unfortunate is that we generally won't patch viewer.mjs too much as the patches need to be very carefully tuned on every PDF.js update.

I will look into your code changes, and see if there is a way to make it an addon.

In terms of trimming. I am never a fan of it, at least the implementation. I recalled that once I tried to refurbish the feature but failed due to the complex and not-very-intelligible implementation of viewer.mjs. Will return to trimming shortly and see if there can be a more elegent way of doing it.

James-Yu commented 3 months ago

@quoc-ho Can you please try on https://github.com/James-Yu/LaTeX-Workshop/tree/better-viewer-trimming branch with the fix? This branch uses a much simpler trimming logic, which may comply with your flicker-free implementation. If so, the next step will be isolating the logic.

quoc-ho commented 3 months ago

@James-Yu The option to trim seems to have completely disappeared on the pdf viewer.

James-Yu commented 3 months ago

Yes, sorry for the miss. The trimming option is moved to be a config item latex-workshop.view.pdf.trim instead of a dropdown. This allows continuous level of trimming.

quoc-ho commented 3 months ago

Great! The new trimming works well with the patch I made.

Not completely related, but I think it's better to have the trim level available inside the pdfview since people have documents with different margins, which requires different trim levels.

quoc-ho commented 3 months ago

Edit: the thing below is actually not true. What is true is that the more I trim, the better it works without that hack. But maybe it's a timing thing: more trim --> less to draw --> less chance to see the shifted page. And just to confirm again, the hack works well with or without trimming.


And here's the plot twist: if I comment out the hack https://github.com/quoc-ho/LaTeX-Workshop/blob/51fdbe25f15c02144258e55308c3d431f612c358/viewer/viewer.mjs#L9307-L9310, things still work perfectly with the new trim implementation! So somehow, trimming is better than not trimming :)

James-Yu commented 3 months ago

The new trimming should be finalized in the [better-viewer-trimming](https://github.com/James-Yu/LaTeX-Workshop/tree/better-viewer-trimming) branch.

I have tried the patch in your fork, works great. However, it will introduce notable maintenance burden when updating PDF.js. I also tinkered a bit but no luck to find a way to minimize the intrusion. I figure we are aiming for around three injections into viewer.mjs (add consecutive lines, comment out ones, change existing ones, etc.) @quoc-ho may I ask for your help when you are available on this issue? Many thanks.

quoc-ho commented 3 months ago

The new trimming should be finalized in the better-viewer-trimming branch.

The new trimming logic works very well! It's great.

I have tried the patch in your fork, works great. However, it will introduce notable maintenance burden when updating PDF.js. I also tinkered a bit but no luck to find a way to minimize the intrusion. I figure we are aiming for around three injections into viewer.mjs (add consecutive lines, comment out ones, change existing ones, etc.) @quoc-ho may I ask for your help when you are available on this issue? Many thanks.

It does indeed introduce a maintenance burden, especially considering that the viewer from pdfjs is quite convoluted. I'm happy to help. What would you like me to do regarding this issue?


Not completely related but I'm curious why pdfjs is so hard to read. I just found this viewer, which is so much simpler and has almost all the functionality, pretty much everything except for thumbnail (which is quite easy to do) and printing (which we probably don't need).

James-Yu commented 3 months ago

I would suppose an optimal patch is to introduce no more than three locations in the viewer.mjs to apply edits, edits themselves can be multi-line though.

Using pdf.js is because it is almost the standard, and is backed by big open-source community.

quoc-ho commented 3 months ago

I would suppose an optimal patch is to introduce no more than three locations in the viewer.mjs to apply edits, edits themselves can be multi-line though.

Alright! I'll see if I can make it down to 3. I'm not sure how feasible it is. But I use this extension daily to write papers/notes, so I have some motivation :).

Using pdf.js is because it is almost the standard, and is backed by big open-source community.

I wasn't advocating for a change as it would involve much work, at least in the short run. :) But I think that mupdf is as standard as pdf.js (if not more so). It's made by the same people who made Ghostscript. It's also used in many places, such as sumatra pdf and sioyek. On the web, they only became a thing recently thanks to wasm.

quoc-ho commented 3 months ago

I think that the new trimming implementation doesn't work when there are landscape pages. @James-Yu, could you please take a look?

James-Yu commented 3 months ago

Let's move discussions on trimming to #4292 , leave this issue related to flickering.

quoc-ho commented 3 months ago

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. In fact, everything important happens at L11893--L11915. The full diff is here:

https://github.com/James-Yu/LaTeX-Workshop/compare/master...quoc-ho:LaTeX-Workshop:flicker-free-pdf-reload-2#diff-702e93c7f6abb674b78d0d0fc28b58c10a632a7afc2656cb137158827997ba8fR11911

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.

However, currently visible pages that are not the current page still flicker sometimes due to a rerendering for some reason. From the stack trace, it seems to be caused by a scale change triggered by refresh.ts, but commenting out the relevant lines doesn't seem to solve it. I haven't looked into it very much though. Maybe @James-Yu can take a look?

James-Yu commented 3 months ago

The words are very promising! I will look into the new implementation shortly.

As to non current page, it seems that pdfjs uses a different strategy to (delay) render them, which afaik is not caused by the extension. That can be a hard thing to deal with as I still can’t trace it down to the code lines. Things are more obvious if the scale value is small and multiple pages are shown at the same time.

quoc-ho commented 3 months ago

The latest commit in https://github.com/quoc-ho/LaTeX-Workshop/tree/flicker-free-pdf-reload-2 fixed all issues regarding flickering. 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 commented 3 months ago

Implemented in #4295