macOS: Don't call [NSOpenGLContext update] for every frame

Calling update has a cost, and should only be done when the drawable
object changes size or location. Instead of calling update each time
makeCurrent is called, we listen for the appropriate notifications,
limiting the number of update calls significantly.

The code has also been refactored to get rid of the m_activeWindow
member, as the active window can be tracked through the context's
drawable object property.

There is also no need to clear the drawable when a window is hidden,
so the hook into QCocoaWindow can be removed.

The QPlatformNativeInterface hook is internal and can safely be removed.

Task-number: QTBUG-63572
Change-Id: I70e3267f47882e151144bd36a50abe906164429a
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>

5 files changed, 99 insertions, 76 deletions

diff --git a/src/plugins/platforms/cocoa/qcocoaglcontext.h b/src/plugins/platforms/cocoa/qcocoaglcontext.h
index 0e5934bc23..3f7966b247 100644
--- a/src/plugins/platforms/cocoa/qcocoaglcontext.h
+++ b/src/plugins/platforms/cocoa/qcocoaglcontext.h
@@ -41,6 +41,9 @@
 #define QCOCOAGLCONTEXT_H
 
 #include <QtCore/QPointer>
+#include <QtCore/qvector.h>
+#include <QtCore/private/qcore_mac_p.h>
+
 #include <qpa/qplatformopenglcontext.h>
 #include <QtGui/QOpenGLContext>
 #include <QtGui/QWindow>
@@ -65,8 +68,6 @@ public:
     bool isSharing() const override;
     bool isValid() const override;
 
-    void windowWasHidden();
-
     NSOpenGLContext *nativeContext() const;
 
     QFunctionPointer getProcAddress(const char *procName) override;
@@ -74,14 +75,14 @@ public:
 private:
     static NSOpenGLPixelFormat *pixelFormatForSurfaceFormat(const QSurfaceFormat &format);
 
-    bool setActiveWindow(QWindow *window);
+    bool setDrawable(QPlatformSurface *surface);
     void updateSurfaceFormat();
 
     NSOpenGLContext *m_context = nil;
     NSOpenGLContext *m_shareContext = nil;
     QSurfaceFormat m_format;
-    QPointer<QWindow> m_currentWindow;
     bool m_didCheckForSoftwareContext = false;
+    QVarLengthArray<QMacScopedObserver, 3> m_observers;
 };
 
 QT_END_NAMESPACE
diff --git a/src/plugins/platforms/cocoa/qcocoaglcontext.mm b/src/plugins/platforms/cocoa/qcocoaglcontext.mm
index 4d0fa2e28e..cba9e90a78 100644
--- a/src/plugins/platforms/cocoa/qcocoaglcontext.mm
+++ b/src/plugins/platforms/cocoa/qcocoaglcontext.mm
@@ -41,6 +41,8 @@
 #include "qcocoawindow.h"
 #include "qcocoahelpers.h"
 #include <qdebug.h>
+#include <QtCore/qscopedvaluerollback.h>
+#include <QtCore/qatomic.h>
 #include <QtCore/private/qcore_mac_p.h>
 #include <QtPlatformHeaders/qcocoanativecontext.h>
 #include <dlfcn.h>
@@ -320,9 +322,6 @@ void QCocoaGLContext::updateSurfaceFormat()
 
 QCocoaGLContext::~QCocoaGLContext()
 {
-    if (m_currentWindow && m_currentWindow.data()->handle())
-        static_cast<QCocoaWindow *>(m_currentWindow.data()->handle())->setCurrentContext(0);
-
     [m_context release];
 }
 
@@ -331,6 +330,14 @@ bool QCocoaGLContext::makeCurrent(QPlatformSurface *surface)
     qCDebug(lcQpaOpenGLContext) << "Making" << m_context << "current"
         << "in" << QThread::currentThread() << "for" << surface;
 
+    // No need to make context current if it already is. This also ensures
+    // that we only lock the context once, meaning we don't need to keep
+    // track of how many times we've locked it to undo it in doneCurrent().
+    // Note that we're not using QOpenGLContext::currentContext() here, as
+    // that has already been updated to match context() before this call.
+    if ([NSOpenGLContext currentContext] == m_context)
+        return true;
+
     Q_ASSERT(surface->surface()->supportsOpenGL());
 
     if (surface->surface()->surfaceClass() == QSurface::Offscreen) {
@@ -338,11 +345,14 @@ bool QCocoaGLContext::makeCurrent(QPlatformSurface *surface)
         return true;
     }
 
