Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Text layer constantly re-rendered when zooming large PDF (or large zoom) #18022

Closed
nicolo-ribaudo opened this issue Apr 29, 2024 · 1 comment · Fixed by #18077
Closed

Text layer constantly re-rendered when zooming large PDF (or large zoom) #18022

nicolo-ribaudo opened this issue Apr 29, 2024 · 1 comment · Fixed by #18077
Assignees
@Snuffleupagus
Labels
performance text-selection viewer

Comments

@nicolo-ribaudo
Copy link
Contributor

Attach (recommended) or Link to PDF file here: A1.pdf (it also happens with the Trace-based JIT ... PDF, but you have to be close to the max zoom to see it)

Configuration:

  • Web browser and its version: Firefox 124, Firefox Android 125, Chrome Android 123
  • Operating system and its version:
  • PDF.js version: The one in that firefox version, and the current main
  • Is a browser extension: no

Steps to reproduce the problem:

  1. Add a logpoint in

    pdf.js/web/pdf_page_view.js

    Line 710 in 627fe2d

    this.eventBus.dispatch("pagerendered", {
  2. Zoom enough so that the size is bigger than this.maxCanvasPixels, so zooming becomes css-only

What is the expected behavior? (add screenshot)

It should respect defaultZoomDelay, and do not re-draw the text layer (and not emit pagerendered) until when zooming is done.

When zooming at small enough scales that the size is less than this.maxCanvasPixels (i.e. less than 100% in that A1.pdf), the event handler that handles zoom does very little work:

Stack Chart (over ~300ms)
image

Flame Graph (over ~300ms)
image

What went wrong? (add screenshot)

When zooming at big sizes, a lot of the time is spent re-rendering the text layer.

Stack Chart (over ~200ms)
image

Flame Graph (over ~200ms)
image

As you can see from the two stack charts, most of the time in the first one is spend doing nothing while in the second one it's doing something. This isn't really noticeable on my desktop, but it introduces some lagging on my phone (both in Firefox and Chrome, but Chrome is doing noticeably worse).

Other info:

The relevant code is

pdf.js/web/pdf_page_view.js

Lines 654 to 717 in 627fe2d

if (this.canvas) {
let onlyCssZoom = false;
if (this.#hasRestrictedScaling) {
if (
(typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) &&
this.maxCanvasPixels === 0
) {
onlyCssZoom = true;
} else if (this.maxCanvasPixels > 0) {
const { width, height } = this.viewport;
const { sx, sy } = this.outputScale;
onlyCssZoom =
((Math.floor(width) * sx) | 0) * ((Math.floor(height) * sy) | 0) >
this.maxCanvasPixels;
}
}
const postponeDrawing =
!onlyCssZoom && drawingDelay >= 0 && drawingDelay < 1000;
if (postponeDrawing || onlyCssZoom) {
if (
postponeDrawing &&
this.renderingState !== RenderingStates.FINISHED
) {
this.cancelRendering({
keepZoomLayer: true,
keepAnnotationLayer: true,
keepAnnotationEditorLayer: true,
keepXfaLayer: true,
keepTextLayer: true,
cancelExtraDelay: drawingDelay,
});
// It isn't really finished, but once we have finished
// to postpone, we'll call this.reset(...) which will set
// the rendering state to INITIAL, hence the next call to
// PDFViewer.update() will trigger a redraw (if it's mandatory).
this.renderingState = RenderingStates.FINISHED;
// Ensure that the thumbnails won't become partially (or fully) blank,
// if the sidebar is opened before the actual rendering is done.
this.#useThumbnailCanvas.directDrawing = false;
}
this.cssTransform({
target: this.canvas,
redrawAnnotationLayer: true,
redrawAnnotationEditorLayer: true,
redrawXfaLayer: true,
redrawTextLayer: !postponeDrawing,
hideTextLayer: postponeDrawing,
});
if (postponeDrawing) {
// The "pagerendered"-event will be dispatched once the actual
// rendering is done, hence don't dispatch it here as well.
return;
}
this.eventBus.dispatch("pagerendered", {
source: this,
pageNumber: this.id,
cssTransform: true,
timestamp: performance.now(),
error: this.#renderError,
});
return;
.

Looking at #16507, it seems like the "fast" behavior is indeed the expected behavior: page re-rendering shouldn't happen until the "final" rendering. However, a combination of 7bca3c8 and 0ee2a35 changed how PDF.js behaves in this case:

  • 7bca3c8 disabled the rendering delay when PDF.js is configured to use css-only zoom, because there won't be a "next render"
  • 0ee2a35 unified the "css-only zoom because configured as such" and "css-only zoom because the canvas would be too big" cases

I think there are two possible solutions, but I saw that there were many regressions related to this rendering delay so I'm not sure about what's best:

  1. onlyCssZoom should only affect whether or not we cancel the ongoing rendering, and not whether or not we delay the text layer and the "pagerendered" event
  2. we distinguish the "onlyCssZoom because of this.maxCanvasPixels === 0" and "onlyCssZoom because the canvas would be too big" cases
@Snuffleupagus Snuffleupagus self-assigned this Apr 29, 2024
@nicolo-ribaudo
Copy link
Contributor Author

@Snuffleupagus I see that you assigned this to yourself -- I'd be happy to provide a PR if you want.

I think the proposed approach 1 is better, because it also avoids the performance problem when maxCanvasPixels === 0, but it only works if .update() is called multiple times also in that case (which I'm not 100% sure about, due to "with CSS-only zooming rendering is only expected to happen once per page." in 7bca3c8).

If neither (1) nor (2) seem good, I could also help by just writing tests for this.

@Snuffleupagus Snuffleupagus linked a pull request May 13, 2024 that will close this issue
[api-minor] Respect the drawingDelay also when CSS-only zooming is used (issue 18022) #18077
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Snuffleupagus Snuffleupagus

Labels
performance text-selection viewer
Projects
None yet
Milestone
No milestone
3 participants
@timvandermeij @Snuffleupagus @nicolo-ribaudo