Adds streams-lib polyfill and exports ReadableStream from shared/util. #8396

mukulmishra18 · 2017-05-10T11:15:09Z

This is the first PR of project Streams API in PDF.js. This allow PDF.js to use Streams API in the process of networking and rendering. For that purpose, this PR:

Fork Streams API ref-implementation and convert it to commonjs module.
Import/Export ReadableStream from shared/util.js.
Adds test for ReadableStream to check it's working.

Increase in file size(in kb) after adding this code, in pdf.js and pdf.worker.js:

pdf.js: 468.8(after) - 343.7(before) = ~125
pdf.worker.js: 1400(after) - 1300(before) = 100

Snuffleupagus

First of all, please note that Travis has a lot of complaints about the code. In this case, since it concerns external code, I'd suggest simply adding external/streams/ to https://github.com/mozilla/pdf.js/blob/master/.eslintignore. Edit: Remember that running gulp lint helps catch these kind of issues locally.

Second of all, this will get bundled into all build targets by default. I imagine that we don't want to do that, at least not at the present time, since it would cause e.g. the FIREFOX/MOZCENTRAL builds to become quite a lot larger (for code that we currently don't use in production).

mukulmishra18 · 2017-05-10T12:01:11Z

I'd suggest simply adding external/streams/ to https://github.com/mozilla/pdf.js/blob/master/.eslintignore

Thanks for you suggestion. I will add the file.

this will get bundled into all build targets by default.

No idea what we have to do for this. Maybe we can compress the polyfill. Have to ask @yurydelendik .

yurydelendik · 2017-05-10T12:54:32Z

the FIREFOX/MOZCENTRAL builds to become quite a lot larger

We need to follow https://bugzilla.mozilla.org/show_bug.cgi?id=1272697 for that. I'm expecting landing of streamable getTextContent as soon as we land this PR, so the code will be using streams api in production.

timvandermeij · 2017-05-10T21:22:27Z

It looks like Travis does not see the library as a module. Is it a valid CommonJS module?

Moreover, the file looks built. I suspect one of the next steps will be to fetch the original from the repository and build it ourselves? In case the upstream library changes, it should be easy for us to rebuild our version from source.

yurydelendik · 2017-05-10T21:53:09Z

TODO:

replace const assert = require('assert'); -- it adds some useless for us node.js stubs
on gulp lib and gulp dist copy "external/streams/" to the build/external/streams/ and build/dist/external/streams/ folder
use LICENSE.md from reference implementation in the external/streams/

yurydelendik · 2017-05-12T19:42:51Z

gulpfile.js

    gulp.src([
      'src/{core,display}/*.js',
      'src/shared/{compatibility,util}.js',
      'src/{pdf,pdf.worker}.js',
-    ], {base: 'src/'}),
+    ], {base: 'src/'})


too many irrelevant changes. let's make original marge as var buildLib = merge([...]); and then add

return merge([ buildLib, gulp.src('external/streams/streams-lib.js', {base: '.'}) .pipe(gulp.dest('build/')) ]);

Okay, sure. Looks more clear. Thanks for suggestion.

yurydelendik · 2017-05-12T19:43:55Z

gulpfile.js

