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.75% 88.94% -0.82%
==========================================
Files 262 263 +1
Lines 66727 67087 +360
==========================================
- Hits 59893 59671 -222
- Misses 6834 7416 +582
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 |
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:
|
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 |
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 |
96dda63 to
cca0198
Compare
GitHub Actions
Inactive
| ? "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.
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. |
| static #appendOperatorList(renderTaskState, fnArray, argsArray, lastChunk) { | ||
| const { operatorList } = renderTaskState; | ||
| if (fnArray) { | ||
| operatorList.fnArray.push(...fnArray); |
There was a problem hiding this comment.
It could fails with a large array, so it should be a little more robust.
| #worker = null; | ||
|
|
||
| #rendererHandler = null; | ||
|
|
||
| #capability = Promise.withResolvers(); |
There was a problem hiding this comment.
Why are these fields not sorted alphabetically?
| class RendererWorker { | ||
| #worker = null; | ||
|
|
||
| #rendererHandler = null; |
There was a problem hiding this comment.
Why is this not called #messageHandler, since that's what it'll contain?
| try { | ||
| const { rendererSrc } = RendererWorker; | ||
| const worker = new Worker(rendererSrc, { type: "module" }); |
There was a problem hiding this comment.
Given that this code looks like it was just copied from the existing PDFWorker implementation, why isn't this fully consistent with the following code?
| try { | |
| const { rendererSrc } = RendererWorker; | |
| const worker = new Worker(rendererSrc, { type: "module" }); | |
| let { rendererSrc } = RendererWorker; | |
| try { | |
| // Wraps rendererSrc path into blob URL, if the former does not belong | |
| // to the same origin. | |
| if ( | |
| typeof PDFJSDev !== "undefined" && | |
| PDFJSDev.test("GENERIC") && | |
| !PDFWorker._isSameOrigin(window.location, rendererSrc) | |
| ) { | |
| rendererSrc = PDFWorker._createCDNWrapper( | |
| new URL(rendererSrc, window.location).href | |
| ); | |
| } | |
| const worker = new Worker(rendererSrc, { type: "module" }); |
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.
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.
Forwarded CopyLocalImage messages carry no pixel data, so the renderer worker could stall waiting for an image the core worker never re-sent. Forward the main thread's decoded image when the renderer worker's local copy fails.
…derer worker Clear a page's objects before dropping them in cleanupPage, so ImageBitmaps are closed and not leaked.
…derer worker Register the render-task state before awaiting GPU initialization, so a cleanup arriving during initGPU() can abort the task instead of being ignored.
…derer worker A function-valued operationsFilter cannot be structured-cloned and was silently dropped. Send a precomputed mask for the ops in each chunk instead.
Rendering partialCrop tasks directly into the test canvas bypassed the renderer worker. Always render into a separate canvas and copy the result back for cropping.
getContext("2d") throws once a canvas has been transferred to the
renderer worker; read the pixels via createImageBitmap instead.
Read RendererWorker.rendererSrc inside the try-block, so a missing GlobalWorkerOptions.rendererSrc rejects the capability instead of throwing synchronously.
…derer worker A caller that supplies its own canvasContext expects the pixels to land there; gate worker rendering on the absence of canvasContext rather than on a canvas being passed.
Rename #rendererHandler to #messageHandler, matching PDFWorker.
Rename #worker to #webWorker, matching PDFWorker, and register the initialize() listeners in a consistent order.
Reorder the static class members alphabetically and move the public initializeFromPort() below the private methods.
Reorder the rendererSrc getter/setter alphabetically.
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.