Bug 1325516 (flattened) - Misc fixes and simplify IMPL_COLOR_READ_FORMAT/TYPE and ensure that we only return valid ones. - r=daoshengmu a=lizzard
authorJeff Gilbert <jgilbert@mozilla.com>
Thu, 22 Dec 2016 17:08:35 -0800
changeset 353257 2d2153ad43c9ee058b21eeec125720fd30b54ffe
parent 353256 dd2a8702cb8f0aab3e51e24150518dff610e4f2c
child 353258 f1877cf8ba418215cc955b85b56ac674a54230b7
push id6795
push userjlund@mozilla.com
push dateMon, 23 Jan 2017 14:19:46 +0000
treeherdermozilla-esr52@76101b503191 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdaoshengmu, lizzard
bugs1325516
milestone52.0a2
Bug 1325516 (flattened) - Misc fixes and simplify IMPL_COLOR_READ_FORMAT/TYPE and ensure that we only return valid ones. - r=daoshengmu a=lizzard
dom/canvas/WebGL2ContextFramebuffers.cpp
dom/canvas/WebGLContext.h
dom/canvas/WebGLContextBuffers.cpp
dom/canvas/WebGLContextGL.cpp
dom/canvas/WebGLContextState.cpp
dom/canvas/WebGLFormats.h
dom/canvas/WebGLFramebuffer.cpp
dom/canvas/WebGLRenderbuffer.cpp
--- a/dom/canvas/WebGL2ContextFramebuffers.cpp
+++ b/dom/canvas/WebGL2ContextFramebuffers.cpp
@@ -127,33 +127,37 @@ ValidateBackbufferAttachmentEnum(WebGLCo
         return false;
     }
 }
 
 static bool
 ValidateFramebufferAttachmentEnum(WebGLContext* webgl, const char* funcName,
                                   GLenum attachment)
 {
-    if (attachment >= LOCAL_GL_COLOR_ATTACHMENT0 &&
-        attachment <= webgl->LastColorAttachmentEnum())
-    {
-        return true;
-    }
-
     switch (attachment) {
     case LOCAL_GL_DEPTH_ATTACHMENT:
     case LOCAL_GL_STENCIL_ATTACHMENT:
     case LOCAL_GL_DEPTH_STENCIL_ATTACHMENT:
         return true;
+    }
 
-    default:
+    if (attachment < LOCAL_GL_COLOR_ATTACHMENT0) {
         webgl->ErrorInvalidEnum("%s: attachment: invalid enum value 0x%x.",
                                 funcName, attachment);
         return false;
     }
+
+    if (attachment > webgl->LastColorAttachmentEnum()) {
+        // That these errors have different types is ridiculous.
+        webgl->ErrorInvalidOperation("%s: Too-large LOCAL_GL_COLOR_ATTACHMENTn.",
+                                     funcName);
+        return false;
+    }
+
+    return true;
 }
 
 bool
 WebGLContext::ValidateInvalidateFramebuffer(const char* funcName, GLenum target,
                                             const dom::Sequence<GLenum>& attachments,
                                             ErrorResult* const out_rv,
                                             std::vector<GLenum>* const scopedVector,
                                             GLsizei* const out_glNumAttachments,
--- a/dom/canvas/WebGLContext.h
+++ b/dom/canvas/WebGLContext.h
@@ -653,16 +653,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);
@@ -681,16 +687,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/WebGLContextBuffers.cpp
+++ b/dom/canvas/WebGLContextBuffers.cpp
@@ -98,17 +98,17 @@ WebGLContext::ValidateIndexedBufferSlot(
         break;
 
     default:
         ErrorInvalidEnum("%s: Bad `target`: 0x%04x", funcName, target);
         return nullptr;
     }
 
     if (index >= bindings->size()) {
-        ErrorInvalidOperation("%s: `index` >= %s.", funcName, maxIndexEnum);
+        ErrorInvalidValue("%s: `index` >= %s.", funcName, maxIndexEnum);
         return nullptr;
     }
 
     return &(*bindings)[index];
 }
 
 ////////////////////////////////////////
 
@@ -243,16 +243,21 @@ WebGLContext::BindBufferRange(GLenum tar
                                       &indexedBinding))
     {
         return;
     }
 
     if (buffer && !buffer->ValidateCanBindToTarget(funcName, target))
         return;
 