-    QWindow *window = static_cast<QCocoaWindow *>(surface)->window();
-    if (!setActiveWindow(window)) {
-        qCDebug(lcQpaOpenGLContext) << "Failed to activate window, skipping makeCurrent";
+    if (!setDrawable(surface))
         return false;
-    }
+
+    // The context may be owned and used by a dedicated render thread, but
+    // we will get notifications that trigger update() on the main thread,
+    // so we need to guard against concurrent uses of the context. We hold
+    // this lock until swapBuffer() or doneCurrent() gets called.
+    CGLLockContext(m_context.CGLContextObj);
 
     [m_context makeCurrentContext];
 
@@ -363,43 +373,74 @@ bool QCocoaGLContext::makeCurrent(QPlatformSurface *surface)
         }
     }
 
-    update();
     return true;
 }
 
-bool QCocoaGLContext::setActiveWindow(QWindow *window)
+/*!
+    Sets the drawable object of the NSOpenGLContext, which is the
+    frame buffer that is the target of OpenGL drawing operations.
+*/
+bool QCocoaGLContext::setDrawable(QPlatformSurface *surface)
 {
-    if (window == m_currentWindow.data())
+    Q_ASSERT(surface->surface()->surfaceClass() == QSurface::Window);
+    NSView *view = static_cast<QCocoaWindow *>(surface)->view();
+
+    if (view == m_context.view)
         return true;
 
-    Q_ASSERT(window->handle());
-    QCocoaWindow *cocoaWindow = static_cast<QCocoaWindow *>(window->handle());
-    NSView *view = cocoaWindow->view();
+    m_observers.clear();
 
     if ((m_context.view = view) != view) {
-        qCDebug(lcQpaOpenGLContext) << "Associating" << view << "with" << m_context << "failed";
+        qCInfo(lcQpaOpenGLContext) << "Failed to set" << view << "as drawable for" << m_context;
         return false;
     }
 
-    qCDebug(lcQpaOpenGLContext) << m_context << "now associated with" << m_context.view;
+    qCInfo(lcQpaOpenGLContext) << "Set drawable for" << m_context << "to" << m_context.view;
+
+    auto updateCallback = [&]() { update(); };
 
-    if (m_currentWindow && m_currentWindow.data()->handle())
-        static_cast<QCocoaWindow *>(m_currentWindow.data()->handle())->setCurrentContext(0);
+    if (view.layer) {
+        m_observers.append(QMacScopedObserver(view, NSViewFrameDidChangeNotification, updateCallback));
+        m_observers.append(QMacScopedObserver(view.window, NSWindowDidChangeScreenNotification, updateCallback));
+    } else {
+        m_observers.append(QMacScopedObserver(view, NSViewGlobalFrameDidChangeNotification, updateCallback));
+    }
 
-    m_currentWindow = window;
+    m_observers.append(QMacScopedObserver([NSApplication sharedApplication],
+        NSApplicationDidChangeScreenParametersNotification, updateCallback));
 
-    cocoaWindow->setCurrentContext(this);
     return true;
 }
 
-// NSOpenGLContext is not re-entrant (https://openradar.appspot.com/37064579)
+// NSOpenGLContext is not re-entrant, which means that even when using separate
+// contexts per thread, per view, and window, calls into the API will still deadlock.
+// Note that this is different from the use of CGLLockContext and CGLUnlockContext
+// to prevent concurrent access to the _same_ context from two different threads.
+// The latter is expected due to NSOpenGLContext not being thread-safe, while the
+// former is working around bugs in NSOpenGLContext that make it not re-entrant.
+// For more information see https://openradar.appspot.com/37064579
 static QMutex s_contextMutex;
 
 void QCocoaGLContext::update()
 {
+    // Updating the context may result in a call to [NSSurface setFrame:], which
+    // will recurse back here through NSViewGlobalFrameDidChangeNotification. We
+    // could use a recursive mutex to prevent a deadlock, but since they are slower
+    // we opt for a manual recursion check.
+    static QAtomicPointer<void> updatingThread = nullptr;
+    if (updatingThread == QThread::currentThreadId())
+        return;
+
+    // Guard against concurrent access to the context in the case where there
+    // is a dedicated render thread operating on the context. See makeCurrent().
+    CGLLockContext(m_context.CGLContextObj);
+
     QMutexLocker locker(&s_contextMutex);
+    QScopedValueRollback<QAtomicPointer<void>> rollback(updatingThread, QThread::currentThreadId());
     qCInfo(lcQpaOpenGLContext) << "Updating" << m_context << "for" << m_context.view;
     [m_context update];
+
+    CGLUnlockContext(m_context.CGLContextObj);
 }
 
 void QCocoaGLContext::swapBuffers(QPlatformSurface *surface)
