-
Notifications
You must be signed in to change notification settings - Fork 10k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure that textLayers can be rendered in parallel, without interfering with each other #18731
Ensure that textLayers can be rendered in parallel, without interfering with each other #18731
Conversation
7a4cc9a
to
4504e63
Compare
6035174
to
92cc697
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/99e4628a7c07bd7/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/7a74393f4fc2ab2/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/99e4628a7c07bd7/output.txt Total script time: 31.25 mins
Image differences available at: http://54.241.84.105:8877/99e4628a7c07bd7/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/7a74393f4fc2ab2/output.txt Total script time: 46.50 mins
Image differences available at: http://54.193.163.58:8877/7a74393f4fc2ab2/reftest-analyzer.html#web=eq.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the test.
LGTM.
…ng with each other Note that the textContent is returned in "chunks" from the API, through the use of `ReadableStream`s, and on the main-thread we're (normally) using just one temporary canvas in order to measure the size of the textLayer `span`s; see the [`#layout`](https://github.com/mozilla/pdf.js/blob/5b4c2fe1a845169ac2b4f8f6335337c434077637/src/display/text_layer.js#L396-L428) method. *Order of events, for parallel textLayer rendering:* 1. Call [`render`](https://github.com/mozilla/pdf.js/blob/5b4c2fe1a845169ac2b4f8f6335337c434077637/src/display/text_layer.js#L155-L177) of the textLayer for page A. 2. Immediately call `render` of the textLayer for page B. 3. The first text-chunk for pageA arrives, and it's parsed/layout which means updating the cached [fontSize/fontFamily](https://github.com/mozilla/pdf.js/blob/5b4c2fe1a845169ac2b4f8f6335337c434077637/src/display/text_layer.js#L409-L413) for the textLayer of page A. 4. The first text-chunk for pageB arrives, which means updating the cached fontSize/fontFamily *for the textLayer of page B* since this data is unique to each `TextLayer`-instance. 5. The second text-chunk for pageA arrives, and we don't update the canvas-font since the cached fontSize/fontFamily still apply from step 3 above. Where this potentially breaks down is between the last steps, since we're using just one temporary canvas for all measurements but have *individual* fontSize/fontFamily caches for each textLayer. Hence it's possible that the canvas-font has actually changed, despite the cached values suggesting otherwise, and to address this we instead cache the fontSize/fontFamily globally through a new (static) helper method. *Note:* Includes a basic unit-test, using dummy text-content, which fails on `master` and passes with this patch. Finally, pun intended, ensure that temporary textLayer-data is cleared *before* the `render`-promise resolves to avoid any intermittent problems in the unit-tests.
92cc697
to
5b3d3c7
Compare
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/b81d004fdda1f43/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/97a53bd4c55d06e/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/b81d004fdda1f43/output.txt Total script time: 30.98 mins
Image differences available at: http://54.241.84.105:8877/b81d004fdda1f43/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/97a53bd4c55d06e/output.txt Total script time: 47.37 mins
Image differences available at: http://54.193.163.58:8877/97a53bd4c55d06e/reftest-analyzer.html#web=eq.log |
Note that the textContent is returned in "chunks" from the API, through the use of
ReadableStream
s, and on the main-thread we're (normally) using just one temporary canvas in order to measure the size of the textLayerspan
s; see the#layout
method.Order of events, for parallel textLayer rendering:
render
of the textLayer for page A.render
of the textLayer for page B.TextLayer
-instance.Where this potentially breaks down is between the last steps, since we're using just one temporary canvas for all measurements but have individual fontSize/fontFamily caches for each textLayer.
Hence it's possible that the canvas-font has actually changed, despite the cached values suggesting otherwise, and to address this we instead cache the fontSize/fontFamily globally through a new (static) helper method.
Note: Includes a basic unit-test, using dummy text-content, which fails on
master
and passes with this patch.Finally, pun intended, ensure that temporary textLayer-data is cleared before the
render
-promise resolves to avoid any intermittent problems in the unit-tests.Closes #18709