We are recording a video file from <canvas>. We are trying to extract raw pixel data from <canvas> using getPixelData() and write it to a file. The data is unencoded ImageData object containing Typed Array of UInt8s. We cannot find file writer which accepts UInt8 data (no converter)... or on the other side of the coin canvas doesn't return byte strings or related format which is eaten by file writers. Here is the example code how we do it currently. However, as it converts UInt8 data to byte string in Javascript loop, it kills all the performance: setOutputFilename : UseXPCOM(function(filename) { this.outputFilename = filename; this.outputFile = Components.classes["@mozilla.org/file/local;1"].createInstance(Components.interfaces.nsILocalFile); this.outputFile.initWithPath(filename); var stream = Components.classes["@mozilla.org/network/file-output-stream;1"].createInstance(Components.interfaces.nsIFileOutputStream); // PR_WRONLY + PR_CREATE_FILE + PR_APPEND, default mode, no behaviour flags stream.init(this.outputFile, 0x02 | 0x10, 00600, false); this.stream = Components.classes["@mozilla.org/binaryoutputstream;1"].createInstance(Components.interfaces.nsIBinaryOutputStream); this.stream.setOutputStream(stream); }), grabFrame : UseXPCOM(function() { function convertToByteString(array) { var len = (array.length + 9999) / 10000; var str = ''; var f = String.fromCharCode; for (var i = 0; i < len; i++) { var off = i * 10000; str += f.apply(null, array.subarray(off, off + 10000)); } return str; } var ctx = this.producer.getCanvasContext(); var dimensions = this.producer.getDimensions(); var imageData = ctx.getImageData(0, 0, dimensions.width, dimensions.height); var data = imageData.data; data = convertToByteString(data); this.stream.writeBytes(data, data.length); }), In the most optimal situation * We grab the pixel data as UInt8 * Pixel data is asynchronously written to a file, leaving UI thread to render the next frame * File write accepts Typed Array natively and can convert UInt8 to bytes on native level

cc'ing people who might know the right people

Sounds like the kind of thing that should be supported in XPConnect.

Yeah, this shouldn't be too hard, as long as there aren't any snakes in the grass. We just have to teach XPCConvert about typed arrays. Taking, though not making any guarantees about the timeline. :-)

Here is a test case which times Array.slice() and byte array conversion approaches which is related to the discussion what's the most efficient way to get <canvas> data as serializiable array. http://jsfiddle.net/unRwB/ It appears that slicing Uint8ClampedArray is slow.

Ah, right. array_slice is slow in many cases when not slicing an actual array. I filed bug 699237 on an obvious speedup there.

I discussed this at length with Mikko and co yesterday, and they expressed interest in taking a crack at it. Mikko, are you guys working on it? I'm happy to provide the needed mentoring and support! :-)

Yep, I will look in the serialization code more closely on the weekend; did not seem completely impossible task for even a newbie like me ;)

