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

[api-minor] Remove the forceClamped-functionality in the Streams (issue 14849) #14856

Merged
merged 1 commit into from
Apr 29, 2022

Conversation

Snuffleupagus
Copy link
Collaborator

As it turns out, most of the code-paths in the PDFImage-class won't actually pass the TypedArray (containing the image-data) to the ColorSpace-code. Hence we generally don't need to force the image-data to be a Uint8ClampedArray, and can just as well directly use a Uint8Array instead.

In the following cases we're returning the data without any ColorSpace-parsing, and the exact TypedArray used shouldn't matter:

In the following cases the image-data is only used internally, and again the exact TypedArray used shouldn't matter:

Please note: This is tagged api-minor because it's API-observable, given that some image/mask-data will now be returned as Uint8Array rather than using Uint8ClampedArray unconditionally. However, that seems like a small price to pay to (slightly) reduce memory usage during image-conversion.

@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/7d4552c3466ee7a/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/ce4c00180169461/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/ce4c00180169461/output.txt

Total script time: 24.46 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 10

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/7d4552c3466ee7a/output.txt

Total script time: 26.83 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 1

Image differences available at: http://54.193.163.58:8877/7d4552c3466ee7a/reftest-analyzer.html#web=eq.log

@Snuffleupagus Snuffleupagus marked this pull request as ready for review April 29, 2022 08:35
src/core/image.js Outdated Show resolved Hide resolved
src/core/image.js Outdated Show resolved Hide resolved
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you very much.
I'm not sure it'll induce an observable perf impact but for sure it'll help to decrease memory use.

…ssue 14849)

As it turns out, most of the code-paths in the `PDFImage`-class won't actually pass the TypedArray (containing the image-data) to the `ColorSpace`-code. Hence we *generally* don't need to force the image-data to be a `Uint8ClampedArray`, and can just as well directly use a `Uint8Array` instead.

In the following cases we're returning the data without any `ColorSpace`-parsing, and the exact TypedArray used shouldn't matter:
 - https://github.com/mozilla/pdf.js/blob/b72a4483276d65bef32cff269eb40923c1363f2d/src/core/image.js#L714
 - https://github.com/mozilla/pdf.js/blob/b72a4483276d65bef32cff269eb40923c1363f2d/src/core/image.js#L751

In the following cases the image-data is only used *internally*, and again the exact TypedArray used shouldn't matter:
 - https://github.com/mozilla/pdf.js/blob/b72a4483276d65bef32cff269eb40923c1363f2d/src/core/image.js#L762 with the actual image-data being defined (as `Uint8ClampedArray`) further below
 - https://github.com/mozilla/pdf.js/blob/b72a4483276d65bef32cff269eb40923c1363f2d/src/core/image.js#L837

*Please note:* This is tagged `api-minor` because it's API-observable, given that *some* image/mask-data will now be returned as `Uint8Array` rather than using `Uint8ClampedArray` unconditionally. However, that seems like a small price to pay to (slightly) reduce memory usage during image-conversion.
@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.241.84.105:8877/bee211611b68419/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.193.163.58:8877/73f6df5de7ad8b7/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/bee211611b68419/output.txt

Total script time: 24.50 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 5
  different first/second rendering: 2

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/73f6df5de7ad8b7/output.txt

Total script time: 27.39 mins

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

@Snuffleupagus Snuffleupagus merged commit 24d5d5d into mozilla:master Apr 29, 2022
@Snuffleupagus Snuffleupagus deleted the rm-forceClamped branch April 29, 2022 13:19
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.

Investigate if we can safely remove the use of Uint8clampedArray when it's useless
3 participants