From c4d715260bd0ed5f3b6d38a63a2659715342c90a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20R=C3=B8dal?= Date: Fri, 29 Oct 2010 13:37:43 +0200 Subject: Prevented threading related crash in OpenGL module. If the last shallow copy of a QImage which is cached in the QGLTextureCache is destroyed in a thread at the same time as the QGLContext which the texture was initialized in is active in a different thread, the QImage thread incorrectly tries to make the context active in two threads at once. To prevent this from happening it's best to always do the texture clean-up in the main thread. Task-number: QT-4238 Reviewed-by: Eskil Abrahamsen Blomfeldt --- src/opengl/qgl.cpp | 7 ++- src/opengl/qgl_p.h | 68 +++++++++++++++++--------- tests/auto/qgl/tst_qgl.cpp | 118 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+), 24 deletions(-) diff --git a/src/opengl/qgl.cpp b/src/opengl/qgl.cpp index dbd295f..84cfa97 100644 --- a/src/opengl/qgl.cpp +++ b/src/opengl/qgl.cpp @@ -127,7 +127,12 @@ Q_GLOBAL_STATIC(QGLDefaultOverlayFormat, defaultOverlayFormatInstance) Q_GLOBAL_STATIC(QGLSignalProxy, theSignalProxy) QGLSignalProxy *QGLSignalProxy::instance() { - return theSignalProxy(); + QGLSignalProxy *proxy = theSignalProxy(); + if (proxy && proxy->thread() != qApp->thread()) { + if (proxy->thread() == QThread::currentThread()) + proxy->moveToThread(qApp->thread()); + } + return proxy; } diff --git a/src/opengl/qgl_p.h b/src/opengl/qgl_p.h index f86c77f..18b2765 100644 --- a/src/opengl/qgl_p.h +++ b/src/opengl/qgl_p.h @@ -434,20 +434,6 @@ public: static void setCurrentContext(QGLContext *context); }; -// ### make QGLContext a QObject in 5.0 and remove the proxy stuff -class Q_OPENGL_EXPORT QGLSignalProxy : public QObject -{ - Q_OBJECT -public: - QGLSignalProxy() : QObject() {} - void emitAboutToDestroyContext(const QGLContext *context) { - emit aboutToDestroyContext(context); - } - static QGLSignalProxy *instance(); -Q_SIGNALS: - void aboutToDestroyContext(const QGLContext *context); -}; - Q_DECLARE_OPERATORS_FOR_FLAGS(QGLExtensions::Extensions) // Temporarily make a context current if not already current or @@ -490,6 +476,48 @@ private: QGLContext *m_ctx; }; +// ### make QGLContext a QObject in 5.0 and remove the proxy stuff +class Q_OPENGL_EXPORT QGLSignalProxy : public QObject +{ + Q_OBJECT +public: + QGLSignalProxy() : QObject() { + qRegisterMetaType("GLuint"); + connect(this, SIGNAL(freeTexture(QGLContext *, QPixmapData *, GLuint)), + this, SLOT(freeTexture_slot(QGLContext *, QPixmapData *, GLuint))); + } + void emitAboutToDestroyContext(const QGLContext *context) { + emit aboutToDestroyContext(context); + } + void emitFreeTexture(QGLContext *context, QPixmapData *boundPixmap, GLuint id) { + emit freeTexture(context, boundPixmap, id); + } + static QGLSignalProxy *instance(); +Q_SIGNALS: + void aboutToDestroyContext(const QGLContext *context); + void freeTexture(QGLContext *context, QPixmapData *boundPixmap, GLuint id); + +private slots: + void freeTexture_slot(QGLContext *context, QPixmapData *boundPixmap, GLuint id) { +#if defined(Q_WS_X11) + if (boundPixmap) { + QGLContext *oldContext = const_cast(QGLContext::currentContext()); + context->makeCurrent(); + // Although glXReleaseTexImage is a glX call, it must be called while there + // 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. + QGLContextPrivate::unbindPixmapFromTexture(boundPixmap); + glDeleteTextures(1, &id); + oldContext->makeCurrent(); + return; + } +#endif + QGLShareContextScope scope(context); + glDeleteTextures(1, &id); + } +}; + class QGLTexture { public: QGLTexture(QGLContext *ctx = 0, GLuint tx_id = 0, GLenum tx_target = GL_TEXTURE_2D, @@ -506,16 +534,10 @@ public: ~QGLTexture() { if (options & QGLContext::MemoryManagedBindOption) { Q_ASSERT(context); - QGLShareContextScope scope(context); -#if defined(Q_WS_X11) - // Although glXReleaseTexImage is a glX call, it must be called while there - // 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. - if (boundPixmap) - QGLContextPrivate::unbindPixmapFromTexture(boundPixmap); +#if !defined(Q_WS_X11) + QPixmapData *boundPixmap = 0; #endif - glDeleteTextures(1, &id); + QGLSignalProxy::instance()->emitFreeTexture(context, boundPixmap, id); } } diff --git a/tests/auto/qgl/tst_qgl.cpp b/tests/auto/qgl/tst_qgl.cpp index e411508..4220b45 100644 --- a/tests/auto/qgl/tst_qgl.cpp +++ b/tests/auto/qgl/tst_qgl.cpp @@ -96,6 +96,7 @@ private slots: void shareRegister(); void qglContextDefaultBindTexture(); void textureCleanup(); + void threadImages(); }; tst_QGL::tst_QGL() @@ -2253,6 +2254,123 @@ void tst_QGL::textureCleanup() #endif } +namespace ThreadImages { + +class Producer : public QObject +{ + Q_OBJECT +public: + Producer() + { + startTimer(20); + + QThread *thread = new QThread; + thread->start(); + + connect(this, SIGNAL(destroyed()), thread, SLOT(quit())); + + moveToThread(thread); + connect(thread, SIGNAL(finished()), thread, SLOT(deleteLater())); + } + +signals: + void imageReady(const QImage &image); + +protected: + void timerEvent(QTimerEvent *) + { + QImage image(256, 256, QImage::Format_RGB32); + QLinearGradient g(0, 0, 0, 256); + g.setColorAt(0, QColor(255, 180, 180)); + g.setColorAt(1, Qt::white); + g.setSpread(QGradient::ReflectSpread); + + QBrush brush(g); + brush.setTransform(QTransform::fromTranslate(0, delta)); + delta += 10; + + QPainter p(&image); + p.fillRect(image.rect(), brush); + + if (images.size() > 10) + images.removeFirst(); + + images.append(image); + + emit imageReady(image); + } + +private: + QList images; + int delta; +}; + + +class DisplayWidget : public QGLWidget +{ + Q_OBJECT +public: + DisplayWidget(QWidget *parent) : QGLWidget(parent) {} + void paintEvent(QPaintEvent *) + { + QPainter p(this); + p.drawImage(rect(), m_image); + } + +public slots: + void setImage(const QImage &image) + { + m_image = image; + update(); + } + +private: + QImage m_image; +}; + +class Widget : public QWidget +{ + Q_OBJECT +public: + Widget() + : iterations(0) + , display(0) + { + startTimer(400); + } + + int iterations; + +protected: + void timerEvent(QTimerEvent *) + { + ++iterations; + + delete display; + display = new DisplayWidget(this); + connect(&producer, SIGNAL(imageReady(const QImage &)), display, SLOT(setImage(const QImage &))); + + display->setGeometry(rect()); + display->show(); + } + +private: + Producer producer; + DisplayWidget *display; +}; + +} + +void tst_QGL::threadImages() +{ + ThreadImages::Widget *widget = new ThreadImages::Widget; + widget->show(); + + while (widget->iterations <= 5) { + qApp->processEvents(); + } +} + class tst_QGLDummy : public QObject { Q_OBJECT -- cgit v0.12