Replace CanvasPixelArray with Uint8ClampedArray; optimizations; bugfixes #604
Conversation
|
CI failed for node 0.8 and 0.10 as expected. Package.json specifies the engine must be 0.12 or later. |
|
This is really cool! Good job 👍
Tests is passing on my local machine, code looks good. I would like it if we could support Node.js 0.10 but maybe that just isn't viable. Could you update the |
|
Realized from the failed tests that it's |
|
Thanks :)
|
|
Could you amend the last commit to also remove line 6 of the travis config, that was specific only to node.js version 0.8. |
|
Done |
|
As far as node 0.10 support, could make this version 2 and update the readme to be clear that v2 is for node 0.12+ only. |
|
Perfect 👍 Looks good to me, I'm happy to merge but I'll give the other maintainers some time to give their comments. I also think that it might be a good idea to release this as version 2.0.0. Need to think a bit about this. |
|
Yep, backwards-incompatible changes need new major version since we follow semver |
|
@zbjornson I think I detected a small "bug" with your performance fix thought. When Maybe it's better to check if |
|
@LinusU I was actually a bit confused about that and should have brought it up. I saw someone else added the equivalent-ish to that to getImageData (https://github.com/Automattic/node-canvas/pull/549/files), but I didn't understand why the same behavior should happen for (The biggest gain actually comes from how the source is accessed and destination is set -- working entirely with uint8s instead of bitshifting in/out of a uint32.) This look okay to you? // Context2d::PutImageData
if (a == 0) {
*dstRow++ = 0;
// ...
} else if (a == 255) {
*dstRow++ = b;
// ...
} else {
*dstRow++ = b * alpha;
// ...
} |
|
That looks very good, I also think that the intent is clearer. When I look at that I immediately understand it. Also, in GetImageData, couldn't we do something like this to get rid of the bitshifting there? - uint32_t *pixel = row + x + sx;
- uint8_t a = *pixel >> 24;
- uint8_t r = *pixel >> 16;
- uint8_t g = *pixel >> 8;
- uint8_t b = *pixel;
+ uint8_t *pixel = row + x + sx;And then access as |
|
Yeah, we can do the same thing in GetImageData -- I tried it and it was only slightly faster (not worth the regression risk), but I'll benchmark it again now that I'm more comfortable with what these functions do and have the test suit working. Commit coming in a few hours. Thanks! |
|
OK, bug fixed in putImageData. Benchmarked two variations of GetImageData, both of which were slower: (Aside, I tried using the benchmarking suite in the repo, which gave some really surprising timings. For example, |
|
This would be very good to merge before any more work goes into moving towards @zbjornson Is there anything more you would like to work on or do you feel comfortable with merging this? |
|
@LinusU I'm good with merging. Thanks! Hopefully sometime soon I can work on some other bugs :) |
|
This still uses the external array data API, which has been entirely removed. |
|
Right -- it's removed from the version of v8 used by node 3.0. (That's the API change I was referring to in my original PR message.) As soon as node-gyp works with iojs 3, I will add compiler directives that change which API is used. The API we need to switch to is not present in node 0.12-2.x (the non-externalizing API). |
|
node-gyp works with io.js 3, so does pangyp |
|
Would it not be possible to retain support for 0.10 (and / or 0.8) via https://github.com/joyent/node/blob/v0.10.40-release/src/v8_typed_array.cc ? |
|
Possibly -- let me look at that. Will also try to get node-gyp working with iojs 3 again. |
|
@zbjornson Did you get a chance to look at the part that kkoopa mentioned? |
|
Sorry -- swamped last week. Working on it today. |
|
No problem, tell me if you need help with anything! |
|
I don't think that that branch is done yet. My plan was to get this merged and then I would take a stab at completing the Nan switch. Do you need anything from Nan 2.x? |
|
It looks like Nan 2.0 is required for iojs 3.0, so I can only work on supporting node 0.8/0.10 without it. I'm working on 0.8/0.10 now. Perhaps after I finish that, we merge this branch with support only through 2.x, update/rebase #580 and then I can make the IndexedProperties changes for 3.0 there? Let me know what you want to do. |
|
That sounds perfect 👍 |
|
Done. No regressions in the browser/visual tests. Set travis to iojs 2.4.0, but 2.5.0 works as well. |
| clampedArray->SetIndexedPropertiesToExternalArrayData(dst, kExternalUint8ClampedArray, size); | ||
| #else | ||
| // TODO | ||
| #endif |
There was a problem hiding this comment.
Instead of doing nothing if NODE_MAJOR_VERSION < 3 I would rather that we gave a compiler error. Basically, I would prefer it if line 754, 756, 757 and 758 where gone.
Makes sense?
|
@zbjornson Could you remove your changes to |
Added some argument testing/manipulation to match WebKit/Moz behaviors. Additionally benchmarked: * branching on `if (a == 0 || a == 255)`, as it is * not branching (always doing the alpha calculation) * Mozilla's implementation found here: https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#5083 Mozilla's is insignificantly faster (p=0.17) :) so left it as-is.
Benchmark:
var canvas = new Canvas(300, 600);
var ctx = canvas.getContext("2d");
// any manipulation of canvas/ctx here.
var data = ctx.getImageData(0,0,300,600);
// time 1000x:
ctx.putImageData(data, 0, 0);
Negative arguments were causing source image data to be painted wrapped-around the other side of the canvas.
|
All done! |
|
@zbjornson if you want to jump to NAN v2, you can either battle with merging in my PR or you can do it yourself. Use this script https://gist.github.com/thlorenz/7e9d8ad15566c99fd116 as a first pass but it'll need some love afterwards. Mostly, the |
|
@LinusU are you waiting for anything from me? Probably best to do the nan 2 upgrade in a separate PR. |
|
Sorry for the delay, this is awesome! Merging now :) I'll release this as a |
Replace CanvasPixelArray with Uint8ClampedArray; optimizations; bugfixes
|
Sounds perfect. Will start working on iojs 3/nan 2 this week. Thanks! |
|
So I somehow managed to miss the actual cutting of a release. I'll get this out asap. |
|
Okay, here is what happened. I tagged the release, pushed it to npm, updated I have now pushed it to the main repo 👍 |
This updates quite a bit of stuff:
Tested primarily on node 2.4.0 (iojs) but uses v8 APIs available to node 0.12 and later. Node 3.0 (when v8 upgrades to 4.4) will need a quick API update but I'm poised to make those changes. I can't get node-gyp to work with iojs 3.0 nightlies right now, else I'd put in compiler directives.