For JPX decoding only read next packet after checking for byte aligning. #5350

CodingFabian · 2014-09-26T21:39:47Z

This patch makes the image from #5349 appear correctly, however artefacts
for the last packet seem to still be there.
This patch also optimizes some "in-checks" and adds a few header parsings.

yurydelendik · 2014-09-26T21:45:47Z

/botio-windows preview

pdfjsbot · 2014-09-26T21:45:48Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/320587b54bb289a/output.txt

pdfjsbot · 2014-09-26T21:46:39Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/320587b54bb289a/output.txt

Total script time: 0.85 mins

Published

Viewer: http://107.22.172.223:8877/320587b54bb289a/web/viewer.html
B2G Viewer: http://107.22.172.223:8877/320587b54bb289a/extensions/b2g/content/web/viewer.html
Extension: http://107.22.172.223:8877/320587b54bb289a/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.22.172.223:8877/320587b54bb289a/extensions/firefox/pdf.js.amo.xpi

CodingFabian · 2014-09-26T21:47:08Z

the main "fix" is to invoke "var packet = packetsIterator.nextPacket();" only when the byte aligning has happened.
I would appreciate a botio run to see if/how it generally affects the other pdfs.
There still seems to be an issue with the last element, because that one is colorful.
somehow i feel there is some bit/byte skipping logic off.

CodingFabian · 2014-09-26T21:57:03Z

this patch also makes #4814 render better - the screenshot by @yurydelendik where the girl is blurry is fixed. but the document still has other issues

yurydelendik · 2014-09-26T22:21:42Z

/botio test

pdfjsbot · 2014-09-26T22:21:43Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/a0c1b5392c8c250/output.txt

pdfjsbot · 2014-09-26T22:21:43Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2014-09-26T22:24:34Z

From: Bot.io (Windows)

Failed

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

Total script time: 2.85 mins

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

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

pdfjsbot · 2014-09-26T22:43:51Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/a0c1b5392c8c250/output.txt

Total script time: 22.13 mins

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

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

CodingFabian · 2014-09-27T07:44:15Z

interesting, seems like the 3591 issue is the same issue in reverse :)
Thanks for the run, I will consider this PR WIP. Maybe I did through the bit-reading code at some time. It feels for me that there is an 1 off error somewhere...

CodingFabian · 2014-09-27T08:42:18Z

fixed the regression of #3591. still want to look a bit more into it.

CodingFabian · 2014-09-27T12:43:17Z

I would say the fix is correct. There is an issue with the last tile when its dimension is small. I added a comment to #4358, that i assume that it is caused by not limiting the decoding size correctly for small tiles.

CodingFabian · 2014-10-07T07:31:20Z

I think its good to merge. who can verify for me @yurydelendik , @Snuffleupagus, @fkaelberer

fkaelberer · 2014-10-16T09:40:45Z

src/core/jpx.js

+              var colorspace = readUint32(data, position + 3);
+              switch (colorspace) {
+                case 16:
+                  warn('sRGB colorspace not supported');


sRGB and grayscale are the two supported colorspaces, aren't they?

Well yes :-) I only expanded the todo from the previous code. This was not handling colorspaces at all. Maybe I should reword this to warn: Todo ?

CodingFabian · 2014-10-16T16:08:32Z

@fkaelberer thanks for the review Felix.
I made the change you suggested and yes manual checks look better for a few documents.
maybe @yurydelendik can run regression tests for me?
While i believe there is still a problem with the decoding (the random readBits(1) and alignTobyte seem odd) this patch makes the images we checked to render better now.

timvandermeij · 2014-10-16T18:09:23Z

Travis complains: src/core/jpx.js: line 95, col 15, 'info' is not defined.

CodingFabian · 2014-10-16T18:54:15Z

strange, travis didnt complain earlier and i did not remove the import. thanks for pointing this out

fkaelberer · 2014-10-20T22:16:37Z

src/core/jpx.js

+            // TODO
+            break;
+          default:
+            warn('Unsupported header type ' + tbox);


This should make the errors more human readable:

default: var headerType = String.fromCharCode((tbox >> 24) & 0xFF, (tbox >> 16) & 0xFF, (tbox >> 8) & 0xFF, tbox & 0xFF); warn('Unsupported header type ' + tbox + ' (' + headerType + ')');

CodingFabian · 2014-10-21T07:32:56Z

@fkaelberer okay i reverted some of the experimental changes which did not have benefit.
it is now basically moving the alignByte out of the if check.
Besides that some additional header parsing for debugging aid and new style array access.
together with your #5426 it renders #5349 perfectly.

fkaelberer · 2014-10-21T07:57:54Z

Nice! The code looks very good to me.

yurydelendik · 2014-10-21T19:38:45Z

src/core/jpx.js

+                  // this indicates a YUV colorspace
+                  break;
+                default:
+                  warn('Unknown colorspace');


nit: here and below, add break; for default:

This patch makes the image from mozilla#5349 appear correctly, the artefacts for the last packet are fixed in mozilla#5426. This patch also optimizes some "in-checks" and adds a few header parsings.

yurydelendik · 2014-10-21T22:39:24Z

/botio-windows test

pdfjsbot · 2014-10-21T22:39:25Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2014-10-21T22:58:53Z

From: Bot.io (Windows)

Success

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

Total script time: 19.46 mins

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

For JPX decoding only read next packet after checking for byte aligning.

yurydelendik · 2014-10-21T23:05:38Z

Thank you

CodingFabian force-pushed the issue-5349 branch from ee0592f to 7b0f638 Compare September 26, 2014 21:49

timvandermeij added image-quality image-jpx labels Sep 26, 2014

