Adds Streams API support for networking task of PDF.js project. #8617

mukulmishra18 · 2017-07-05T20:18:04Z

This PR aims to add Streams API support for networking task of PDF.js. For the same, network.js file is moved to the main thread and PDFNetwokStream is implemented at worker to talk to main via sendWithStream and/or sendWithPromise method of MessageHandler.

Worker will ask for data(when needed) by sending stream messages to main side. PDF data is returned from the handlers(main to worker) in small chunks.

TODO:

enqueue needs to access tranfers parameter
check how size and highwatermark work (for now highwatermark = 1 and every chunk size = 1)

Snuffleupagus · 2017-07-06T10:39:04Z

src/core/worker.js

@@ -197,6 +197,115 @@ IPDFStreamRangeReader.prototype = {
 }

 /** @implements {IPDFStream} */
+class PDFNetworkStream {


Given the IRC discussion yesterday, I'm assuming that we shouldn't use class here (or elsewhere), since this code is in src/core!?

Hmm..., I see. Okay, I will change it to the way we implement class in PDF.js, like: IPDFStream. Although, I still think class looks nice :-)

Snuffleupagus · 2017-07-06T10:44:22Z

src/core/worker.js

@@ -197,6 +197,115 @@ IPDFStreamRangeReader.prototype = {
 }

 /** @implements {IPDFStream} */
