From 9eb9065c7911659bcbbbfd5e7b286e5873571c81 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 21 Jan 2017 12:27:57 +0100 Subject: [PATCH] Ensure that we use the *correct* `paintedViewport` in `PDFPageView.cssTransform`, to avoid visual glitches on quick rotations (PR 7738 follow-up) *This fixes a regression from commit https://github.com/mozilla/pdf.js/pull/7738/commits/c9a0955c9c7ed857c98696732c6edeb6f4901f43, i.e. PR 7738.* Currently if you quickly rotate a document at least *twice*,[1] such that rendering of a page hasn't finished for the first rotation before the last rotation is triggered, the `cssTransform` method can fail to update the page correctly leading to it looking temporarily distorted. The reason why things break is that previously we stored the `viewport` on the canvas DOM element, meaning that when it was accessed in `cssTransform` is was guaranteed to point to the `viewport` of the `zoomLayer` canvas. Generally you want to avoid storing data on DOM elements this way, and during the `PDFPageView` refactoring needed to support SVG rendering, the previous `viewport` was instead stored directly on `PDFPageView`. However, the problem is first of all that the `paintedViewport` only stores the *last* `viewport` computed, and second of all that there're no guarantees that it actually applies to the current `zoomLayer` canvas. If a document is rotated slowly enough that rendering finishes *before* the next rotation then this problem doesn't exist, but for sufficiently quick rotations rendering will be cancelled at least once and the `paintedViewport` could thus be bogus. The solution for the above problems is to ensure that we track the correct `viewport` for each DOM element (canvas or svg),[2] which seemed easist to do with a `WeakMap`.[3] --- [1] I'm able to reproduce this using the `tracemonkey` file, but please note that for pages with few operations, i.e. that render very quickly, the effect may be hard to spot. [2] One other possible solution that I briefly considered, was to wait until rendering finished before storing the current `viewport`. However, that would have caused issues with rotating a page before the *first* rendering operation had finished. [3] This regression took me way longer to both figure out, and fix, than I'd like to admit :-) --- web/pdf_page_view.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index 45b79754f..b7d3a8647 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -97,7 +97,7 @@ var PDFPageView = (function PDFPageViewClosure() { this.renderer = options.renderer || RendererType.CANVAS; this.paintTask = null; - this.paintedViewport = null; + this.paintedViewportMap = new WeakMap(); this.renderingState = RenderingStates.INITIAL; this.resume = null; this.error = null; @@ -170,6 +170,7 @@ var PDFPageView = (function PDFPageViewClosure() { } if (this.canvas && !currentZoomLayerNode) { + this.paintedViewportMap.delete(this.canvas); // Zeroing the width and height causes Firefox to release graphics // resources immediately, which can greatly reduce memory consumption. this.canvas.width = 0; @@ -177,11 +178,9 @@ var PDFPageView = (function PDFPageViewClosure() { delete this.canvas; } if (this.svg) { + this.paintedViewportMap.delete(this.svg); delete this.svg; } - if (!currentZoomLayerNode) { - this.paintedViewport = null; - } this.loadingIconDiv = document.createElement('div'); this.loadingIconDiv.className = 'loadingIcon'; @@ -281,7 +280,7 @@ var PDFPageView = (function PDFPageViewClosure() { Math.floor(height) + 'px'; // The canvas may have been originally rotated, rotate relative to that. var relativeRotation = this.viewport.rotation - - this.paintedViewport.rotation; + this.paintedViewportMap.get(target).rotation; var absRotation = Math.abs(relativeRotation); var scaleX = 1, scaleY = 1; if (absRotation === 90 || absRotation === 270) { @@ -434,9 +433,10 @@ var PDFPageView = (function PDFPageViewClosure() { } if (self.zoomLayer) { + var zoomLayerCanvas = self.zoomLayer.firstChild; + self.paintedViewportMap.delete(zoomLayerCanvas); // Zeroing the width and height causes Firefox to release graphics // resources immediately, which can greatly reduce memory consumption. - var zoomLayerCanvas = self.zoomLayer.firstChild; zoomLayerCanvas.width = 0; zoomLayerCanvas.height = 0; @@ -578,7 +578,7 @@ var PDFPageView = (function PDFPageViewClosure() { canvas.style.width = roundToDivide(viewport.width, sfx[1]) + 'px'; canvas.style.height = roundToDivide(viewport.height, sfy[1]) + 'px'; // Add the viewport so it's known what it was originally drawn with. - this.paintedViewport = viewport; + this.paintedViewportMap.set(canvas, viewport); // Rendering area var transform = !outputScale.scaled ? null : @@ -643,7 +643,7 @@ var PDFPageView = (function PDFPageViewClosure() { return svgGfx.getSVG(opList, actualSizeViewport).then(function (svg) { ensureNotCancelled(); self.svg = svg; - self.paintedViewport = actualSizeViewport; + self.paintedViewportMap.set(svg, actualSizeViewport); svg.style.width = wrapper.style.width; svg.style.height = wrapper.style.height;