CodingFabian changed the title ~~For JPX decoding only read next packet after checking for byte aligning.~~ WIP: For JPX decoding only read next packet after checking for byte aligning. Sep 27, 2014

CodingFabian force-pushed the issue-5349 branch from 7b0f638 to 2c5711d Compare September 27, 2014 08:41

timvandermeij added the 4-work-in-progress label Sep 27, 2014

timvandermeij changed the title ~~WIP: For JPX decoding only read next packet after checking for byte aligning.~~ For JPX decoding only read next packet after checking for byte aligning. Sep 27, 2014

timvandermeij removed the 4-work-in-progress label Sep 27, 2014

brendandahl assigned yurydelendik Oct 8, 2014

fkaelberer reviewed Oct 16, 2014
View reviewed changes

CodingFabian force-pushed the issue-5349 branch from 2c5711d to 35b72ef Compare October 16, 2014 16:06

CodingFabian force-pushed the issue-5349 branch from 35b72ef to 0b4dfe7 Compare October 16, 2014 18:55

fkaelberer mentioned this pull request Oct 20, 2014

Fix JPX "Out of Packets" Error (issues 4358, 4659, 4814) #5426

Merged

fkaelberer reviewed Oct 20, 2014
View reviewed changes

CodingFabian force-pushed the issue-5349 branch from 0b4dfe7 to 009cc3d Compare October 21, 2014 07:30

CodingFabian force-pushed the issue-5349 branch from 009cc3d to 808c8e1 Compare October 21, 2014 07:33

yurydelendik reviewed Oct 21, 2014
View reviewed changes

For JPX decoding only read next packet after checking for byte aligning.

b2d7c28

This patch makes the image from mozilla#5349 appear correctly, the artefacts for the last packet are fixed in mozilla#5426. This patch also optimizes some "in-checks" and adds a few header parsings.

CodingFabian force-pushed the issue-5349 branch from 808c8e1 to b2d7c28 Compare October 21, 2014 20:07

yurydelendik added a commit that referenced this pull request Oct 21, 2014

Merge pull request #5350 from CodingFabian/issue-5349

8bfc4b8

For JPX decoding only read next packet after checking for byte aligning.

yurydelendik merged commit 8bfc4b8 into mozilla:master Oct 21, 2014

CodingFabian deleted the issue-5349 branch October 22, 2014 06:25

CodingFabian mentioned this pull request Dec 20, 2014

Fixed empty packet parsing bug #5566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For JPX decoding only read next packet after checking for byte aligning. #5350

For JPX decoding only read next packet after checking for byte aligning. #5350

CodingFabian commented Sep 26, 2014

yurydelendik commented Sep 26, 2014

pdfjsbot commented Sep 26, 2014

pdfjsbot commented Sep 26, 2014

CodingFabian commented Sep 26, 2014

CodingFabian commented Sep 26, 2014

yurydelendik commented Sep 26, 2014

pdfjsbot commented Sep 26, 2014

pdfjsbot commented Sep 26, 2014

pdfjsbot commented Sep 26, 2014

pdfjsbot commented Sep 26, 2014

CodingFabian commented Sep 27, 2014

CodingFabian commented Sep 27, 2014

CodingFabian commented Sep 27, 2014

CodingFabian commented Oct 7, 2014

fkaelberer Oct 16, 2014

CodingFabian Oct 16, 2014

CodingFabian commented Oct 16, 2014

timvandermeij commented Oct 16, 2014

CodingFabian commented Oct 16, 2014

fkaelberer Oct 20, 2014

CodingFabian commented Oct 21, 2014

fkaelberer commented Oct 21, 2014

yurydelendik Oct 21, 2014

yurydelendik commented Oct 21, 2014

pdfjsbot commented Oct 21, 2014

pdfjsbot commented Oct 21, 2014

yurydelendik commented Oct 21, 2014

For JPX decoding only read next packet after checking for byte aligning. #5350

For JPX decoding only read next packet after checking for byte aligning. #5350

Conversation

CodingFabian commented Sep 26, 2014

yurydelendik commented Sep 26, 2014

pdfjsbot commented Sep 26, 2014

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 26, 2014

From: Bot.io (Windows)

Success

Published

CodingFabian commented Sep 26, 2014

CodingFabian commented Sep 26, 2014

yurydelendik commented Sep 26, 2014

pdfjsbot commented Sep 26, 2014

From: Bot.io (Linux)

Received

pdfjsbot commented Sep 26, 2014

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 26, 2014

From: Bot.io (Windows)

Failed

pdfjsbot commented Sep 26, 2014

From: Bot.io (Linux)

Failed

CodingFabian commented Sep 27, 2014

CodingFabian commented Sep 27, 2014

CodingFabian commented Sep 27, 2014

CodingFabian commented Oct 7, 2014

fkaelberer Oct 16, 2014

Choose a reason for hiding this comment

CodingFabian Oct 16, 2014

Choose a reason for hiding this comment

CodingFabian commented Oct 16, 2014

timvandermeij commented Oct 16, 2014

CodingFabian commented Oct 16, 2014

fkaelberer Oct 20, 2014

Choose a reason for hiding this comment

CodingFabian commented Oct 21, 2014

fkaelberer commented Oct 21, 2014

yurydelendik Oct 21, 2014

Choose a reason for hiding this comment

yurydelendik commented Oct 21, 2014

pdfjsbot commented Oct 21, 2014

From: Bot.io (Windows)

Received

pdfjsbot commented Oct 21, 2014

From: Bot.io (Windows)

Success

yurydelendik commented Oct 21, 2014