+    if (buffer && !size) {
+        ErrorInvalidValue("%s: size must be non-zero for non-null buffer.", funcName);
+        return;
+    }
+
     ////
 
     gl->MakeCurrent();
 
     switch (target) {
     case LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER:
         if (offset % 4 != 0 || size % 4 != 0) {
             ErrorInvalidValue("%s: For %s, `offset` and `size` must be multiples of 4.",
--- a/dom/canvas/WebGLContextGL.cpp
+++ b/dom/canvas/WebGLContextGL.cpp
@@ -1373,116 +1373,129 @@ 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);
+
+    // ES2_compatibility always returns RGBA/UNSIGNED_BYTE, so branch on actual IsGLES().
+    // Also OSX+NV generates an error here.
+    if (!gl->IsGLES())
+        return defaultPI;
+
+    webgl::PackingInfo implPI;
+    gl->fGetIntegerv(LOCAL_GL_IMPLEMENTATION_COLOR_READ_FORMAT, (GLint*)&implPI.format);
+    gl->fGetIntegerv(LOCAL_GL_IMPLEMENTATION_COLOR_READ_TYPE, (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->IsGLES()) {
-        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)
 {
@@ -1502,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
@@ -388,58 +388,34 @@ WebGLContext::GetParameter(JSContext* cx
             GLint i = 0;
             gl->fGetIntegerv(pname, &i);
             return JS::NumberValue(uint32_t(i));
         }
 
         case LOCAL_GL_GENERATE_MIPMAP_HINT:
             return JS::NumberValue(mGenerateMipmapHint);
 
+        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->IsGLES()) {
-                // ES2_compatibility always returns UNSIGNED_BYTE here, so
-                // branch on actual IsGLES().
-                // Also OSX+NV generates an error here.
-                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->IsGLES()) {
-                // ES2_compatibility always returns UNSIGNED_BYTE here, so
-                // branch on actual IsGLES().
-                // Also OSX+NV generates an error here.
-                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;
             }
+            return JS::NumberValue(uint32_t(ret));
+        }
 
-            // 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));
-        }
         // int
         case LOCAL_GL_STENCIL_REF:
         case LOCAL_GL_STENCIL_BACK_REF: {
             GLint stencilBits = 0;
             if (!GetStencilBits(&stencilBits))
                 return JS::NullValue();
 
             // Assuming stencils have 8 bits
--- a/dom/canvas/WebGLFormats.h
+++ b/dom/canvas/WebGLFormats.h
@@ -232,16 +232,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;
 
--- a/dom/canvas/WebGLFramebuffer.cpp
+++ b/dom/canvas/WebGLFramebuffer.cpp
@@ -884,16 +884,22 @@ WebGLFramebuffer::ValidateForRead(const 
     }
 
     if (!mColorReadBuffer->IsDefined()) {
         mContext->ErrorInvalidOperation("%s: The READ_BUFFER attachment is not defined.",
                                         funcName);
         return false;
     }
 
+    if (mColorReadBuffer->Samples()) {
+        mContext->ErrorInvalidOperation("%s: The READ_BUFFER attachment is multisampled.",
+                                        funcName);
+        return false;
+    }
+
     *out_format = mColorReadBuffer->Format();
     mColorReadBuffer->Size(out_width, out_height);
     return true;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Resolution and caching
 
@@ -1236,16 +1242,25 @@ WebGLFramebuffer::DrawBuffers(const char
         //  equal to that of the MAX_DRAW_BUFFERS_WEBGL parameter."
         // This means that if buffers.Length() isn't larger than MaxDrawBuffers, it won't
         // be larger than MaxColorAttachments.
         const auto& cur = buffers[i];
         if (cur == LOCAL_GL_COLOR_ATTACHMENT0 + i) {
             const auto& attach = mColorAttachments[i];
             newColorDrawBuffers.push_back(&attach);
         } else if (cur != LOCAL_GL_NONE) {
+            const bool isColorEnum = (cur >= LOCAL_GL_COLOR_ATTACHMENT0 &&
+                                      cur < mContext->LastColorAttachmentEnum());
+            if (cur != LOCAL_GL_BACK &&
+                !isColorEnum)
+            {
+                mContext->ErrorInvalidEnum("%s: Unexpected enum in buffers.", funcName);
+                return;
+            }
+
             mContext->ErrorInvalidOperation("%s: `buffers[i]` must be NONE or"
                                             " COLOR_ATTACHMENTi.",
                                             funcName);
             return;
         }
     }
 
     ////
--- a/dom/canvas/WebGLRenderbuffer.cpp
+++ b/dom/canvas/WebGLRenderbuffer.cpp
@@ -200,17 +200,17 @@ WebGLRenderbuffer::RenderbufferStorage(c
     mContext->MakeContextCurrent();
 
     if (!usage->maxSamplesKnown) {
         const_cast<webgl::FormatUsageInfo*>(usage)->ResolveMaxSamples(mContext->gl);
     }
     MOZ_ASSERT(usage->maxSamplesKnown);
 
     if (samples > usage->maxSamples) {
-        mContext->ErrorInvalidValue("%s: `samples` is out of the valid range.", funcName);
+        mContext->ErrorInvalidOperation("%s: `samples` is out of the valid range.", funcName);
         return;
     }
 
     // Validation complete.
 
     const GLenum error = DoRenderbufferStorage(samples, usage, width, height);
     if (error) {
         const char* errorName = mContext->ErrorName(error);