Some threading improvements to the GL module

 - Make bindTexture threadsafe
 - Document which areas of QGLWidget needs locking in order
   to do out-of-gui thread rendering
 - Testcases to prove that these cases work

Reviewed-by: Trond

3 files changed, 56 insertions, 18 deletions

diff --git a/src/opengl/qgl.cpp b/src/opengl/qgl.cpp
index cfcbbf8..f3c66f2 100644
--- a/src/opengl/qgl.cpp
+++ b/src/opengl/qgl.cpp
@@ -1708,6 +1708,7 @@ QGLTextureCache::~QGLTextureCache()
 
 void QGLTextureCache::insert(QGLContext* ctx, qint64 key, QGLTexture* texture, int cost)
 {
+    QWriteLocker locker(&m_lock);
     if (m_cache.totalCost() + cost > m_cache.maxCost()) {
         // the cache is full - make an attempt to remove something
         const QList<qint64> keys = m_cache.keys();
@@ -1725,6 +1726,7 @@ void QGLTextureCache::insert(QGLContext* ctx, qint64 key, QGLTexture* texture, i
 
 bool QGLTextureCache::remove(QGLContext* ctx, GLuint textureId)
 {
+    QWriteLocker locker(&m_lock);
     QList<qint64> keys = m_cache.keys();
     for (int i = 0; i < keys.size(); ++i) {
         QGLTexture *tex = m_cache.object(keys.at(i));
@@ -1739,6 +1741,7 @@ bool QGLTextureCache::remove(QGLContext* ctx, GLuint textureId)
 
 void QGLTextureCache::removeContextTextures(QGLContext* ctx)
 {
+    QWriteLocker locker(&m_lock);
     QList<qint64> keys = m_cache.keys();
     for (int i = 0; i < keys.size(); ++i) {
         const qint64 &key = keys.at(i);
@@ -1761,9 +1764,6 @@ QGLTextureCache* QGLTextureCache::instance()
 */
 void QGLTextureCache::imageCleanupHook(qint64 cacheKey)
 {
-    // ### remove when the GL texture cache becomes thread-safe
-    if (qApp->thread() != QThread::currentThread())
-        return;
     QGLTexture *texture = instance()->getTexture(cacheKey);
     if (texture && texture->options & QGLContext::MemoryManagedBindOption)
         instance()->remove(cacheKey);
@@ -1772,13 +1772,10 @@ void QGLTextureCache::imageCleanupHook(qint64 cacheKey)
 
 void QGLTextureCache::cleanupTextures(QPixmapData* pmd)
 {
-    // ### remove when the GL texture cache becomes thread-safe
-    if (qApp->thread() == QThread::currentThread()) {
-        const qint64 cacheKey = pmd->cacheKey();
-        QGLTexture *texture = instance()->getTexture(cacheKey);
-        if (texture && texture->options & QGLContext::MemoryManagedBindOption)
-            instance()->remove(cacheKey);
-    }
+    const qint64 cacheKey = pmd->cacheKey();
+    QGLTexture *texture = instance()->getTexture(cacheKey);
+    if (texture && texture->options & QGLContext::MemoryManagedBindOption)
+       instance()->remove(cacheKey);
 }
 
 void QGLTextureCache::cleanupBeforePixmapDestruction(QPixmapData* pmd)
@@ -2393,7 +2390,7 @@ QGLTexture* QGLContextPrivate::bindTexture(const QImage &image, GLenum target, G
 #ifndef QT_NO_DEBUG
     GLenum error = glGetError();
     if (error != GL_NO_ERROR) {
-        qWarning(" - texture upload failed, error code 0x%x\n", error);
+        qWarning(" - texture upload failed, error code 0x%x, enum: %d (%x)\n", error, target, target);
     }
 #endif
 
@@ -3292,6 +3289,7 @@ void QGLContextPrivate::setCurrentContext(QGLContext *context)
 */
 
 
+
 /*****************************************************************************
   QGLWidget implementation
  *****************************************************************************/
@@ -3412,6 +3410,15 @@ void QGLContextPrivate::setCurrentContext(QGLContext *context)
     One approach to doing this is shown in the
     \l{Overpainting Example}{Overpainting} example.
 
+    \section1 Threading
+
+    It is possible to render into a QGLWidget from another thread, but it
+    requires that all access to the GL context is safe guarded. The Qt GUI
+    thread will try to use the context in resizeEvent and paintEvent, so in
+    order for threaded rendering using a GL widget to work, these functions
+    need to be intercepted in the GUI thread and handled accordingly in the
+    application.
+
     \e{OpenGL is a trademark of Silicon Graphics, Inc. in the United States and other
     countries.}
 
diff --git a/src/opengl/qgl_p.h b/src/opengl/qgl_p.h
index efd2090..5c4638e 100644
--- a/src/opengl/qgl_p.h
+++ b/src/opengl/qgl_p.h
@@ -528,14 +528,14 @@ public:
     ~QGLTextureCache();
 
     void insert(QGLContext *ctx, qint64 key, QGLTexture *texture, int cost);
-    void remove(quint64 key) { m_cache.remove(key); }
+    inline void remove(quint64 key);
+    inline int size();
+    inline void setMaxCost(int newMax);
+    inline int maxCost();
+    inline QGLTexture* getTexture(quint64 key);
+
     bool remove(QGLContext *ctx, GLuint textureId);
     void removeContextTextures(QGLContext *ctx);
-    int size() { return m_cache.size(); }
-    void setMaxCost(int newMax) { m_cache.setMaxCost(newMax); }
-    int maxCost() {return m_cache.maxCost(); }
-    QGLTexture* getTexture(quint64 key) { return m_cache.object(key); }
-
     static QGLTextureCache *instance();
     static void deleteIfEmpty();
     static void imageCleanupHook(qint64 cacheKey);
@@ -544,8 +544,38 @@ public:
 
 private:
     QCache<qint64, QGLTexture> m_cache;
+    QReadWriteLock m_lock;
 };
 
+int QGLTextureCache::size() {
+    QReadLocker locker(&m_lock);
+    return m_cache.size();
+}
+
+void QGLTextureCache::setMaxCost(int newMax)
+{
+    QWriteLocker locker(&m_lock);
+    m_cache.setMaxCost(newMax);
+}
+
+int QGLTextureCache::maxCost()
+{
+    QReadLocker locker(&m_lock);
+    return m_cache.maxCost();
+}
+
+QGLTexture* QGLTextureCache::getTexture(quint64 key)
+{
+    QReadLocker locker(&m_lock);
+    return m_cache.object(key);
+}
+
+void QGLTextureCache::remove(quint64 key)
+{
+    QWriteLocker locker(&m_lock);
+    m_cache.remove(key);
+}
+
 
 extern Q_OPENGL_EXPORT QPaintEngine* qt_qgl_paint_engine();
 
diff --git a/src/opengl/qgl_win.cpp b/src/opengl/qgl_win.cpp
index b5f952e..5ab944a 100644
--- a/src/opengl/qgl_win.cpp
+++ b/src/opengl/qgl_win.cpp
@@ -1280,8 +1280,9 @@ void QGLContext::reset()
 void QGLContext::makeCurrent()
 {
     Q_D(QGLContext);
-    if (d->rc == wglGetCurrentContext() || !d->valid)        // already current
+    if (d->rc == wglGetCurrentContext() || !d->valid)       // already current
         return;
+
     if (d->win) {
         d->dc = GetDC(d->win);
         if (!d->dc) {