From 8ea83f7030cc0e17aed8ea51b339a5a64a701544 Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Thu, 8 Apr 2021 12:21:52 +0200
Subject: [PATCH 1/3] Convert some properties, on `PDFThumbnailView`-instances,
to local variables
These properties are always updated/used together, and there's no other methods which depend on just one of them, hence they're changed into local variables instead.
Looking through the history of this code, it seems they were converted *from* local variables and to properties all the way back in PR 2914; however as far as I can tell from that diff it doesn't seem to have been necessary even back then!?
---
web/pdf_thumbnail_view.js | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/web/pdf_thumbnail_view.js b/web/pdf_thumbnail_view.js
index ac8899089..cbb919fc3 100644
--- a/web/pdf_thumbnail_view.js
+++ b/web/pdf_thumbnail_view.js
@@ -122,13 +122,13 @@ class PDFThumbnailView {
};
this.disableCanvasToImageConversion = disableCanvasToImageConversion;
- this.pageWidth = this.viewport.width;
- this.pageHeight = this.viewport.height;
- this.pageRatio = this.pageWidth / this.pageHeight;
+ const pageWidth = this.viewport.width,
+ pageHeight = this.viewport.height,
+ pageRatio = pageWidth / pageHeight;
this.canvasWidth = THUMBNAIL_WIDTH;
- this.canvasHeight = (this.canvasWidth / this.pageRatio) | 0;
- this.scale = this.canvasWidth / this.pageWidth;
+ this.canvasHeight = (this.canvasWidth / pageRatio) | 0;
+ this.scale = this.canvasWidth / pageWidth;
this.l10n = l10n;
@@ -172,12 +172,12 @@ class PDFThumbnailView {
this.cancelRendering();
this.renderingState = RenderingStates.INITIAL;
- this.pageWidth = this.viewport.width;
- this.pageHeight = this.viewport.height;
- this.pageRatio = this.pageWidth / this.pageHeight;
+ const pageWidth = this.viewport.width,
+ pageHeight = this.viewport.height,
+ pageRatio = pageWidth / pageHeight;
- this.canvasHeight = (this.canvasWidth / this.pageRatio) | 0;
- this.scale = this.canvasWidth / this.pageWidth;
+ this.canvasHeight = (this.canvasWidth / pageRatio) | 0;
+ this.scale = this.canvasWidth / pageWidth;
this.div.removeAttribute("data-loaded");
const ring = this.ring;
From 32a00b9b2b085e5828a2fbc99d21f5580aad1565 Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Thu, 8 Apr 2021 12:22:07 +0200
Subject: [PATCH 2/3] Stop looping over `childNodes`, in
`PDFThumbnailView.reset`, when removing the thumbnail
A loop is less efficient than just overwriting the content, which is what we've generally been using (for years) in other parts of the code-base (see e.g. `BaseViewer` and `PDFThumbnailViewer`).
---
web/pdf_thumbnail_view.js | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/web/pdf_thumbnail_view.js b/web/pdf_thumbnail_view.js
index cbb919fc3..cb4b4b63d 100644
--- a/web/pdf_thumbnail_view.js
+++ b/web/pdf_thumbnail_view.js
@@ -181,10 +181,7 @@ class PDFThumbnailView {
this.div.removeAttribute("data-loaded");
const ring = this.ring;
- const childNodes = ring.childNodes;
- for (let i = childNodes.length - 1; i >= 0; i--) {
- ring.removeChild(childNodes[i]);
- }
+ ring.textContent = ""; // Remove the thumbnail from the DOM.
const borderAdjustment = 2 * THUMBNAIL_CANVAS_BORDER_WIDTH;
ring.style.width = this.canvasWidth + borderAdjustment + "px";
ring.style.height = this.canvasHeight + borderAdjustment + "px";
From d8e07946503edfe64efbdfafdcf3a84a87462e0c Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Thu, 8 Apr 2021 12:22:19 +0200
Subject: [PATCH 3/3] Improve the image quality of thumbnails rendered by
`PDFThumbnailView.draw` (issue 8233)
The reason for the fairly large discrepancy, in the thumbnail quality, between the `draw`/`setImage`-methods is that in the former case we *directly* render the thumbnails at the final size that they'll appear at in the sidebar. In the latter case, we instead downsize the (generally) much larger "regular" pages.
To address this, I'm thus proposing that we let `PDFThumbnailView.draw` render thumbnails at *twice* their intended size and then downsize them to the final size.
Obviously this will increase *peak* memory usage during thumbnail rendering in `PDFThumbnailView.draw`, since doubling the width/height of a `canvas` will lead to its pixel-count increasing by a factor of `4`. Furthermore, since you need four components per pixel (given that it's RGBA-data), this will thus lead to the *temporary* thumbnail `canvas`-sizes increasing by a factor of `16` during rendering. Hence why rendering thumbnails at their "original" scale, i.e. using something like `PDFPageProxy.getViewport({ scale: 1 });`, would be an absolutely terrible idea!
To reduce the size and scope of these changes, I've tried to re-factor and re-use as much of the existing downsizing-implementation already present in `PDFThumbnailView` as possible.
While this will generally *not* make thumbnails rendered by `PDFThumbnailView.draw` look *identical* to those based on the rendered pages (via `PDFThumbnailView.setImage`), it's a considerable improvement as far as I'm concerned and enough to call the issue fixed.
*Please note:* This patch will not lead to *any* additional overhead, in either memory usage or parsing, for thumbnails which are based on the rendered pages.
---
web/pdf_thumbnail_view.js | 107 +++++++++++++++++++++-----------------
1 file changed, 59 insertions(+), 48 deletions(-)
diff --git a/web/pdf_thumbnail_view.js b/web/pdf_thumbnail_view.js
index cb4b4b63d..178fe755c 100644
--- a/web/pdf_thumbnail_view.js
+++ b/web/pdf_thumbnail_view.js
@@ -17,6 +17,7 @@ import { getOutputScale } from "./ui_utils.js";
import { RenderingCancelledException } from "pdfjs-lib";
import { RenderingStates } from "./pdf_rendering_queue.js";
+const DRAW_UPSCALE_FACTOR = 2; // See comment in `PDFThumbnailView.draw` below.
const MAX_NUM_SCALING_STEPS = 3;
const THUMBNAIL_CANVAS_BORDER_WIDTH = 1; // px
const THUMBNAIL_WIDTH = 98; // px
@@ -65,7 +66,7 @@ const TempImageFactory = (function TempImageFactoryClosure() {
ctx.fillStyle = "rgb(255, 255, 255)";
ctx.fillRect(0, 0, width, height);
ctx.restore();
- return tempCanvas;
+ return [tempCanvas, tempCanvas.getContext("2d")];
},
destroyCanvas() {
@@ -226,11 +227,10 @@ class PDFThumbnailView {
/**
* @private
*/
- _getPageDrawContext() {
- const canvas = document.createElement("canvas");
+ _getPageDrawContext(upscaleFactor = 1) {
// Keep the no-thumbnail outline visible, i.e. `data-loaded === false`,
// until rendering/image conversion is complete, to avoid display issues.
- this.canvas = canvas;
+ const canvas = document.createElement("canvas");
if (
typeof PDFJSDev === "undefined" ||
@@ -241,50 +241,48 @@ class PDFThumbnailView {
const ctx = canvas.getContext("2d", { alpha: false });
const outputScale = getOutputScale(ctx);
- canvas.width = (this.canvasWidth * outputScale.sx) | 0;
- canvas.height = (this.canvasHeight * outputScale.sy) | 0;
- canvas.style.width = this.canvasWidth + "px";
- canvas.style.height = this.canvasHeight + "px";
+ canvas.width = (upscaleFactor * this.canvasWidth * outputScale.sx) | 0;
+ canvas.height = (upscaleFactor * this.canvasHeight * outputScale.sy) | 0;
const transform = outputScale.scaled
? [outputScale.sx, 0, 0, outputScale.sy, 0, 0]
: null;
- return [ctx, transform];
+ return { ctx, canvas, transform };
}
/**
* @private
*/
- _convertCanvasToImage() {
- if (!this.canvas) {
- return;
- }
+ _convertCanvasToImage(canvas) {
if (this.renderingState !== RenderingStates.FINISHED) {
- return;
+ throw new Error("_convertCanvasToImage: Rendering has not finished.");
}
- const className = "thumbnailImage";
+ const reducedCanvas = this._reduceImage(canvas);
if (this.disableCanvasToImageConversion) {
- this.canvas.className = className;
+ reducedCanvas.className = "thumbnailImage";
this._thumbPageCanvas.then(msg => {
- this.canvas.setAttribute("aria-label", msg);
+ reducedCanvas.setAttribute("aria-label", msg);
});
+ reducedCanvas.style.width = this.canvasWidth + "px";
+ reducedCanvas.style.height = this.canvasHeight + "px";
+
+ this.canvas = reducedCanvas;
this.div.setAttribute("data-loaded", true);
- this.ring.appendChild(this.canvas);
+ this.ring.appendChild(reducedCanvas);
return;
}
const image = document.createElement("img");
- image.className = className;
+ image.className = "thumbnailImage";
this._thumbPageCanvas.then(msg => {
image.setAttribute("aria-label", msg);
});
-
image.style.width = this.canvasWidth + "px";
image.style.height = this.canvasHeight + "px";
- image.src = this.canvas.toDataURL();
+ image.src = reducedCanvas.toDataURL();
this.image = image;
this.div.setAttribute("data-loaded", true);
@@ -292,9 +290,8 @@ class PDFThumbnailView {
// Zeroing the width and height causes Firefox to release graphics
// resources immediately, which can greatly reduce memory consumption.
- this.canvas.width = 0;
- this.canvas.height = 0;
- delete this.canvas;
+ reducedCanvas.width = 0;
+ reducedCanvas.height = 0;
}
draw() {
@@ -322,17 +319,25 @@ class PDFThumbnailView {
if (error instanceof RenderingCancelledException) {
return;
}
-
this.renderingState = RenderingStates.FINISHED;
- this._convertCanvasToImage();
+ this._convertCanvasToImage(canvas);
if (error) {
throw error;
}
};
- const [ctx, transform] = this._getPageDrawContext();
- const drawViewport = this.viewport.clone({ scale: this.scale });
+ // Render the thumbnail at a larger size and downsize the canvas (similar
+ // to `setImage`), to improve consistency between thumbnails created by
+ // the `draw` and `setImage` methods (fixes issue 8233).
+ // NOTE: To primarily avoid increasing memory usage too much, but also to
+ // reduce downsizing overhead, we purposely limit the up-scaling factor.
+ const { ctx, canvas, transform } = this._getPageDrawContext(
+ DRAW_UPSCALE_FACTOR
+ );
+ const drawViewport = this.viewport.clone({
+ scale: DRAW_UPSCALE_FACTOR * this.scale,
+ });
const renderContinueCallback = cont => {
if (!this.renderingQueue.isHighestPriority(this)) {
this.renderingState = RenderingStates.PAUSED;
@@ -356,20 +361,24 @@ class PDFThumbnailView {
const resultPromise = renderTask.promise.then(
function () {
- finishRenderTask(null);
+ return finishRenderTask(null);
},
function (error) {
- finishRenderTask(error);
+ return finishRenderTask(error);
}
);
- // Only trigger cleanup, once rendering has finished, when the current
- // pageView is *not* cached on the `BaseViewer`-instance.
resultPromise.finally(() => {
+ // Zeroing the width and height causes Firefox to release graphics
+ // resources immediately, which can greatly reduce memory consumption.
+ canvas.width = 0;
+ canvas.height = 0;
+
+ // Only trigger cleanup, once rendering has finished, when the current
+ // pageView is *not* cached on the `BaseViewer`-instance.
const pageCached = this.linkService.isPageCached(this.id);
- if (pageCached) {
- return;
+ if (!pageCached) {
+ this.pdfPage?.cleanup();
}
- this.pdfPage?.cleanup();
});
return resultPromise;
@@ -382,18 +391,23 @@ class PDFThumbnailView {
if (this.renderingState !== RenderingStates.INITIAL) {
return;
}
- const img = pageView.canvas;
- if (!img) {
+ const { canvas, pdfPage } = pageView;
+ if (!canvas) {
return;
}
if (!this.pdfPage) {
- this.setPdfPage(pageView.pdfPage);
+ this.setPdfPage(pdfPage);
}
-
this.renderingState = RenderingStates.FINISHED;
+ this._convertCanvasToImage(canvas);
+ }
+
+ /**
+ * @private
+ */
+ _reduceImage(img) {
+ const { ctx, canvas } = this._getPageDrawContext();
- const [ctx] = this._getPageDrawContext();
- const canvas = ctx.canvas;
if (img.width <= 2 * canvas.width) {
ctx.drawImage(
img,
@@ -406,18 +420,15 @@ class PDFThumbnailView {
canvas.width,
canvas.height
);
- this._convertCanvasToImage();
- return;
+ return canvas;
}
-
// drawImage does an awful job of rescaling the image, doing it gradually.
let reducedWidth = canvas.width << MAX_NUM_SCALING_STEPS;
let reducedHeight = canvas.height << MAX_NUM_SCALING_STEPS;
- const reducedImage = TempImageFactory.getCanvas(
+ const [reducedImage, reducedImageCtx] = TempImageFactory.getCanvas(
reducedWidth,
reducedHeight
);
- const reducedImageCtx = reducedImage.getContext("2d");
while (reducedWidth > img.width || reducedHeight > img.height) {
reducedWidth >>= 1;
@@ -460,7 +471,7 @@ class PDFThumbnailView {
canvas.width,
canvas.height
);
- this._convertCanvasToImage();
+ return canvas;
}
get _thumbPageTitle() {
@@ -492,7 +503,7 @@ class PDFThumbnailView {
this._thumbPageCanvas.then(msg => {
if (this.image) {
this.image.setAttribute("aria-label", msg);
- } else if (this.disableCanvasToImageConversion && this.canvas) {
+ } else if (this.canvas) {
this.canvas.setAttribute("aria-label", msg);
}
});