Makes PDF data reading Streams API friendly. #6879

yurydelendik · 2016-01-15T17:43:44Z

Adds PDFWorkerStream and PDFNetworkStream with classes that can provide readers (that are compatible with ReadableByteStream, see Streams API).

In the future PDFNetworkStream can be re-written to support Fetch API. The PDFWorkerStream can be refactored to be more lightweight and move most of I/O logic to the main thread.

yurydelendik · 2016-01-15T17:44:57Z

/cc @Rob--W shall help with #5319

Snuffleupagus · 2016-01-15T19:39:16Z

src/core/network.js

+    }
+  };
+
+  function PDFNetworkStreamRangeRequest(manager, begin, end) {


Nit: for consistency with PDFNetworkStreamFullRequestReader, shouldn't this be named PDFNetworkStreamRangeRequestReader instead?

Rob--W · 2016-01-15T23:17:48Z

Which such a refactor, I'd like to see more unit tests that verifies that the network layer works as intended. That makes it much easier to build upon this interface (e.g. adding support for the streams API).

yurydelendik · 2016-02-09T21:46:26Z

/botio test

pdfjsbot · 2016-02-09T21:46:27Z

From: Bot.io (Linux)

Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://107.21.233.14:8877/e8254fd4aa4c7cc/output.txt

pdfjsbot · 2016-02-09T21:46:27Z

From: Bot.io (Windows)

Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/b9ed08ef04529cc/output.txt

pdfjsbot · 2016-02-09T22:07:04Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/b9ed08ef04529cc/output.txt

Total script time: 20.61 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2016-02-09T22:08:12Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/e8254fd4aa4c7cc/output.txt

Total script time: 21.75 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

Rob--W · 2016-02-11T20:26:24Z

src/shared/util.js

+
+function arraysToBytes(arr) {
+  if (arr.length === 1 && (arr instanceof Uint8Array)) {
+    return arr[0]; // shortcut


Why are you returning a 8-bit integer here? Or should arr instanceof be arr[0] instanceof ?

yurydelendik · 2016-02-11T23:15:41Z

/botio test

pdfjsbot · 2016-02-11T23:15:43Z

From: Bot.io (Linux)

Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://107.21.233.14:8877/4db634964845804/output.txt

pdfjsbot · 2016-02-11T23:15:43Z

From: Bot.io (Windows)

Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/b9d5ad22a94637b/output.txt

pdfjsbot · 2016-02-11T23:36:32Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/b9d5ad22a94637b/output.txt

Total script time: 20.82 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2016-02-11T23:37:17Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/4db634964845804/output.txt

Total script time: 21.57 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2016-02-18T19:33:43Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/4cd3f72a2fbce1b/output.txt

Total script time: 14.56 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://107.21.233.14:8877/4cd3f72a2fbce1b/reftest-analyzer.html#web=eq.log

yurydelendik · 2016-02-18T19:39:08Z

/botio-linux test

pdfjsbot · 2016-02-18T19:39:08Z

From: Bot.io (Linux)

Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://107.21.233.14:8877/354031c9f11c02f/output.txt

pdfjsbot · 2016-02-18T19:39:30Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/57a3ffa353c2399/output.txt

Total script time: 20.34 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2016-02-18T20:00:57Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/354031c9f11c02f/output.txt

Total script time: 21.80 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

yurydelendik · 2016-02-18T21:02:45Z

And in the interface all functions are anonymous. Is that intentional? With the similarly named methods in the different interfaces (e.g. read), grepping may be easier if the interface name is included in the function name.

Actually, there is no point on applying function names (it was legacy practice to ensure we are getting proper stack trace reports). IDEs are capable to navigate between code and jsdoc (as well as show pop-up hints). Not sure how can we be more useful here in grep situation, also once we start moving the code to the ES6 we will probably completely abandon this naming conversion in favor of class definition.

timvandermeij · 2016-02-26T21:50:09Z

/botio-linux preview

pdfjsbot · 2016-02-26T21:50:10Z

From: Bot.io (Linux)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/23904393ada2d2e/output.txt

pdfjsbot · 2016-02-26T21:50:59Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/23904393ada2d2e/output.txt

Total script time: 0.81 mins

Published

timvandermeij · 2016-02-26T21:55:51Z

/botio test

pdfjsbot · 2016-02-26T21:55:51Z

From: Bot.io (Linux)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/bda3d85b7158ee5/output.txt

pdfjsbot · 2016-02-26T21:55:51Z

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/05a169b92a7efb1/output.txt

pdfjsbot · 2016-02-26T22:16:10Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/05a169b92a7efb1/output.txt

Total script time: 20.30 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2016-02-26T22:19:43Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/bda3d85b7158ee5/output.txt

Total script time: 23.85 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://107.21.233.14:8877/bda3d85b7158ee5/reftest-analyzer.html#web=eq.log

timvandermeij · 2016-02-26T22:20:12Z

/botio-linux test

pdfjsbot · 2016-02-26T22:20:12Z

From: Bot.io (Linux)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/e64f64ef17dec2d/output.txt

pdfjsbot · 2016-02-26T22:42:03Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/e64f64ef17dec2d/output.txt

Total script time: 21.85 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

Makes PDF data reading Streams API friendly.

Snuffleupagus · 2016-03-01T16:28:12Z

Nice patch!

However, it seems that the issue @timvandermeij mentioned on IRC a few days back is still present in the patch that landed, please see http://logs.glob.uno/?c=mozilla%23pdfjs&s=26+Feb+2016&e=26+Feb+2016#c46715.

For example, open http://ftp.acc.umu.se/mirror/CTAN/info/lshort/english/lshort.pdf#disableRange=true&disableStream=true and check the browser console (Ctrl+Shift+J):

Error: pdfjs/core/network module is not loaded pdf.worker.js:2340:5
assert@resource://pdf.js/build/pdf.worker.js:2356:5
getPdfManager@resource://pdf.js/build/pdf.worker.js:41789:11
wphCreateDocumentHandler/setupDoc@resource://pdf.js/build/pdf.worker.js:41930:7
wphReady@resource://pdf.js/build/pdf.worker.js:42171:7
messageHandlerComObjOnMessage@resource://pdf.js/build/pdf.worker.js:3704:9
EventListener.handleEvent*MessageHandler@resource://pdf.js/build/pdf.worker.js:3710:3
wphCreateDocumentHandler@resource://pdf.js/build/pdf.worker.js:41719:19
wphSetupDoc@resource://pdf.js/build/pdf.worker.js:41706:14
messageHandlerComObjOnMessage/<@resource://pdf.js/build/pdf.worker.js:3681:18
promise callback*messageHandlerComObjOnMessage@resource://pdf.js/build/pdf.worker.js:3680:9
EventListener.handleEvent*MessageHandler@resource://pdf.js/build/pdf.worker.js:3710:3
initializeWorker@resource://pdf.js/build/pdf.worker.js:42218:17
pdfjsWrapper/<@resource://pdf.js/build/pdf.worker.js:42226:3
pdfjsWrapper/<@resource://pdf.js/build/pdf.worker.js:41395:5
pdfjsWrapper@resource://pdf.js/build/pdf.worker.js:41393:2
@resource://pdf.js/build/pdf.worker.js:40:13
@resource://pdf.js/build/pdf.worker.js:25:1
@resource://pdf.js/build/pdf.worker.js:18:2
 pdf.worker.js:2341:5
PDF 47642ae5bebee4e458b77187094e0f92 [1.5 pdfTeX-1.40.14 / LaTeX with hyperref package] (PDF.js: 1.4.109 [WebGL]) viewer.js:7135:7

yurydelendik · 2016-03-01T23:09:47Z

Hmm, worked for me at http://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#disableAutoFetch=true&disableStream=true

Snuffleupagus · 2016-03-02T09:22:56Z

Hmm, worked for me at http://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#disableAutoFetch=true&disableStream=true

In order to trigger this, both disableRange and disableStream need to be set (as in my link above), and you might need to do a skip-cache reload as well.
Another, simpler way, to trigger this in Firefox is to open a local file by dragging it into the browser window. In that case you don't need to set any disable... parameters.

Edit: I should probably have clarified one thing, the PDF file will still load/render correctly in the above cases, it's just that there's a lot of console spew because of the asserthere https://github.com/mozilla/pdf.js/blob/master/src/core/worker.js#L551.

Also, the error message isn't entirely correct since network.js is loaded, it's just that the code in question code is only included when //#if !(FIREFOX || MOZCENTRAL).

Snuffleupagus · 2016-03-02T12:49:54Z

As outlined in #6879 (comment), this is not a breaking error. But it'd be nice to get rid of all that console output (caused by the assert), so couldn't we just do something like the diff below?

diff --git a/src/core/worker.js b/src/core/worker.js
index c78b52e..4e4446e 100644
--- a/src/core/worker.js
+++ b/src/core/worker.js
@@ -548,8 +548,14 @@ var WorkerMessageHandler = PDFJS.WorkerMessageHandler = {
         if (source.chunkedViewerLoading) {
           pdfStream = new PDFWorkerStream(source, handler);
         } else {
+//#if (FIREFOX || MOZCENTRAL)
+//        pdfManagerCapability.reject(new Error('PDFNetworkStream is ' +
+//          'undefined in the addon/built-in version.'));
+//        return pdfManagerCapability.promise;
+//#else
           assert(PDFNetworkStream, 'pdfjs/core/network module is not loaded');
           pdfStream = new PDFNetworkStream(data);
+//#endif
         }
       } catch (ex) {
         pdfManagerCapability.reject(ex);

yurydelendik · 2016-03-02T15:42:53Z

As outlined in #6879 (comment), this is not a breaking error. But it'd be nice to get rid of all that console output (caused by the assert), so couldn't we just do something like the diff below?

Problem that it shall not be called without source.chunkedViewerLoading set in add-on. I'm trying to understand how it's happening.

yurydelendik · 2016-03-02T15:52:19Z

It's due to this case https://github.com/mozilla/pdf.js/blob/master/web/viewer.js#L512 and we already resolved a promise at https://github.com/mozilla/pdf.js/blob/master/src/core/worker.js#L537 for that (that's why it is working).

yurydelendik · 2016-03-02T15:54:53Z

src/core/worker.js

@@ -196,144 +541,113 @@ var WorkerMessageHandler = PDFJS.WorkerMessageHandler = {
        } catch (ex) {
          pdfManagerCapability.reject(ex);
        }
-        return pdfManagerCapability.promise;


Shall not be removed?

Reverts back un-need change made at #6879.

yurydelendik force-pushed the streams branch from 4f4bd3c to 84eb398 Compare January 15, 2016 19:28

Snuffleupagus reviewed Jan 15, 2016
View reviewed changes

timvandermeij added the core label Jan 15, 2016

yurydelendik force-pushed the streams branch from 84eb398 to 36798fa Compare February 9, 2016 20:55

Rob--W reviewed Feb 11, 2016
View reviewed changes

yurydelendik force-pushed the streams branch 3 times, most recently from b83ca0c to f73eaad Compare February 11, 2016 23:07

yurydelendik assigned Rob--W Feb 12, 2016

yurydelendik added a commit that referenced this pull request Mar 1, 2016

Merge pull request #6879 from yurydelendik/streams

22341c0

Makes PDF data reading Streams API friendly.

yurydelendik merged commit 22341c0 into mozilla:master Mar 1, 2016

timvandermeij mentioned this pull request Mar 1, 2016

Error: pdfjs/core/network module is not loaded #7048

Closed

yurydelendik reviewed Mar 2, 2016
View reviewed changes

yurydelendik added a commit to yurydelendik/pdf.js that referenced this pull request Mar 2, 2016

Reverts back un-need change made at mozilla#6879.

a022f6f

yurydelendik added a commit that referenced this pull request Mar 2, 2016

Merge pull request #7052 from yurydelendik/issue7048

e190603

Reverts back un-need change made at #6879.

Snuffleupagus mentioned this pull request Jun 7, 2020

Remove unused methods from NetworkManager, in src/display/network.js #11976

Merged

Makes PDF data reading Streams API friendly. #6879

Makes PDF data reading Streams API friendly. #6879

Conversation

yurydelendik commented Jan 15, 2016

yurydelendik commented Jan 15, 2016

Snuffleupagus Jan 15, 2016

Choose a reason for hiding this comment

Rob--W commented Jan 15, 2016

yurydelendik commented Feb 9, 2016

pdfjsbot commented Feb 9, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 9, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 9, 2016

From: Bot.io (Windows)

Success

pdfjsbot commented Feb 9, 2016

From: Bot.io (Linux)

Success

Rob--W Feb 11, 2016

Choose a reason for hiding this comment

yurydelendik commented Feb 11, 2016

pdfjsbot commented Feb 11, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 11, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 11, 2016

From: Bot.io (Windows)

Success

pdfjsbot commented Feb 11, 2016

From: Bot.io (Linux)

Success

pdfjsbot commented Feb 18, 2016

From: Bot.io (Linux)

Failed

yurydelendik commented Feb 18, 2016

pdfjsbot commented Feb 18, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 18, 2016

From: Bot.io (Windows)

Success

pdfjsbot commented Feb 18, 2016

From: Bot.io (Linux)

Success

yurydelendik commented Feb 18, 2016

timvandermeij commented Feb 26, 2016

pdfjsbot commented Feb 26, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 26, 2016

From: Bot.io (Linux)

Success

Published

timvandermeij commented Feb 26, 2016

pdfjsbot commented Feb 26, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 26, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 26, 2016

From: Bot.io (Windows)

Success

pdfjsbot commented Feb 26, 2016

From: Bot.io (Linux)

Failed

timvandermeij commented Feb 26, 2016

pdfjsbot commented Feb 26, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 26, 2016

From: Bot.io (Linux)

Success

Snuffleupagus commented Mar 1, 2016

yurydelendik commented Mar 1, 2016