Bug 1325516 - Simplify IMPL_COLOR_READ_FORMAT/TYPE and ensure that we only return valid ones. - r=daoshengmu draft
authorJeff Gilbert <jgilbert@mozilla.com>
Thu, 22 Dec 2016 17:08:35 -0800
changeset 453224 aec01ecea70d20a1c9d80778e66eeb5ac303f367
parent 453210 73fe2100ac25597b4427535a252c9a1b9a5e17a0
child 453225 35e1c0d007ae1eb57e812c34687326006d601326
push id39603
push userbmo:jgilbert@mozilla.com
push dateFri, 23 Dec 2016 01:54:15 +0000
reviewersdaoshengmu
bugs1325516
milestone53.0a1
Bug 1325516 - Simplify IMPL_COLOR_READ_FORMAT/TYPE and ensure that we only return valid ones. - r=daoshengmu MozReview-Commit-ID: KPicqcBy8l1
dom/canvas/WebGLContext.h
dom/canvas/WebGLContextGL.cpp
dom/canvas/WebGLContextState.cpp
dom/canvas/WebGLFormats.h
--- a/dom/canvas/WebGLContext.h
+++ b/dom/canvas/WebGLContext.h
@@ -654,16 +654,22 @@ public:
     bool IsVertexArray(const WebGLVertexArray* vao);
     void LineWidth(GLfloat width);
     void LinkProgram(WebGLProgram& prog);
     void PixelStorei(GLenum pname, GLint param);
     void PolygonOffset(GLfloat factor, GLfloat units);
 
     already_AddRefed<layers::SharedSurfaceTextureClient> GetVRFrame();
     bool StartVRPresentation();
+
+    ////
+
+    webgl::PackingInfo
+    ValidImplementationColorReadPI(const webgl::FormatUsageInfo* usage) const;
+
 protected:
     bool ReadPixels_SharedPrecheck(ErrorResult* const out_error);
     void ReadPixelsImpl(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format,
                         GLenum type, void* data, uint32_t dataLen);
     bool DoReadPixelsAndConvert(const webgl::FormatInfo* srcFormat, GLint x, GLint y,
                                 GLsizei width, GLsizei height, GLenum format,
                                 GLenum destType, void* dest, uint32_t dataLen,
                                 uint32_t rowStride);
@@ -682,16 +688,18 @@ public:
 
     void ReadPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format,
                     GLenum type, WebGLsizeiptr offset, ErrorResult& out_error);
 
     void ReadPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format,
                     GLenum type, const dom::ArrayBufferView& dstData, GLuint dstOffset,
                     ErrorResult& out_error);
 
+    ////
+
     void RenderbufferStorage(GLenum target, GLenum internalFormat,
                              GLsizei width, GLsizei height);
 protected:
     void RenderbufferStorage_base(const char* funcName, GLenum target,
                                   GLsizei samples, GLenum internalformat,
                                   GLsizei width, GLsizei height);
 public:
     void SampleCoverage(GLclampf value, WebGLboolean invert);
--- a/dom/canvas/WebGLContextGL.cpp
+++ b/dom/canvas/WebGLContextGL.cpp
@@ -1374,116 +1374,128 @@ WebGLContext::ReadPixels(GLint x, GLint 
     }
 
     gl->MakeCurrent();
     const ScopedLazyBind lazyBind(gl, LOCAL_GL_PIXEL_PACK_BUFFER, buffer);
 
     ReadPixelsImpl(x, y, width, height, format, type, (void*)offset, bytesAfterOffset);
 }
 