@@ -410,37 +451,52 @@ void QCocoaGLContext::swapBuffers(QPlatformSurface *surface)
     if (surface->surface()->surfaceClass() == QSurface::Offscreen)
         return; // Nothing to do
 
-    QWindow *window = static_cast<QCocoaWindow *>(surface)->window();
-    if (!setActiveWindow(window)) {
-        qCWarning(lcQpaOpenGLContext) << "Failed to activate window, skipping swapBuffers";
+    if (!setDrawable(surface)) {
+        qCWarning(lcQpaOpenGLContext) << "Can't flush" << m_context
+            << "without" << surface << "as drawable";
         return;
     }
 
     QMutexLocker locker(&s_contextMutex);
     [m_context flushBuffer];
+
+    // We're done flushing, and should release the lock we have on the
+    // context. To ensure that we're not leaving the context current
+    // without a lock held on it, we need to couple this with actually
+    // clearing the context. This should not be a performance hit for the
+    // case where the same context is made current and then cleared, and
+    // QOpenGLContext::swapBuffers is documented as requiring makeCurrent
+    // again before beginning a new frame, so the user can't expect the
+    // context to be current after a call to swapBuffers(). We explicitly
+    // go via QOpenGLContext for this, instead of calling our platform
+    // method directly, as that will ensure QOpenGLContext records the
+    // fact that there is no longer a current context. We then end up
+    // in QCocoaGLContext::doneCurrent, where we clear the lock.
+    context()->doneCurrent();
 }
 
 void QCocoaGLContext::doneCurrent()
 {
-    qCDebug(lcQpaOpenGLContext) << "Clearing current context"
-        << [NSOpenGLContext currentContext] << "in" << QThread::currentThread();
+    auto currentContext = QOpenGLContext::currentContext();
+    if (!currentContext)
+        return;
 
-    if (m_currentWindow && m_currentWindow.data()->handle())
-        static_cast<QCocoaWindow *>(m_currentWindow.data()->handle())->setCurrentContext(nullptr);
+    // QOpenGLContext::doneCurrent() clears the current context, but can
+    // be called on any context, not necessarily the current one. Since
+    // we rely on unlocking the context lock we must propagate the call
+    // to the right context.
+    if (context() != currentContext) {
+        currentContext->doneCurrent();
+        return;
+    }
 
-    m_currentWindow.clear();
+    Q_ASSERT([NSOpenGLContext currentContext] == m_context);
 
-    [NSOpenGLContext clearCurrentContext];
-}
+    qCDebug(lcQpaOpenGLContext) << "Clearing current context"
+        << [NSOpenGLContext currentContext] << "in" << QThread::currentThread();
 
-void QCocoaGLContext::windowWasHidden()
-{
-    // If the window is hidden, we need to unset the m_currentWindow
-    // variable so that succeeding makeCurrent's will not abort prematurely
-    // because of the optimization in setActiveWindow.
-    // Doing a full doneCurrent here is not preferable, because the GL context
-    // might be rendering in a different thread at this time.
-    m_currentWindow.clear();
+    [NSOpenGLContext clearCurrentContext];
+    CGLUnlockContext(m_context.CGLContextObj);
 }
 
 QSurfaceFormat QCocoaGLContext::format() const
