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.

Sign up for GitHub

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

Jump to bottom

Fixes #3591 / list unsupported JPX options #4639

Closed
wants to merge 1 commit into from
Closed

Fixes #3591 / list unsupported JPX options #4639

wants to merge 1 commit into from

Conversation

fkaelberer
Copy link
Contributor

  1. Fixes Document doesn't show scanned image, also fonts are ugly #3591 (case 0xFF55)
  2. Only print the unsupported JPX coding options, not all parameters (applies to Out of memory (big files). Firefox and pdf.js #4308).

@Snuffleupagus
Copy link
Collaborator

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/9a7d42528b3988d/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/9a7d42528b3988d/output.txt

Total script time: 0.38 mins

Published

@Snuffleupagus
Copy link
Collaborator

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/1475497c77ed5e0/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

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

Total script time: 21.61 mins

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

@pdfjsbot
Copy link

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/1475497c77ed5e0/output.txt

Total script time: 25.61 mins

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

yurydelendik
yurydelendik reviewed Apr 17, 2014
View reviewed changes
src/core/jpx.js
unsupported += ', predictableTermination';
}
if (unsupported !== '') {
throw 'Unsupported COD options: ' + unsupported.substring(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make 'unsupported' as array and use push() and join(). Also, can you change this line to create an Error object:

throw new Error('Unsupported COD options: ' + unsupported.join(', '));

@fkaelberer
Copy link
Contributor Author

You're right, join() is much nicer.
How can we stop this chain of error messages on throw new Error()?

"Warning: JPX error: Error: Unsupported COD options: sopMarkerUsed, ... . Trying to recover" pdf.worker.js:200
Warning: Unable to decode image: TypeError: codingStyleParameters is undefined" pdf.worker.js:200
"Warning: Dependent image isn't ready yet" pdf.js:200

@yurydelendik yurydelendik self-assigned this Apr 28, 2014
@timvandermeij
Copy link
Contributor

@fkaelberer Could you squash the commits?

@fkaelberer
Copy link
Contributor Author

Done.

@p01
Copy link
Contributor

p01 commented May 13, 2014

Looks good.
I wonder how to deal with broken images in general: Ignoring them altogether or displaying a placeholder image.

@yurydelendik
Copy link
Contributor

Looks good. But I'm thinking about throw 'message'; usage. Can it be replaced to throw new Error('message'); to have a stack trace?

@fkaelberer
Copy link
Contributor Author

The error messages are already very long (see my comment above), and throwing a new Error will make it even longer. Output like Warning: JPX error: Error: Unsupported COD options: ... is not very beautiful.

yurydelendik
yurydelendik reviewed May 14, 2014
View reviewed changes
src/core/jpx.js
if (this.failOnCorruptedImage) {
error('JPX error: ' + e);
if (doNotRecover || this.failOnCorruptedImage) {
throw 'JPX error: ' + e;
Copy link
Contributor

Choose a reason for hiding this comment

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

how about making this one just throw e; and warning below something like warn('Trying to recover: ' + e.message) (we may need to prefix all thrown exceptions above with 'JPX error: ')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that doesn't change the output, does it?
Maybe we can stay with the current commit which prints

"Warning: Unable to decode image: JPX error: Unsupported COD options (sopMarkerUsed, ephMarkerUsed, resetContextProbabilities, terminationOnEachCodingPass, verticalyStripe, predictableTermination)" pdf.worker.js:200
"Warning: Dependent image isn't ready yet" pdf.js:200

instead of

"Warning: JPX error: Unsupported COD options: {"entropyCoderWithCustomPrecincts":false,"sopMarkerUsed":true,"ephMarkerUsed":true,"progressionOrder":1,"layersCount":1,"multipleComponentTransform":1,"decompositionLevelsCount":5,"xcb":6,"ycb":6,"selectiveArithmeticCodingBypass":false,"resetContextProbabilities":true,"terminationOnEachCodingPass":true,"verticalyStripe":true,"predictableTermination":true,"segmentationSymbolUsed":true,"reversibleTransformation":0}. Trying to recover" pdf.worker.js:201
"Warning: Unable to decode image: TypeError: codingStyleParameters is undefined" pdf.worker.js:201
"Warning: Dependent image isn't ready yet" pdf.js:201

@yurydelendik yurydelendik added this to the 2014 Q2 milestone Jun 12, 2014
@yurydelendik yurydelendik mentioned this pull request Jun 13, 2014
Merged
@yurydelendik
Copy link
Contributor

Thank you for the patch (landed in #4939)

@fkaelberer fkaelberer deleted the issue3591 branch June 14, 2014 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@yurydelendik yurydelendik

Labels
None yet
Projects
None yet
Milestone
2014 Q2
Development

Successfully merging this pull request may close these issues.

Document doesn't show scanned image, also fonts are ugly
6 participants
@fkaelberer @Snuffleupagus @pdfjsbot @timvandermeij @p01 @yurydelendik