Closed Bug 571029 Opened 12 years ago Closed 12 years ago

Update ReadPixels to take an ArrayBuffer instead of returning one


(Core :: Canvas: WebGL, defect)

Not set





(Reporter: vlad, Assigned: bjacob)




(1 file, 3 obsolete files)

As per spec change:

 - Updated readPixels() specification to take ArrayBufferView as
   argument rather than returning it. Specified behavior for
   out-of-range pixels and mismatches between type and pixels
   arguments, the latter for texImage2D as well.
What's an ArrayBufferView? It's mentioned, not defined, in the spec.
OK, sorry for the stupid question above, I understand now what an ArrayBufferView is, taking care of this.
Attached patch ReadPixels API change (obsolete) — Splinter Review
Here's my draft patch for this. But I need help: my quickstub for readPixels is not actually being used, making readPixels not work at all. I have no idea why it's not used. The only relevant info I can find is this gcc warning:

../../../../dist/include/CustomQS_WebGL.h:217: warning: ‘JSBool nsICanvasRenderingContextWebGL_ReadPixels(JSContext*, uintN, jsval*)’ defined but not used

that is generated when compiling the quickstubs. Indeed, the generated  dom_quickstubs.cpp file doesn't contain ReadPixels.
In dom_quickstubs.qsconf, you need to remove the line in the interface list that negates readpixels from being quickstubbed.  Search for readpixels in that file.
Thanks! That was it. Don't know how I missed it, with all the grepping I've been doing!
Attached patch ReadPixels API change, ready (obsolete) — Splinter Review
OK, this time it's ready for review.

This patch also removes the log messages about using old API, as discussed on IRC.
Attachment #451596 - Attachment is obsolete: true
Attachment #452006 - Flags: review?(vladimir)
Attached patch ReadPixels API change, v3 (obsolete) — Splinter Review
This updated version cleans a bit the helper function computing byteLength for the old API.
Attachment #452006 - Attachment is obsolete: true
Attachment #452013 - Flags: review?(vladimir)
Attachment #452006 - Flags: review?(vladimir)
This new version adds 2 things:
 - fixes also bug 572797, allow non-fitting rectanges
 - checks that width and height are nonnegative, return INVALID_VALUE if negative.
Attachment #452013 - Attachment is obsolete: true
Attachment #452077 - Flags: review?(vladimir)
Attachment #452013 - Flags: review?(vladimir)
Blocks: 572797
Comment on attachment 452077 [details] [diff] [review]
ReadPixels API change, v4

>+        /*** END old API deprecated code ***/
>+    } else if (argc == 7 && JSVAL_IS_OBJECT(argv[6])) {

this needs to be && !JSVAL_IS_PRIMITIVE(argv[6]) -- JSVAL_NULL is an object and a primitive, so with just the JSVAL_IS_OBJECT test, we could end up with a null argv6 and crash in js_IsArrayBuffer etc.

>+        if (   x >= boundWidth
>+            || x+width <= 0
>+            || y >= boundHeight
>+            || y+height <= 0) {

^ Style nit: { on its own line after a multi-line if.
Attachment #452077 - Flags: review?(vladimir) → review+
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.