-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Fix error reading concatenated pdfs #5910
Fix error reading concatenated pdfs #5910
Conversation
It's great that you've included a test-case in the patch, but looking at it I'm not sure that we're actually allowed to include (and redistribute) the PDF file in the PDF.js repo. At the top of the pages, it says:
Would you be able to create a reduced test-case, without any potentially private data? |
I tried to and didn't manage to create quite the same problem - I created one which exhibited the same behavior in PDF.js, but it caused Adobe Reader to segfault (whereas Adobe reads the one I attached fine). I think it has to do with the object IDs in the two files, and if they are overlapping. I'll try again to create one which has the same behavior, perhaps taking the current PDF and removing some of the streams. |
1b153ad
to
f1f8393
Compare
I've replaced it with a new test file. It's got the same structure, but I've snipped out the content streams. |
"file": "pdfs/issue5909.pdf", | ||
"md5": "65c169b6f540b27ac0ff2738a80d1e14", | ||
"rounds": 1, | ||
"type": "eq" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can change this to a load
test instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, yeah I can seeing as it's just a blank PDF - it's not going to render anything anyway. I'll make that change and push up a new commit.
f1f8393
to
9727c78
Compare
Updated the test to just a load test. |
Looks like the windows failure is false, some file locking issue? |
@jordan-thoms Yes, that is not your fault. It has to be corrected on the Windows bot. |
}, | ||
{ "id": "issue5909", | ||
"file": "pdfs/issue5909.pdf", | ||
"md5": "65c169b6f540b27ac0ff2738a80d1e14", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bots complain:
WARNING: MD5 of file "pdfs/issue5909.pdf" does not match file. Expected "65c169b6f540b27ac0ff2738a80d1e14" computed "51a724136c0c10008bd061a78ea4b8fc"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jordan-thoms Perhaps you forgot to update the MD5 hash after updating the test file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes. I'll push up a new commit.
9727c78
to
d0ea772
Compare
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/25064c4eb05e32b/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/25064c4eb05e32b/output.txt Total script time: 0.84 mins Published |
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/a6a728e3c4c0bce/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/7fc423f0409923b/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/7fc423f0409923b/output.txt Total script time: 18.16 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/a6a728e3c4c0bce/output.txt Total script time: 23.26 mins
|
Fix error reading concatenated pdfs
Nice, thank you! |
This fixes issue #5909