571029 - Update ReadPixels to take an ArrayBuffer instead of returning one

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

What's an ArrayBufferView? It's mentioned, not defined, in the spec.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

OK, sorry for the stupid question above, I understand now what an ArrayBufferView is, taking care of this.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: ReadPixels API change

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.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Thanks! That was it. Don't know how I missed it, with all the grepping I've been doing!

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: ReadPixels API change, ready

OK, this time it's ready for review.

This patch also removes the log messages about using old API, as discussed on IRC.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: ReadPixels API change, v3

This updated version cleans a bit the helper function computing byteLength for the old API.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: ReadPixels API change, v4

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.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/c98f635be282