+class PDFNetworkStream {
+  constructor(data, msgHandler) {
+    this._data = data;


Nit: Underscore is usually used to denote a property, or method, that should be considered "private". However, in both PDFNetworkStreamReader and PDFNetworkStreamRangeReader below, you're directly accessing _data (and _msgHandler).
To me, this feels like abusing notation, and it'd be better it you change the properties to this.data = data (and this.msgHandler) here instead. (Another option would be to keep them "private" here, and add getters.)

Sorry, I will change it to this.data = data and this.msgHandler = msgHandler.

Snuffleupagus · 2017-07-06T16:30:00Z

src/shared/util.js

-    let sendStreamRequest = ({ stream, chunk, success, reason, }) => {
+    let sendStreamRequest = ({ stream, chunk, transfers,
+                               success, reason, }) => {
+      if (transfers) {


I have to ask: Why isn't it sufficient to simply always call

this.postMessage({ sourceName, targetName, stream, streamId, chunk, success, reason, }, transfers);

and just let the postMessage method deal with it?
Also, don't you need to add similar code in _processStreamMessage below as well?

and just let the postMessage method deal with it?

Yeah, right. Seems like this.postMessage is doing same thing, that I am trying here. Okay, calling just this.postMessage sounds like a good idea. Thanks.

Also, don't you need to add similar code in _processStreamMessage below as well?

sendStreamResponse mostly used for signaling *_complete messages e.g. pull_complete(to resolve callbacks), and I don't think we are going to send transfers in these types of messages, at least for now. Maybe we require this in future, but not sure. @yurydelendik, do we require this?

I don't understand the concern, but sendStreamRequest will have optional transfers, and I recommend just call this.postMessage({...}, transfers) as mentioned above

Just thinking, do we really need same changes in sendStreamResponse? like:

let sendStreamResponse = ({ stream, transfers, success, reason, }) => { this.postMessage({ sourceName, targetName, stream, success, streamId, reason, }, transfers); };

yurydelendik · 2017-07-06T21:49:45Z

display/api.js has one more pdfjs/core/network reference that can be removed.
transfers can be used now

-            sink.enqueue(result.value);
+            sink.enqueue(new Uint8Array(result.value), 1, [result.value]);

and notice, the result.value array buffer is wrapped into Uint8Array, which needs to be unwrapped:

   read() {
-    return this._reader.read();
+    return this._reader.read().then(({value, done}) => {
+      if (done) { return {value: undefined, done: true}; }
+      return {value: value.buffer, done: false};
+    });;
   },

yurydelendik · 2017-07-06T21:50:58Z

src/display/api.js

+      }, this);
+
+      messageHandler.on('GetRangeReader', function(data, sink) {
+        this._PDFNetworkStream = new PDFNetworkStream(data.data);


no need to create new PDFNetworkStream (and data field can be removed)

Done in new commit. Thanks.

Snuffleupagus · 2017-07-07T09:20:48Z

src/core/worker.js

+  this.data = data;
+  let source = data.source;
+  this._contentLength = source.length;
+  this.msgHandler = msgHandler;


Nit: Let's place this.msgHandler = msgHandler; right after the this.data = data; line, to group the properties that are set from the provided parameters.

Done in new commit. Thanks.

Snuffleupagus · 2017-07-07T09:21:52Z

src/core/worker.js

+  this._isStreamingSupported = false;
+
+  let readableStream = this._msgHandler.sendWithStream('GetReader',
+                         this._data);


Nit: Indentation, let's align this directly under 'GetReader' instead.

Done. Thanks

Snuffleupagus · 2017-07-07T09:22:35Z

src/core/worker.js

+  this._reader = readableStream.getReader();
+
+  this._headersReady = this._msgHandler.sendWithPromise('ReaderHeadersReady').
+    then((data) => {


Nit: Two more spaces of indentation on this line please.

Snuffleupagus · 2017-07-07T09:24:01Z

src/core/worker.js

+  this.onProgress = null;
+
+  let readableStream = this._msgHandler.sendWithStream('GetRangeReader',
+                        { begin, end, });


Nit: Indentation, let's align this directly under 'GetRangeReader' instead.

Done. Thanks.

Snuffleupagus · 2017-07-07T09:24:51Z

src/display/api.js

-        Promise.all([SystemJS.import('pdfjs/core/network'),
-                     SystemJS.import('pdfjs/core/worker')]).then((modules) => {
+        Promise.resolve(SystemJS.import('pdfjs/core/worker')).
+          then((modules) => {


Nit: Please indent this line with two more spaces, and also change it to then(worker) => { instead.

Done in new commit. Thanks.

Snuffleupagus · 2017-07-07T09:26:18Z

src/display/api.js

+            // Enqueue data chunk into sink, and transfer it
+            // to other side as `Transferable` object.
+            sink.enqueue(new Uint8Array(value), 1, [value]);
+          }).catch(function(reason) {


No need for an intermediate function here, }).catch(sink.error); should work just as well.

Sure, Thanks for pointing out.

Snuffleupagus · 2017-07-07T09:26:41Z

src/display/api.js

+              return;
+            }
+            sink.enqueue(new Uint8Array(value), 1, [value]);
+          }).catch(function(reason) {


No need for an intermediate function here, }).catch(sink.error); should work just as well.

Done. Thanks.

Snuffleupagus · 2017-07-07T09:28:39Z

src/shared/util.js

+    let sendStreamRequest = ({ stream, chunk, transfers,
+                               success, reason, }) => {
+      this.postMessage({ sourceName, targetName, stream, streamId,
+                           chunk, success, reason, }, transfers);


Nit: alignment.

Sorry, done with new commit. Thanks.

yurydelendik · 2017-07-07T15:49:01Z

src/worker_loader.js

@@ -28,7 +28,6 @@ importScripts('./shared/compatibility.js');
 importScripts('../node_modules/systemjs/dist/system.js');
 importScripts('../systemjs.config.js');

-Promise.all([SystemJS.import('pdfjs/core/network'),
-             SystemJS.import('pdfjs/core/worker')]).then(function () {
+Promise.resolve(SystemJS.import('pdfjs/core/worker')).then(function () {


Here and in api.js, you can directly use SystemJS.import without wrapping with Promise.resolve:

SystemJS.import('pdfjs/core/worker').then(function () {...

Done in new commit. Thanks.

yurydelendik · 2017-07-07T15:50:44Z

src/core/worker.js

+  this._contentLength = null;
+  this._isRangeSupported = false;
+  this._isStreamingSupported = false;
+  let queueingStrategy = { highWaterMark: 100,


let's remove queueingStrategy from here and range reader, or make explicit highWaterMark:1 and size:()=>1

Removed in new commit. Thanks.

yurydelendik · 2017-07-07T16:13:40Z

src/display/api.js

+      }, this);
+
+      messageHandler.on('GetRangeReader', function(data, sink) {
+        this._rangeReader =


change to let rangeReader it must be unique for every new range request.

Done. Thanks.

yurydelendik · 2017-07-10T15:46:18Z

src/display/network.js

+    if (initialData && initialData.length > 0) {
+      this._queuedChunks.push(initialData);
+    }
+    this._msgHandler = msgHandler;


we don't need this._msgHandler and msgHandler

Sorry, done with new commit.

yurydelendik · 2017-07-10T18:37:51Z

src/display/api.js

+          this._networkStream = new PDFWorkerStream(data.source);
+        } else {
+          this._networkStream = new PDFNetworkStream(data);
+        }


move this code to the WorkerTransport constructor and remove this.networkStreamCapability promise

Done with new commit. Thanks.

Snuffleupagus · 2017-07-11T08:46:46Z

src/display/api.js

@@ -1522,6 +1522,14 @@ var WorkerTransport = (function WorkerTransportClosure() {
    this.destroyCapability = null;
    this._passwordCapability = null;

+    this._networkStream = null;
+    this._fullReader = null;
+    if (pdfDataRangeTransport /* === true = chunkedViewerLoading */) {


Nit: The comment isn't completely easy to read at first, and furthermore I'm not sure how much value it adds!?

Removed with new commit. Thanks.

yurydelendik · 2017-07-27T20:29:57Z

/botio-linux preview

pdfjsbot · 2017-07-27T20:29:58Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/f0e8b6d03703f3d/output.txt

yurydelendik · 2017-07-27T20:30:02Z

/botio test

pdfjsbot · 2017-07-27T20:30:02Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/31567a9079e83b3/output.txt

pdfjsbot · 2017-07-27T20:30:02Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/02ee71ae88e44dc/output.txt

pdfjsbot · 2017-07-27T20:32:17Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/f0e8b6d03703f3d/output.txt

Total script time: 2.32 mins

Published

pdfjsbot · 2017-07-27T20:48:52Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/31567a9079e83b3/output.txt

Total script time: 16.58 mins

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

pdfjsbot · 2017-07-27T20:59:22Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/02ee71ae88e44dc/output.txt

Total script time: 29.32 mins

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

network.js file moved to main thread and `PDFNetworkStream` implemented at worker thread, that is used to ask for data whenever worker needs.

yurydelendik · 2017-07-27T21:05:00Z

/botio-linux preview

pdfjsbot · 2017-07-27T21:05:00Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @yurydelendik received. Current queue size: 1

Live output at: http://54.67.70.0:8877/b49b2e015836bb8/output.txt

pdfjsbot · 2017-07-27T21:09:45Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/b49b2e015836bb8/output.txt

Total script time: 2.32 mins

Published

yurydelendik · 2017-07-27T21:15:03Z

Thank you for the patch.

Snuffleupagus · 2017-07-28T13:06:19Z

It seems that this patch may have caused some weird behaviour in the Firefox addon with disableAutoFetch = true, as evident by strange loading bar behaviour when opening http://mirrors.ctan.org/info/lshort/english/lshort.pdf#disableStream=true&disableAutoFetch=true.[1]

Looking at data received through onProgress, it seems that with this patch the entire PDF file is fetched at once thus bypassing the disableAutoFetch setting. When doing skip-cache reloads, I'm getting percent equal to (in order) 101, 100, 101 when setting a break-point in PDFViewerApplication.progress.

[1] It may be necessary to do "skip-cache" reloads, i.e. Ctrl+F5 to ensure that the PDF data isn't cached.

yurydelendik · 2017-07-28T13:11:39Z

It seems that this patch may have caused some weird behaviour in the Firefox addon

I was encountering with behavior before: the initial XHR fetches quickly entire file when cached. I think it's normal for small files. We just need to pay attention if data and events is handled properly in chunks and streams.

Snuffleupagus · 2017-07-28T13:15:53Z

I was encountering with behavior before: the initial XHR fetches quickly entire file when cached.

Huh, I'm seeing this when doing skip-cache reloads (as indicated above), so I don't think caching should be relevant then!?

yurydelendik · 2017-07-28T13:30:25Z

Not sure I'm getting the same behavior locally: PDFViewerApplication.progress reports proper percent values in ascending order (that start from 4).

Snuffleupagus · 2017-07-28T13:34:22Z

Not sure I'm getting the same behavior locally: PDFViewerApplication.progress reports proper percent values in ascending order.

I can confirm that it works locally (with gulp server), but I'm seeing the weird behaviour with the Firefox addon.

yurydelendik · 2017-07-28T13:35:39Z

but I'm seeing the weird behaviour with the Firefox addon.

FWIW I'm testing on Nightly (e10s) with current development addon. (non-e10s is unusably broken)

Adds Streams API support for networking task of PDF.js project.

Snuffleupagus reviewed Jul 6, 2017

View reviewed changes

mukulmishra18 force-pushed the network-streaming branch from 3ac72c1 to 4ad80e1 Compare July 6, 2017 15:40

Snuffleupagus reviewed Jul 6, 2017

View reviewed changes

yurydelendik reviewed Jul 6, 2017

View reviewed changes

mukulmishra18 force-pushed the network-streaming branch from b8be2db to 02baba8 Compare July 7, 2017 08:17

Snuffleupagus reviewed Jul 7, 2017

View reviewed changes

mukulmishra18 force-pushed the network-streaming branch from 02baba8 to a13a4f8 Compare July 7, 2017 09:37

Snuffleupagus added core 4-work-in-progress labels Jul 7, 2017

yurydelendik reviewed Jul 7, 2017

View reviewed changes

mukulmishra18 force-pushed the network-streaming branch from 01ad046 to 70909a2 Compare July 7, 2017 20:18

yurydelendik changed the title ~~[WIP]Adds Streams API support for networking task of PDF.js project.~~ Adds Streams API support for networking task of PDF.js project. Jul 7, 2017

mukulmishra18 force-pushed the network-streaming branch from 70909a2 to 9951972 Compare July 7, 2017 21:15

yurydelendik reviewed Jul 10, 2017

View reviewed changes

mukulmishra18 force-pushed the network-streaming branch from 92b4dbd to 79821fa Compare July 10, 2017 18:05

yurydelendik reviewed Jul 10, 2017

View reviewed changes

mukulmishra18 force-pushed the network-streaming branch from 79821fa to a317816 Compare July 10, 2017 20:09

Snuffleupagus reviewed Jul 11, 2017

View reviewed changes

mukulmishra18 force-pushed the network-streaming branch from a317816 to cf02bcf Compare July 11, 2017 11:58

mukulmishra18 force-pushed the network-streaming branch 2 times, most recently from 288e9c1 to ec6de1a Compare July 27, 2017 19:22

timvandermeij removed the 4-work-in-progress label Jul 27, 2017

Adds Streams API support for networking task of PDF.js project.

1091067

network.js file moved to main thread and `PDFNetworkStream` implemented at worker thread, that is used to ask for data whenever worker needs.

mukulmishra18 force-pushed the network-streaming branch from ec6de1a to 1091067 Compare July 27, 2017 21:03

yurydelendik merged commit 343b4dc into mozilla:master Jul 27, 2017

Snuffleupagus mentioned this pull request Jul 29, 2017

Ensure that the loadingBar isn't displayed again when the entire file has already been fetched #8718

Merged

mukulmishra18 mentioned this pull request Jul 29, 2017

Reduces the amount of data send via GetDocRequest. #8721

Merged

Snuffleupagus mentioned this pull request Nov 4, 2017

Re-factor how parameters are passed to the network streams #9037

Merged

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8617 from mukulmishra18/network-streaming

b2dc355

Adds Streams API support for networking task of PDF.js project.

Snuffleupagus mentioned this pull request Mar 28, 2021

Remove the file://-URL special-case from webViewerOpenFileViaURL #13157

Merged

Adds Streams API support for networking task of PDF.js project. #8617

Adds Streams API support for networking task of PDF.js project. #8617

Conversation

mukulmishra18 commented Jul 5, 2017 • edited by yurydelendik Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

mukulmishra18 Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented Jul 27, 2017

pdfjsbot commented Jul 27, 2017

From: Bot.io (Linux m4)

Received

yurydelendik commented Jul 27, 2017

pdfjsbot commented Jul 27, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 27, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 27, 2017

From: Bot.io (Linux m4)

Success

Published

pdfjsbot commented Jul 27, 2017

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jul 27, 2017

From: Bot.io (Windows)

Success

yurydelendik commented Jul 27, 2017

pdfjsbot commented Jul 27, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 27, 2017

From: Bot.io (Linux m4)

Success

Published

mukulmishra18 commented Jul 5, 2017 •

edited by yurydelendik

Loading

Snuffleupagus Jul 6, 2017 •

edited

Loading

mukulmishra18 Jul 6, 2017 •

edited

Loading

Snuffleupagus Jul 6, 2017 •

edited

Loading

Snuffleupagus Jul 7, 2017 •

edited

Loading

Snuffleupagus Jul 7, 2017 •

edited

Loading

Snuffleupagus Jul 7, 2017 •

edited

Loading

Snuffleupagus Jul 7, 2017 •

edited

Loading

yurydelendik Jul 7, 2017 •

edited

Loading

Snuffleupagus commented Jul 28, 2017 •

edited

Loading

yurydelendik commented Jul 28, 2017 •

edited

Loading

yurydelendik commented Jul 28, 2017 •

edited

Loading