Update ReadPixels to take an ArrayBuffer instead of returning one

RESOLVED FIXED

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: vlad, Assigned: bjacob)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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
(Assignee)

Comment 1

8 years ago
What's an ArrayBufferView? It's mentioned, not defined, in the spec.
(Assignee)

Comment 2

8 years ago
OK, sorry for the stupid question above, I understand now what an ArrayBufferView is, taking care of this.
(Assignee)

Comment 3

8 years ago
Created attachment 451596 [details] [diff] [review]
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.
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.
(Assignee)

Comment 5

8 years ago
Thanks! That was it. Don't know how I missed it, with all the grepping I've been doing!
(Assignee)

Comment 6

8 years ago
Created attachment 452006 [details] [diff] [review]
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.
Attachment #451596 - Attachment is obsolete: true
Attachment #452006 - Flags: review?(vladimir)
(Assignee)

Comment 7

8 years ago
Created attachment 452013 [details] [diff] [review]
ReadPixels API change, v3

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)
(Assignee)

Comment 8

8 years ago
Created attachment 452077 [details] [diff] [review]
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.
Attachment #452013 - Attachment is obsolete: true
Attachment #452077 - Flags: review?(vladimir)
Attachment #452013 - Flags: review?(vladimir)
(Assignee)

Updated

8 years ago
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
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.