Convert canvas thumbnails to PNG #6299
Conversation
|
|
||
| this.canvas = canvas; | ||
| this.div.setAttribute('data-loaded', true); | ||
| this.ring.appendChild(canvas); | ||
| //this.ring.appendChild(canvas); |
There was a problem hiding this comment.
Please remove the commented-out code here and above.
|
@zinking Once you have addressed the review comment, please squash the commits into one commit. See https://github.com/mozilla/pdf.js/wiki/Squashing-Commits on how to do that easily. |
|
It appears that the following from #6035 (comment) isn't implemented by the PR yet:
Edit: I'm wondering if a better solution for this PR wouldn't be to add a method to |
zinking
force-pushed
the
6035_png_thumb_nail
branch
from
9e4f3f7 to
d1b15d9
Compare
|
The solution looks fine to me. @Snuffleupagus I think we can leave without reencoding-on-cleanup part -- unless there is a clear indication the encoding of the png takes unreasonable amount of CPU. It will use some CPU but I'm thinking in overall not using lots of canvases might be a better choice. It will be better if we will have some benchmark with this PR. Also, using canvas'es toBlob() instead of toDataURL in some way might help with memory usage (similar to PDFJS.createObjectURL but it shall be async). |
|
Looks good to me too with the nit addressed and re-squashed after that. |
|
@zinking Do you have time to finish up this PR by addressing the comments above? |
|
@timvandermeij do you mean manually testing the memory usage ? I can do the rough testing using a few documents at hand, and provide some statistics, but this |
|
Yes, all that needs to be done is address the nit above and manually test the memory usage before and after your patch. |
The test is conducted on OSX 10.9.5 , Chrome 44.0.2403.157 (64 bit) looks saving memory as expected. |
|
@zinking Great, thank you for confirming that! Could you squash the commits into one commit? Then this should be good to merge. |
zinking
force-pushed
the
6035_png_thumb_nail
branch
from
8da4a70 to
161def7
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/560a640ae4811a3/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/560a640ae4811a3/output.txt Total script time: 0.64 mins Published |
Convert canvas thumbnails to PNG
|
Thank you for the patch! |
fixes #6035 , converting canvas thumb nails to png images