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

This reverts commit 823acb069d.

It caused some test failures in qtdeclarative and etc.

Task-number: QTBUG-69891
Change-Id: I2e4038a46de254834e6389c63f6dad0c2e523b8e
Reviewed-by: Tor Arne Vestbø <tor.arne.vestbo@qt.io>

src/plugins/platforms/cocoa/qcocoaglcontext.h

@@ -41,9 +41,6 @@
 #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>
@@ -68,6 +65,8 @@ public:
     bool isSharing() const override;
     bool isValid() const override;
 
+    void windowWasHidden();
+
     NSOpenGLContext *nativeContext() const;
 
     QFunctionPointer getProcAddress(const char *procName) override;
@@ -75,14 +74,14 @@ public:
 private:
     static NSOpenGLPixelFormat *pixelFormatForSurfaceFormat(const QSurfaceFormat &format);
 
-    bool setDrawable(QPlatformSurface *surface);
+    bool setActiveWindow(QWindow *window);
     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

src/plugins/platforms/cocoa/qcocoaglcontext.mm

@@ -41,8 +41,6 @@
 #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>
@@ -322,6 +320,9 @@ void QCocoaGLContext::updateSurfaceFormat()
 
 QCocoaGLContext::~QCocoaGLContext()
 {
+    if (m_currentWindow && m_currentWindow.data()->handle())
+        static_cast<QCocoaWindow *>(m_currentWindow.data()->handle())->setCurrentContext(0);
+
     [m_context release];
 }
 
@@ -330,14 +331,6 @@ 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) {
@@ -345,14 +338,11 @@ bool QCocoaGLContext::makeCurrent(QPlatformSurface *surface)
         return true;
     }
 
-    if (!setDrawable(surface))
+    QWindow *window = static_cast<QCocoaWindow *>(surface)->window();
+    if (!setActiveWindow(window)) {
+        qCDebug(lcQpaOpenGLContext) << "Failed to activate window, skipping makeCurrent";
         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];
 
@@ -373,74 +363,43 @@ 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)
 {
-    Q_ASSERT(surface->surface()->surfaceClass() == QSurface::Window);
+    if (window == m_currentWindow.data())
-    NSView *view = static_cast<QCocoaWindow *>(surface)->view();
-
-    if (view == m_context.view)
         return true;
 
-    m_observers.clear();
+    Q_ASSERT(window->handle());
+    QCocoaWindow *cocoaWindow = static_cast<QCocoaWindow *>(window->handle());
+    NSView *view = cocoaWindow->view();
 
     if ((m_context.view = view) != view) {
-        qCInfo(lcQpaOpenGLContext) << "Failed to set" << view << "as drawable for" << m_context;
+        qCDebug(lcQpaOpenGLContext) << "Associating" << view << "with" << m_context << "failed";
         return false;
     }
 
-    qCInfo(lcQpaOpenGLContext) << "Set drawable for" << m_context << "to" << m_context.view;
+    qCDebug(lcQpaOpenGLContext) << m_context << "now associated with" << 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_currentWindow = window;
-        m_observers.append(QMacScopedObserver(view, NSViewFrameDidChangeNotification, updateCallback));
-        m_observers.append(QMacScopedObserver(view.window, NSWindowDidChangeScreenNotification, updateCallback));
-    } else {
-        m_observers.append(QMacScopedObserver(view, NSViewGlobalFrameDidChangeNotification, updateCallback));
-    }
-
-    m_observers.append(QMacScopedObserver([NSApplication sharedApplication],
-        NSApplicationDidChangeScreenParametersNotification, updateCallback));
 
+    cocoaWindow->setCurrentContext(this);
     return true;
 }
 
-// NSOpenGLContext is not re-entrant, which means that even when using separate
+// NSOpenGLContext is not re-entrant (https://openradar.appspot.com/37064579)
-// 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)
@@ -451,52 +410,37 @@ void QCocoaGLContext::swapBuffers(QPlatformSurface *surface)
     if (surface->surface()->surfaceClass() == QSurface::Offscreen)
         return; // Nothing to do
 
-    if (!setDrawable(surface)) {
+    QWindow *window = static_cast<QCocoaWindow *>(surface)->window();
-        qCWarning(lcQpaOpenGLContext) << "Can't flush" << m_context
+    if (!setActiveWindow(window)) {
-            << "without" << surface << "as drawable";
+        qCWarning(lcQpaOpenGLContext) << "Failed to activate window, skipping swapBuffers";
         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()
 {
-    auto currentContext = QOpenGLContext::currentContext();
-    if (!currentContext)
-        return;
-
-    // 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;
-    }
-
-    Q_ASSERT([NSOpenGLContext currentContext] == m_context);
-
     qCDebug(lcQpaOpenGLContext) << "Clearing current context"
         << [NSOpenGLContext currentContext] << "in" << QThread::currentThread();
 
+    if (m_currentWindow && m_currentWindow.data()->handle())
+        static_cast<QCocoaWindow *>(m_currentWindow.data()->handle())->setCurrentContext(nullptr);
+
+    m_currentWindow.clear();
+
     [NSOpenGLContext clearCurrentContext];
-    CGLUnlockContext(m_context.CGLContextObj);
+}
+
+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();
 }
 
 QSurfaceFormat QCocoaGLContext::format() const

src/plugins/platforms/cocoa/qcocoanativeinterface.mm

@@ -102,6 +102,10 @@ 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)

src/plugins/platforms/cocoa/qcocoawindow.h

@@ -169,6 +169,11 @@ 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;
@@ -248,6 +253,9 @@ 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;

src/plugins/platforms/cocoa/qcocoawindow.mm

@@ -41,6 +41,9 @@
 #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"
@@ -148,6 +151,9 @@ 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)
@@ -397,6 +403,10 @@ 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)
@@ -1324,6 +1334,18 @@ 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.