From d41ee052e4e81426ad1afa6a90123abdb436c505 Mon Sep 17 00:00:00 2001 From: Jeff Gilbert Date: Fri, 24 Oct 2014 16:52:35 -0700 Subject: [PATCH] Bug 1088345 - Improve glGetError handling. - r=kamidphish --- dom/canvas/WebGLContextUtils.cpp | 2 +- dom/canvas/WebGLContextValidate.cpp | 11 +- gfx/gl/GLContext.cpp | 5 +- gfx/gl/GLContext.h | 161 ++++++++++++--------------- gfx/gl/GLScreenBuffer.cpp | 10 +- gfx/gl/SharedSurfaceGL.cpp | 23 ++-- gfx/layers/opengl/CompositorOGL.cpp | 2 +- gfx/layers/opengl/TextureHostOGL.cpp | 6 +- 8 files changed, 101 insertions(+), 119 deletions(-) diff --git a/dom/canvas/WebGLContextUtils.cpp b/dom/canvas/WebGLContextUtils.cpp index e32cfb2b03a9..b93c733ca9bf 100644 --- a/dom/canvas/WebGLContextUtils.cpp +++ b/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. diff --git a/dom/canvas/WebGLContextValidate.cpp b/dom/canvas/WebGLContextValidate.cpp index 5a161df5df96..8f562d509e2a 100644 --- a/dom/canvas/WebGLContextValidate.cpp +++ b/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, - // it is also to reset the error flags so that a subsequent WebGL getError call will give the correct result. - error = gl->GetAndClearError(); + // Notice that the point of calling fGetError here is not only to check for + // errors, but also to reset the error flags so that a subsequent WebGL + // 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; diff --git a/gfx/gl/GLContext.cpp b/gfx/gl/GLContext.cpp index afc788f6a0a6..f2089d3d9553 100644 --- a/gfx/gl/GLContext.cpp +++ b/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 - mIsInLocalErrorCheck(false), -#endif + mTopError(LOCAL_GL_NO_ERROR), + mLocalErrorScope(nullptr), mSharedContext(sharedContext), mCaps(caps), mScreen(nullptr), diff --git a/gfx/gl/GLContext.h b/gfx/gl/GLContext.h index 16827ee141e2..459255bdf114 100644 --- a/gfx/gl/GLContext.h +++ b/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 mGLErrorQueue; + GLenum RawGetErrorAndClear() { + GLenum err = RawGetError(); -public: - GLenum fGetError() { - if (!mGLErrorQueue.empty()) { - GLenum err = mGLErrorQueue.front(); - mGLErrorQueue.pop(); - return err; - } + if (err) + while (RawGetError()) {} - 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) { - mGLErrorQueue.push(err); +public: + GLenum FlushErrors() { + GLenum err = RawGetErrorAndClear(); + if (!mTopError) + mTopError = err; + return err; } - void GetAndPushAllErrors() { - while (true) { - GLenum err = GetUnpushedError(); - if (!err) - break; + // We smash all errors together, so you never have to loop on this. We + // guarantee that immediately after this call, there are no errors left. + GLenum fGetError() { + FlushErrors(); - PushError(err); - } + GLenum err = mTopError; + mTopError = LOCAL_GL_NO_ERROR; + return err; } //////////////////////////////////// // Use this safer option. + class LocalErrorScope; + private: -#ifdef MOZ_GL_DEBUG - bool mIsInLocalErrorCheck; -#endif + LocalErrorScope* mLocalErrorScope; public: - class ScopedLocalErrorCheck { - GLContext* const mGL; + class LocalErrorScope { + 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->mIsInLocalErrorCheck); - mGL->mIsInLocalErrorCheck = true; -#endif - mGL->GetAndPushAllErrors(); + MOZ_ASSERT(!mGL.mLocalErrorScope); + mGL.mLocalErrorScope = this; + + mGL.FlushErrors(); + + mOldTop = mGL.mTopError; + mGL.mTopError = LOCAL_GL_NO_ERROR; } - GLenum GetLocalError() { -#ifdef MOZ_GL_DEBUG - MOZ_ASSERT(mGL->mIsInLocalErrorCheck); - mGL->mIsInLocalErrorCheck = false; -#endif - + GLenum GetError() { 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); - if (this != currentGLContext) { - printf_stderr("Fatal: %s called on non-current context %p. " - "The current context for this thread is %p.\n", - glFunction, this, currentGLContext); - NS_ABORT(); + printf_stderr("[gl:%p] > %s\n", this, funcName); + + GLContext* tlsContext = (GLContext*)PR_GetThreadPrivate(sCurrentGLContextTLS); + if (this != tlsContext) { + printf_stderr("Fatal: %s called on non-current context %p. The" + " 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(); - PushError(err); + GLenum err = FlushErrors(); - if (DebugMode() & DebugTrace) - printf_stderr("[gl:%p] < %s [0x%04x]\n", this, glFunction, err); + if (DebugMode() & DebugTrace) { + 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"); } } } diff --git a/gfx/gl/GLScreenBuffer.cpp b/gfx/gl/GLScreenBuffer.cpp index 404205b5e5e7..cf34ebb7fc30 100755 --- a/gfx/gl/GLScreenBuffer.cpp +++ b/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 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 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; } diff --git a/gfx/gl/SharedSurfaceGL.cpp b/gfx/gl/SharedSurfaceGL.cpp index 84dcd0fa91a3..ab170b250935 100644 --- a/gfx/gl/SharedSurfaceGL.cpp +++ b/gfx/gl/SharedSurfaceGL.cpp @@ -26,12 +26,14 @@ SharedSurface_Basic::Create(GLContext* gl, UniquePtr 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 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(); - if (err) { - prodGL->fDeleteTextures(1, &tex); - return Move(ret); - } + GLenum err = localError.GetError(); + MOZ_ASSERT_IF(err != LOCAL_GL_NO_ERROR, err == LOCAL_GL_OUT_OF_MEMORY); + if (err) { + prodGL->fDeleteTextures(1, &tex); + return Move(ret); + } - ownsTex = true; + ownsTex = true; } ret.reset( new SharedSurface_GLTexture(prodGL, consGL, size, diff --git a/gfx/layers/opengl/CompositorOGL.cpp b/gfx/layers/opengl/CompositorOGL.cpp index 147e054163e1..6e71a698ec51 100644 --- a/gfx/layers/opengl/CompositorOGL.cpp +++ b/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", diff --git a/gfx/layers/opengl/TextureHostOGL.cpp b/gfx/layers/opengl/TextureHostOGL.cpp index 0dd4cb935d47..5540d5c8d804 100644 --- a/gfx/layers/opengl/TextureHostOGL.cpp +++ b/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(); -#endif + gl()->FlushErrors(); + mSurfTex->UpdateTexImage(); ApplyFilterToBoundTexture(gl(), aFilter, mTextureTarget);