diff --git a/src/plugins/platforms/cocoa/qcocoanativeinterface.mm b/src/plugins/platforms/cocoa/qcocoanativeinterface.mm
index 228df50d86..7979e430ac 100644
--- a/src/plugins/platforms/cocoa/qcocoanativeinterface.mm
+++ b/src/plugins/platforms/cocoa/qcocoanativeinterface.mm
@@ -102,10 +102,6 @@ void *QCocoaNativeInterface::nativeResourceForWindow(const QByteArray &resourceS
 
     if (resourceString == "nsview") {
         return static_cast<QCocoaWindow *>(window->handle())->m_view;
-#ifndef QT_NO_OPENGL
-    } else if (resourceString == "nsopenglcontext") {
-        return static_cast<QCocoaWindow *>(window->handle())->currentContext()->nativeContext();
-#endif
     } else if (resourceString == "nswindow") {
         return static_cast<QCocoaWindow *>(window->handle())->nativeWindow();
 #if QT_CONFIG(vulkan)
diff --git a/src/plugins/platforms/cocoa/qcocoawindow.h b/src/plugins/platforms/cocoa/qcocoawindow.h
index 225c7eda84..8f1bdb8af0 100644
--- a/src/plugins/platforms/cocoa/qcocoawindow.h
+++ b/src/plugins/platforms/cocoa/qcocoawindow.h
@@ -169,11 +169,6 @@ public:
     NSUInteger windowStyleMask(Qt::WindowFlags flags);
     void setWindowZoomButton(Qt::WindowFlags flags);
 
-#ifndef QT_NO_OPENGL
-    void setCurrentContext(QCocoaGLContext *context);
-    QCocoaGLContext *currentContext() const;
-#endif
-
     bool setWindowModified(bool modified) override;
 
     void setFrameStrutEventsEnabled(bool enabled) override;
@@ -253,9 +248,6 @@ public: // for QNSView
     bool m_inSetVisible;
     bool m_inSetGeometry;
     bool m_inSetStyleMask;
-#ifndef QT_NO_OPENGL
-    QCocoaGLContext *m_glContext;
-#endif
     QCocoaMenuBar *m_menubar;
 
     bool m_needsInvalidateShadow;
diff --git a/src/plugins/platforms/cocoa/qcocoawindow.mm b/src/plugins/platforms/cocoa/qcocoawindow.mm
index 3148501006..b79804fd0b 100644
--- a/src/plugins/platforms/cocoa/qcocoawindow.mm
+++ b/src/plugins/platforms/cocoa/qcocoawindow.mm
@@ -41,9 +41,6 @@
 #include "qcocoascreen.h"
 #include "qnswindowdelegate.h"
 #include "qcocoaeventdispatcher.h"
-#ifndef QT_NO_OPENGL
-#include "qcocoaglcontext.h"
-#endif
 #include "qcocoahelpers.h"
 #include "qcocoanativeinterface.h"
 #include "qnsview.h"
@@ -151,9 +148,6 @@ QCocoaWindow::QCocoaWindow(QWindow *win, WId nativeHandle)
     , m_inSetVisible(false)
     , m_inSetGeometry(false)
     , m_inSetStyleMask(false)
-#ifndef QT_NO_OPENGL
-    , m_glContext(nullptr)
-#endif
     , m_menubar(nullptr)
     , m_needsInvalidateShadow(false)
     , m_hasModalSession(false)
@@ -403,10 +397,6 @@ void QCocoaWindow::setVisible(bool visible)
             [m_view setHidden:NO];
     } else {
         // qDebug() << "close" << this;
-#ifndef QT_NO_OPENGL
-        if (m_glContext)
-            m_glContext->windowWasHidden();
-#endif
         QCocoaEventDispatcher *cocoaEventDispatcher = qobject_cast<QCocoaEventDispatcher *>(QGuiApplication::instance()->eventDispatcher());
         QCocoaEventDispatcherPrivate *cocoaEventDispatcherPrivate = nullptr;
         if (cocoaEventDispatcher)
@@ -1334,18 +1324,6 @@ bool QCocoaWindow::windowIsPopupType(Qt::WindowType type) const
     return ((type & Qt::Popup) == Qt::Popup);
 }
 
-#ifndef QT_NO_OPENGL
-void QCocoaWindow::setCurrentContext(QCocoaGLContext *context)
-{
-    m_glContext = context;
-}
-
-QCocoaGLContext *QCocoaWindow::currentContext() const
-{
-    return m_glContext;
-}
-#endif
-
 /*!
     Checks if the window is the content view of its immediate NSWindow.