Bug 1088345 - Improve glGetError handling. - r=kamidphish

dom/canvas/WebGLContextUtils.cpp

@@ -674,7 +674,7 @@ GLenum
 WebGLContext::GetAndFlushUnderlyingGLErrors()
 {
     // Get and clear GL error in ALL cases.
-    GLenum error = gl->GetAndClearError();
+    GLenum error = gl->fGetError();
 
     // Only store in mUnderlyingGLError if is hasn't already recorded an
     // error.

dom/canvas/WebGLContextValidate.cpp

@@ -1621,7 +1621,7 @@ WebGLContext::InitAndValidateGL()
             // and check OpenGL error for INVALID_ENUM.
 
             // before we start, we check that no error already occurred, to prevent hiding it in our subsequent error handling
-            error = gl->GetAndClearError();
+            error = gl->fGetError();
             if (error != LOCAL_GL_NO_ERROR) {
                 GenerateWarning("GL error 0x%x occurred during WebGL context initialization!", error);
                 return false;
@@ -1634,7 +1634,7 @@ WebGLContext::InitAndValidateGL()
             gl->fGetIntegerv(LOCAL_GL_MAX_VERTEX_OUTPUT_COMPONENTS, &maxVertexOutputComponents);
             gl->fGetIntegerv(LOCAL_GL_MAX_FRAGMENT_INPUT_COMPONENTS, &minFragmentInputComponents);
 
-            error = gl->GetAndClearError();
+            error = gl->fGetError();
             switch (error) {
                 case LOCAL_GL_NO_ERROR:
                     mGLMaxVaryingVectors = std::min(maxVertexOutputComponents, minFragmentInputComponents) / 4;
@@ -1694,9 +1694,10 @@ WebGLContext::InitAndValidateGL()
     // Mesa can only be detected with the GL_VERSION string, of the form "2.1 Mesa 7.11.0"
     mIsMesa = strstr((const char *)(gl->fGetString(LOCAL_GL_VERSION)), "Mesa");
 
-    // notice that the point of calling GetAndClearError here is not only to check for error,
+    // Notice that the point of calling fGetError here is not only to check for
-    // it is also to reset the error flags so that a subsequent WebGL getError call will give the correct result.
+    // errors, but also to reset the error flags so that a subsequent WebGL
-    error = gl->GetAndClearError();
+    // getError call will give the correct result.
+    error = gl->fGetError();
     if (error != LOCAL_GL_NO_ERROR) {
         GenerateWarning("GL error 0x%x occurred during WebGL context initialization!", error);
         return false;

gfx/gl/GLContext.cpp

@@ -292,9 +292,8 @@ GLContext::GLContext(const SurfaceCaps& caps,
     mVendor(GLVendor::Other),
     mRenderer(GLRenderer::Other),
     mHasRobustness(false),
-#ifdef MOZ_GL_DEBUG
+    mTopError(LOCAL_GL_NO_ERROR),
-    mIsInLocalErrorCheck(false),
+    mLocalErrorScope(nullptr),
-#endif
     mSharedContext(sharedContext),
     mCaps(caps),
     mScreen(nullptr),

gfx/gl/GLContext.h

@@ -580,110 +580,83 @@ public:
         }
     }
 
-    /** \returns the first GL error, and guarantees that all GL error flags are cleared,
-     * i.e. that a subsequent GetError call will return NO_ERROR
-     */
-    GLenum GetAndClearError() {
-        // the first error is what we want to return
-        GLenum error = fGetError();
-
-        if (error) {
-            // clear all pending errors
-            while(fGetError()) {}
-        }
-
-        return error;
-    }
-
 private:
-    GLenum raw_fGetError() {
+    GLenum mTopError;
+
+    GLenum RawGetError() {
         return mSymbols.fGetError();
     }
 
-    std::queue<GLenum> mGLErrorQueue;
+    GLenum RawGetErrorAndClear() {
+        GLenum err = RawGetError();
 
-public:
+        if (err)
-    GLenum fGetError() {
+            while (RawGetError()) {}
-        if (!mGLErrorQueue.empty()) {
-            GLenum err = mGLErrorQueue.front();
-            mGLErrorQueue.pop();
-            return err;
-        }
 
-        return GetUnpushedError();
-    }
-
-private:
-    GLenum GetUnpushedError() {
-        return raw_fGetError();
-    }
-
-    void ClearUnpushedErrors() {
-        while (GetUnpushedError()) {
-            // Discard errors.
-        }
-    }
-
-    GLenum GetAndClearUnpushedErrors() {
-        GLenum err = GetUnpushedError();
-        if (err) {
-            ClearUnpushedErrors();
-        }
         return err;
     }
 
-    void PushError(GLenum err) {
+public:
-        mGLErrorQueue.push(err);
+    GLenum FlushErrors() {
+        GLenum err = RawGetErrorAndClear();
+        if (!mTopError)
+            mTopError = err;
+        return err;
     }
 
-    void GetAndPushAllErrors() {
+    // We smash all errors together, so you never have to loop on this. We
-        while (true) {
+    // guarantee that immediately after this call, there are no errors left.
-            GLenum err = GetUnpushedError();
+    GLenum fGetError() {
-            if (!err)
+        FlushErrors();
-                break;
 
-            PushError(err);
+        GLenum err = mTopError;
-        }
+        mTopError = LOCAL_GL_NO_ERROR;
+        return err;
     }
 
     ////////////////////////////////////
     // Use this safer option.
+    class LocalErrorScope;
+
 private:
-#ifdef MOZ_GL_DEBUG
+    LocalErrorScope* mLocalErrorScope;
-    bool mIsInLocalErrorCheck;
-#endif
 
 public:
-    class ScopedLocalErrorCheck {
+    class LocalErrorScope {
-        GLContext* const mGL;
+        GLContext& mGL;
+        GLenum mOldTop;
         bool mHasBeenChecked;
 
     public:
-        explicit ScopedLocalErrorCheck(GLContext* gl)
+        explicit LocalErrorScope(GLContext& gl)
             : mGL(gl)
             , mHasBeenChecked(false)
         {
-#ifdef MOZ_GL_DEBUG
+            MOZ_ASSERT(!mGL.mLocalErrorScope);
-            MOZ_ASSERT(!mGL->mIsInLocalErrorCheck);
+            mGL.mLocalErrorScope = this;
-            mGL->mIsInLocalErrorCheck = true;
+
-#endif
+            mGL.FlushErrors();
-            mGL->GetAndPushAllErrors();
+
+            mOldTop = mGL.mTopError;
+            mGL.mTopError = LOCAL_GL_NO_ERROR;
         }
 
-        GLenum GetLocalError() {
+        GLenum GetError() {
-#ifdef MOZ_GL_DEBUG
-            MOZ_ASSERT(mGL->mIsInLocalErrorCheck);
-            mGL->mIsInLocalErrorCheck = false;
-#endif
-
             MOZ_ASSERT(!mHasBeenChecked);
             mHasBeenChecked = true;
 
-            return mGL->GetAndClearUnpushedErrors();
+            return mGL.fGetError();
         }
 
-        ~ScopedLocalErrorCheck() {
+        ~LocalErrorScope() {
             MOZ_ASSERT(mHasBeenChecked);
+
+            MOZ_ASSERT(mGL.fGetError() == LOCAL_GL_NO_ERROR);
+
+            mGL.mTopError = mOldTop;
+
+            MOZ_ASSERT(mGL.mLocalErrorScope == this);
+            mGL.mLocalErrorScope = nullptr;
         }
     };
 
@@ -722,43 +695,47 @@ private:
 # endif
 #endif
 
-    void BeforeGLCall(const char* glFunction) {
+    void BeforeGLCall(const char* funcName) {
         MOZ_ASSERT(IsCurrent());
-        if (DebugMode()) {
-            GLContext *currentGLContext = nullptr;
 
-            currentGLContext = (GLContext*)PR_GetThreadPrivate(sCurrentGLContextTLS);
+        if (DebugMode()) {
+            FlushErrors();
 
             if (DebugMode() & DebugTrace)
-                printf_stderr("[gl:%p] > %s\n", this, glFunction);
+                printf_stderr("[gl:%p] > %s\n", this, funcName);
-            if (this != currentGLContext) {
+
-                printf_stderr("Fatal: %s called on non-current context %p. "
+            GLContext* tlsContext = (GLContext*)PR_GetThreadPrivate(sCurrentGLContextTLS);
-                              "The current context for this thread is %p.\n",
+            if (this != tlsContext) {
-                              glFunction, this, currentGLContext);
+                printf_stderr("Fatal: %s called on non-current context %p. The"
-                NS_ABORT();
+                              " current context for this thread is %p.\n",
+                              funcName, this, tlsContext);
+                MOZ_CRASH("GLContext is not current.");
             }
         }
     }
 
-    void AfterGLCall(const char* glFunction) {
+    void AfterGLCall(const char* funcName) {
         if (DebugMode()) {
             // calling fFinish() immediately after every GL call makes sure that if this GL command crashes,
             // the stack trace will actually point to it. Otherwise, OpenGL being an asynchronous API, stack traces
             // tend to be meaningless
             mSymbols.fFinish();
-            GLenum err = GetUnpushedError();
+            GLenum err = FlushErrors();
-            PushError(err);
 
-            if (DebugMode() & DebugTrace)
+            if (DebugMode() & DebugTrace) {
-                printf_stderr("[gl:%p] < %s [0x%04x]\n", this, glFunction, err);
+                printf_stderr("[gl:%p] < %s [%s (0x%04x)]\n", this, funcName,
+                              GLErrorToString(err), err);
+            }
+
+            if (err != LOCAL_GL_NO_ERROR &&
+                !mLocalErrorScope)
+            {
+                printf_stderr("[gl:%p] %s: Generated unexpected %s error."
+                              " (0x%04x)\n", this, funcName,
+                              GLErrorToString(err), err);
 
-            if (err != LOCAL_GL_NO_ERROR) {
-                printf_stderr("GL ERROR: %s generated GL error %s(0x%04x)\n",
-                              glFunction,
-                              GLErrorToString(err),
-                              err);
                 if (DebugMode() & DebugAbortOnError)
-                    NS_ABORT();
+                    MOZ_CRASH("MOZ_GL_DEBUG_ABORT_ON_ERROR");
             }
         }
     }

gfx/gl/GLScreenBuffer.cpp

@@ -600,7 +600,7 @@ DrawBuffer::Create(GLContext* const gl,
             pStencilRB = nullptr;
     }
 
-    GLContext::ScopedLocalErrorCheck localError(gl);
+    GLContext::LocalErrorScope localError(*gl);
 
     CreateRenderbuffersForOffscreen(gl, formats, size, caps.antialias,
                                     pColorMSRB, pDepthRB, pStencilRB);
@@ -612,7 +612,8 @@ DrawBuffer::Create(GLContext* const gl,
     UniquePtr<DrawBuffer> ret( new DrawBuffer(gl, size, fb, colorMSRB,
                                               depthRB, stencilRB) );
 
-    GLenum err = localError.GetLocalError();
+    GLenum err = localError.GetError();
+    MOZ_ASSERT_IF(err != LOCAL_GL_NO_ERROR, err == LOCAL_GL_OUT_OF_MEMORY);
     if (err || !gl->IsFramebufferComplete(fb))
         return false;
 
@@ -659,7 +660,7 @@ ReadBuffer::Create(GLContext* gl,
     GLuint* pDepthRB   = caps.depth   ? &depthRB   : nullptr;
     GLuint* pStencilRB = caps.stencil ? &stencilRB : nullptr;
 
-    GLContext::ScopedLocalErrorCheck localError(gl);
+    GLContext::LocalErrorScope localError(*gl);
 
     CreateRenderbuffersForOffscreen(gl, formats, surf->mSize, caps.antialias,
                                     nullptr, pDepthRB, pStencilRB);
@@ -689,7 +690,8 @@ ReadBuffer::Create(GLContext* gl,
     UniquePtr<ReadBuffer> ret( new ReadBuffer(gl, fb, depthRB,
                                               stencilRB, surf) );
 
-    GLenum err = localError.GetLocalError();
+    GLenum err = localError.GetError();
+    MOZ_ASSERT_IF(err != LOCAL_GL_NO_ERROR, err == LOCAL_GL_OUT_OF_MEMORY);
     if (err || !gl->IsFramebufferComplete(fb)) {
         ret = nullptr;
     }

gfx/gl/SharedSurfaceGL.cpp

@@ -26,12 +26,14 @@ SharedSurface_Basic::Create(GLContext* gl,
     UniquePtr<SharedSurface_Basic> ret;
     gl->MakeCurrent();
 
-    GLContext::ScopedLocalErrorCheck localError(gl);
+    GLContext::LocalErrorScope localError(*gl);
     GLuint tex = CreateTexture(gl, formats.color_texInternalFormat,
                                formats.color_texFormat,
                                formats.color_texType,
                                size);
-    GLenum err = localError.GetLocalError();
+
+    GLenum err = localError.GetError();
+    MOZ_ASSERT_IF(err != LOCAL_GL_NO_ERROR, err == LOCAL_GL_OUT_OF_MEMORY);
     if (err) {
         gl->fDeleteTextures(1, &tex);
         return Move(ret);
@@ -121,17 +123,18 @@ SharedSurface_GLTexture::Create(GLContext* prodGL,
     UniquePtr<SharedSurface_GLTexture> ret;
 
     if (!tex) {
-      GLContext::ScopedLocalErrorCheck localError(prodGL);
+        GLContext::LocalErrorScope localError(*prodGL);
 
-      tex = CreateTextureForOffscreen(prodGL, formats, size);
+        tex = CreateTextureForOffscreen(prodGL, formats, size);
 
-      GLenum err = localError.GetLocalError();
+        GLenum err = localError.GetError();
-      if (err) {
+        MOZ_ASSERT_IF(err != LOCAL_GL_NO_ERROR, err == LOCAL_GL_OUT_OF_MEMORY);
-          prodGL->fDeleteTextures(1, &tex);
+        if (err) {
-          return Move(ret);
+            prodGL->fDeleteTextures(1, &tex);
-      }
+            return Move(ret);
+        }
 
-      ownsTex = true;
+        ownsTex = true;
     }
 
     ret.reset( new SharedSurface_GLTexture(prodGL, consGL, size,

gfx/layers/opengl/CompositorOGL.cpp

@@ -702,7 +702,7 @@ CompositorOGL::CreateFBOWithTexture(const IntRect& aRect, bool aCopyFromSource,
                               LOCAL_GL_UNSIGNED_BYTE,
                               buf);
     }
-    GLenum error = mGLContext->GetAndClearError();
+    GLenum error = mGLContext->fGetError();
     if (error != LOCAL_GL_NO_ERROR) {
       nsAutoCString msg;
       msg.AppendPrintf("Texture initialization failed! -- error 0x%x, Source %d, Source format %d,  RGBA Compat %d",

gfx/layers/opengl/TextureHostOGL.cpp

@@ -452,11 +452,11 @@ SurfaceTextureSource::BindTexture(GLenum aTextureUnit, gfx::Filter aFilter)
   }
 
   gl()->fActiveTexture(aTextureUnit);
-#ifndef DEBUG
+
   // SurfaceTexture spams us if there are any existing GL errors, so
   // we'll clear them here in order to avoid that.
-  gl()->GetAndClearError();
+  gl()->FlushErrors();
-#endif
+
   mSurfTex->UpdateTexImage();
 
   ApplyFilterToBoundTexture(gl(), aFilter, mTextureTarget);