Bug 697560 - Generate WebGL errors in readPixels according to spec - r=bjacob

* * *
Bug 697560 - Disallow conformance failures on read-pixels-test - r=bjacob

content/canvas/src/WebGLContext.h

@@ -580,7 +580,7 @@ protected:
                                 int jsArrayType,
                                 int srcFormat, bool srcPremultiplied);
     nsresult ReadPixels_base(WebGLint x, WebGLint y, WebGLsizei width, WebGLsizei height,
-                             WebGLenum format, WebGLenum type, void *data, PRUint32 byteLength);
+                             WebGLenum format, WebGLenum type, JSObject* pixels);
     nsresult TexParameter_base(WebGLenum target, WebGLenum pname,
                                WebGLint *intParamPtr, WebGLfloat *floatParamPtr);
 

content/canvas/src/WebGLContextGL.cpp

@@ -3340,7 +3340,7 @@ WebGLContext::ReadPixels(PRInt32)
 
 nsresult
 WebGLContext::ReadPixels_base(WebGLint x, WebGLint y, WebGLsizei width, WebGLsizei height,
-                              WebGLenum format, WebGLenum type, void *data, PRUint32 byteLength)
+                              WebGLenum format, WebGLenum type, JSObject* pixels)
 {
     if (HTMLCanvasElement()->IsWriteOnly() && !nsContentUtils::IsCallerTrustedForRead()) {
         LogMessageIfVerbose("ReadPixels: Not allowed");
@@ -3350,43 +3350,61 @@ WebGLContext::ReadPixels_base(WebGLint x, WebGLint y, WebGLsizei width, WebGLsiz
     if (width < 0 || height < 0)
         return ErrorInvalidValue("ReadPixels: negative size passed");
 
-    // there's nothing to do in this case, since we won't read any pixels
-    if (width == 0 || height == 0)
-        return NS_OK;
+    if (!pixels)
+        return ErrorInvalidValue("ReadPixels: null array passed");
 
     WebGLsizei boundWidth = mBoundFramebuffer ? mBoundFramebuffer->width() : mWidth;
     WebGLsizei boundHeight = mBoundFramebuffer ? mBoundFramebuffer->height() : mHeight;
 
-    PRUint32 size = 0;
-    bool badFormat = false, badType = false;
+    void* data = JS_GetTypedArrayData(pixels);
+    PRUint32 dataByteLen = JS_GetTypedArrayByteLength(pixels);
+    int dataType = JS_GetTypedArrayType(pixels);
+
+    PRUint32 channels = 0;
+
+    // Check the format param
     switch (format) {
-    case LOCAL_GL_RGBA:
-        size = 4;
-        break;
-    default:
-        badFormat = true;
-        break;
+        case LOCAL_GL_ALPHA:
+            channels = 1;
+            break;
+        case LOCAL_GL_RGB:
+            channels = 3;
+            break;
+        case LOCAL_GL_RGBA:
+            channels = 4;
+            break;
+        default:
+            return ErrorInvalidEnum("readPixels: Bad format");
     }
 
+    PRUint32 bytesPerPixel = 0;
+    int requiredDataType = 0;
+
+    // Check the type param
     switch (type) {
-    case LOCAL_GL_UNSIGNED_BYTE:
-        break;
-    default:
-        badType = true;
-        break;
+        case LOCAL_GL_UNSIGNED_BYTE:
+            bytesPerPixel = 1 * channels;
+            requiredDataType = js::TypedArray::TYPE_UINT8;
+            break;
+        case LOCAL_GL_UNSIGNED_SHORT_4_4_4_4:
+        case LOCAL_GL_UNSIGNED_SHORT_5_5_5_1:
+        case LOCAL_GL_UNSIGNED_SHORT_5_6_5:
+            bytesPerPixel = 2;
+            requiredDataType = js::TypedArray::TYPE_UINT16;
+            break;
+        default:
+            return ErrorInvalidEnum("readPixels: Bad type");
     }
 
-    if (badFormat && badType)
-        return ErrorInvalidOperation("readPixels: bad format and type");
-    if (badFormat)
-        return ErrorInvalidEnumInfo("readPixels: format", format);
-    if (badType)
-        return ErrorInvalidEnumInfo("ReadPixels: type", type);
+    // Check the pixels param type
+    if (dataType != requiredDataType)
+        return ErrorInvalidOperation("readPixels: Mismatched type/pixels types");
 
+    // Check the pixels param size
     CheckedUint32 checked_neededByteLength =
-        GetImageSize(height, width, size, mPixelStorePackAlignment);
+        GetImageSize(height, width, bytesPerPixel, mPixelStorePackAlignment);
 
-    CheckedUint32 checked_plainRowSize = CheckedUint32(width) * size;
+    CheckedUint32 checked_plainRowSize = CheckedUint32(width) * bytesPerPixel;
 
     CheckedUint32 checked_alignedRowSize =
         RoundedToNextMultipleOf(checked_plainRowSize, mPixelStorePackAlignment);
@@ -3394,9 +3412,28 @@ WebGLContext::ReadPixels_base(WebGLint x, WebGLint y, WebGLsizei width, WebGLsiz
     if (!checked_neededByteLength.valid())
         return ErrorInvalidOperation("ReadPixels: integer overflow computing the needed buffer size");
 
-    if (checked_neededByteLength.value() > byteLength)
+    if (checked_neededByteLength.value() > dataByteLen)
         return ErrorInvalidOperation("ReadPixels: buffer too small");
 
+    // Check the format and type params to assure they are an acceptable pair (as per spec)
+    switch (format) {
+        case LOCAL_GL_RGBA: {
+            switch (type) {
+                case LOCAL_GL_UNSIGNED_BYTE:
+                    break;
+                default:
+                    return ErrorInvalidOperation("readPixels: Invalid format/type pair");
+            }
+            break;
+        }
+        default:
+            return ErrorInvalidOperation("readPixels: Invalid format/type pair");
+    }
+
+    // there's nothing to do in this case, since we won't read any pixels
+    if (width == 0 || height == 0)
+        return NS_OK;
+
     MakeContextCurrent();
 
     if (mBoundFramebuffer) {
@@ -3407,7 +3444,6 @@ WebGLContext::ReadPixels_base(WebGLint x, WebGLint y, WebGLsizei width, WebGLsiz
         EnsureBackbufferClearedAsNeeded();
     }
 
-
     if (CanvasUtils::CheckSaneSubrectSize(x, y, width, height, boundWidth, boundHeight)) {
         // the easy case: we're not reading out-of-range pixels
         gl->fReadPixels(x, y, width, height, format, type, data);
@@ -3421,7 +3457,7 @@ WebGLContext::ReadPixels_base(WebGLint x, WebGLint y, WebGLsizei width, WebGLsiz
 
         // zero the whole destination buffer. Too bad for the part that's going to be overwritten, we're not
         // 100% efficient here, but in practice this is a quite rare case anyway.
-        memset(data, 0, byteLength);
+        memset(data, 0, dataByteLen);
 
         if (   x >= boundWidth
             || x+width <= 0
@@ -3449,7 +3485,7 @@ WebGLContext::ReadPixels_base(WebGLint x, WebGLint y, WebGLsizei width, WebGLsiz
 
         // now, same computation as above to find the size of the intermediate buffer to allocate for the subrect
         // no need to check again for integer overflow here, since we already know the sizes aren't greater than before
-        PRUint32 subrect_plainRowSize = subrect_width * size;
+        PRUint32 subrect_plainRowSize = subrect_width * bytesPerPixel;
 	// There are checks above to ensure that this doesn't overflow.
         PRUint32 subrect_alignedRowSize = 
             RoundedToNextMultipleOf(subrect_plainRowSize, mPixelStorePackAlignment).value();
@@ -3465,7 +3501,7 @@ WebGLContext::ReadPixels_base(WebGLint x, WebGLint y, WebGLsizei width, WebGLsiz
             GLint subrect_y_in_dest_buffer = subrect_y - y;
             memcpy(static_cast<GLubyte*>(data)
                      + checked_alignedRowSize.value() * (subrect_y_in_dest_buffer + y_inside_subrect)
-                     + size * subrect_x_in_dest_buffer, // destination
+                     + bytesPerPixel * subrect_x_in_dest_buffer, // destination
                    subrect_data + subrect_alignedRowSize * y_inside_subrect, // source
                    subrect_plainRowSize); // size
         }
@@ -3527,9 +3563,7 @@ WebGLContext::ReadPixels_array(WebGLint x, WebGLint y, WebGLsizei width, WebGLsi
     if (mContextLost)
         return NS_OK;
 
-    return ReadPixels_base(x, y, width, height, format, type,
-                           pixels ? JS_GetTypedArrayData(pixels) : 0,
-                           pixels ? JS_GetTypedArrayByteLength(pixels) : 0);
+    return ReadPixels_base(x, y, width, height, format, type, pixels);
 }
 
 NS_IMETHODIMP

content/canvas/test/webgl/failing_tests_linux.txt

@@ -4,7 +4,6 @@ conformance/glsl/misc/shader-with-256-character-identifier.frag.html
 conformance/glsl/misc/shader-with-long-line.html
 conformance/misc/uninitialized-test.html
 conformance/programs/gl-get-active-attribute.html
-conformance/reading/read-pixels-test.html
 conformance/renderbuffers/framebuffer-object-attachment.html
 conformance/textures/texture-mips.html
 conformance/uniforms/gl-uniform-bool.html

content/canvas/test/webgl/failing_tests_mac.txt

@@ -5,7 +5,6 @@ conformance/glsl/misc/glsl-long-variable-names.html
 conformance/glsl/misc/shader-with-256-character-identifier.frag.html
 conformance/glsl/misc/shader-with-long-line.html
 conformance/programs/program-test.html
-conformance/reading/read-pixels-test.html
 conformance/renderbuffers/framebuffer-object-attachment.html
 conformance/state/gl-object-get-calls.html
 conformance/textures/tex-input-validation.html

content/canvas/test/webgl/failing_tests_windows.txt

@@ -5,7 +5,6 @@ conformance/glsl/functions/glsl-function-mod-gentype.html
 conformance/glsl/misc/glsl-long-variable-names.html
 conformance/glsl/misc/shader-with-256-character-identifier.frag.html
 conformance/glsl/misc/shader-with-long-line.html
-conformance/reading/read-pixels-test.html
 conformance/renderbuffers/framebuffer-object-attachment.html
 conformance/more/conformance/quickCheckAPI-S_V.html
 conformance/more/functions/copyTexImage2D.html