Offload canvas drawing operations to a worker #20729
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #20729 +/- ##
==========================================
- Coverage 89.65% 87.25% -2.40%
==========================================
Files 260 261 +1
Lines 66010 70195 +4185
==========================================
+ Hits 59181 61251 +2070
- Misses 6829 8944 +2115
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @nicolo-ribaudo received. Current queue size: 0 Live output at: http://54.193.163.58:8877/e0227fb9789b076/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @nicolo-ribaudo received. Current queue size: 0 Live output at: http://54.241.84.105:8877/b6e4acc21cb49a8/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/b6e4acc21cb49a8/output.txt Total script time: 46.91 mins
Image differences available at: http://54.241.84.105:8877/b6e4acc21cb49a8/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/e0227fb9789b076/output.txt Total script time: 90.47 mins
Image differences available at: http://54.193.163.58:8877/e0227fb9789b076/reftest-analyzer.html#web=eq.log |
Aditi-1400
force-pushed
the
worker-drawing
branch
from
03ad6c3 to
1afc3d4
Compare
|
I was looking through the reftest failures, it seems like the only real ones (the others are minor pixel differences due to the different rendering pipeline, but not visible to humans) are:
|
Aditi-1400
force-pushed
the
worker-drawing
branch
4 times, most recently
from
1d6de6c to
43a73d7
Compare
There's also gradient difference in 17069, also there are differences in 19022, same as issue8092 |
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @nicolo-ribaudo received. Current queue size: 0 Live output at: http://54.193.163.58:8877/832219967f9b55d/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @nicolo-ribaudo received. Current queue size: 0 Live output at: http://54.241.84.105:8877/659eb95d2835e52/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/659eb95d2835e52/output.txt Total script time: 46.69 mins
Image differences available at: http://54.241.84.105:8877/659eb95d2835e52/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/832219967f9b55d/output.txt Total script time: 78.06 mins
Image differences available at: http://54.193.163.58:8877/832219967f9b55d/reftest-analyzer.html#web=eq.log |
Aditi-1400
force-pushed
the
worker-drawing
branch
2 times, most recently
from
134955c to
ba0fa2c
Compare
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @nicolo-ribaudo received. Current queue size: 0 Live output at: http://54.193.163.58:8877/528a274cb665c1e/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @nicolo-ribaudo received. Current queue size: 0 Live output at: http://54.241.84.105:8877/67316c0b179c2dc/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/67316c0b179c2dc/output.txt Total script time: 46.49 mins
Image differences available at: http://54.241.84.105:8877/67316c0b179c2dc/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/528a274cb665c1e/output.txt Total script time: 75.41 mins
Image differences available at: http://54.193.163.58:8877/528a274cb665c1e/reftest-analyzer.html#web=eq.log |
Aditi-1400
force-pushed
the
worker-drawing
branch
2 times, most recently
from
96dda63 to
cca0198
Compare
Aditi-1400
temporarily deployed
to
code-coverage
GitHub Actions
Inactive
From: Bot.io (Windows)ReceivedCommand cmd_browsertest from @Aditi-1400 received. Current queue size: 0 Live output at: http://54.193.163.58:8877/6d3106a5424b20c/output.txt |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/6d3106a5424b20c/output.txt Total script time: 1.68 mins
Image differences available at: http://54.193.163.58:8877/6d3106a5424b20c/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/0b2cff5f6dbf79c/output.txt Total script time: 19.74 mins
|
|
I've updated the canvas filter detection in commit c44cfbc to a minimal detection, from the commit message: Also drops the annotation-appearance scan in document.js. The earlier shape forced StartRenderPage to wait on _parsedAnnotation, as Jonas pointed out, it was making it slower, the first paint is now no longer gated on annotation parsing. TR in annotation appearance streams should be rare enough that the correctness concession is acceptable. The walker now covers page-level ExtGState plus Form XObject Resources, which captures the realistic cases for TR while keeping the per-page scan. |
| if (graphicStates instanceof Dict) { | ||
| for (let graphicState of graphicStates.getRawValues()) { | ||
| if (graphicState instanceof Ref) { | ||
| if (processed.has(graphicState)) { | ||
| continue; | ||
| } | ||
| try { | ||
| graphicState = xref.fetch(graphicState); | ||
| } catch (ex) { | ||
| info(`hasCanvasFilters - failed to fetch ExtGState: "${ex}".`); | ||
| // A fetch failure means we can't inspect the resource, so fall | ||
| // back to main-thread rendering rather than misclassify a corrupt | ||
| // PDF as filter-free. | ||
| return true; | ||
| } | ||
| } | ||
| if (!(graphicState instanceof Dict)) { | ||
| continue; | ||
| } | ||
| if (graphicState.objId) { | ||
| processed.put(graphicState.objId); | ||
| } | ||
| try { | ||
| if (this._hasTransferMaps(graphicState.get("TR"))) { | ||
| return true; | ||
| } | ||
| } catch (ex) { | ||
| info(`hasCanvasFilters - failed to inspect filter data: "${ex}".`); | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const xObjects = node.get("XObject"); | ||
| if (xObjects instanceof Dict) { | ||
| for (let xObject of xObjects.getRawValues()) { | ||
| if (xObject instanceof Ref) { | ||
| if (processed.has(xObject)) { | ||
| continue; | ||
| } | ||
| try { | ||
| xObject = xref.fetch(xObject); | ||
| } catch (ex) { | ||
| info(`hasCanvasFilters - failed to fetch XObject: "${ex}".`); | ||
| return true; | ||
| } | ||
| } | ||
| if (!(xObject instanceof BaseStream)) { | ||
| continue; | ||
| } | ||
| if (xObject.dict.objId) { | ||
| processed.put(xObject.dict.objId); | ||
| } | ||
| const xResources = xObject.dict.get("Resources"); | ||
| if (!(xResources instanceof Dict)) { | ||
| continue; | ||
| } | ||
| if (xResources.objId && processed.has(xResources.objId)) { | ||
| continue; | ||
| } | ||
|
|
||
| nodes.push(xResources); | ||
| if (xResources.objId) { | ||
| processed.put(xResources.objId); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The two loops look very similar, maybe we could have an helper function to avoid redundancy.
There was a problem hiding this comment.
I did try doing that, in one of the earlier versions of the patch, I think this is probably the commit which captures the idea, later though I felt it added too much complexity and reduced readability of the code without significant returns, so I isolated them, let me know if you prefer extracting the common bits though, it should be a minimal effort to go back to that approach
| this._canvas.resetWorkerCanvas = () => { | ||
| rendererHandler.send("ResetCanvas", { renderTaskId }); | ||
| }; | ||
| } catch (ex) { |
There was a problem hiding this comment.
What does happen if the worker throws ? this catch won't catch it, right ?
There was a problem hiding this comment.
Yes, however we are not expecting the worker to throw by this point, the worker rejecting is caught later at await initPromise → .catch(complete) → render promise rejects. This is only for setup failures, while/before transferring canvas since we want to fallback to main-thread there, which is not possible if the worker rejects so we just cancel the rendering in that case
@Snuffleupagus and it's worth mentioning that for now that rendering in the worker is using software rendering at least in Firefox (see https://bugzilla.mozilla.org/show_bug.cgi?id=2025755); I think that HWA is enabled in Chrome but I'm not sure. |
| try { | ||
| rendererHandler.send(action, data); | ||
| } catch { | ||
| // Ignore errors if the renderer worker has been destroyed. |
There was a problem hiding this comment.
What does happen if something somewhere is waiting for the result of this forwarding ?
| default: | ||
| throw new Error(`Got unknown object type ${type}`); | ||
| } | ||
| forwardToRenderer("obj", [id, pageIndex, type, imageData]); |
There was a problem hiding this comment.
imageData should be transfered or is there any reasons for cloning it ?
There was a problem hiding this comment.
We cannot transfer it unconditionally here because we also need a copy on the main-thread for fallback in case the worker rendering fails
| } | ||
| // Keep the renderer worker's document-level state in sync with the main | ||
| // thread. | ||
| this.rendererHandler?.send("Cleanup", { keepLoadedFonts }); |
There was a problem hiding this comment.
The above this.messageHandler.sendWithPromise("Cleanup", null); is awaited, why not this one ?
There was a problem hiding this comment.
Oh, because, it's simply just a send instead of sendWithPromise, nothing in the handler is async either, it's just cleanup to sync with main, and we are not waiting for this to finish in any of the steps later.
| // Worker rendering is also disabled when canvas filters (TR) are present | ||
| // because OffscreenCanvas's OffscreenCanvasRenderingContext2D ignores | ||
| // `.filter` values set from a data URL. See bug 2011237. | ||
| let useWorkerRendering = |
There was a problem hiding this comment.
We must really have a pref in app_options.js for enabling/disabling this feature.
| resources, | ||
| this.nonBlendModesSet | ||
| ), | ||
| hasCanvasFilters, |
There was a problem hiding this comment.
Why use a temporary variable here, rather than in-lining the call?
| hasCanvasFilters, | |
| hasCanvasFilters: partialEvaluator.hasCanvasFilters(resources), |
| // styleElement is only intended for development/testing purposes. | ||
| const styleElement = | ||
| typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING") | ||
| ? src.styleElement | ||
| : null; | ||
| // Custom DOM contexts are main-thread only and cannot be transferred to | ||
| // the renderer worker. | ||
| const hasCustomOwnerDocument = | ||
| src.ownerDocument !== undefined && | ||
| src.ownerDocument !== globalThis.document; | ||
| const hasCustomStyleElement = !!styleElement; | ||
| const disableWorkerRendering = | ||
| src.disableWorkerRendering === true || | ||
| typeof Worker === "undefined" || | ||
| typeof MessageChannel === "undefined" || | ||
| !FeatureTest.isOffscreenCanvasSupported || | ||
| hasCustomOwnerDocument || | ||
| hasCustomStyleElement; |
There was a problem hiding this comment.
Speaking as someone that's spent a bunch of time re-factoring API code to increase readability: Why are you just dumping a bunch of things here under the "wrong" sections?
| // styleElement is only intended for development/testing purposes. | |
| const styleElement = | |
| typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING") | |
| ? src.styleElement | |
| : null; | |
| // Custom DOM contexts are main-thread only and cannot be transferred to | |
| // the renderer worker. | |
| const hasCustomOwnerDocument = | |
| src.ownerDocument !== undefined && | |
| src.ownerDocument !== globalThis.document; | |
| const hasCustomStyleElement = !!styleElement; | |
| const disableWorkerRendering = | |
| src.disableWorkerRendering === true || | |
| typeof Worker === "undefined" || | |
| typeof MessageChannel === "undefined" || | |
| !FeatureTest.isOffscreenCanvasSupported || | |
| hasCustomOwnerDocument || | |
| hasCustomStyleElement; | |
| // Parameters only intended for development/testing purposes. | |
| const styleElement = | |
| typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING") | |
| ? src.styleElement | |
| : null; |
The rest should go at the end of the // Parameters whose default values depend on other parameters.-block below (and can also be simplified).
const disableWorkerRendering =
src.disableWorkerRendering === true ||
typeof Worker === "undefined" ||
typeof MessageChannel === "undefined" ||
!FeatureTest.isOffscreenCanvasSupported ||
ownerDocument !== globalThis.document ||
!!styleElement;There was a problem hiding this comment.
Apologies, this was an oversight on my part, I've fixed this and did a pass to look for any other places with similar issues, I fixed the ones I could find, I'll take another look.
Also, I would like to add, that I really appreciate the time and work you put into improving the readability, it's been very helpful to me and everyone else contributing, I intend to try my best to keep up the standard :)
| // When rendererHandler is used, the OptionalContentConfig needs to be | ||
| // transferred to the renderer | ||
| const optionalContentConfigDataPromise = this._transport.rendererHandler | ||
| ? this._transport.getOptionalContentConfigData() | ||
| : null; | ||
|
|
There was a problem hiding this comment.
Sorry to say, but the OptionalContentConfig handling in this patch is neither correct nor particularly readable.
Hence PR #21349 ought to land first, and this patch re-factored to use it, since that'll allow considerable simplification here.
There was a problem hiding this comment.
Oh thank you, this did make it much cleaner, I have rebased on top of this
| if (typeof OffscreenCanvas === "undefined") { | ||
| throw new Error("OffscreenCanvas is not supported"); | ||
| } |
There was a problem hiding this comment.
Is this check even necessary, since OffscreenCanvas support is checked in the API before enabling worker-rendering?
| const rendererWorkerFileConfig = createWebpackConfig(defines, { | ||
| filename: defines.MINIFIED ? "pdf.renderer.min.mjs" : "pdf.renderer.mjs", |
There was a problem hiding this comment.
Since this is a worker, it should be built correctly; compare with the "normal" pdf.worker.mjs file:
Lines 538 to 544 in b43ef1c
| const SVG_NS = "http://www.w3.org/2000/svg"; | ||
| const RENDERER_SRC = "../build/generic/build/pdf.renderer.mjs"; | ||
|
|
||
| GlobalWorkerOptions.rendererSrc = RENDERER_SRC; |
There was a problem hiding this comment.
Why not set this in the same place that the pre-existing workerSrc is set?
| * @param {RendererWorkerParameters} params - The worker initialization | ||
| * parameters. | ||
| */ | ||
| class RendererWorker { |
There was a problem hiding this comment.
It doesn't appear that the verbosity parameter is sent to the worker-thread, compare with the existing PDFWorker implementation, why not?
There was a problem hiding this comment.
This now gets sent to renderer worker.
|
The new |
| ? "resource://pdf.js/build/pdf.renderer.mjs" | ||
| : "../build/pdf.renderer.mjs"; | ||
| } | ||
|
|
There was a problem hiding this comment.
Why is this code placed here, since it really ought to be handled in the same way as the workerSrc instead?
Lines 553 to 563 in e9a946e
There was a problem hiding this comment.
Removed this block with build-target defaults was removed from api.js; RendererWorker.rendererSrc now throws if GlobalWorkerOptions.rendererSrc is unset, mirroring PDFWorker.workerSrc.
If rendererSrc is unset, the throw is caught, the worker capability rejects, and just disables worker rendering with a warning.
Move the commonobj/obj resolution logic from WorkerTransport.setupMessageHandler into a reusable ObjectHandler class. This enables sharing the object resolution logic between the main thread (WorkerTransport) and the renderer worker.
There was a problem hiding this comment.
As mentioned in #20729 (comment) the RendererWorker must report coverage data; note how #20729 (comment) reports significantly reduced coverage with this patch.
Edit: Also, the commit history should be cleaned-up before this lands by folding any "fixup" commits into their parent commits, in order to keep the commit history clean.
| renderTaskId: this._renderTaskId, | ||
| enableHWA: this._enableHWA, | ||
| enableWebGPU: this._enableWebGPU, | ||
| optionalContentConfig: optionalContentConfig?.serializable ?? null, |
There was a problem hiding this comment.
The optionalContentConfig must be available here, otherwise there's a bug somewhere else.
| optionalContentConfig: optionalContentConfig?.serializable ?? null, | |
| optionalContentConfig: optionalContentConfig.serializable, |
| const optionalContentConfig = data.optionalContentConfig | ||
| ? OptionalContentConfig.fromSerializable(data.optionalContentConfig) | ||
| : new OptionalContentConfig(null); |
There was a problem hiding this comment.
Again, it shouldn't be possible for the optionalContentConfig-data to be undefined here.
| const optionalContentConfig = data.optionalContentConfig | |
| ? OptionalContentConfig.fromSerializable(data.optionalContentConfig) | |
| : new OptionalContentConfig(null); | |
| const optionalContentConfig = | |
| OptionalContentConfig.fromSerializable(data.optionalContentConfig); |
| } | ||
| internalRenderTask.initializeGraphics({ | ||
| const { transparency, hasCanvasFilters = false } = | ||
| typeof renderPageData === "object" && renderPageData !== null |
There was a problem hiding this comment.
Nit: This can be shortened.
| typeof renderPageData === "object" && renderPageData !== null | |
| renderPageData && typeof renderPageData === "object" |
I'll do it in a follow-up. |
Introduce the RendererWorker class for offloading canvas rendering to a dedicated Web Worker. Alongside, it adds RendererMessageHandler, GlobalWorkerOptions.rendererSrc configuration, entrypoints for pdf.renderer.js bundle and build targets in gulpfile. No rendering changes are introduced in this commit, this is a setup for later commits that wire-up graphics execution and object forwarding.
Add a hasCanvasFilters method to PartialEvaluator that walks the page-level ExtGState dictionaries, and of Form XObject resources, to detect transfer functions (TR) that require DOM SVG filters. Such filters are unavailable on OffscreenCanvas, so detecting them up front lets the display layer fall back to main-thread rendering for affected pages. The detection is deliberately limited to TR: SMask rendering already has a pixel-buffer fallback in canvas.js, and TR inside tiling patterns, Type3 glyph streams or annotation appearance streams is rare enough in practice that walking those sub-resources (and gating first paint on annotation parsing) isn't worth it.
Add object forwarding so the renderer worker receives the same commonobj/obj messages as the main thread WorkerTransport now forwards commonobj/obj to the rendererHandler. Additionally, allow _startRenderPage propagates hasCanvasFilters from core layer.
…orker This decides whether to use worker rendering based on hasCanvasFilters, pageColors, dependency/image tracking. It transfers the canvas via transferControlToOffscreen and sends operator list chunks incrementally. In the renderer worker, it adds the functionality to initialize graphics and execute the operator list. Since operator lists are now posted across threads, Path2D objects are no longer materialized into argsArray; they are cached in a pathCache map on the operator list instead, keeping it structured-cloneable.
Update the viewer and tests to handle canvases that have been transferred to an OffscreenCanvas via the renderer worker.
Thread the enableWebGPU flag from getDocument() through WorkerTransport and InternalRenderTask to the renderer worker's InitializeGraphics handler, where it triggers GPU device initialization.
8 participants
Commit descriptions
Note that commit hashes might change in the future due to commit edits later
1. 34ab2b6: Extract ObjectHandler from WorkerTransport …
Notable changes:
this.shouldCreatePageObjsis added in object handler, it does not affect the main-thread rendering, it is set to false, but for worker-rendering, the renderer worker keeps only Map<pageIndex, PDFObjects>, not PDFPageProxy instances. So object_handler.js (line 126) has to handle both shapes.The
shouldCreatePageObjspart exists because renderer-worker obj messages can arrive before InitializeGraphics has called#getPageObjs(pageIndex). In that case the renderer still must cache the image/pattern object, otherwise later CanvasGraphics will hit an unresolved dependency while executing the operator list.Relevent code:
Open questions:
ObjectHandlerclass?2. 741ca8d: Adds RendererWorker class for offloading canvas …
Notable changes
This commit just sets up the renderer worker while following the same pattern as PDFWorker setup.
disableWorkerRenderingfor disabling the worker-rendering. Note that the flag is only checked once to check whether we should set up theRendererHandler, all the further decisions to use the worker-rendering are deferred to whetherRendererHandleris notnull.WorkerAPI,OffScreenCanvasis not supported, a customownerDocumentis present, since custom ownerDocument can be iframe document etc. which cannot be transferred to a worker and the worker cannot create DOM nodes inside it. Same forstyleElement, it is used byFontLoader, and is also a DOM element.In the renderer worker, fonts would instead be loaded via the FontFace API (self.fonts.add()). When a custom styleElement is provided (testing scenarios), it forces CSS-based font loading instead of the FontFace API. Since CSS font loading doesn't work in a worker context, the renderer worker can't render fonts correctly if styleElement is in use.
3. c28992d: Add canvas filter detection in core layer …
Due to bug https://bugzilla.mozilla.org/show_bug.cgi?id=2011237, since there's no DOM access from a worker the filter has to be defined with an external URL which is not currently supported in
OffscreenCanvasRenderingContext2DNotable changes:
hasCanvasFiltersmethod is very similar tohasBlendModeswhere both of these methods traverse the resource graph to check the presence of canvas filters.hasCanvasFiltershowever returnstrueconservatively. I considererd reusing some of the code fromhasBlendModesbut that made the patch more complex and hard to read.4. ffe8e9a: Add object forwarding between main thread and renderer worker …
Open questions
At present the way renderer works is we use the main thread for forwarding the objs/commonObjs and font fallback instead of PDFworker directly sending it to the renderer worker. So
objs/commonObjsare duplicated and also adds an additional hop.The commonobjs/objs are still being duplicated for sending to each of main-thread and renderer, which can be fixed in the following two ways:
a. Use SAB
b. Move the InternalRenderTask entirely to the renderer worker, I think this is possible and this is the approach I would prefer, to not have main thread deal with objs/commonObjs at all, but this is a much larger refactor which I am not sure should be a part of this patch.
While fixing a. appears to be somewhat easy, I have tried with SAB and the current version of sending to both main and renderer is not the best approach because there is a race-condition that it introduces that causes browser tests to almost always timeout, I've tried fixing several but so far, the tests still timeout, if we want to remove the forwarding, I can spend more time looking into forwarding, however I think if we remove the dependency on main thread entirely, it should fix both issues.
For now there's a TODO comment to remove the forwarding in the future.
5. 9ddbd41: Add graphics initialization and operator list execution in renderer worker …
Notable changes
a.
keepRendererCanvas: This flag is added to to still clear normal page state, but ask the renderer worker to keep the transferred canvas alive. This lets PDF.js free operator lists, page objects, fonts/images tied to the page, etc., without making already-rendered visible pages go blank when scrolling etc.b. In main-thread rendering, CanvasGraphics gets the actual OptionalContentConfig instance directly. In renderer-worker rendering, that object cannot be sent as-is: it has class methods/private fields. There is a change to pass the plain data we receive from PDFWorker and rebuilding the object on the worker side.
c. We also need to transfer the annotation canvases, so we find the annotations with
hasOwnCanvas, and send it to renderer worker. It also caches the transferred canvases in_transferredAnnotationCanvasIds.d. Worker rendering is disabled when we have canvas filters and page colors for reasons described above and it falls back to main-thread rendering. It is also disabled when dependencyTracker and imagesTracker are present, this would require making them transferable, which is not too complex and can be done in future iterations.
e. Operator list is sent to renderer worker in chunks. Sending the full growing operator list every time would be very expensive. So the main thread only sends the new tail of the operator list: from the last sent index to the current length. After the renderer worker receives it, it appends that partition to its own worker-local operator list.
7. f085c07: Adapt viewer and tests for OffscreenCanvas renderer worker …
Notable changes
getImageDataon the temporary canvas.