Bug 1390386 - Remove duplicate IsCurrent checks in MakeCurrentImpls. - r=jrmuizel draft
authorJeff Gilbert <jgilbert@mozilla.com>
Tue, 15 Aug 2017 13:21:37 -0700
changeset 702517 cb36af392e0439e89270f6ea22ffd014b33055ff
parent 702516 dbc0e03cf34882d45606212bb838def7102622ff
child 702518 e4573196860f0ef3a806b57d73ef6fb612c3a399
push id90545
push userbmo:jgilbert@mozilla.com
push dateThu, 23 Nov 2017 11:45:26 +0000
reviewersjrmuizel
bugs1390386
milestone59.0a1
Bug 1390386 - Remove duplicate IsCurrent checks in MakeCurrentImpls. - r=jrmuizel MozReview-Commit-ID: LZeLbciWnic
gfx/gl/GLContext.cpp
gfx/gl/GLContext.h
gfx/gl/GLContextCGL.h
gfx/gl/GLContextEAGL.h
gfx/gl/GLContextEGL.h
gfx/gl/GLContextGLX.h
gfx/gl/GLContextProviderCGL.mm
gfx/gl/GLContextProviderEAGL.mm
gfx/gl/GLContextProviderEGL.cpp
gfx/gl/GLContextProviderGLX.cpp
gfx/gl/GLContextProviderWGL.cpp
gfx/gl/GLContextWGL.h
--- a/gfx/gl/GLContext.cpp
+++ b/gfx/gl/GLContext.cpp
@@ -3026,28 +3026,35 @@ GetBytesPerTexel(GLenum format, GLenum t
 }
 
 bool
 GLContext::MakeCurrent(bool aForce) const
 {
     if (MOZ_UNLIKELY( IsDestroyed() ))
         return false;
 
-    if (mUseTLSIsCurrent && !aForce && sCurrentContext.get() == this) {
-        MOZ_ASSERT(IsCurrent());
-        return true;
+    if (MOZ_LIKELY( !aForce )) {
+        bool isCurrent;
+        if (mUseTLSIsCurrent) {
+            isCurrent = (sCurrentContext.get() == this);
+        } else {
+            isCurrent = IsCurrentImpl();
+        }
+        if (MOZ_LIKELY( isCurrent )) {
+            MOZ_ASSERT(IsCurrentImpl());
+            return true;
+        }
     }
 
-    if (!MakeCurrentImpl(aForce))
+    if (!MakeCurrentImpl())
         return false;
 
     if (mUseTLSIsCurrent) {
         sCurrentContext.set(this);
     }
-
     return true;
 }
 
 void
 GLContext::ResetSyncCallCount(const char* resetReason) const
 {
     if (ShouldSpew()) {
         printf_stderr("On %s, mSyncGLCallCount = %" PRIu64 "\n",
--- a/gfx/gl/GLContext.h
+++ b/gfx/gl/GLContext.h
@@ -293,24 +293,27 @@ public:
      * If this context is double-buffered, returns TRUE.
      */
     virtual bool IsDoubleBuffered() const {
         return false;
     }
 
     virtual GLContextType GetContextType() const = 0;
 
+    virtual bool IsCurrentImpl() const = 0;
+    virtual bool MakeCurrentImpl() const = 0;
+
     bool IsCurrent() const {
         if (mImplicitMakeCurrent)
             return MakeCurrent();
 
         return IsCurrentImpl();
     }
 
-    virtual bool IsCurrentImpl() const = 0;
+    bool MakeCurrent(bool aForce = false) const;
 
     /**
      * Get the default framebuffer for this context.
      */
     virtual GLuint GetDefaultFramebuffer() {
         return 0;
     }
 
@@ -675,17 +678,17 @@ private:
 // Record the name of the GL call for better hang stacks on Android.
 #define ANDROID_ONLY_PROFILER_LABEL AUTO_PROFILER_LABEL(__func__, GRAPHICS);
 #else
 #define ANDROID_ONLY_PROFILER_LABEL
 #endif
 
 #define BEFORE_GL_CALL \
         ANDROID_ONLY_PROFILER_LABEL \
-        if (BeforeGLCall(MOZ_FUNCTION_NAME)) { \
+        if (MOZ_LIKELY( BeforeGLCall(MOZ_FUNCTION_NAME) )) { \
             do { } while (0)
 
 #define AFTER_GL_CALL \
             AfterGLCall(MOZ_FUNCTION_NAME); \
         } \
         do { } while (0)
 
     void BeforeGLCall_Debug(const char* funcName) const;
@@ -694,17 +697,17 @@ private:
 
     bool BeforeGLCall(const char* const funcName) const {
         if (mImplicitMakeCurrent) {
             if (MOZ_UNLIKELY( !MakeCurrent() )) {
                 OnImplicitMakeCurrentFailure(funcName);
                 return false;
             }
         }
-        MOZ_ASSERT(IsCurrent());
+        MOZ_ASSERT(IsCurrentImpl());
 
         if (mDebugFlags) {
             BeforeGLCall_Debug(funcName);
         }
         return true;
     }
 
     void AfterGLCall(const char* const funcName) const {
@@ -3311,21 +3314,17 @@ public:
     // the GL function pointers!
     void MarkDestroyed();
 
 // -----------------------------------------------------------------------------
 // Everything that isn't standard GL APIs
 protected:
     typedef gfx::SurfaceFormat SurfaceFormat;
 
-    virtual bool MakeCurrentImpl(bool aForce) const = 0;
-
 public:
-    bool MakeCurrent(bool aForce = false) const;
-
     virtual bool Init() = 0;
 
     virtual bool SetupLookupFunction() = 0;
 
     virtual void ReleaseSurface() {}
 
     bool IsDestroyed() const {
         // MarkDestroyed will mark all these as null.
--- a/gfx/gl/GLContextCGL.h
+++ b/gfx/gl/GLContextCGL.h
@@ -40,17 +40,17 @@ public:
         return static_cast<GLContextCGL*>(gl);
     }
 
     bool Init() override;
 
     NSOpenGLContext* GetNSOpenGLContext() const { return mContext; }
     CGLContextObj GetCGLContext() const;
 
-    virtual bool MakeCurrentImpl(bool aForce) const override;
+    virtual bool MakeCurrentImpl() const override;
 
     virtual bool IsCurrentImpl() const override;
 
     virtual GLenum GetPreferredARGB32Format() const override;
 
     virtual bool SetupLookupFunction() override;
 
     virtual bool IsDoubleBuffered() const override;
--- a/gfx/gl/GLContextEAGL.h
+++ b/gfx/gl/GLContextEAGL.h
@@ -38,17 +38,17 @@ public:
     }
 
     bool Init() override;
 
     bool AttachToWindow(nsIWidget* aWidget);
 
     EAGLContext* GetEAGLContext() const { return mContext; }
 
-    virtual bool MakeCurrentImpl(bool aForce) const override;
+    virtual bool MakeCurrentImpl() const override;
 
     virtual bool IsCurrentImpl() const override;
 
     virtual bool SetupLookupFunction() override;
 
     virtual bool IsDoubleBuffered() const override;
 
     virtual bool SwapBuffers() override;
--- a/gfx/gl/GLContextEGL.h
+++ b/gfx/gl/GLContextEGL.h
@@ -68,17 +68,17 @@ public:
 
     virtual bool ReleaseTexImage() override;
 
     void SetEGLSurfaceOverride(EGLSurface surf);
     EGLSurface GetEGLSurfaceOverride() {
         return mSurfaceOverride;
     }
 
-    virtual bool MakeCurrentImpl(bool aForce) const override;
+    virtual bool MakeCurrentImpl() const override;
 
     virtual bool IsCurrentImpl() const override;
 
     virtual bool RenewSurface(widget::CompositorWidget* aWidget) override;
 
     virtual void ReleaseSurface() override;
 
     virtual bool SetupLookupFunction() override;
--- a/gfx/gl/GLContextGLX.h
+++ b/gfx/gl/GLContextGLX.h
@@ -41,17 +41,17 @@ public:
 
     static GLContextGLX* Cast(GLContext* gl) {
         MOZ_ASSERT(gl->GetContextType() == GLContextType::GLX);
         return static_cast<GLContextGLX*>(gl);
     }
 
     bool Init() override;
 
-    virtual bool MakeCurrentImpl(bool aForce) const override;
+    virtual bool MakeCurrentImpl() const override;
 
     virtual bool IsCurrentImpl() const override;
 
     virtual bool SetupLookupFunction() override;
 
     virtual bool IsDoubleBuffered() const override;
 
     virtual bool SwapBuffers() override;
--- a/gfx/gl/GLContextProviderCGL.mm
+++ b/gfx/gl/GLContextProviderCGL.mm
@@ -105,25 +105,21 @@ GLContextCGL::Init()
 
 CGLContextObj
 GLContextCGL::GetCGLContext() const
 {
     return static_cast<CGLContextObj>([mContext CGLContextObj]);
 }
 
 bool
-GLContextCGL::MakeCurrentImpl(bool aForce) const
+GLContextCGL::MakeCurrentImpl() const
 {
-    if (!aForce && [NSOpenGLContext currentContext] == mContext) {
-        return true;
-    }
-
     if (mContext) {
         [mContext makeCurrentContext];
-        MOZ_ASSERT(IsCurrent());
+        MOZ_ASSERT(IsCurrentImpl());
         // Use non-blocking swap in "ASAP mode".
         // ASAP mode means that rendering is iterated as fast as possible.
         // ASAP mode is entered when layout.frame_rate=0 (requires restart).
         // If swapInt is 1, then glSwapBuffers will block and wait for a vblank signal.
         // When we're iterating as fast as possible, however, we want a non-blocking
         // glSwapBuffers, which will happen when swapInt==0.
         GLint swapInt = gfxPrefs::LayoutFrameRate() == 0 ? 0 : 1;
         [mContext setValues:&swapInt forParameter:NSOpenGLCPSwapInterval];
--- a/gfx/gl/GLContextProviderEAGL.mm
+++ b/gfx/gl/GLContextProviderEAGL.mm
@@ -108,22 +108,18 @@ GLContextEAGL::RecreateRB()
     fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, mBackbufferFB);
     fFramebufferRenderbuffer(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_COLOR_ATTACHMENT0,
                              LOCAL_GL_RENDERBUFFER, mBackbufferRB);
 
     return LOCAL_GL_FRAMEBUFFER_COMPLETE == fCheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER);
 }
 
 bool
-GLContextEAGL::MakeCurrentImpl(bool aForce) const
+GLContextEAGL::MakeCurrentImpl() const
 {
-    if (!aForce && [EAGLContext currentContext] == mContext) {
-        return true;
-    }
-
     if (mContext) {
         if(![EAGLContext setCurrentContext:mContext]) {
             return false;
         }
     }
     return true;
 }
 
--- a/gfx/gl/GLContextProviderEGL.cpp
+++ b/gfx/gl/GLContextProviderEGL.cpp
@@ -348,45 +348,37 @@ GLContextEGL::SetEGLSurfaceOverride(EGLS
     }
 
     mSurfaceOverride = surf;
     DebugOnly<bool> ok = MakeCurrent(true);
     MOZ_ASSERT(ok);
 }
 
 bool
-GLContextEGL::MakeCurrentImpl(bool aForce) const
+GLContextEGL::MakeCurrentImpl() const
 {
-    bool succeeded = true;
+    const EGLSurface surface = (mSurfaceOverride != EGL_NO_SURFACE) ? mSurfaceOverride
+                                                                    : mSurface;
+    if (surface == EGL_NO_SURFACE) {
+        MOZ_CRASH("EGL_NO_SURFACE");
+        return false;
+    }
 
-    // Assume that EGL has the same problem as WGL does,
-    // where MakeCurrent with an already-current context is
-    // still expensive.
-    bool needsMakeCurrent = (aForce || sEGLLibrary.fGetCurrentContext() != mContext);
-    if (needsMakeCurrent) {
-        EGLSurface surface = mSurfaceOverride != EGL_NO_SURFACE
-                              ? mSurfaceOverride
-                              : mSurface;
-        if (surface == EGL_NO_SURFACE) {
-            return false;
-        }
-        succeeded = sEGLLibrary.fMakeCurrent(EGL_DISPLAY(),
-                                              surface, surface,
-                                              mContext);
-        if (!succeeded) {
-            int eglError = sEGLLibrary.fGetError();
-            if (eglError == LOCAL_EGL_CONTEXT_LOST) {
-                mContextLost = true;
-                NS_WARNING("EGL context has been lost.");
-            } else {
-                NS_WARNING("Failed to make GL context current!");
+    const bool succeeded = sEGLLibrary.fMakeCurrent(EGL_DISPLAY(), surface, surface,
+                                                    mContext);
+    if (!succeeded) {
+        const auto eglError = sEGLLibrary.fGetError();
+        if (eglError == LOCAL_EGL_CONTEXT_LOST) {
+            mContextLost = true;
+            NS_WARNING("EGL context has been lost.");
+        } else {
+            NS_WARNING("Failed to make GL context current!");
 #ifdef DEBUG
-                printf_stderr("EGL Error: 0x%04x\n", eglError);
+            printf_stderr("EGL Error: 0x%04x\n", eglError);
 #endif
-            }
         }
     }
 
     return succeeded;
 }
 
 bool
 GLContextEGL::IsCurrentImpl() const
--- a/gfx/gl/GLContextProviderGLX.cpp
+++ b/gfx/gl/GLContextProviderGLX.cpp
@@ -601,45 +601,34 @@ GLContextGLX::Init()
     // so we'll also check for ARB_framebuffer_object
     if (!IsExtensionSupported(EXT_framebuffer_object) && !IsSupported(GLFeature::framebuffer_object))
         return false;
 
     return true;
 }
 
 bool
-GLContextGLX::MakeCurrentImpl(bool aForce) const
+GLContextGLX::MakeCurrentImpl() const
 {
-    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 || mGLX->fGetCurrentContext() != mContext) {
-        if (mGLX->IsMesa()) {
-          // Read into the event queue to ensure that Mesa receives a
-          // DRI2InvalidateBuffers event before drawing. See bug 1280653.
-          Unused << XPending(mDisplay);
-        }
-
-        succeeded = mGLX->fMakeCurrent(mDisplay, mDrawable, mContext);
-        NS_ASSERTION(succeeded, "Failed to make GL context current!");
-
-        if (!IsOffscreen() && mGLX->SupportsSwapControl()) {
-            // Many GLX implementations default to blocking until the next
-            // VBlank when calling glXSwapBuffers. We want to run unthrottled
-            // in ASAP mode. See bug 1280744.
-            const bool isASAP = (gfxPrefs::LayoutFrameRate() == 0);
-            mGLX->fSwapInterval(mDisplay, mDrawable, isASAP ? 0 : 1);
-        }
+    if (mGLX->IsMesa()) {
+        // Read into the event queue to ensure that Mesa receives a
+        // DRI2InvalidateBuffers event before drawing. See bug 1280653.
+        Unused << XPending(mDisplay);
     }
 
+    const bool succeeded = mGLX->fMakeCurrent(mDisplay, mDrawable, mContext);
+    NS_ASSERTION(succeeded, "Failed to make GL context current!");
+
+    if (!IsOffscreen() && mGLX->SupportsSwapControl()) {
+        // Many GLX implementations default to blocking until the next
+        // VBlank when calling glXSwapBuffers. We want to run unthrottled
+        // in ASAP mode. See bug 1280744.
+        const bool isASAP = (gfxPrefs::LayoutFrameRate() == 0);
+        mGLX->fSwapInterval(mDisplay, mDrawable, isASAP ? 0 : 1);
+    }
     return succeeded;
 }
 
 bool
 GLContextGLX::IsCurrentImpl() const
 {
     return mGLX->fGetCurrentContext() == mContext;
 }
--- a/gfx/gl/GLContextProviderWGL.cpp
+++ b/gfx/gl/GLContextProviderWGL.cpp
@@ -318,29 +318,20 @@ GLContextWGL::Init()
     SetupLookupFunction();
     if (!InitWithPrefix("gl", true))
         return false;
 
     return true;
 }
 
 bool
-GLContextWGL::MakeCurrentImpl(bool aForce) const
+GLContextWGL::MakeCurrentImpl() const
 {
-    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 || sWGLLib.mSymbols.fGetCurrentContext() != mContext) {
-        succeeded = sWGLLib.mSymbols.fMakeCurrent(mDC, mContext);
-        NS_ASSERTION(succeeded, "Failed to make GL context current!");
-    }
-
+    const bool succeeded = sWGLLib.mSymbols.fMakeCurrent(mDC, mContext);
+    NS_ASSERTION(succeeded, "Failed to make GL context current!");
     return succeeded;
 }
 
 bool
 GLContextWGL::IsCurrentImpl() const
 {
     return sWGLLib.mSymbols.fGetCurrentContext() == mContext;
 }
--- a/gfx/gl/GLContextWGL.h
+++ b/gfx/gl/GLContextWGL.h
@@ -40,17 +40,17 @@ public:
 
     static GLContextWGL* Cast(GLContext* gl) {
         MOZ_ASSERT(gl->GetContextType() == GLContextType::WGL);
         return static_cast<GLContextWGL*>(gl);
     }
 
     bool Init() override;
 
-    virtual bool MakeCurrentImpl(bool aForce) const override;
+    virtual bool MakeCurrentImpl() const override;
 
     virtual bool IsCurrentImpl() const override;
 
     void SetIsDoubleBuffered(bool aIsDB);
 
     virtual bool IsDoubleBuffered() const override;
 
     virtual bool SwapBuffers() override;