-
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
Faster 1 bpp image drawing #4466
Conversation
Travis somewhat complains: src/display/canvas.js: line 551, col 150, Line is too long. src/display/canvas.js: line 462, col 12, 'performance' is not defined. src/display/canvas.js: line 546, col 15, 'performance' is not defined. |
my bad, looks like my git commit -p included parts of the performance test comparing the different versions. I'll fix this when I get access to a laptop. |
Done. Travis seems happier now. |
if (srcPos >= srcLength) { | ||
break; | ||
var srcLastLine = srcLength - (width >> 3); | ||
var nativeTypedArray = 'set' in dest && 'subarray' in src; |
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.
We decided to move the support for the IE9 to the compatibility.js (see #4420).
I'm not finding the use of new Uint32Array() and polyfill for that. Let's special-case it in compatibility.js: for Uint32Array constructor with array, and we will create/return an object instead with the numerical index setters that will tweak the array's items. (CanvasPixelArray may be extended to return itself in the buffer getter as well). Let me know I you need help with creating such polyfill.
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.
That sounds much cleaner, however I'm not sure what's the most efficient way to define numerical setters for an array of 33M Uint32s. Defining 33M setters is not gonna cut it, or do you know a way to define arbitrary numerical setters ?
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.
Why 33M? Looks like fullChunkHeight == 16, so I'm expecting amount of setters ~256K. Also, we can try to cache them on the object's prototype.
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.
Actually the setters are on the Uint32Array, so we're rather talking about ~80.000 unique setters. Still a big number.The poor souls on IE9+ will suffer a greater pain.
Is that more or less what you have in mind ?
for (var i = 0; i < this.length; i++)
{
Object.defineProperty(this, String(i),
{
set: (function(i) {
var index = i * 4;
return function(newValue){
// this._Uint8Array refers to the Uint8Array passed in the
// constructor of the Uint32Array
var dest = this._Uint8view;
dest[index++] = newValue & 255;
dest[index++] = (newValue >> 8) & 255;
dest[index++] = (newValue >> 16) & 255;
dest[index++] = (newValue >> 24) & 255;
};
})(i)
})
}
Also I took the IE11's F12 Developer Tools for a spin to get a profile of how IE fares with a test documents containing 10 600 DPI JBIG2 images. Decoding one of these takes ~1s in Firefox, ~1.2 s in Opera, and ~15s in IE11. This makes me all the more doubtful about creating tens of thousands of unique setters for IE.
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.
Here is my prototype/testing https://gist.github.com/yurydelendik/9624811 . It's expected IE9 be a little bit slower. Decoding on my low end machine of 4000x8000 image takes about 7 sec, not sure what are the expectation here, but carrying code that used by small amount of users is not really acceptable, since we cannot really test or properly maintain it.
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 1 Live output at: http://107.21.233.14:8877/6f8a3901f1a42be/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 1 Live output at: http://107.22.172.223:8877/25bb0c0c5caae8a/output.txt |
From: Bot.io (Linux)FailedFull output at http://107.21.233.14:8877/6f8a3901f1a42be/output.txt Total script time: 25.87 mins
Image differences available at: http://107.21.233.14:8877/6f8a3901f1a42be/reftest-analyzer.html#web=eq.log |
Hmm, loosing some pixels/lines at |
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/25bb0c0c5caae8a/output.txt Total script time: 37.26 mins
Image differences available at: http://107.22.172.223:8877/25bb0c0c5caae8a/reftest-analyzer.html#web=eq.log |
|
||
ctx.putImageData(chunkImgData, 0, i * fullChunkHeight); | ||
dest32[destPos++] = (srcByte & mask) ? 0xFFFFFFFF : 0xFF000000; |
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.
Looks like there is an assumption the all platforms will be little-endian based. We shall not assume there will no be big-endian Uint32Array.
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.
True.
Where would you rather see the detection of endianness; compatibility.js, util.js, canvas.js ?
About the endianness of dest32, I could force little-endian but this could be significantly slower on big-endian platforms. Better make two local variables: black
and white
based on the endianness of the platform.
edit: s/smaller/slower
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 detection can be added to the util.js.
I like making "black and white based on the endianness of the platform" thing
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.21.233.14:8877/9612e6a50988967/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 1 Live output at: http://107.22.172.223:8877/4760ddaa259933d/output.txt |
From: Bot.io (Linux)FailedFull output at http://107.21.233.14:8877/9612e6a50988967/output.txt Total script time: 3.44 mins
Image differences available at: http://107.21.233.14:8877/9612e6a50988967/reftest-analyzer.html#web=eq.log |
|
||
function Uint32ArrayView(buffer) { | ||
this.buffer = buffer; | ||
this.byteLength = buffer.byteLength; |
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.
this.byteLength = buffer.length';
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/4760ddaa259933d/output.txt Total script time: 3.95 mins
Image differences available at: http://107.22.172.223:8877/4760ddaa259933d/reftest-analyzer.html#web=eq.log |
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.21.233.14:8877/983f252c3470781/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.22.172.223:8877/5e685f388fc9423/output.txt |
From: Bot.io (Linux)FailedFull output at http://107.21.233.14:8877/983f252c3470781/output.txt Total script time: 25.11 mins
Image differences available at: http://107.21.233.14:8877/983f252c3470781/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/5e685f388fc9423/output.txt Total script time: 36.10 mins
Image differences available at: http://107.22.172.223:8877/5e685f388fc9423/reftest-analyzer.html#web=eq.log |
There are some issues with issue1658-page10. Could you also squash the commits in one? |
for (var i = 3; i < destDataLength; i += 4) { | ||
dest[i] = 255; | ||
} | ||
var srcLastLine = srcLength - (width >> 3); |
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.
unused variable
Done. |
canvas.width = canvas.height = 1; | ||
var ctx = canvas.getContext('2d'); | ||
var imageData = ctx.createImageData(1, 1); | ||
return (typeof imageData.data.buffer != 'undefined'); |
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.
Nit: use strict equality.
The following changes make putBinaryImageData 2.2x faster. * Use a Uint32Array to draw whole pixels instead component by component * Unroll the inner most loop * Added lazy PDFJS.hasCanvasTypedArrays, PDFJS.isLittleEndian and compatibility Uint32ArrayView for browsers using the old CanvasPixelArray
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 1 Live output at: http://107.21.233.14:8877/300c346bdced9fa/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 1 Live output at: http://107.22.172.223:8877/dedaaf92938fc97/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/300c346bdced9fa/output.txt Total script time: 25.87 mins
|
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/dedaaf92938fc97/output.txt Total script time: 36.17 mins
|
Faster 1 bpp image drawing
Nice. Thank you for the patch |
Moving up the end condition and using Uint32Array view of the destination array where possible gives about ~36% speed improvement on 33M pixels B&W images ( typical B&W scanned document ), going from ~1180ms down to ~753ms.