From 5b4030b08dd9dd7840b6abdc703ddaa3ae61b4f8 Mon Sep 17 00:00:00 2001 From: Jeff Gilbert Date: Mon, 31 Oct 2011 16:55:01 -0700 Subject: [PATCH] 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 | 2 +- content/canvas/src/WebGLContextGL.cpp | 102 ++++++++++++------ .../canvas/test/webgl/failing_tests_linux.txt | 1 - .../canvas/test/webgl/failing_tests_mac.txt | 1 - .../test/webgl/failing_tests_windows.txt | 1 - 5 files changed, 69 insertions(+), 38 deletions(-) diff --git a/content/canvas/src/WebGLContext.h b/content/canvas/src/WebGLContext.h index 3bbaab6cd58..6ae016cd1f0 100644 --- a/content/canvas/src/WebGLContext.h +++ b/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); diff --git a/content/canvas/src/WebGLContextGL.cpp b/content/canvas/src/WebGLContextGL.cpp index b24ce744a31..fcfcd6d3779 100644 --- a/content/canvas/src/WebGLContextGL.cpp +++ b/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,53 +3350,90 @@ 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 = + CheckedUint32 checked_alignedRowSize = RoundedToNextMultipleOf(checked_plainRowSize, mPixelStorePackAlignment); 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(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 diff --git a/content/canvas/test/webgl/failing_tests_linux.txt b/content/canvas/test/webgl/failing_tests_linux.txt index 1fe75f365ea..5154d13a6d3 100644 --- a/content/canvas/test/webgl/failing_tests_linux.txt +++ b/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 diff --git a/content/canvas/test/webgl/failing_tests_mac.txt b/content/canvas/test/webgl/failing_tests_mac.txt index e96afc11edd..29aa3ade47b 100644 --- a/content/canvas/test/webgl/failing_tests_mac.txt +++ b/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 diff --git a/content/canvas/test/webgl/failing_tests_windows.txt b/content/canvas/test/webgl/failing_tests_windows.txt index 85aceeea03f..09be7cb4216 100644 --- a/content/canvas/test/webgl/failing_tests_windows.txt +++ b/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