Created attachment 417313 [details] [diff] [review] implement Uint8ClampedArray Once we have typed arrays in bug 532774, it's pretty straightforward to extend it to an array type that implements the clamping semantics demanded by CanvasPixelArray. We can then emit equivalent custom trace code as well. Here's Uint8ClampedArray.
Created attachment 420844 [details] [diff] [review] v1 This works, and implements get/put/crateImageData. 2x speedup on something like http://www.galbraiths.org/benchmarks/pixastic.html, likely more on things that do repeated get/puts.
Just applying the patch for bug 532774 (and bug 533659 and the custom quickstubs thing) doesn't make this patch apply to t-m.... Was it developed against m-c instead?
Created attachment 423606 [details] [diff] [review] part 1, js side typed array changes Split up the v1 patch into three pieces -- this is the first one, that adds the core Uint8Clamped type that PixelArray needs.
Created attachment 423607 [details] [diff] [review] part 2, js side tracing additions part 2, tracing support for Uint8Clamped
Created attachment 423608 [details] [diff] [review] part 3, content side changes part 3, content side changes -- canvas code fixes and the custom quickstub code.
Comment on attachment 423608 [details] [diff] [review] part 3, content side changes >+nsIDOMCanvasRenderingContext2D_CreateImageData(JSContext *cx, uintN argc, jsval *vp) >+ if (!JS_ValueToECMAUint32(cx, argv[0], &w) || >+ !JS_ValueToECMAUint32(cx, argv[1], &h)) This doesn't look like it'll do the right thing for negative inputs. >+ JSObject *result = JS_NewObject(cx, NULL, NULL, NULL); The comment on why this needs to be created after the array got lost. Put it back? >+ JSAutoTempValueRooter rr(cx, result); >+ if (!result) >+ return JS_FALSE; Should this and the array case explicitly throw NS_ERROR_OUT_OF_MEMORY? >+nsIDOMCanvasRenderingContext2D_GetImageData(JSContext *cx, uintN argc, jsval *vp) >+ !JS_ValueToECMAUint32(cx, argv[2], &w) || >+ !JS_ValueToECMAUint32(cx, argv[3], &h)) See above. Is there a reason the >- if (len > (((PRUint32)0xfff00000)/sizeof(jsval))) check is done in the _explicit method instead of here? Does it need to be done at all now? >+ // then build the object Again, this lost the comment about _why_ the object allocation needs to come after the array allocation. >+nsIDOMCanvasRenderingContext2D_PutImageData(JSContext *cx, uintN argc, jsval *vp) >+ if (!JSVAL_IS_OBJECT(argv[0]) || JSVAL_IS_NULL(argv[0])) if (JSVAL_IS_PRIMITIVE(argv[0])) >+ !JS_ValueToECMAUint32(cx, v, &w)) Again, negative number issues.... >+ !JS_ValueToECMAUint32(cx, v, &h)) And here. And v needs to be rooted if we're doing a JS_GetProperty into it. >+ if (!JS_GetProperty(cx, dataObject, "data", &v) || >+ !JSVAL_IS_OBJECT(v)) >+ return JS_FALSE; >+ darray = JSVAL_TO_OBJECT(v); darray might be null now. You probably wanted JSVAL_IS_PRIMITIVE, not !JSVAL_IS_OBJECT. >+ *vp = JSVAL_VOID; Is this needed? Please check with Jason. >+++ b/content/canvas/src/nsCanvasRenderingContext2D.cpp >+nsCanvasRenderingContext2D::GetImageData_explicit(PRInt32 x, PRInt32 y, PRUint32 w, PRUint32 h, >+ src = aData + (destStride * j); Shouldn't src already have the right value here (just like dest) from the end of the previous iteration? >+nsCanvasRenderingContext2D::PutImageData_explicit(PRInt32 x, PRInt32 y, PRUint32 w, PRUint32 h, >+ src = aData + (w * 4 * j); Again, src should already have the right value from the previous loop iteration. >+ dst = imgsurf->Data() + imgsurf->Stride() * j; Can't tell you whether this would, though. >+ /* Do the awful premultiply */ Leave the SSE/MMX comment that used to be here? >+++ b/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl >+ [noscript] void getImageData_explicit(in long x, in long y, in unsigned long width, in unsigned long height, >+ [array, size_is(dataLen)] in octet dataPtr, in unsigned long dataLen); >+ [noscript] void putImageData_explicit(in long x, in long y, in unsigned long width, in unsigned long height, >+ [array, size_is(dataLen)] in octet dataPtr, in unsigned long dataLen); Can we get some docs about how dataLen should be related to width+height, at least?
(In reply to comment #6) > >+ *vp = JSVAL_VOID; > > Is this needed? Please check with Jason. Yes. |*vp| is the callee on entry, and fast natives must set a return value in non-error cases.
Comment on attachment 423606 [details] [diff] [review] part 1, js side typed array changes >+ inline uint8 operator= (const uint8 x) { It strikes me as a little funny that these arguments are const and that the return type here is uint8 rather than uint8_clamped&. But I can't imagine either one makes any difference at all to the compiler.
Part 1 and part 2 checked in; fixed issues in comment #8 on checkin. (Compiler didn't seem to care for sure, but it looked neater) http://hg.mozilla.org/tracemonkey/rev/763e4245ae9f http://hg.mozilla.org/tracemonkey/rev/f7017e7dfb20 Working on updated patch for part 3 now.
Created attachment 425354 [details] [diff] [review] part 3, content side changes, v2 Updated for review comments. Boris, note that this has to be applied on top of bug 534735 but is pretty much independent of it -- it's just that they both need/want to create Canvas2D_QS.h, which doesn't exist yet. I'll fix the checkin order for whichever gets r+ first.
Comment on attachment 425354 [details] [diff] [review] part 3, content side changes, v2 > Again, this lost the comment about _why_ the object allocation needs to come > after the array allocation. This comment wasn't addressed. > if (JSVAL_IS_PRIMITIVE(argv[0])) Nor this one. >+ uint32 wi, hi; Those should be int32. r=bzbarsky with those three issues fixed.
vlad, were you waiting on something else from me here before landing?
Nope, vacation interfered.. waiting on final try server builds now.
Finally landed and stuck -- http://hg.mozilla.org/mozilla-central/rev/c1cc02524fbb
*** Bug 453355 has been marked as a duplicate of this bug. ***
