Closed Bug 571029 Opened 12 years ago Closed 12 years ago

Update ReadPixels to take an ArrayBuffer instead of returning one

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: bjacob)

References

Details

Attachments

(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.

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/doc/spec/WebGL-spec.html#5.13.12
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+
http://hg.mozilla.org/mozilla-central/rev/c98f635be282
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.