-static bool
-ValidateReadPixelsFormatAndType(const webgl::FormatInfo* srcFormat,
-                                const webgl::PackingInfo& pi, gl::GLContext* gl,
-                                WebGLContext* webgl)
+static webgl::PackingInfo
+DefaultReadPixelPI(const webgl::FormatUsageInfo* usage)
 {
-    // Check the format and type params to assure they are an acceptable pair (as per spec)
-    GLenum mainFormat;
-    GLenum mainType;
-
-    switch (srcFormat->componentType) {
-    case webgl::ComponentType::Float:
-        mainFormat = LOCAL_GL_RGBA;
-        mainType = LOCAL_GL_FLOAT;
-        break;
-
-    case webgl::ComponentType::UInt:
-        mainFormat = LOCAL_GL_RGBA_INTEGER;
-        mainType = LOCAL_GL_UNSIGNED_INT;
-        break;
+    MOZ_ASSERT(usage->IsRenderable());
+
+    switch (usage->format->componentType) {
+    case webgl::ComponentType::NormUInt:
+        return { LOCAL_GL_RGBA, LOCAL_GL_UNSIGNED_BYTE };
 
     case webgl::ComponentType::Int:
-        mainFormat = LOCAL_GL_RGBA_INTEGER;
-        mainType = LOCAL_GL_INT;
-        break;
-
-    case webgl::ComponentType::NormInt:
-    case webgl::ComponentType::NormUInt:
-        mainFormat = LOCAL_GL_RGBA;
-        mainType = LOCAL_GL_UNSIGNED_BYTE;
-        break;
+        return { LOCAL_GL_RGBA_INTEGER, LOCAL_GL_INT };
+
+    case webgl::ComponentType::UInt:
+        return { LOCAL_GL_RGBA_INTEGER, LOCAL_GL_UNSIGNED_INT };
+
+    case webgl::ComponentType::Float:
+        return { LOCAL_GL_RGBA, LOCAL_GL_FLOAT };
 
     default:
-        gfxCriticalNote << "Unhandled srcFormat->componentType: "
-                        << (uint32_t)srcFormat->componentType;
-        webgl->ErrorInvalidOperation("readPixels: Unhandled srcFormat->componentType."
-                                     " Please file a bug!");
-        return false;
+        MOZ_CRASH();
     }
-
-    if (pi.format == mainFormat && pi.type == mainType)
-        return true;
-
-    //////
-    // OpenGL ES 3.0.4 p194 - When the internal format of the rendering surface is
-    // RGB10_A2, a third combination of format RGBA and type UNSIGNED_INT_2_10_10_10_REV
-    // is accepted.
-
-    if (webgl->IsWebGL2() &&
-        srcFormat->effectiveFormat == webgl::EffectiveFormat::RGB10_A2 &&
-        pi.format == LOCAL_GL_RGBA &&
-        pi.type == LOCAL_GL_UNSIGNED_INT_2_10_10_10_REV)
-    {
-        return true;
-    }
-
-    //////
+}
+
+static bool
+ArePossiblePackEnums(const WebGLContext* webgl, const webgl::PackingInfo& pi)
+{
     // OpenGL ES 2.0 $4.3.1 - IMPLEMENTATION_COLOR_READ_{TYPE/FORMAT} is a valid
     // combination for glReadPixels()...
 
     // So yeah, we are actually checking that these are valid as /unpack/ formats, instead
     // of /pack/ formats here, but it should cover the INVALID_ENUM cases.
-    if (!webgl->mFormatUsage->AreUnpackEnumsValid(pi.format, pi.type)) {
-        webgl->ErrorInvalidEnum("readPixels: Bad format and/or type.");
+    if (!webgl->mFormatUsage->AreUnpackEnumsValid(pi.format, pi.type))
         return false;
-    }
 
     // Only valid when pulled from:
     // * GLES 2.0.25 p105:
     //   "table 3.4, excluding formats LUMINANCE and LUMINANCE_ALPHA."
     // * GLES 3.0.4 p193:
     //   "table 3.2, excluding formats DEPTH_COMPONENT and DEPTH_STENCIL."
     switch (pi.format) {
     case LOCAL_GL_LUMINANCE:
     case LOCAL_GL_LUMINANCE_ALPHA:
     case LOCAL_GL_DEPTH_COMPONENT:
     case LOCAL_GL_DEPTH_STENCIL:
-        webgl->ErrorInvalidEnum("readPixels: Invalid format: 0x%04x", pi.format);
         return false;
     }
 
-    if (pi.type == LOCAL_GL_UNSIGNED_INT_24_8) {
-        webgl->ErrorInvalidEnum("readPixels: Invalid type: 0x%04x", pi.type);
+    if (pi.type == LOCAL_GL_UNSIGNED_INT_24_8)
+        return false;
+
+    return true;
+}
+
+webgl::PackingInfo
+WebGLContext::ValidImplementationColorReadPI(const webgl::FormatUsageInfo* usage) const
+{
+    const auto defaultPI = DefaultReadPixelPI(usage);
+    if (!gl->IsSupported(gl::GLFeature::ES2_compatibility))
+        return defaultPI;
+
+    webgl::PackingInfo implPI;
+    gl->fGetIntegerv(LOCAL_GL_IMPLEMENTATION_COLOR_READ_FORMAT, (GLint*)&implPI.format);
+    gl->fGetIntegerv(LOCAL_GL_IMPLEMENTATION_COLOR_READ_FORMAT, (GLint*)&implPI.type);
+
+    if (!ArePossiblePackEnums(this, implPI))
+        return defaultPI;
+
+    return implPI;
+}
+
+static bool
+ValidateReadPixelsFormatAndType(const webgl::FormatUsageInfo* srcUsage,
+                                const webgl::PackingInfo& pi, gl::GLContext* gl,
+                                WebGLContext* webgl)
+{
+    const char funcName[] = "readPixels";
+
+    if (!ArePossiblePackEnums(webgl, pi)) {
+        webgl->ErrorInvalidEnum("%s: Unexpected format or type.", funcName);
         return false;
     }
 
+    ////
+
+    const auto defaultPI = DefaultReadPixelPI(srcUsage);
+    if (pi == defaultPI)
+        return true;
+
+    ////
+
+    // OpenGL ES 3.0.4 p194 - When the internal format of the rendering surface is
+    // RGB10_A2, a third combination of format RGBA and type UNSIGNED_INT_2_10_10_10_REV
+    // is accepted.
+
+    if (webgl->IsWebGL2() &&
+        srcUsage->format->effectiveFormat == webgl::EffectiveFormat::RGB10_A2 &&
+        pi.format == LOCAL_GL_RGBA &&
+        pi.type == LOCAL_GL_UNSIGNED_INT_2_10_10_10_REV)
+    {
+        return true;
+    }
+
+    ////
+
     MOZ_ASSERT(gl->IsCurrent());
-    if (gl->IsSupported(gl::GLFeature::ES2_compatibility)) {
-        const auto auxFormat = gl->GetIntAs<GLenum>(LOCAL_GL_IMPLEMENTATION_COLOR_READ_FORMAT);
-        const auto auxType = gl->GetIntAs<GLenum>(LOCAL_GL_IMPLEMENTATION_COLOR_READ_TYPE);
-
-        if (auxFormat && auxType &&
-            pi.format == auxFormat && pi.type == auxType)
-        {
-            return true;
-        }
-    }
-
-    //////
-
-    webgl->ErrorInvalidOperation("readPixels: Invalid format or type.");
+    const auto implPI = webgl->ValidImplementationColorReadPI(srcUsage);
+    if (pi == implPI)
+        return true;
+
+    ////
+
+    webgl->ErrorInvalidOperation("%s: Incompatible format or type.", funcName);
     return false;
 }
 
 void
 WebGLContext::ReadPixelsImpl(GLint x, GLint y, GLsizei rawWidth, GLsizei rawHeight,
                              GLenum packFormat, GLenum packType, void* dest,
                              uint32_t dataLen)
 {
@@ -1503,17 +1515,17 @@ WebGLContext::ReadPixelsImpl(GLint x, GL
     uint32_t srcWidth;
     uint32_t srcHeight;
     if (!ValidateCurFBForRead("readPixels", &srcFormat, &srcWidth, &srcHeight))
         return;
 
     //////
 
     const webgl::PackingInfo pi = {packFormat, packType};
-    if (!ValidateReadPixelsFormatAndType(srcFormat->format, pi, gl, this))
+    if (!ValidateReadPixelsFormatAndType(srcFormat, pi, gl, this))
         return;
 
     uint8_t bytesPerPixel;
     if (!webgl::GetBytesPerPixel(pi, &bytesPerPixel)) {
         ErrorInvalidOperation("readPixels: Unsupported format and type.");
         return;
     }
 
--- a/dom/canvas/WebGLContextState.cpp
+++ b/dom/canvas/WebGLContextState.cpp
@@ -385,52 +385,32 @@ WebGLContext::GetParameter(JSContext* cx
         case LOCAL_GL_BLEND_DST_ALPHA:
         case LOCAL_GL_BLEND_EQUATION_RGB:
         case LOCAL_GL_BLEND_EQUATION_ALPHA:
         case LOCAL_GL_GENERATE_MIPMAP_HINT: {
             GLint i = 0;
             gl->fGetIntegerv(pname, &i);
             return JS::NumberValue(uint32_t(i));
         }
+        case LOCAL_GL_IMPLEMENTATION_COLOR_READ_FORMAT:
         case LOCAL_GL_IMPLEMENTATION_COLOR_READ_TYPE: {
             const webgl::FormatUsageInfo* usage;
             uint32_t width, height;
             if (!ValidateCurFBForRead(funcName, &usage, &width, &height))
                 return JS::NullValue();
 
-            GLint i = 0;
-            if (gl->IsSupported(gl::GLFeature::ES2_compatibility)) {
-                gl->fGetIntegerv(pname, &i);
-            } else {
-                i = LOCAL_GL_UNSIGNED_BYTE;
-            }
-
-            return JS::NumberValue(uint32_t(i));
-        }
-        case LOCAL_GL_IMPLEMENTATION_COLOR_READ_FORMAT: {
-            const webgl::FormatUsageInfo* usage;
-            uint32_t width, height;
-            if (!ValidateCurFBForRead(funcName, &usage, &width, &height))
-                return JS::NullValue();
+            const auto implPI = ValidImplementationColorReadPI(usage);
 
-            GLint i = 0;
-            if (gl->IsSupported(gl::GLFeature::ES2_compatibility)) {
-                gl->fGetIntegerv(pname, &i);
+            GLenum ret;
+            if (pname == LOCAL_GL_IMPLEMENTATION_COLOR_READ_FORMAT) {
+                ret = implPI.format;
             } else {
-                i = LOCAL_GL_RGBA;
+                ret = implPI.type;
             }
-
-            // OpenGL ES 3.0.4 p112 Table 3.2 shows that read format SRGB_ALPHA is
-            // not supported. And if internal format of fbo is SRGB8_ALPHA8, then
-            // IMPLEMENTATION_COLOR_READ_FORMAT is SRGB_ALPHA which is not supported
-            // by ReadPixels. So, just return RGBA here.
-            if (i == LOCAL_GL_SRGB_ALPHA)
-                i = LOCAL_GL_RGBA;
-
-            return JS::NumberValue(uint32_t(i));
+            return JS::NumberValue(uint32_t(ret));
         }
         // int
         case LOCAL_GL_STENCIL_REF:
         case LOCAL_GL_STENCIL_BACK_REF: {
             GLint stencilBits = 0;
             if (!GetStencilBits(&stencilBits))
                 return JS::NullValue();
 
--- a/dom/canvas/WebGLFormats.h
+++ b/dom/canvas/WebGLFormats.h
@@ -264,16 +264,21 @@ struct PackingInfo
 
     bool operator <(const PackingInfo& x) const
     {
         if (format != x.format)
             return format < x.format;
 
         return type < x.type;
     }
+
+    bool operator ==(const PackingInfo& x) const {
+        return (format == x.format &&
+                type == x.type);
+    }
 };
 
 struct DriverUnpackInfo
 {
     GLenum internalFormat;
     GLenum unpackFormat;
     GLenum unpackType;