(In reply to Antti Haapala from comment #7) > Yep, I will look in the serialization code more closely on the weekend; did > not seem completely impossible task for even a newbie like me ;) Awesome! Transferring ownership of the bug over to you. Ping me with any question. :-)

With this patch, write throughput of large chunks of data - 1M sized Uint8ClampedArrays directly to file are now limited only by disk speed instead of CPU usage. A debug build shows actual file writes being 3-4 times faster when compared to just the datatype conversions on release build of FF7.0.

Comment on attachment 572269 [details] [diff] [review] Proposal for converting JS TypedArrays to native in XPConnect Great! Is your patch ready for review? When it is, set the "review" flag to '?', and enter the person you want to review the code. In this case, I'm probably a good first-round reviewer. Once I'm satisfied, we can flag mrbkap for final approval.

Comment on attachment 572269 [details] [diff] [review] Proposal for converting JS TypedArrays to native in XPConnect >--- a/js/xpconnect/src/XPCConvert.cpp >+++ b/js/xpconnect/src/XPCConvert.cpp >+XPCConvert::JSTypedArray2Native(XPCCallContext& ccx, void **d, >+ JSObject *jsarray, JSUint32 count, >+ JSUint32 capacity, >+ const nsXPTType& type, uintN* pErr) nsresult* pErr, I'd think. That certainly expresses the meaning better. >+ void* output = nsnull; >+ PRUint8 tagPart = type.TagPart(); Why declare those here? Please move them as close to their first use as possible. (I don't think you actually need the tagPart local.) >+ JSObject *tArr = js::TypedArray::getTypedArray(jsarray); >+ if (!tArr) { >+ if (pErr) >+ *pErr = NS_ERROR_XPC_BAD_CONVERT_JS; Should return here too? Test? >+ if (len < count || capacity < count) { I have no idea what those parameters mean, but it seems strange that you have to check them. (Bobby?) >+ return JS_FALSE; Please use true/false throughout, even if the return value is JSBool. >+ // check that the tag type matches the expected; and then allocate >+ // the memory and copy the elements by memcpy Capital letters and full stops, please. >+#define CHECK_TARGET_AND_POPULATE(_tagTyp, _type) \ >+ size_t max = PR_UINT32_MAX / sizeof(_type); \ It seems that js/src and js/public use UINT32_MAX, and js/xpconnect uses PR_UINT32_MAX, so I guess you picked correctly. >+ if (capacity > max || \ >+ nsnull == (output = nsMemory::Alloc(capacity * sizeof(_type)))) { \ Don't compare to nsnull. >+ memcpy(output, js::TypedArray::getDataOffset(tArr), \ >+ count * sizeof(_type)); \ Indentation is off here. Can't this be a function? How about static bool CheckTargetAndPopulate(PRUint8 tagType, size_t typeSize, JSUint32 count, JSUint32 capacity, JSObject* tArr, void** output, nsresult* pErr) { if (tagPart != tagType) { if (pErr) *pErr = NS_ERROR_XPC_BAD_CONVERT_JS; return false; } size_t max = PR_UINT32_MAX / typeSize; if (capacity > max || !(*output = nsMemory::Alloc(capacity * typeSize))) { if (pErr) *pErr = NS_ERROR_OUT_OF_MEMORY; return false; } memcpy(*output, js::TypedArray::getDataOffset(tArr), count * typeSize); } and then call as case js::TypedArray::TYPE_INT8: if (!CheckTargetAndPopulate(nsXPTType::T_I8, sizeof(int8), count, capacity, tArr, &output, pErr)) { return false; } break; That's a bit more typing, but I think it's clearer. >+failure: >+ return JS_FALSE; This looks silly. Please return false directly instead of goto, unless I missed something.

(In reply to Ms2ger from comment #11) > Comment on attachment 572269 [details] [diff] [review] [diff] [details] [review] > >+ if (len < count || capacity < count) { > > I have no idea what those parameters mean, but it seems strange that you > have to check them. (Bobby?) length_is actually just went away with a patch that I landed: bug 677788. So you'll need to pull the latest code, and then just use 'len' here (which, paradoxically, was the variable that corresponded to size_is, while capacity corresponded to length_is). Otherwise, the check is fine. > Please use true/false throughout, even if the return value is JSBool. XPConnect is js-style, which uses JS_TRUE/JS_FALSE until something changes. So belay that order unless mrbkap says otherwise. > Can't this be a function? How about > > static bool I agree with getting rid of macros where possible. But make this JS_ALWAYS_INLINE to make sure we don't pay any performance penalty. Other than that, I'm on board with Ms2ger's review. Once you address those comments, go ahead and reflag me. ;-)

Well, to my newbie defense, I tried to follow the style of the existing code as much as possible ;) I think that the entire XPCConvert.cpp has quite interesting coding style probably bc some of it was already written in <1998 :D

(In reply to Antti Haapala from comment #13) > Well, to my newbie defense, I tried to follow the style of the existing code > as much as possible ;) I think that the entire XPCConvert.cpp has quite > interesting coding style probably bc some of it was already written in <1998 > :D Yeah, I think Ms2ger and I both failed to mention the impressiveness of this patch. :-) XPConnect is probably the most feared module of code in all of Mozilla, in part due to the "interesting coding style". So we're in this funny spot of trying to improve it and keep it consistent at the same time. But yes, this is a pretty damn pro first patch. I'm giving a talk in Berlin this weekend about how to get started hacking gecko, and I think I'm going to use this as an example of what's possible. ;-)

As Bobby said, I do appreciate the work you've done here! (I note that js/src/ does use true/false for JSBool, btw.)

Incorporated all input from previous review.

Comment on attachment 572774 [details] [diff] [review] Updated patch to reflect input from ms2ger, bholley. >--- a/js/xpconnect/src/XPCConvert.cpp >+++ b/js/xpconnect/src/XPCConvert.cpp >+XPCConvert::JSTypedArray2Native(XPCCallContext& ccx, I'd like an assert that js_IsTypedArray(tArray) is true here. >+ jsuint len = js::TypedArray::getLength(tArray); Please use uint32 or JSUint32 (AIUI, we want to get rid of jsuint.) >+ if (pErr) >+ pErr = NS_OK; Uh, I don't think this is what you meant. Looks good otherwise.

Comment on attachment 572774 [details] [diff] [review] Updated patch to reflect input from ms2ger, bholley. >--- a/js/xpconnect/src/xpcprivate.h >+++ b/js/xpconnect/src/xpcprivate.h >@@ -3312,17 +3312,24 @@ public: > */ > static JSBool NativeArray2JS(XPCLazyCallContext& ccx, > jsval* d, const void** s, > const nsXPTType& type, const nsID* iid, > JSUint32 count, nsresult* pErr); > > static JSBool JSArray2Native(XPCCallContext& ccx, void** d, jsval s, > JSUint32 count, const nsXPTType& type, >- const nsID* iid, uintN* pErr); >+ const nsID* iid, nsresult* pErr); Oh, and it would probably be good to fix the one caller that doesn't pass null to use an nsresult (<http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNative.cpp#2750>).

- Added NS_PRECONDITION assert for js_IsTypedArray(jsArray); - jsuint -> JSUint32 - uintN err -> nsresult err in caller. (the caller later passes a pointer to it to XPCConvert::JSStringWithSize2Native; that function was changed to use nsresult* pErr too). - and finally a note to self: macros are evil! pErr = NS_OK would have emitted warnings, had NS_OK been a const int, or had the macro used had other value than 0.

Bobby: https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style#Boolean_Types :)

(In reply to Ms2ger from comment #20) > Bobby: > https://wiki.mozilla.org/JavaScript:SpiderMonkey: > C%2B%2B_Coding_Style#Boolean_Types :) Fair enough - Given your interest in the subject, you are hereby charged with writing a patch to replace JS_TRUE/JS_FALSE with true/false in XPConnect. ;-)

Comment on attachment 572913 [details] [diff] [review] Try 3, fixed the errors noticed by bholley, ms2ger Ms2ger obsoleted part of my review that I wrote on the train. Curses! :-) diff --git a/js/xpconnect/src/XPCConvert.cpp b/js/xpconnect/src/XPCConvert.cpp > +// JSarray2Native whenever a TypedArray is met. ArrayBuffers > + NS_PRECONDITION(jsArray, "bad param"); > + NS_PRECONDITION(d, "bad param"); > + NS_PRECONDITION(js_IsTypedArray(jsArray), "not a typed array"); NS_PRECONDITION isn't fatal, so you should use JS_ASSERT or NS_ABORT_IF_FALSE here. > + // After js_IsTypedArray, this should not fail. In that case, just do an assertion here, and kill the error handling. + // Check the actual length of the input array against the + // given size_is nit: end this with a period. > + // XXX Is there any edge case that does not like non-canonical NaNs? Do you have any reason to believe that the behavior here will differ from regular arrays? I understand that we're memcpy-ing here rather than calling JS_ValueToNumber, but the JS_ValueToNumber appears to just return the data as a double. If there's no major reason for concern, just remove the comment here. > + // XXX Is there any edge case that does not like non-canonical NaNs? and here. diff --git a/js/xpconnect/tests/unit/test_params.js b/js/xpconnect/tests/unit/test_params.js > + // check that the given call results in exception being thrown. > + function doTypedArrayMismatchTest(name, val1, val1Size, val2, val2Size, exception) { > + var comparator = arrayComparator(standardComparator); > + var error = false; > + try { > + doIsTest(name, val1, val1Size, val2, val2Size, comparator); > + error = true; > + } > + catch (e) { > + if (exception && e.toString().indexOf(exception) == -1) > + throw "Exception was not " + exception; > + } > + if (error) { > + throw "TypedArray mismatch failed to raise exception"; > + } > + } > + It's better to do ok(false) than to throw in a test context. Also, it's best not to rely on the precise exception name. So let's do something like this: function doTypedArrayMismatchTest(name, val1, val1Size, val2, val2Size, exception) { var comparator = arrayComparator(standardComparator); var error = false; try { doIsTest(name, val1, val1Size, val2, val2Size, comparator); ok(false, "TypedArray mismatch failed to raise exception."); } catch (e) { ok(true, "Threw exception as expected."); } } > + doIsTest("testShortArray", Int16Array([2, 4, -6]), 3, Int16Array([1773, -32768, 32767, 7]), 4, arrayComparator(standardComparator)); > + > doIsTest("testLongLongArray", [-10000000000], 1, [1, 3, 1234511234551], 3, arrayComparator(standardComparator)); As mentioned on IRC, it'd be great if you feel like converting testLongLongArray to testDoubleArray. That'd give us 3 things: 1 - Test coverage for 64-bit array values. 2 - Test coverage for float values in arrays. 3 - Test coverage for another kind of typed array. I originally didn't do this because I didn't want to bother with nested comparator types (since doubles need a float comparator). But now that arrayComparator is a generator function, you can just do arrayComparator(fuzzComparator). If you really don't want to do this though, you don't have to. :-)

(In reply to Bobby Holley (:bholley) from comment #22) > NS_PRECONDITION isn't fatal, so you should use JS_ASSERT or NS_ABORT_IF_FALSE here. Then there are pretty many baaad lines of code in XPCConvert already. > Do you have any reason to believe that the behavior here will differ from > regular arrays? I understand that we're memcpy-ing here rather than calling > JS_ValueToNumber, but the JS_ValueToNumber appears to just return the data > as a double. If there's no major reason for concern, just remove the comment > here. Hngh, if I knew for sure either way, the comment would not be there. However, Float32Array and Float64Array seem to have special code for converting values TO js, to ensure that all NaNs are coded using the same bit pattern in jsval at least. In case the float array is a type-punned int array, then this is not the case necessarily. I believe it is highly unlikely that anything in the XPCOM world would depend on it, but I am not 100.0 % sure, only 99.999..., or so :)

(In reply to Antti Haapala from comment #23) > (In reply to Bobby Holley (:bholley) from comment #22) > > > NS_PRECONDITION isn't fatal, so you should use JS_ASSERT or NS_ABORT_IF_FALSE here. > > Then there are pretty many baaad lines of code in XPCConvert already. Yeah, and pretty much all over gecko. :\ Ideally we'd just make all assertions fatal, but then we'd hit them all over the place, because we often hit non-fatal assertions in our test suite. So the best thing to do is just to ensure that all _new_ assertions are fatal. > > > Do you have any reason to believe that the behavior here will differ from > > regular arrays? I understand that we're memcpy-ing here rather than calling > > JS_ValueToNumber, but the JS_ValueToNumber appears to just return the data > > as a double. If there's no major reason for concern, just remove the comment > > here. > > Hngh, if I knew for sure either way, the comment would not be there. > However, Float32Array and Float64Array seem to have special code for > converting values TO js, to ensure that all NaNs are coded using the same > bit pattern in jsval at least. In case the float array is a type-punned int > array, then this is not the case necessarily. I believe it is highly > unlikely that anything in the XPCOM world would depend on it, but I am not > 100.0 % sure, only 99.999..., or so :) Yeah, I'm pretty sure that's just there to let the JIT make better assumptions about typed arrays, and that XPCOM doesn't care. So I'd just remove the comment. ;-)

(In reply to Bobby Holley (:bholley) from comment #22) > It's better to do ok(false) than to throw in a test context. Also, it's best > not to rely on the precise exception name. So let's do something like this: Btw, ok was not defined, it is "do_check_true" there. Also, there are 2 different definitions for that function; make -C js/xpconnect/tests xpcshell-tests results in do_check_true being a function that considers its second argument being "stack", whereas in component_import.js, there is a function that accepts "cond" and "text".. %)

Ok, here we go again. Now testing also for Float64Arrays.

(In reply to Antti Haapala from comment #25) > Btw, ok was not defined, it is "do_check_true" there. Whoops! Yeah, sorry, 'ok' is from mochitests (another test suite within mozilla). Also, there are 2 > different definitions for that function; make -C js/xpconnect/tests > xpcshell-tests results in do_check_true being a function that considers its > second argument being "stack", whereas in component_import.js, there is a > function that accepts "cond" and "text".. %) hmm, cond and text are the ones we'd normally expect...

(In reply to Bobby Holley (:bholley) from comment #27) > hmm, cond and text are the ones we'd normally expect... Oh wait, there I go thinking in mochitest land again. Looks like it's standard just to pass one argument to do_check_true.

Comment on attachment 573287 [details] [diff] [review] Addressed the issues noticed by bholley, yet again >diff --git a/js/xpconnect/tests/idl/xpctest_params.idl b/js/xpconnect/tests/idl/xpctest_params.idl >- void testLongLongArray(in unsigned long aLength, [array, size_is(aLength)] in long long a, >- inout unsigned long bLength, [array, size_is(bLength)] inout long long b, >- out unsigned long rvLength, [retval, array, size_is(rvLength)] out long long rv); >+ void testDoubleArray(in unsigned long aLength, [array, size_is(aLength)] in double a, >+ inout unsigned long bLength, [array, size_is(bLength)] inout double b, >+ out unsigned long rvLength, [retval, array, size_is(rvLength)] out double rv); Normally when you change an IDL file you have to update the uuid at the top of it with a new one ( from http://mozilla.pettay.fi/cgi-bin/mozuuid.pl , for example). This is to ensure that any extension authors using binary components will know that the interfaces have changed. But since this is a test interface, it doesn't really matter. >+ // Check that the given call results in an exception being thrown. >+ function doTypedArrayMismatchTest(name, val1, val1Size, val2, val2Size) { >+ var comparator = arrayComparator(standardComparator); >+ var error = false; >+ try { >+ doIsTest(name, val1, val1Size, val2, val2Size, comparator); >+ >+ // An exception was not thrown as would have been expected. >+ do_check_true(false); >+ } >+ catch (e) { >+ } It would be good to have a do_check_true within the catch block. Looks good otherwise! r+ with the catchblock fix. Flagging peterv for high level review.

Comment on attachment 573287 [details] [diff] [review] Addressed the issues noticed by bholley, yet again I think peterv is pretty busy these days. I'm an unofficial XPConnect peer in things I understand, and this one qualifies. However, I want another pair of eyes on the JSAPI stuff. Flagging evilpie.

Comment on attachment 573287 [details] [diff] [review] Addressed the issues noticed by bholley, yet again Review of attachment 573287 [details] [diff] [review]: ----------------------------------------------------------------- Very cool, patch and the results seem to be impressive. Please don't be annoyed or offended, you did great work, but please remove the "private" typed array methods and use the JS_FRIEND_API functions defined at the end of jstypedarray.h. r+ after that :O

* Included only jstypedarray.h and use the JS_FRIEND_API funcs from there. * Added do_check_true(true); assertion into the catch statement. * Changed the UUID in the idl file. * Modified Int16Array test case to also test aliased arrays! * Changed JS_TRUE/JS_FALSE to true/false. * Changed the test code to not stuff negative values into an array of uints ;)

Comment on attachment 577150 [details] [diff] [review] Addressed issues found by evilpie, bholley Looks good! r=bholley assuming evilpie OKs the jsapi stuff.

https://hg.mozilla.org/mozilla-central/rev/d662c4cfabae Thanks for the awesome work Antti. bholley, somebody should blog about this. You want to handle that?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #35) > bholley, somebody should blog about this. You want to handle that? Done: http://bholley.wordpress.com/2011/12/13/typed-arrays-supported-in-xpconnect/

Wow. Very nice. A patch for a broadly-applicable feature from a new contributor to -- of all things! -- xpconnect! That's some pretty high-quality drive, and a prime example of the hacker ethos. Should you ever find yourself in Mountain View (California) or a gathering of other Mozillians, I'm sure you will have no problem finding people excited to buy you a beverage of your choice. ;-)

So you only accept typed arrays of the correct type, but then go and copy all of the bits anyway? Why can't you just pass the bits directly?

Hi Justin, I already was there and took my share of Mozilla's generous hospitality in San Francisco in the form of beef jerky and view over not-the-famous-bridge during Plone Conference. But we'll keep the offer in mind when we are around there next time :) Just to note that this patch was purely business driven and MUST in our requirements list. Even if the patch was not made for the sake of the love of Firefox, Mozilla's open spirit and easy to approach staff made it possible.

(In reply to neil@parkwaycc.co.uk from comment #38) > So you only accept typed arrays of the correct type, but then go and copy > all of the bits anyway? Why can't you just pass the bits directly? It's not ideal, but doing the buffer sharing would require some more significant architectural work that I didn't want to make ztane do. I just filed bug 710806 to keep track of this.

Well, clearly someone could also implement copy-on-write pages for TypedArrays, and other buffers in Mozilla. And even hardware MMU support. One can easily be deceived to think that "200 times faster is much better than 100 times faster". I see it this way: for the most demanding web application, at first the javascript code would be spending 80 % of all cpu time converting arrays using the "obvious way of array slice" to pass through XPConnect, and now after this patch say 2 % of all cpu time; your "perfect patch" then would cut the CPU consumption of this really freaky, corner case application by another 1 %. Peanuts, I say.