-gulp.task('dist', ['dist-repo-git']);
+gulp.task('dist', ['dist-repo-git'], function () {
+  gulp.src('external/streams/streams-lib.js', {base: '.'})
+    .pipe(gulp.dest('build/dist/'));


this belong in the dist-repo-prepare task

Added test for ReadableStream. Adds ref-implementation license-header in streams-lib and change gulp task to copy external/streams/ in build/ external/streams/ and build/dist/external/streams folder. Adds README.md and LICENSE.md

Snuffleupagus · 2017-05-19T21:29:35Z

While I'd wish that the size impact of this patch was a bit smaller, I've got no formal objections here :-)
So, as far as I'm concerned, we can land this PR (with passing tests of course)!

/botio test

pdfjsbot · 2017-05-19T21:29:36Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/533ea717f83341b/output.txt

pdfjsbot · 2017-05-19T21:29:36Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/6bd04c9c974ea96/output.txt

pdfjsbot · 2017-05-19T21:47:02Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/6bd04c9c974ea96/output.txt

Total script time: 17.42 mins

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

Image differences available at: http://54.67.70.0:8877/6bd04c9c974ea96/reftest-analyzer.html#web=eq.log

pdfjsbot · 2017-05-19T21:55:31Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/533ea717f83341b/output.txt

Total script time: 25.92 mins

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

Snuffleupagus · 2017-05-20T08:58:40Z

/botio-linux test

pdfjsbot · 2017-05-20T08:58:40Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2017-05-20T09:17:53Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 19.20 mins

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

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

Snuffleupagus · 2017-05-20T09:23:16Z

It unfortunately seems that loading all of this new code has made the tests even more unreliable :-(
Even the unit-tests now timeout frequently, whereas they're fine when run against master.

mukulmishra18 · 2017-05-20T13:20:47Z

It unfortunately seems that loading all of this new code has made the tests even more unreliable :-(
Even the unit-tests now timeout frequently, whereas they're fine when run against master.

Not sure why this is happening(as all unit-tests are passing on my local machine) and what will be the potential solution of this :-(

But i think we can reduce the size of streams-lib file by removing all except ReadableStream(as we only need this for now) from streams.lib.entry.js file, as we are using this file to bundle streams-lib.

Snuffleupagus · 2017-05-20T13:52:22Z

Not sure why this is happening(as all unit-tests are passing on my local machine) and what will be the potential solution of this :-(

This is a pre-existing issue affecting only the Chrome browser on the Linux bot, and one that has thus far prevented landing for example PR #8284.
So while it's not the fault of this patch itself, it will probably still make landing this difficult at the moment (until a solution/workaround for the timeouts have been found).

Note that e.g. compared to PR #8284, the current PR seems to cause even worse timeouts on the Linux bot.

yurydelendik · 2017-05-30T21:42:07Z

/botio test

pdfjsbot · 2017-05-30T21:42:08Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/8ca3c6c39e0ef85/output.txt

pdfjsbot · 2017-05-30T21:42:08Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/0e387beaad90426/output.txt

pdfjsbot · 2017-05-30T21:59:52Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/8ca3c6c39e0ef85/output.txt

Total script time: 17.73 mins

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

pdfjsbot · 2017-05-30T22:13:29Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/0e387beaad90426/output.txt

Total script time: 30.14 mins

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

yurydelendik · 2017-05-31T13:44:04Z

Thank you for the patch.

As a follow up let's check if we can bring code from https://github.com/mukulmishra18/streams-lib/ as is here.

Adds streams-lib polyfill and exports ReadableStream from shared/util.

Snuffleupagus reviewed May 10, 2017

View reviewed changes

mukulmishra18 force-pushed the streams-lib branch from 85bb940 to d5147c6 Compare May 10, 2017 12:01

timvandermeij added the core label May 10, 2017

mukulmishra18 force-pushed the streams-lib branch from 0a3aecd to a454dd8 Compare May 12, 2017 19:03

yurydelendik reviewed May 12, 2017

View reviewed changes

mukulmishra18 force-pushed the streams-lib branch 2 times, most recently from 453f613 to 21b1b01 Compare May 12, 2017 20:30

mukulmishra18 force-pushed the streams-lib branch from 21b1b01 to c9f44f3 Compare May 19, 2017 18:57

yurydelendik merged commit bd288df into mozilla:master May 31, 2017

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

Merge pull request mozilla#8396 from mukulmishra18/streams-lib

1aef36f

Adds streams-lib polyfill and exports ReadableStream from shared/util.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds streams-lib polyfill and exports ReadableStream from shared/util. #8396

Adds streams-lib polyfill and exports ReadableStream from shared/util. #8396

mukulmishra18 commented May 10, 2017 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

mukulmishra18 commented May 10, 2017

yurydelendik commented May 10, 2017

timvandermeij commented May 10, 2017

yurydelendik commented May 10, 2017 •

edited

Loading

yurydelendik May 12, 2017

mukulmishra18 May 12, 2017

yurydelendik May 12, 2017

Snuffleupagus commented May 19, 2017

pdfjsbot commented May 19, 2017

pdfjsbot commented May 19, 2017

pdfjsbot commented May 19, 2017

pdfjsbot commented May 19, 2017

Snuffleupagus commented May 20, 2017

pdfjsbot commented May 20, 2017

pdfjsbot commented May 20, 2017

Snuffleupagus commented May 20, 2017 •

edited

Loading

mukulmishra18 commented May 20, 2017

Snuffleupagus commented May 20, 2017

yurydelendik commented May 30, 2017

pdfjsbot commented May 30, 2017

pdfjsbot commented May 30, 2017

pdfjsbot commented May 30, 2017

pdfjsbot commented May 30, 2017

yurydelendik commented May 31, 2017

Adds streams-lib polyfill and exports ReadableStream from shared/util. #8396

Adds streams-lib polyfill and exports ReadableStream from shared/util. #8396

Conversation

mukulmishra18 commented May 10, 2017 • edited Loading

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

mukulmishra18 commented May 10, 2017

yurydelendik commented May 10, 2017

timvandermeij commented May 10, 2017

yurydelendik commented May 10, 2017 • edited Loading

yurydelendik May 12, 2017

Choose a reason for hiding this comment

mukulmishra18 May 12, 2017

Choose a reason for hiding this comment

yurydelendik May 12, 2017

Choose a reason for hiding this comment

Snuffleupagus commented May 19, 2017

pdfjsbot commented May 19, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented May 19, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented May 19, 2017

From: Bot.io (Linux m4)

Failed

pdfjsbot commented May 19, 2017

From: Bot.io (Windows)

Success

Snuffleupagus commented May 20, 2017

pdfjsbot commented May 20, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented May 20, 2017

From: Bot.io (Linux m4)

Failed

Snuffleupagus commented May 20, 2017 • edited Loading

mukulmishra18 commented May 20, 2017

Snuffleupagus commented May 20, 2017

yurydelendik commented May 30, 2017

pdfjsbot commented May 30, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented May 30, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented May 30, 2017

From: Bot.io (Linux m4)

Success

pdfjsbot commented May 30, 2017

From: Bot.io (Windows)

Success

yurydelendik commented May 31, 2017

mukulmishra18 commented May 10, 2017 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

yurydelendik commented May 10, 2017 •

edited

Loading

Snuffleupagus commented May 20, 2017 •

edited

Loading