Closed Bug 534467 Opened 10 years ago Closed 10 years ago

optimize CanvasPixelArray

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

References

Details

Attachments

(3 files, 3 obsolete files)

Attached patch implement Uint8ClampedArray (obsolete) — Splinter Review
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.
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #417313 - Attachment is obsolete: true
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?
Split up the v1 patch into three pieces -- this is the first one, that adds the core Uint8Clamped type that PixelArray needs.
Attachment #420844 - Attachment is obsolete: true
Attachment #423606 - Flags: review?(jorendorff)
part 2, tracing support for Uint8Clamped
Attachment #423607 - Flags: review?(gal)
Attached patch part 3, content side changes (obsolete) — Splinter Review
part 3, content side changes -- canvas code fixes and the custom quickstub code.
Attachment #423608 - Flags: review?(bzbarsky)
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?
Attachment #423608 - Flags: review?(bzbarsky) → review-
(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.
Attachment #423607 - Flags: review?(gal) → review+
Attachment #423606 - Flags: review?(jorendorff) → review+
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.
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.
Attachment #423608 - Attachment is obsolete: true
Attachment #425354 - Flags: review?(bzbarsky)
Attachment #425354 - Flags: review?(bzbarsky) → review+
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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Duplicate of this bug: 453355
Depends on: 551326
You need to log in before you can comment on or make changes to this bug.