Back out patches from bug 749678, except for the include guards - no review, fix regression (Bug 753350)
authorBenoit Jacob <bjacob@mozilla.com>
Sat, 12 May 2012 19:23:56 -0400
changeset 93826 3e0f7b9a39d7300abf1e3bb048e8c17b9db0c0b4
parent 93825 f77082549e0e87febe9e6b7116c4c514272de4a3
child 93827 366ab61b0af74eba87742c3495c32bda388f5cb6
push id9348
push userbjacob@mozilla.com
push dateSat, 12 May 2012 23:25:02 +0000
treeherdermozilla-inbound@3e0f7b9a39d7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs749678, 753350
milestone15.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Back out patches from bug 749678, except for the include guards - no review, fix regression (Bug 753350) Unfortunately, in-process plugins using OpenGL break the assumption made by these patches, that the current GL context is only changed by GLContext::MakeCurrent. Another issue, regardless of in-process, is that our host-side code in nsCoreAnimationSupport.mm uses direct CGL calls, bypassing GLContext.
gfx/gl/GLContext.cpp
gfx/gl/GLContext.h
gfx/gl/GLContextProviderCGL.mm
gfx/gl/GLContextProviderEGL.cpp
gfx/gl/GLContextProviderGLX.cpp
gfx/gl/GLContextProviderOSMesa.cpp
gfx/gl/GLContextProviderWGL.cpp
gfx/thebes/gfxPlatform.cpp
--- a/gfx/gl/GLContext.cpp
+++ b/gfx/gl/GLContext.cpp
@@ -59,18 +59,19 @@
 #include "mozilla/Preferences.h"
 #include "mozilla/Util.h" // for DebugOnly
 
 using namespace mozilla::gfx;
 
 namespace mozilla {
 namespace gl {
 
-tls::key GLContextTLSStorage::sTLSKey;
-bool GLContextTLSStorage::sTLSKeyAlreadyCreated = false;
+#ifdef DEBUG
+PRUintn GLContext::sCurrentGLContextTLS = -1;
+#endif
 
 PRUint32 GLContext::sDebugMode = 0;
 
 // define this here since it's global to GLContextProvider, not any
 // specific implementation
 const ContextFormat ContextFormat::BasicRGBA32Format(ContextFormat::BasicRGBA32);
 
 #define MAX_SYMBOL_LENGTH 128
--- a/gfx/gl/GLContext.h
+++ b/gfx/gl/GLContext.h
@@ -62,17 +62,16 @@
 #include "nsISupportsImpl.h"
 #include "prlink.h"
 
 #include "nsDataHashtable.h"
 #include "nsHashKeys.h"
 #include "nsRegion.h"
 #include "nsAutoPtr.h"
 #include "nsThreadUtils.h"
-#include "thread_helper.h"
 
 typedef char realGLboolean;
 
 #include "GLContextSymbols.h"
 
 #include "mozilla/mozalloc.h"
 
 namespace mozilla {
@@ -520,87 +519,16 @@ struct THEBES_API ContextFormat
     int green, minGreen;
     int blue, minBlue;
     int alpha, minAlpha;
     int samples;
 
     int colorBits() const { return red + green + blue; }
 };
 
-/*
- * This is a helper class to do the little bit of TLS storage that we need
- * to allow GLContext to keep track of the current GLContext for a given thread.
- *
- * This is mostly an optimization to avoid calling MakeCurrent on an
- * already-current context,which depending on OpenGL libraries/drivers can be
- * very expensive. An earlier optimization consisted in calling
- * getCurrentContext to check if the context was already current, but
- * even that was shown to be very slow at least on Mac and on Linux NVIDIA,
- * see bug 749678.
- *
- * In a general setting, we would have to do a TLS lookup on every MakeCurrent
- * call. But in GLContext, we currently assume that we only ever make GL calls
- * on a given GLContext in the same thread that created it (the "owning thread").
- * That assumption allows us to avoid doing a TLS lookup on every MakeCurrent
- * call. It's checked by assertions in MOZ_GL_DEBUG mode.
- *
- * The way this works is that inside each GLContext, we store a pointer to the
- * TLS pointer to the current context for this thread. This pointer-to-pointer
- * (mStorage->mCurrentGLContext) is set during GL context creation: that's where
- * we rely on the assumption that all GL calls on a given context are made on
- * the same thread that created that context.
- */
-class GLContextTLSStorage
-{
-    struct Storage
-    {
-        GLContext *mCurrentGLContext;
-
-        NS_INLINE_DECL_REFCOUNTING(Storage)
-
-        Storage() : mCurrentGLContext(nsnull) {}
-
-        ~Storage() {
-            // avoid keeping a dangling TLS pointer to the Storage object being destroyed
-            tls::set<Storage>(sTLSKey, nsnull);
-        }
-    };
-
-    nsRefPtr<Storage> mStorage;
-    static tls::key sTLSKey;
-    static bool sTLSKeyAlreadyCreated;
-
-public:
-
-    GLContextTLSStorage() {
-        if (!sTLSKeyAlreadyCreated) {
-            tls::create(&sTLSKey);
-            sTLSKeyAlreadyCreated = true;
-        }
-
-        mStorage = tls::get<Storage>(sTLSKey);
-
-        if (!mStorage) {
-            mStorage = new Storage;
-            tls::set<Storage>(sTLSKey, mStorage);
-        }
-    }
-
-    ~GLContextTLSStorage() {
-    }
-
-    GLContext *CurrentGLContext() const {
-        return mStorage->mCurrentGLContext;
-    }
-
-    void SetCurrentGLContext(GLContext *c) {
-        mStorage->mCurrentGLContext = c;
-    }
-};
-
 class GLContext
     : public GLLibraryLoader
 {
     NS_INLINE_DECL_THREADSAFE_REFCOUNTING(GLContext)
 public:
     GLContext(const ContextFormat& aFormat,
               bool aIsOffscreen = false,
               GLContext *aSharedContext = nsnull)
@@ -642,28 +570,26 @@ public:
         , mGLError(LOCAL_GL_NO_ERROR)
 #endif
     {
         mUserData.Init();
         mOwningThread = NS_GetCurrentThread();
     }
 
     virtual ~GLContext() {
-        NS_ABORT_IF_FALSE(IsDestroyed(), "GLContext implementation must call MarkDestroyed in destructor!");
+        NS_ASSERTION(IsDestroyed(), "GLContext implementation must call MarkDestroyed in destructor!");
 #ifdef DEBUG
         if (mSharedContext) {
             GLContext *tip = mSharedContext;
             while (tip->mSharedContext)
                 tip = tip->mSharedContext;
             tip->SharedContextDestroyed(this);
             tip->ReportOutstandingNames();
         }
 #endif
-        if (this == CurrentGLContext())
-            SetCurrentGLContext(nsnull);
     }
 
     enum ContextFlags {
         ContextFlagsNone = 0x0,
         ContextFlagsGlobal = 0x1
     };
 
     enum GLContextType {
@@ -672,53 +598,29 @@ public:
         ContextTypeCGL,
         ContextTypeGLX,
         ContextTypeEGL,
         ContextTypeOSMesa
     };
 
     virtual GLContextType GetContextType() { return ContextTypeUnknown; }
 
-    virtual bool MakeCurrentImpl() = 0;
-
-    void CheckOwningThreadInDebugMode() {
+    virtual bool MakeCurrentImpl(bool aForce = false) = 0;
+
 #ifdef DEBUG
-        if (!NS_GetCurrentThread()) {
-            // happens during shutdown. Drop this check in that case.
-            return;
-        }
-        if (!IsOwningThreadCurrent())
-        {
-            printf_stderr(
-                "This GL context (%p) is owned by thread %p, but the current thread is %p. "
-                "That's fine by itself, but our current code in GLContext::MakeCurrent, checking "
-                "if the context is already current, relies on the assumption that GL calls on a given "
-                "GLContext are only made by the thread that created that GLContext. If you want to "
-                "start making GL calls from non-owning threads, you'll have to change a few things "
-                "around here, see Bug 749678 comments 13 and 15.\n",
-                this, mOwningThread.get(), NS_GetCurrentThread());
-            NS_ABORT();
-        }
+    static void StaticInit() {
+        PR_NewThreadPrivateIndex(&sCurrentGLContextTLS, NULL);
+    }
 #endif
-    }
 
     bool MakeCurrent(bool aForce = false) {
-        CheckOwningThreadInDebugMode();
-
-        if (!aForce &&
-            this == CurrentGLContext())
-        {
-            return true;
-        }
-
-        bool success = MakeCurrentImpl();
-        if (success) {
-            SetCurrentGLContext(this);
-        }
-        return success;
+#ifdef DEBUG
+        PR_SetThreadPrivate(sCurrentGLContextTLS, this);
+#endif
+        return MakeCurrentImpl(aForce);
     }
 
     bool IsContextLost() { return mContextLost; }
 
     virtual bool SetupLookupFunction() = 0;
 
     virtual void WindowDestroyed() {}
 
@@ -1195,26 +1097,16 @@ public:
 
     GLuint SwapUserReadFBO(GLuint name) {
         GLuint prev = GetUserBoundReadFBO();
         BindUserReadFBO(name);
         return prev;
     }
 
 private:
-
-    GLContext *CurrentGLContext() const {
-        return mTLSStorage.CurrentGLContext();
-    }
-
-    void SetCurrentGLContext(GLContext *c) {
-        mTLSStorage.SetCurrentGLContext(c);
-    }
-
-
     bool mOffscreenFBOsDirty;
 
     void GetShaderPrecisionFormatNonES2(GLenum shadertype, GLenum precisiontype, GLint* range, GLint* precision) {
         switch (precisiontype) {
             case LOCAL_GL_LOW_FLOAT:
             case LOCAL_GL_MEDIUM_FLOAT:
             case LOCAL_GL_HIGH_FLOAT:
                 // Assume IEEE 754 precision
@@ -1732,26 +1624,33 @@ protected:
     ContextFormat mCreationFormat;
     nsRefPtr<GLContext> mSharedContext;
 
     // The thread on which this context was created.
     nsCOMPtr<nsIThread> mOwningThread;
 
     GLContextSymbols mSymbols;
 
+#ifdef DEBUG
+    // GLDebugMode will check that we don't send call
+    // to a GLContext that isn't current on the current
+    // thread.
+    // Store the current context when binding to thread local
+    // storage to support DebugMode on an arbitrary thread.
+    static PRUintn sCurrentGLContextTLS;
+#endif
+
     void UpdateActualFormat();
     ContextFormat mActualFormat;
 
     gfxIntSize mOffscreenSize;
     gfxIntSize mOffscreenActualSize;
     GLuint mOffscreenTexture;
     bool mFlipped;
 
-    GLContextTLSStorage mTLSStorage;
-
     // lazy-initialized things
     GLuint mBlitProgram, mBlitFramebuffer;
     void UseBlitProgram();
     void SetBlitFramebufferForDestTexture(GLuint aTexture);
 
     // Helper to create/resize an offscreen FBO,
     // for offscreen implementations that use FBOs.
     // Note that it does -not- clear the resized buffers.
@@ -1919,23 +1818,26 @@ public:
 
 protected:
     GLenum mGLError;
 
 public:
 
     void BeforeGLCall(const char* glFunction) {
         if (DebugMode()) {
+            GLContext *currentGLContext = NULL;
+
+            currentGLContext = (GLContext*)PR_GetThreadPrivate(sCurrentGLContextTLS);
+
             if (DebugMode() & DebugTrace)
                 printf_stderr("[gl:%p] > %s\n", this, glFunction);
-            CheckOwningThreadInDebugMode();
-            if (this != CurrentGLContext()) {
+            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());
+                               glFunction, this, currentGLContext);
                 NS_ABORT();
             }
         }
     }
 
     void AfterGLCall(const char* glFunction) {
         if (DebugMode()) {
             // calling fFinish() immediately after every GL call makes sure that if this GL command crashes,
--- a/gfx/gl/GLContextProviderCGL.mm
+++ b/gfx/gl/GLContextProviderCGL.mm
@@ -166,18 +166,22 @@ public:
         case NativeGLContext:
             return mContext;
 
         default:
             return nsnull;
         }
     }
 
-    bool MakeCurrentImpl()
+    bool MakeCurrentImpl(bool aForce = false)
     {
+        if (!aForce && [NSOpenGLContext currentContext] == mContext) {
+            return true;
+        }
+
         if (mContext) {
             [mContext makeCurrentContext];
         }
         return true;
     }
 
     bool SetupLookupFunction()
     {
--- a/gfx/gl/GLContextProviderEGL.cpp
+++ b/gfx/gl/GLContextProviderEGL.cpp
@@ -415,17 +415,17 @@ public:
                                                LOCAL_EGL_BACK_BUFFER);
         if (success == LOCAL_EGL_FALSE)
             return false;
 
         mBound = false;
         return true;
     }
 
-    bool MakeCurrentImpl() {
+    bool MakeCurrentImpl(bool aForce = false) {
         bool succeeded = true;
 
         // Assume that EGL has the same problem as WGL does,
         // where MakeCurrent with an already-current context is
         // still expensive.
 #ifndef MOZ_WIDGET_QT
         if (!mSurface) {
             // We need to be able to bind NO_SURFACE when we don't
@@ -437,34 +437,35 @@ public:
             if (!succeeded && sEGLLibrary.fGetError() == LOCAL_EGL_CONTEXT_LOST) {
                 mContextLost = true;
                 NS_WARNING("EGL context has been lost.");
             }
             NS_ASSERTION(succeeded, "Failed to make GL context current!");
             return succeeded;
         }
 #endif
-
+        if (aForce || sEGLLibrary.fGetCurrentContext() != mContext) {
 #ifdef MOZ_WIDGET_QT
-        // Shared Qt GL context need to be informed about context switch
-        if (mSharedContext) {
-            QGLContext* qglCtx = static_cast<QGLContext*>(static_cast<GLContextEGL*>(mSharedContext.get())->mPlatformContext);
-            if (qglCtx) {
-                qglCtx->doneCurrent();
+            // Shared Qt GL context need to be informed about context switch
+            if (mSharedContext) {
+                QGLContext* qglCtx = static_cast<QGLContext*>(static_cast<GLContextEGL*>(mSharedContext.get())->mPlatformContext);
+                if (qglCtx) {
+                    qglCtx->doneCurrent();
+                }
             }
+#endif
+            succeeded = sEGLLibrary.fMakeCurrent(EGL_DISPLAY(),
+                                                 mSurface, mSurface,
+                                                 mContext);
+            if (!succeeded && sEGLLibrary.fGetError() == LOCAL_EGL_CONTEXT_LOST) {
+                mContextLost = true;
+                NS_WARNING("EGL context has been lost.");
+            }
+            NS_ASSERTION(succeeded, "Failed to make GL context current!");
         }
-#endif
-        succeeded = sEGLLibrary.fMakeCurrent(EGL_DISPLAY(),
-                                                mSurface, mSurface,
-                                                mContext);
-        if (!succeeded && sEGLLibrary.fGetError() == LOCAL_EGL_CONTEXT_LOST) {
-            mContextLost = true;
-            NS_WARNING("EGL context has been lost.");
-        }
-        NS_ASSERTION(succeeded, "Failed to make GL context current!");
 
         return succeeded;
     }
 
 #ifdef MOZ_WIDGET_QT
     virtual bool
     RenewSurface() {
         /* We don't support renewing on QT because we don't create the surface ourselves */
--- a/gfx/gl/GLContextProviderGLX.cpp
+++ b/gfx/gl/GLContextProviderGLX.cpp
@@ -788,20 +788,30 @@ TRY_AGAIN_NO_SHARING:
         if (!IsExtensionSupported(EXT_framebuffer_object))
             return false;
 
         InitFramebuffers();
 
         return true;
     }
 
-    bool MakeCurrentImpl()
+    bool MakeCurrentImpl(bool aForce = false)
     {
-        bool succeeded = sGLXLibrary.xMakeCurrent(mDisplay, mDrawable, mContext);
-        NS_ASSERTION(succeeded, "Failed to make GL context current!");
+        bool succeeded = true;
+
+        // With the ATI FGLRX driver, glxMakeCurrent is very slow even when the context doesn't change.
+        // (This is not the case with other drivers such as NVIDIA).
+        // So avoid calling it more than necessary. Since GLX documentation says that:
+        //     "glXGetCurrentContext returns client-side information.
+        //      It does not make a round trip to the server."
+        // I assume that it's not worth using our own TLS slot here.
+        if (aForce || sGLXLibrary.xGetCurrentContext() != mContext) {
+            succeeded = sGLXLibrary.xMakeCurrent(mDisplay, mDrawable, mContext);
+            NS_ASSERTION(succeeded, "Failed to make GL context current!");
+        }
 
         return succeeded;
     }
 
     bool SetupLookupFunction()
     {
         mLookupFunc = (PlatformLookupFunction)&GLXLibrary::xGetProcAddress;
         return true;
--- a/gfx/gl/GLContextProviderOSMesa.cpp
+++ b/gfx/gl/GLContextProviderOSMesa.cpp
@@ -208,17 +208,17 @@ public:
         if (!SetupLookupFunction()) return false;
 
         // OSMesa's different from the other GL providers, it renders to an image surface, not to a pbuffer
         sOSMesaLibrary.fPixelStore(OSMESA_Y_UP, 0);
 
         return InitWithPrefix("gl", true);
     }
 
-    bool MakeCurrentImpl()
+    bool MakeCurrentImpl(bool aForce = false)
     {
         bool succeeded
           = sOSMesaLibrary.fMakeCurrent(mContext, mThebesSurface->Data(),
                                         LOCAL_GL_UNSIGNED_BYTE,
                                         mThebesSurface->Width(),
                                         mThebesSurface->Height());
         NS_ASSERTION(succeeded, "Failed to make OSMesa context current!");
 
--- a/gfx/gl/GLContextProviderWGL.cpp
+++ b/gfx/gl/GLContextProviderWGL.cpp
@@ -328,20 +328,28 @@ public:
         SetupLookupFunction();
         if (!InitWithPrefix("gl", true))
             return false;
 
         InitFramebuffers();
         return true;
     }
 
-    bool MakeCurrentImpl()
+    bool MakeCurrentImpl(bool aForce = false)
     {
-        bool succeeded = sWGLLibrary.fMakeCurrent(mDC, mContext);
-        NS_ASSERTION(succeeded, "Failed to make GL context current!");
+        BOOL succeeded = true;
+
+        // wglGetCurrentContext seems to just pull the HGLRC out
+        // of its TLS slot, so no need to do our own tls slot.
+        // You would think that wglMakeCurrent would avoid doing
+        // work if mContext was already current, but not so much..
+        if (aForce || sWGLLibrary.fGetCurrentContext() != mContext) {
+            succeeded = sWGLLibrary.fMakeCurrent(mDC, mContext);
+            NS_ASSERTION(succeeded, "Failed to make GL context current!");
+        }
 
         return succeeded;
     }
 
     void SetIsDoubleBuffered(bool aIsDB) {
         mIsDoubleBuffered = aIsDB;
     }
 
--- a/gfx/thebes/gfxPlatform.cpp
+++ b/gfx/thebes/gfxPlatform.cpp
@@ -296,16 +296,20 @@ gfxPlatform::Init()
 #elif defined(XP_OS2)
     gPlatform = new gfxOS2Platform;
 #elif defined(ANDROID)
     gPlatform = new gfxAndroidPlatform;
 #else
     #error "No gfxPlatform implementation available"
 #endif
 
+#ifdef DEBUG
+    mozilla::gl::GLContext::StaticInit();
+#endif
+
     nsresult rv;
 
 #if defined(XP_MACOSX) || defined(XP_WIN) || defined(ANDROID) // temporary, until this is implemented on others
     rv = gfxPlatformFontList::Init();
     if (NS_FAILED(rv)) {
         NS_RUNTIMEABORT("Could not initialize gfxPlatformFontList");
     }
 #endif