From 1465b1670fe518a3b8f97b15b79f66c010b97d14 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 15 Aug 2021 19:28:47 +0200 Subject: [PATCH 1/2] [src/display/api.js] Move the `getRenderingIntent` helper function into `WorkerTransport` By doing this re-factoring *separately*, since it's mostly a mechanical change, the size/scope of the next patch will be reduced somewhat. --- src/display/api.js | 58 +++++++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 26 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index 1fa6367e3..086de8f72 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -515,29 +515,6 @@ function _fetchDocument(worker, source, pdfDataRangeTransport, docId) { }); } -function getRenderingIntent(intent, { renderForms = false, isOpList = false }) { - let renderingIntent = RenderingIntentFlag.DISPLAY; // Default value. - switch (intent) { - case "any": - renderingIntent = RenderingIntentFlag.ANY; - break; - case "display": - break; - case "print": - renderingIntent = RenderingIntentFlag.PRINT; - break; - default: - warn(`getRenderingIntent - invalid intent: ${intent}`); - } - if (renderForms) { - renderingIntent += RenderingIntentFlag.ANNOTATION_FORMS; - } - if (isOpList) { - renderingIntent += RenderingIntentFlag.OPLIST; - } - return renderingIntent; -} - /** * @typedef {Object} OnProgressParameters * @property {number} loaded - Currently loaded number of bytes. @@ -1302,7 +1279,7 @@ class PDFPageProxy { * {Array} of the annotation objects. */ getAnnotations({ intent = "display" } = {}) { - const renderingIntent = getRenderingIntent(intent, {}); + const renderingIntent = this._transport.getRenderingIntent(intent, {}); let promise = this._annotationPromises.get(renderingIntent); if (!promise) { @@ -1358,7 +1335,7 @@ class PDFPageProxy { this._stats.time("Overall"); } - const renderingIntent = getRenderingIntent(intent, { + const renderingIntent = this._transport.getRenderingIntent(intent, { renderForms: renderInteractiveForms === true, }); // If there was a pending destroy, cancel it so no cleanup happens during @@ -1497,7 +1474,9 @@ class PDFPageProxy { } } - const renderingIntent = getRenderingIntent(intent, { isOpList: true }); + const renderingIntent = this._transport.getRenderingIntent(intent, { + isOpList: true, + }); let intentState = this._intentStates.get(renderingIntent); if (!intentState) { intentState = Object.create(null); @@ -2354,6 +2333,33 @@ class WorkerTransport { return shadow(this, "annotationStorage", new AnnotationStorage()); } + getRenderingIntent(intent, { renderForms = false, isOpList = false }) { + let renderingIntent = RenderingIntentFlag.DISPLAY; // Default value. + + switch (intent) { + case "any": + renderingIntent = RenderingIntentFlag.ANY; + break; + case "display": + break; + case "print": + renderingIntent = RenderingIntentFlag.PRINT; + break; + default: + warn(`getRenderingIntent - invalid intent: ${intent}`); + } + + if (renderForms) { + renderingIntent += RenderingIntentFlag.ANNOTATION_FORMS; + } + + if (isOpList) { + renderingIntent += RenderingIntentFlag.OPLIST; + } + + return renderingIntent; + } + destroy() { if (this.destroyCapability) { return this.destroyCapability.promise; From a7f0301f2181646e97bcea4c9b97e2cfe6c07e22 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 15 Aug 2021 19:57:42 +0200 Subject: [PATCH 2/2] [Regression] Re-factor the *internal* `includeAnnotationStorage` handling, since it's currently subtly wrong *This patch is very similar to the recently fixed `renderInteractiveForms`-options, see PR 13867.* As far as I can tell, this *subtle* bug has existed ever since `AnnotationStorage`-support was first added in PR 12106 (a little over a year ago). The value of the `includeAnnotationStorage`-option, as passed to the `PDFPageProxy.render` method, will (potentially) affect the size/content of the operatorList that's returned from the worker (for documents with forms). Given that operatorLists will generally, unless they contain huge images, be cached in the API, repeated `PDFPageProxy.render` calls where the form-data has been changed by the user in between, can thus *wrongly* return a cached operatorList. In the viewer we're only using the `includeAnnotationStorage`-option when printing, which is probably why this has gone unnoticed for so long. Note that we, for performance reasons, don't cache printing-operatorLists in the API. However, there's nothing stopping an API-user from using the `includeAnnotationStorage`-option during "normal" rendering, which could thus result in *subtle* (and difficult to understand) rendering bugs. In order to handle this, we need to know if the `AnnotationStorage`-instance has been updated since the last `PDFPageProxy.render` call. The most "correct" solution would obviously be to create a hash of the `AnnotationStorage` contents, however that would require adding a bunch of code, complexity, and runtime overhead. Given that operatorList caching in the API doesn't have to be perfect[1], but only have to avoid *false* cache-hits, we can simplify things significantly be only keeping track of the last time that the `AnnotationStorage`-data was modified. *Please note:* While working on this patch, I also noticed that the `renderInteractiveForms`- and `includeAnnotationStorage`-options in the `PDFPageProxy.render` method are mutually exclusive.[2] Given that the various Annotation-related options in `PDFPageProxy.render` have been added at different times, this has unfortunately led to the current "messy" situation.[3] --- [1] Note how we're already not caching operatorLists for pages with *huge* images, in order to save memory, hence there's no guarantee that operatorLists will always be cached. [2] Setting both to `true` will result in undefined behaviour, since trying to insert `AnnotationStorage`-values into fields that are being excluded from the operatorList-building will obviously not work, which isn't at all clear from the documentation. [3] My intention is to try and fix this in a follow-up PR, and I've got a WIP patch locally, however it will result in a number of API-observable changes. --- .eslintrc | 1 + src/core/document.js | 13 +++- src/core/worker.js | 1 + src/display/annotation_storage.js | 15 +++- src/display/api.js | 120 +++++++++++++++++------------- src/shared/util.js | 14 +++- 6 files changed, 107 insertions(+), 57 deletions(-) diff --git a/.eslintrc b/.eslintrc index fe0717c23..d1744b65f 100644 --- a/.eslintrc +++ b/.eslintrc @@ -49,6 +49,7 @@ "unicorn/no-new-buffer": "error", "unicorn/no-instanceof-array": "error", "unicorn/no-useless-spread": "error", + "unicorn/prefer-date-now": "error", "unicorn/prefer-string-starts-ends-with": "error", // Possible errors diff --git a/src/core/document.js b/src/core/document.js index 361a9acab..f8455f5e4 100644 --- a/src/core/document.js +++ b/src/core/document.js @@ -320,7 +320,14 @@ class Page { }); } - getOperatorList({ handler, sink, task, intent, annotationStorage }) { + getOperatorList({ + handler, + sink, + task, + intent, + cacheKey, + annotationStorage = null, + }) { const contentStreamPromise = this.getContentStream(handler); const resourcesPromise = this.loadResources([ "ColorSpace", @@ -354,7 +361,7 @@ class Page { this.nonBlendModesSet ), pageIndex: this.pageIndex, - intent, + cacheKey, }); return partialEvaluator @@ -377,7 +384,7 @@ class Page { pageOpList.flush(true); return { length: pageOpList.totalLength }; } - const renderForms = !!(intent & RenderingIntentFlag.ANNOTATION_FORMS), + const renderForms = !!(intent & RenderingIntentFlag.ANNOTATIONS_FORMS), intentAny = !!(intent & RenderingIntentFlag.ANY), intentDisplay = !!(intent & RenderingIntentFlag.DISPLAY), intentPrint = !!(intent & RenderingIntentFlag.PRINT); diff --git a/src/core/worker.js b/src/core/worker.js index 949044d08..6a98cb5ef 100644 --- a/src/core/worker.js +++ b/src/core/worker.js @@ -688,6 +688,7 @@ class WorkerMessageHandler { sink, task, intent: data.intent, + cacheKey: data.cacheKey, annotationStorage: data.annotationStorage, }) .then( diff --git a/src/display/annotation_storage.js b/src/display/annotation_storage.js index e1d9c1744..f4f5c52ca 100644 --- a/src/display/annotation_storage.js +++ b/src/display/annotation_storage.js @@ -21,6 +21,7 @@ import { objectFromMap } from "../shared/util.js"; class AnnotationStorage { constructor() { this._storage = new Map(); + this._timeStamp = Date.now(); this._modified = false; // Callbacks to signal when the modification state is set or reset. @@ -41,8 +42,7 @@ class AnnotationStorage { * @returns {Object} */ getValue(key, defaultValue) { - const obj = this._storage.get(key); - return obj !== undefined ? obj : defaultValue; + return this._storage.get(key) ?? defaultValue; } /** @@ -64,10 +64,11 @@ class AnnotationStorage { } } } else { - this._storage.set(key, value); modified = true; + this._storage.set(key, value); } if (modified) { + this._timeStamp = Date.now(); this._setModified(); } } @@ -108,6 +109,14 @@ class AnnotationStorage { get serializable() { return this._storage.size > 0 ? this._storage : null; } + + /** + * PLEASE NOTE: Only intended for usage within the API itself. + * @ignore + */ + get lastModified() { + return this._timeStamp.toString(); + } } export { AnnotationStorage }; diff --git a/src/display/api.js b/src/display/api.js index 086de8f72..c072999ff 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -1279,15 +1279,15 @@ class PDFPageProxy { * {Array} of the annotation objects. */ getAnnotations({ intent = "display" } = {}) { - const renderingIntent = this._transport.getRenderingIntent(intent, {}); + const intentArgs = this._transport.getRenderingIntent(intent, {}); - let promise = this._annotationPromises.get(renderingIntent); + let promise = this._annotationPromises.get(intentArgs.cacheKey); if (!promise) { promise = this._transport.getAnnotations( this._pageIndex, - renderingIntent + intentArgs.renderingIntent ); - this._annotationPromises.set(renderingIntent, promise); + this._annotationPromises.set(intentArgs.cacheKey, promise); } return promise; } @@ -1335,8 +1335,9 @@ class PDFPageProxy { this._stats.time("Overall"); } - const renderingIntent = this._transport.getRenderingIntent(intent, { + const intentArgs = this._transport.getRenderingIntent(intent, { renderForms: renderInteractiveForms === true, + includeAnnotationStorage: includeAnnotationStorage === true, }); // If there was a pending destroy, cancel it so no cleanup happens during // this call to render. @@ -1346,10 +1347,10 @@ class PDFPageProxy { optionalContentConfigPromise = this._transport.getOptionalContentConfig(); } - let intentState = this._intentStates.get(renderingIntent); + let intentState = this._intentStates.get(intentArgs.cacheKey); if (!intentState) { intentState = Object.create(null); - this._intentStates.set(renderingIntent, intentState); + this._intentStates.set(intentArgs.cacheKey, intentState); } // Ensure that a pending `streamReader` cancel timeout is always aborted. @@ -1361,9 +1362,9 @@ class PDFPageProxy { const canvasFactoryInstance = canvasFactory || new DefaultCanvasFactory({ ownerDocument: this._ownerDocument }); - const annotationStorage = includeAnnotationStorage - ? this._transport.annotationStorage.serializable - : null; + const intentPrint = !!( + intentArgs.renderingIntent & RenderingIntentFlag.PRINT + ); // If there's no displayReadyCapability yet, then the operatorList // was never requested before. Make the request and create the promise. @@ -1378,11 +1379,7 @@ class PDFPageProxy { if (this._stats) { this._stats.time("Page Request"); } - this._pumpOperatorList({ - pageIndex: this._pageIndex, - intent: renderingIntent, - annotationStorage, - }); + this._pumpOperatorList(intentArgs); } const complete = error => { @@ -1390,10 +1387,7 @@ class PDFPageProxy { // Attempt to reduce memory usage during *printing*, by always running // cleanup once rendering has finished (regardless of cleanupAfterRender). - if ( - this.cleanupAfterRender || - renderingIntent & RenderingIntentFlag.PRINT - ) { + if (this.cleanupAfterRender || intentPrint) { this.pendingCleanup = true; } this._tryCleanup(); @@ -1403,7 +1397,7 @@ class PDFPageProxy { this._abortOperatorList({ intentState, - reason: error, + reason: error instanceof Error ? error : new Error(error), }); } else { internalRenderTask.capability.resolve(); @@ -1429,7 +1423,7 @@ class PDFPageProxy { operatorList: intentState.operatorList, pageIndex: this._pageIndex, canvasFactory: canvasFactoryInstance, - useRequestAnimationFrame: !(renderingIntent & RenderingIntentFlag.PRINT), + useRequestAnimationFrame: !intentPrint, pdfBug: this._pdfBug, }); @@ -1474,13 +1468,13 @@ class PDFPageProxy { } } - const renderingIntent = this._transport.getRenderingIntent(intent, { + const intentArgs = this._transport.getRenderingIntent(intent, { isOpList: true, }); - let intentState = this._intentStates.get(renderingIntent); + let intentState = this._intentStates.get(intentArgs.cacheKey); if (!intentState) { intentState = Object.create(null); - this._intentStates.set(renderingIntent, intentState); + this._intentStates.set(intentArgs.cacheKey, intentState); } let opListTask; @@ -1498,10 +1492,7 @@ class PDFPageProxy { if (this._stats) { this._stats.time("Page Request"); } - this._pumpOperatorList({ - pageIndex: this._pageIndex, - intent: renderingIntent, - }); + this._pumpOperatorList(intentArgs); } return intentState.opListReadCapability.promise; } @@ -1584,14 +1575,14 @@ class PDFPageProxy { this._transport.pageCache[this._pageIndex] = null; const waitOn = []; - for (const [intent, intentState] of this._intentStates) { + for (const intentState of this._intentStates.values()) { this._abortOperatorList({ intentState, reason: new Error("Page was destroyed."), force: true, }); - if (intent & RenderingIntentFlag.OPLIST) { + if (intentState.opListReadCapability) { // Avoid errors below, since the renderTasks are just stubs. continue; } @@ -1649,8 +1640,8 @@ class PDFPageProxy { /** * @private */ - _startRenderPage(transparency, intent) { - const intentState = this._intentStates.get(intent); + _startRenderPage(transparency, cacheKey) { + const intentState = this._intentStates.get(cacheKey); if (!intentState) { return; // Rendering was cancelled. } @@ -1688,19 +1679,32 @@ class PDFPageProxy { /** * @private */ - _pumpOperatorList(args) { - assert( - args.intent, - 'PDFPageProxy._pumpOperatorList: Expected "intent" argument.' - ); + _pumpOperatorList({ renderingIntent, cacheKey }) { + if ( + typeof PDFJSDev === "undefined" || + PDFJSDev.test("!PRODUCTION || TESTING") + ) { + assert( + Number.isInteger(renderingIntent) && renderingIntent > 0, + '_pumpOperatorList: Expected valid "renderingIntent" argument.' + ); + } const readableStream = this._transport.messageHandler.sendWithStream( "GetOperatorList", - args + { + pageIndex: this._pageIndex, + intent: renderingIntent, + cacheKey, + annotationStorage: + renderingIntent & RenderingIntentFlag.ANNOTATIONS_STORAGE + ? this._transport.annotationStorage.serializable + : null, + } ); const reader = readableStream.getReader(); - const intentState = this._intentStates.get(args.intent); + const intentState = this._intentStates.get(cacheKey); intentState.streamReader = reader; const pump = () => { @@ -1749,11 +1753,15 @@ class PDFPageProxy { * @private */ _abortOperatorList({ intentState, reason, force = false }) { - assert( - reason instanceof Error || - (typeof reason === "object" && reason !== null), - 'PDFPageProxy._abortOperatorList: Expected "reason" argument.' - ); + if ( + typeof PDFJSDev === "undefined" || + PDFJSDev.test("!PRODUCTION || TESTING") + ) { + assert( + reason instanceof Error, + '_abortOperatorList: Expected valid "reason" argument.' + ); + } if (!intentState.streamReader) { return; @@ -1787,9 +1795,9 @@ class PDFPageProxy { } // Remove the current `intentState`, since a cancelled `getOperatorList` // call on the worker-thread cannot be re-started... - for (const [intent, curIntentState] of this._intentStates) { + for (const [curCacheKey, curIntentState] of this._intentStates) { if (curIntentState === intentState) { - this._intentStates.delete(intent); + this._intentStates.delete(curCacheKey); break; } } @@ -2333,8 +2341,12 @@ class WorkerTransport { return shadow(this, "annotationStorage", new AnnotationStorage()); } - getRenderingIntent(intent, { renderForms = false, isOpList = false }) { + getRenderingIntent( + intent, + { renderForms = false, includeAnnotationStorage = false, isOpList = false } + ) { let renderingIntent = RenderingIntentFlag.DISPLAY; // Default value. + let lastModified = ""; switch (intent) { case "any": @@ -2350,14 +2362,22 @@ class WorkerTransport { } if (renderForms) { - renderingIntent += RenderingIntentFlag.ANNOTATION_FORMS; + renderingIntent += RenderingIntentFlag.ANNOTATIONS_FORMS; + } + if (includeAnnotationStorage) { + renderingIntent += RenderingIntentFlag.ANNOTATIONS_STORAGE; + + lastModified = this.annotationStorage.lastModified; } if (isOpList) { renderingIntent += RenderingIntentFlag.OPLIST; } - return renderingIntent; + return { + renderingIntent, + cacheKey: `${renderingIntent}_${lastModified}`, + }; } destroy() { @@ -2617,7 +2637,7 @@ class WorkerTransport { } const page = this.pageCache[data.pageIndex]; - page._startRenderPage(data.transparency, data.intent); + page._startRenderPage(data.transparency, data.cacheKey); }); messageHandler.on("commonobj", data => { diff --git a/src/shared/util.js b/src/shared/util.js index a696fe659..9371c3676 100644 --- a/src/shared/util.js +++ b/src/shared/util.js @@ -18,11 +18,23 @@ import "./compatibility.js"; const IDENTITY_MATRIX = [1, 0, 0, 1, 0, 0]; const FONT_IDENTITY_MATRIX = [0.001, 0, 0, 0.001, 0, 0]; +/** + * Refer to the `WorkerTransport.getRenderingIntent`-method in the API, to see + * how these flags are being used: + * - ANY, DISPLAY, and PRINT are the normal rendering intents, note the + * `PDFPageProxy.{render, getOperatorList, getAnnotations}`-methods. + * - ANNOTATIONS_FORMS, and ANNOTATIONS_STORAGE controls which annotations are + * rendered onto the canvas, note the `renderInteractiveForms`- respectively + * `includeAnnotationStorage`-options in the `PDFPageProxy.render`-method. + * - OPLIST is used with the `PDFPageProxy.getOperatorList`-method, note the + * `OperatorList`-constructor (on the worker-thread). + */ const RenderingIntentFlag = { ANY: 0x01, DISPLAY: 0x02, PRINT: 0x04, - ANNOTATION_FORMS: 0x20, + ANNOTATIONS_FORMS: 0x10, + ANNOTATIONS_STORAGE: 0x20, OPLIST: 0x100, };