Soft Masks for Natively Supported(decoded by browser) JPEGs #926
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@pdfjsbot test |
|
Processing command test by user brendandahl. Queue size: 0 Live script output is available (after queueing is done) at: http://184.73.87.52:8989/3111905.txt [bot:processed:3111905] |
ERROR(s) foundATTENTION: There was a snapshot difference: Output: Bot response time: 32.10 mins |
|
I make a review of this one. |
| var buf = new Uint8Array(size * components); | ||
| var tempCanvas = new ScratchCanvas(width, height); | ||
| var tempCtx = tempCanvas.getContext('2d'); | ||
| tempCtx.drawImage(img, 0, 0); |
There was a problem hiding this comment.
At other places in the code, we use tmpCanvas and tmpCtx. Want to rename the variables tempCanvas and tempCtx?
I like the idea of using callbacks, that make the messaging way more easier :) However, I dislike the
What do you think? |
|
This patch looks pritty good - see some comments. @brendandahl, can you add a test PDF to the unit tests to catch regressions in the future? |
That's interesting, that would solve the problem of a call back never being invoked. The one thing I don't like is it makes it much harder to invoke the reply at a later time. For instance say you had to wait on another callback you couldn't just pass it the message and have it reply when it was done. Still thinking... |
|
@pdfjsbot test |
|
Processing command test by user brendandahl. Queue size: 0 Live script output is available (after queueing is done) at: http://184.73.87.52:8989/3115482.txt [bot:processed:3115482] |
ERROR(s) foundATTENTION: There was a snapshot difference: Output: Bot response time: 32.34 mins |
|
Not working in chrome, I'll fix that. |
Ahh, I see. We could check if the return value is a promise and then use value of the resolved promise to send back to the sender. Is that too much complexity? |
|
Should be working on chrome now. Chrome doesn't like the postMessage function being called without the right context. |
Closed
To me that sounds a little too magic. Doing message.reply seems more explicit then letting a return handle things. Now instead of passing in a message we could pass in a promise. So instead of message.reply() you'd do promise.resolve(), but then we'd have to attach data to the promise. Or maybe m.on('do_some_work', function(data, promise) {}); where the second argument 'promise' would be optional. Or maybe we're getting a little too abstract altogether :) |
|
Anyone else have any thoughts on the new framework stuff and if its easy enough to follow? |
|
I like the If you think it's not worth doing the changes, we can keep what we have for now and do the refactoring later if needed. |
|
I've changed it to use promises. It was pretty easy to change and it ends up with less code overall. @pdfjsbot test |
|
Processing command test by user brendandahl. Queue size: 0 Live script output is available (after queueing is done) at: http://184.73.87.52:8989/3151599.txt [bot:processed:3151599] |
ERROR(s) foundATTENTION: There was a snapshot difference: Output: Bot response time: 33.58 mins |
|
Not sure why chrome sizes fails, seems to be working fine locally. |
| var imgData = this.objs.get(objId); | ||
| if (!imgData) { | ||
| error('Dependent image isn\'t ready yet'); | ||
| } |
There was a problem hiding this comment.
to be consistent with code below, the braces shall be removed
|
the nit with the braces can be applied with future fixes (base on IRC's "is starting to get a back log of smask fixes branched from that branch") @pdfjsbot makeref |
|
Processing command makeref by user notmasteryet. Queue size: 1 Live script output is available (after queueing is done) at: http://184.73.87.52:8989/3154690.txt [bot:processed:3154690] |
|
References generated Images pushed to Output: Bot response time: 46.07 mins |
notmasteryet
added a commit
that referenced
this pull request
Dec 15, 2011
Soft Masks for Natively Supported(decoded by browser) JPEGs
This was referenced Jan 2, 2012
Closed
yurydelendik
added a commit
to yurydelendik/pdf.js
that referenced
this pull request
May 2, 2014
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #883, #704, #652.
The main issue was we regressed from the worker code and didn't support soft masks for jpegs anymore. This became more prevalent when we added the CMYK jpeg code since that had a different code path than the natively supported jpegs. The above two issues were from a jpeg that we had to decode, but the jpeg also had a jpeg softmask which was a natively supported jpeg.
I've added back support for jpeg soft masks but the code is now much more complicated than when we previously supported them because of the workers. To try and make this easier I've added two new pieces.
-MessageHandler.send() now accepts a third parameter of a callback. This allows you to send something to main thread or worker and expect a response back e.g.
m.send('do_some_work', data, function(work_result) { alert('look ma ' + worker_result });and then on the other side of things
m.on('do_some_work', function(message) { message.reply('I did some work'); })-I added a new '"static" helper function to Promise named all(). This is a pretty common function in other Promise frameworks. You pass in multiple promises and it creates a wrapper promise that is resolved when all the promises are resolved. This makes it so if both the soft mask and the regular image are natively supported jpegs then we can send them both to the main thread and wait for them.
I expect this to fail on pdkids page 9. This image is displayed wrong currently and will still be displayed wrong after this PR. It is a natively supported JPEG with a flate soft mask. It is still displayed wrong because the soft mask uses an unsupported decode map. I plan to address this in a separate PR and will add the issue number when I create it.
Update: Looks like there's already an issue for the decode array #399.