Skip to content
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

Use Uint8ClampedArray, when returning data, and remove manual clamping in src/core/jpg.js (issue 4901) #8778

Merged
merged 3 commits into from
Aug 14, 2017
Merged

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Aug 14, 2017

This patch removes the clamp0to255 helper function, as well as manual clamping code in src/core/jpg.js.
The adjusted constants in _convertCmykToRgb were taken from CMYK to RGB conversion code found in src/core/colorspace.js.

Please note: There will be some very slight movement in a number of existing test-cases, since Uint8ClampedArray appears to use Math.round (or equivalent) and the old code used (basically) Math.floor.

Fixes #4901.

… mode

At this point, the default viewer is already not usable in older browsers in `gulp server` mode, since we only run the code through Babel as part of the build step.
Hence there shouldn't be much point in manually loading `compatibility.js` in `viewer.html` the way that we've been doing, especially considering that it's already being loaded by `src/shared/util.js`.
…ing in `src/core/jpg.js` (issue 4901)

This patch removes the `clamp0to255` helper function, as well as manual clamping code in `src/core/jpg.js`.
The adjusted constants in `_convertCmykToRgb` were taken from CMYK to RGB conversion code found in `src/core/colorspace.js`.

*Please note:* There will be some very slight movement in a number of existing test-cases, since `Uint8ClampedArray` appears to use `Math.round` (or equivalent) and the old code used (basically) `Math.floor`.
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

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

Total script time: 2.36 mins

Published

@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

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/f3fb7f15631a06d/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/2bd18b763c1a113/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

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

Total script time: 16.58 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/2bd18b763c1a113/output.txt

Total script time: 29.46 mins

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

Image differences available at: http://54.215.176.217:8877/2bd18b763c1a113/reftest-analyzer.html#web=eq.log

@yurydelendik
Copy link
Contributor

yurydelendik commented Aug 14, 2017

I wonder if we shall further clean up our polyfills in util.js and compatibility.js and also remove from global space (like we did with streams). So for typed arrays we can add typedarray_polyfill.js and re-export it in util.js. In files that are using Uint8ClampedArray we will start using imported from util.js (or directly from typedarray_polyfill.js) constructor.

@Snuffleupagus
Copy link
Collaborator Author

In this PR, I figured that it'd make sense to only make the minimal amount of changes required to support Uint8ClampedArray.

I wonder if we shall further clean up our polyfills in util.js and compatibility.js and also remove from global space (like we did with streams). So for typed arrays we can add typedarray_polyfill.js and re-export it in util.js. In files that are using Uint8ClampedArray we will start using imported from util.js (or directly from typedarray_polyfill.js) constructor.

If we want to change this, could that perhaps be done in a follow-up[1] instead to avoid too much scope creep here?


[1] It would probably even qualify for the 5-good-beginner-bug label.

@yurydelendik
Copy link
Contributor

could that perhaps be done in a follow-up

Fair enough. Make sure that this polyfill will not appear in mozilla-central build. Other than that, it looks good

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Aug 14, 2017

Make sure that this polyfill will not appear in mozilla-central build.

Locally on Windows, I get the results as below (all sizes in bytes) for the build/firefox respectively build/mozcentral directories for the relevant builds:

gulp firefox gulp mozcentral
master 6 353 174 3 431 002
patch 6 352 666 3 430 577

@timvandermeij
Copy link
Contributor

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/dca7b4f8fea3f9f/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/7c7b16ed5ccd541/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/7c7b16ed5ccd541/output.txt

Total script time: 15.34 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/dca7b4f8fea3f9f/output.txt

Total script time: 27.67 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@timvandermeij timvandermeij merged commit 6022500 into mozilla:master Aug 14, 2017
@timvandermeij
Copy link
Contributor

Really nice!

@Snuffleupagus Snuffleupagus deleted the jpg-Uint8ClampedArray branch August 15, 2017 07:43
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
Use `Uint8ClampedArray`, when returning data, and remove manual clamping in `src/core/jpg.js` (issue 4901)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants