Refactor texture_from_pixmap to not re-create the gl surface each bind

Make a clear seperation between the GL texture and the GLX pixmap. A
GLXPixmap is valid in any GL context and thus does not need to be
re-created every time the pixmap has changed.

Reviewed-By: Samuel

4 files changed, 151 insertions, 88 deletions

diff --git a/src/opengl/qgl.cpp b/src/opengl/qgl.cpp
index 8bb72d5..a8d1797 100644
--- a/src/opengl/qgl.cpp
+++ b/src/opengl/qgl.cpp
@@ -1488,12 +1488,18 @@ void QGLTextureCache::imageCleanupHook(qint64 cacheKey)
 void QGLTextureCache::pixmapCleanupHook(QPixmap* pixmap)
 {
     // ### remove when the GL texture cache becomes thread-safe
-    if (qApp->thread() != QThread::currentThread())
-        return;
-    const qint64 cacheKey = pixmap->cacheKey();
-    QGLTexture *texture = instance()->getTexture(cacheKey);
-    if (texture && texture->clean)
-        instance()->remove(cacheKey);
+    if (qApp->thread() == QThread::currentThread()) {
+        const qint64 cacheKey = pixmap->cacheKey();
+        QGLTexture *texture = instance()->getTexture(cacheKey);
+        if (texture && texture->clean)
+            instance()->remove(cacheKey);
+    }
+#if defined(Q_WS_X11)
+    QPixmapData *pd = pixmap->data_ptr();
+    // Only need to delete the gl surface if the pixmap is about to be deleted
+    if (pd->ref == 0)
+        QGLContextPrivate::destroyGlSurfaceForPixmap(pd);
+#endif
 }
 
 void QGLTextureCache::deleteIfEmpty()
@@ -2036,11 +2042,11 @@ QGLTexture *QGLContextPrivate::bindTexture(const QPixmap &pixmap, GLenum target,
 #if defined(Q_WS_X11)
     // Try to use texture_from_pixmap
     if (pd->classId() == QPixmapData::X11Class) {
-        QPixmap *thatPixmap = const_cast<QPixmap*>(&pixmap);
-        texture = bindTextureFromNativePixmap(thatPixmap, key, canInvert);
+        texture = bindTextureFromNativePixmap(pd, key, canInvert);
         if (texture) {
             texture->clean = clean;
-            boundPixmaps.insert(thatPixmap->data_ptr(), QPixmap(pixmap));
+            texture->boundPixmap = pd;
+            boundPixmaps.insert(pd, QPixmap(pixmap));
         }
     }
 #endif
diff --git a/src/opengl/qgl_p.h b/src/opengl/qgl_p.h
index a83cc63..f21ab93 100644
--- a/src/opengl/qgl_p.h
+++ b/src/opengl/qgl_p.h
@@ -241,7 +241,9 @@ public:
     quint32 gpm;
     int screen;
     QHash<QPixmapData*, QPixmap> boundPixmaps;
-    QGLTexture *bindTextureFromNativePixmap(QPixmap *pm, const qint64 key, bool internal);
+    QGLTexture *bindTextureFromNativePixmap(QPixmapData*, const qint64 key, bool canInvert);
+    static void destroyGlSurfaceForPixmap(QPixmapData*);
+    static void unbindPixmapFromTexture(QPixmapData*);
 #endif
 #if defined(Q_WS_MAC)
     bool update;
@@ -423,7 +425,8 @@ public:
             // is a current context - the context the pixmap was bound to a texture in.
             // Otherwise the release doesn't do anything and you get BadDrawable errors
             // when you come to delete the context.
-            deleteBoundPixmap();
+            if (boundPixmap)
+                QGLContextPrivate::unbindPixmapFromTexture(boundPixmap);
 #endif
             glDeleteTextures(1, &id);
             if (switch_context && current)
@@ -437,9 +440,9 @@ public:
     bool clean;
     bool yInverted; // NOTE: Y-Inverted textures are for internal use only!
 #if defined(Q_WS_X11)
-    Qt::HANDLE boundPixmap;
-    void deleteBoundPixmap(); // in qgl_x11.cpp/qgl_x11egl.cpp
+    QPixmapData* boundPixmap;
 #endif
+
 };
 
 class QGLTextureCache {
diff --git a/src/opengl/qgl_x11.cpp b/src/opengl/qgl_x11.cpp
index 6381bc2..34e9d38 100644
--- a/src/opengl/qgl_x11.cpp
+++ b/src/opengl/qgl_x11.cpp
@@ -1536,38 +1536,26 @@ void QGLExtensions::init()
     }
 }
 
