-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
Use fewer multiplications in JpegImage._convertCmykToRgb
#11547
Use fewer multiplications in JpegImage._convertCmykToRgb
#11547
Conversation
*Note:* This is inspired by PR 5473, which made similar changes for another kind of JPEG data. Since the implementation in `src/core/jpg.js` only supports 8-bit data, as opposed to similar code in `src/core/colorspace.js`, the computations can be further simplified since the `scale` is always constant. By updating the coefficients, effectively inlining the `scale`, we'll thus avoid *four* multiplications for each loop iteration. Unfortunately I wasn't able, based on a quick look through the test-files, to find a sufficiently *large* CMYK JPEG image in order for these changes to really show up in benchmark results. However, when testing the `cmykjpeg.pdf` manually there's a total of `120 000` fewer multiplication with this patch.
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/a3708a4957c505a/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/58a39edf51d6bc0/output.txt |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/58a39edf51d6bc0/output.txt Total script time: 2.20 mins
Image differences available at: http://54.215.176.217:8877/58a39edf51d6bc0/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/a3708a4957c505a/output.txt Total script time: 19.51 mins
Image differences available at: http://54.67.70.0:8877/a3708a4957c505a/reftest-analyzer.html#web=eq.log |
@brendandahl Could you perhaps fix the Windows bot failure? |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 1 Live output at: http://54.67.70.0:8877/04e30cc3b427165/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/e9ef699baeeb549/output.txt |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/e9ef699baeeb549/output.txt Total script time: 26.21 mins
Image differences available at: http://54.215.176.217:8877/e9ef699baeeb549/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/04e30cc3b427165/output.txt Total script time: 19.74 mins
Image differences available at: http://54.67.70.0:8877/04e30cc3b427165/reftest-analyzer.html#web=eq.log |
Good idea! |
Note: This is inspired by PR #5473, which made similar changes for another kind of JPEG data.
Since the implementation in
src/core/jpg.js
only supports 8-bit data, as opposed to similar code insrc/core/colorspace.js
, the computations can be further simplified since thescale
is always constant.By updating the coefficients, effectively inlining the
scale
, we'll thus avoid four multiplications for each loop iteration.Unfortunately I wasn't able, based on a quick look through the test-files, to find a sufficiently large CMYK JPEG image in order for these changes to really show up in benchmark results. However, when testing the
cmykjpeg.pdf
manually there's a total of120 000
fewer multiplication with this patch.