Support all progression orders #5494
Conversation
|
did you run these errors came up src/core/jpx.js: line 718, col 18, 'c' is already defined. |
|
in js vars get hoisted up to function scope, you should define them once in the beginning |
|
I just wrote at the issue about the fact that I didn't succeed to run testing, and I mean also lint. I just didn't want to delay more the PR... Thanks for feedback. I'll fix it and try to overcome my problems with the testing. |
| var layersCount = tile.codingStyleDefaultParameters.layersCount; | ||
| var componentsCount = siz.Csiz; | ||
|
|
||
| var l, r, c, p; |
There was a problem hiding this comment.
Nit: the indentation is incorrect here, please use spaces instead of tabs when indenting.
|
I've added a few initial review comments (mostly about code style). Once you have addressed those, please squash the commits, see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits. |
|
looks ok to me. I personally do not like the for loop style in some places and the variable names are a bit to verbose, but that might be my personal problem :-) @Snuffleupagus can you trigger a regression test? While I would love to have new PDFs with images covering the recent fixes to jpx.js I think we should at least try to have a look if there was no change to existing functionality (which shouldnt be, but you never know :)) Maybe we already have a test of the changes hidden in the current pdfs. |
|
Thanks for your feedback. Just a few things about coding style:
Anyway I would be happy to learn your coding conventions (I will be greatful to a link documents it, if any exist), in order to align my code with them in following contributions. |
|
None of the comments is related to performance. Maybe in esoteric edge cases, but usually its just about style used here in the project, which is mainly introduced by mozilla contributors. Thats why I kept back on my personal feelings about the style :-) |
|
This PR still needs proper review by a developer, but let's check that there are no regressions. /botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/e38329d42f020ab/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/b72b42a3867cc2c/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/e38329d42f020ab/output.txt Total script time: 20.42 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/b72b42a3867cc2c/output.txt Total script time: 23.87 mins
|
|
lets ping @yurydelendik :-) |
|
Another ping ;) Just wanted to ensure it is not forgotten. |
|
@yurydelendik Could you review this? |
| maxDecompositionLevelsCount = Math.max(maxDecompositionLevelsCount, | ||
| component.codingStyleParameters.decompositionLevelsCount); | ||
| } | ||
| var maxNumPrecinctsInLevel = new Array(maxDecompositionLevelsCount + 1); |
There was a problem hiding this comment.
use new Int32Array(maxDecompositionLevelsCount + 1); (will go over 80 cols, might need to break line)
|
Looks good, thanks! Good to go after commets above are addressed. |
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.22.172.223:8877/e00476742d82402/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.21.233.14:8877/e73c9d2ffbfc1a5/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/e00476742d82402/output.txt Total script time: 17.08 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/e73c9d2ffbfc1a5/output.txt Total script time: 22.66 mins
|
Support all progression orders
|
Thank you for the patch! |
…ders Support all progression orders
Add support for all progression orders not supported yet: RPCL, CPRL,
PCRL.