-#if !defined(glXBindTexImageEXT)
+// Solaris defines glXBindTexImageEXT as part of the GL library
+#if defined(GLX_VERSION_1_3) && !defined(Q_OS_HPUX) && !defined(glXBindTexImageEXT)
 typedef void (*qt_glXBindTexImageEXT)(Display*, GLXDrawable, int, const int*);
-static qt_glXBindTexImageEXT glXBindTexImageEXT = 0;
-#endif
-#if !defined(glXReleaseTexImageEXT)
 typedef void (*qt_glXReleaseTexImageEXT)(Display*, GLXDrawable, int);
+static qt_glXBindTexImageEXT glXBindTexImageEXT = 0;
 static qt_glXReleaseTexImageEXT glXReleaseTexImageEXT = 0;
-#endif
-static bool qt_resolved_texture_from_pixmap = false;
 
-QGLTexture *QGLContextPrivate::bindTextureFromNativePixmap(QPixmap *pm, const qint64 key, bool canInvert)
+bool qt_resolveTextureFromPixmap()
 {
-#if !defined(Q_OS_LINUX)
-    return 0;
-#else
-    Q_Q(QGLContext);
-
-    if (pm->data_ptr()->classId() != QPixmapData::X11Class)
-        return 0;
-    QX11PixmapData *pixmapData = static_cast<QX11PixmapData*>(pm->data_ptr());
-    const QX11Info *x11Info = qt_x11Info(pm);
-
-
-    // Check to see if we have NPOT texture support
-    // TODO: Use GLX_TEXTURE_RECTANGLE_EXT texture target on systems without npot textures
-    if ( !(QGLExtensions::glExtensions & QGLExtensions::NPOTTextures) &&
-         !(QGLFormat::openGLVersionFlags() & QGLFormat::OpenGL_Version_2_0))
-        return 0;
+    static bool resolvedTextureFromPixmap = false;
 
+    if (!resolvedTextureFromPixmap) {
+        resolvedTextureFromPixmap = true;
 
-    if (!qt_resolved_texture_from_pixmap) {
-        qt_resolved_texture_from_pixmap = true;
+        // Check to see if we have NPOT texture support
+        if ( !(QGLExtensions::glExtensions & QGLExtensions::NPOTTextures) &&
+             !(QGLFormat::openGLVersionFlags() & QGLFormat::OpenGL_Version_2_0))
+        {
+            return false; // Can't use TFP without NPOT
+        }
 
         QString glxExt = QLatin1String(glXGetClientString(QX11Info::display(), GLX_EXTENSIONS));
         if (glxExt.contains(QLatin1String("GLX_EXT_texture_from_pixmap"))) {
@@ -1589,81 +1577,141 @@ QGLTexture *QGLContextPrivate::bindTextureFromNativePixmap(QPixmap *pm, const qi
         }
     }
 
-    if (!glXBindTexImageEXT)
-        return 0;
+    return glXBindTexImageEXT && glXReleaseTexImageEXT;
+}
+#endif //defined(GLX_VERSION_1_3) && !defined(Q_OS_HPUX) && !defined(glXBindTexImageEXT)
+
 
+QGLTexture *QGLContextPrivate::bindTextureFromNativePixmap(QPixmapData *pmd, const qint64 key, bool canInvert)
+{
 #if !defined(GLX_VERSION_1_3) || defined(Q_OS_HPUX)
     return 0;
 #else
-    GLXFBConfig    *configList = 0;
-    GLXFBConfig     glxPixmapConfig;
-    int             configCount = 0;
-    bool            hasAlpha = pixmapData->hasAlphaChannel();
-
-    int configAttribs[] = {
-        hasAlpha ? GLX_BIND_TO_TEXTURE_RGBA_EXT : GLX_BIND_TO_TEXTURE_RGB_EXT, True,
-        GLX_DRAWABLE_TYPE, GLX_PIXMAP_BIT,
-        GLX_BIND_TO_TEXTURE_TARGETS_EXT, GLX_TEXTURE_2D_BIT_EXT,
-        // QGLContext::bindTexture() can't return an inverted texture, but QPainter::drawPixmap() can:
-        GLX_Y_INVERTED_EXT, canInvert ? GLX_DONT_CARE : False,
-        XNone
-//        GLX_BIND_TO_MIPMAP_TEXTURE_EXT, False,
-//        GLX_BIND_TO_TEXTURE_TARGETS_EXT, GLX_TEXTURE_1D_BIT_EXT or GLX_TEXTURE_2D_BIT_EXT or GLX_TEXTURE_RECTANGLE_BIT_EXT
-    };
-    configList = glXChooseFBConfig(x11Info->display(), x11Info->screen(), configAttribs, &configCount);
-    if (!configList)
-        return 0;
-    glxPixmapConfig = configList[0];
-    XFree(configList);
-
-    GLXPixmap glxPixmap;
-    int pixmapAttribs[] = {
-        GLX_TEXTURE_FORMAT_EXT, hasAlpha ? GLX_TEXTURE_FORMAT_RGBA_EXT : GLX_TEXTURE_FORMAT_RGB_EXT,
-        GLX_TEXTURE_TARGET_EXT, GLX_TEXTURE_2D_EXT,
-        GLX_MIPMAP_TEXTURE_EXT, False,
-        XNone
-//        GLX_TEXTURE_FORMAT_EXT, GLX_TEXTURE_FORMAT_RGBA_EXT or GLX_TEXTURE_FORMAT_RGB_EXT or GLX_TEXTURE_FORMAT_NONE_EXT,
-//        GLX_TEXTURE_TARGET_EXT, GLX_TEXTURE_2D_EXT or GLX_TEXTURE_RECTANGLE_EXT,
-//        GLX_MIPMAP_TEXTURE_EXT, True or False,
-    };
+    Q_Q(QGLContext);
 
-    // Wrap the X Pixmap into a GLXPixmap:
-    glxPixmap = glXCreatePixmap(x11Info->display(), glxPixmapConfig, pixmapData->handle(), pixmapAttribs);
+    Q_ASSERT(pmd->classId() == QPixmapData::X11Class);
 
-    if (!glxPixmap)
+#if !defined(glXBindTexImageEXT)
+    if (!qt_resolveTextureFromPixmap())
         return 0;
+#endif
 
-    int yInverted;
-    glXGetFBConfigAttrib(x11Info->display(), glxPixmapConfig, GLX_Y_INVERTED_EXT, &yInverted);
+    QX11PixmapData *pixmapData = static_cast<QX11PixmapData*>(pmd);
+    const QX11Info &x11Info = pixmapData->xinfo;
+
+    // Store the configs (Can be static because configs aren't dependent on current context)
+    static GLXFBConfig glxRGBPixmapConfig = 0;
+    static bool RGBConfigInverted = false;
+    static GLXFBConfig glxRGBAPixmapConfig = 0;
+    static bool RGBAConfigInverted = false;
+
+    bool hasAlpha = pixmapData->hasAlphaChannel();
+
+    // Check to see if we need a config
+    if ( (hasAlpha && !glxRGBAPixmapConfig) || (!hasAlpha && !glxRGBPixmapConfig) ) {
+        GLXFBConfig    *configList = 0;
+        int             configCount = 0;
+
+        int configAttribs[] = {
+            hasAlpha ? GLX_BIND_TO_TEXTURE_RGBA_EXT : GLX_BIND_TO_TEXTURE_RGB_EXT, True,
+            GLX_DRAWABLE_TYPE, GLX_PIXMAP_BIT,
+            GLX_BIND_TO_TEXTURE_TARGETS_EXT, GLX_TEXTURE_2D_BIT_EXT,
+            // QGLContext::bindTexture() can't return an inverted texture, but QPainter::drawPixmap() can:
+            GLX_Y_INVERTED_EXT, canInvert ? GLX_DONT_CARE : False,
+            XNone
+        };
+        configList = glXChooseFBConfig(x11Info.display(), x11Info.screen(), configAttribs, &configCount);
+        if (!configList)
+            return 0;
+
+        int yInv;
+        glXGetFBConfigAttrib(x11Info.display(), configList[0], GLX_Y_INVERTED_EXT, &yInv);
+
+        if (hasAlpha) {
+            glxRGBAPixmapConfig = configList[0];
+            RGBAConfigInverted = yInv;
+        }
+        else {
+            glxRGBPixmapConfig = configList[0];
+            RGBConfigInverted = yInv;
+        }
+
+        XFree(configList);
+    }
+
+    // Check to see if the surface is still valid
+    if (pixmapData->gl_surface &&
+        hasAlpha != (pixmapData->flags & QX11PixmapData::GlSurfaceCreatedWithAlpha))
+    {
+        // Surface is invalid!
+        destroyGlSurfaceForPixmap(pixmapData);
+    }
+
+    // Check to see if we need a surface
+    if (!pixmapData->gl_surface) {
+        GLXPixmap glxPixmap;
+        int pixmapAttribs[] = {
+            GLX_TEXTURE_FORMAT_EXT, hasAlpha ? GLX_TEXTURE_FORMAT_RGBA_EXT : GLX_TEXTURE_FORMAT_RGB_EXT,
+            GLX_TEXTURE_TARGET_EXT, GLX_TEXTURE_2D_EXT,
+            GLX_MIPMAP_TEXTURE_EXT, False, // Maybe needs to be don't care
+            XNone
+        };
+
+        // Wrap the X Pixmap into a GLXPixmap:
+        glxPixmap = glXCreatePixmap(x11Info.display(),
+                                    hasAlpha ? glxRGBAPixmapConfig : glxRGBPixmapConfig,
+                                    pixmapData->handle(), pixmapAttribs);
+
+        if (!glxPixmap)
+            return 0;
+
+        pixmapData->gl_surface = (Qt::HANDLE)glxPixmap;
+
+        // Make sure the cleanup hook gets called so we can delete the glx pixmap
+        pixmapData->is_cached = true;
+    }
 
     GLuint textureId;
     glGenTextures(1, &textureId);
     glBindTexture(GL_TEXTURE_2D, textureId);
-    glXBindTexImageEXT(x11Info->display(), glxPixmap, GLX_FRONT_LEFT_EXT, 0);
+    glXBindTexImageEXT(x11Info.display(), (GLXPixmap)pixmapData->gl_surface, GLX_FRONT_LEFT_EXT, 0);
 
     glBindTexture(GL_TEXTURE_2D, textureId);
 
-    QGLTexture *texture = new QGLTexture(q, textureId, GL_TEXTURE_2D, canInvert, yInverted);
-    texture->boundPixmap = glxPixmap;
+    QGLTexture *texture = new QGLTexture(q, textureId, GL_TEXTURE_2D, canInvert, false);
+    texture->yInverted = (hasAlpha && RGBAConfigInverted) || (!hasAlpha && RGBConfigInverted);
+    if (texture->yInverted)
+        pixmapData->flags |= QX11PixmapData::InvertedWhenBoundToTexture;
 
     // We assume the cost of bound pixmaps is zero
     QGLTextureCache::instance()->insert(q, key, texture, 0);
 
     return texture;
 #endif //!defined(GLX_VERSION_1_3) || defined(Q_OS_HPUX)
-#endif //!defined(Q_OS_LINUX
 }
 
-void QGLTexture::deleteBoundPixmap()
+
+void QGLContextPrivate::destroyGlSurfaceForPixmap(QPixmapData* pmd)
 {
-#if defined(GLX_VERSION_1_3) && !defined(Q_OS_HPUX) && defined(Q_OS_LINUX)
-    if (boundPixmap) {
-        glXReleaseTexImageEXT(QX11Info::display(), boundPixmap, GLX_FRONT_LEFT_EXT);
-        glXDestroyPixmap(QX11Info::display(), boundPixmap);
-        boundPixmap = 0;
+#if defined(GLX_VERSION_1_3) && !defined(Q_OS_HPUX)
+    Q_ASSERT(pmd->classId() == QPixmapData::X11Class);
+    QX11PixmapData *pixmapData = static_cast<QX11PixmapData*>(pmd);
+    if (pixmapData->gl_surface) {
+        glXDestroyPixmap(QX11Info::display(), (GLXPixmap)pixmapData->gl_surface);
+        pixmapData->gl_surface = 0;
     }
 #endif
 }
 
+void QGLContextPrivate::unbindPixmapFromTexture(QPixmapData* pmd)
+{
+#if defined(GLX_VERSION_1_3) && !defined(Q_OS_HPUX)
+    Q_ASSERT(pmd->classId() == QPixmapData::X11Class);
+    Q_ASSERT(QGLContext::currentContext());
+    QX11PixmapData *pixmapData = static_cast<QX11PixmapData*>(pmd);
+    if (pixmapData->gl_surface)
+        glXReleaseTexImageEXT(QX11Info::display(), (GLXPixmap)pixmapData->gl_surface, GLX_FRONT_LEFT_EXT);
+#endif
+}
 
 QT_END_NAMESPACE
diff --git a/src/opengl/qgl_x11egl.cpp b/src/opengl/qgl_x11egl.cpp
index c6904fe..6b90a4f 100644
--- a/src/opengl/qgl_x11egl.cpp
+++ b/src/opengl/qgl_x11egl.cpp
@@ -486,13 +486,19 @@ void QGLWidgetPrivate::recreateEglSurface(bool force)
     }
 }
 
-QGLTexture *QGLContextPrivate::bindTextureFromNativePixmap(QPixmap *pm, const qint64 key, bool canInvert)
+
+QGLTexture *QGLContextPrivate::bindTextureFromNativePixmap(QPixmapData*, const qint64 key, bool canInvert)
 {
     // TODO
     return 0;
 }
 
-void QGLTexture::deleteBoundPixmap()
+void QGLContextPrivate::destroyGlSurfaceForPixmap(QPixmapData*)
+{
+    //TODO
+}
+
+void QGLContextPrivate::unbindPixmapFromTexture(QPixmapData*)
 {
     //TODO
 }