From 8e517b1578ad1e5988f440790f5048416a7dd68c Mon Sep 17 00:00:00 2001 From: David Edmundson Date: Wed, 31 Aug 2016 07:44:28 +0100 Subject: [PATCH] WindowThumbnail: Do GL calls in the correct thread WindowThumbnail did some open GL operations, discarding old textures, in the GUI thread. Whislt it's not going to cause a threading issue (as updatePaintNode always ran when the main thread was blocked) we're not meant to mix threads with openGL contexts. It also seems to have a GL leak on nvidia, which was previously masked by the double delete fixed in https://git.reviewboard.kde.org/r/126131/diff/2/ It seems only one worked, and in the applied version we went with the wrong one. This patch makes use of QQuickItem::releaseResources to delete the GL textures on window change and destructor; it's then removed from stopRedirecting so that start/stop redirecting handles xcb on the GUI thread and updatePaintNode/discardPixmap is the GL stuff on the render thread. See http://doc.qt.io/qt-5/qquickitem.html#graphics-resource-handling REVIEW: 128763 BUG: 368066 --- .../core/windowthumbnail.cpp | 158 +++++++++++++----- src/declarativeimports/core/windowthumbnail.h | 6 +- 2 files changed, 119 insertions(+), 45 deletions(-) diff --git a/src/declarativeimports/core/windowthumbnail.cpp b/src/declarativeimports/core/windowthumbnail.cpp index d10699431..ae59f5424 100644 --- a/src/declarativeimports/core/windowthumbnail.cpp +++ b/src/declarativeimports/core/windowthumbnail.cpp @@ -24,6 +24,8 @@ #include #include #include +#include + // X11 #if HAVE_XCB_COMPOSITE #include @@ -44,6 +46,72 @@ typedef GLvoid(*glEGLImageTargetTexture2DOES_func)(GLenum, GLeglImageOES); namespace Plasma { +#if HAVE_XCB_COMPOSITE +#if HAVE_GLX +class DiscardGlxPixmapRunnable : public QRunnable { +public: + DiscardGlxPixmapRunnable( + uint, + QFunctionPointer, + xcb_pixmap_t + ); + void run() Q_DECL_OVERRIDE; +private: + uint m_texture; + QFunctionPointer m_releaseTexImage; + xcb_pixmap_t m_glxPixmap; +}; + +DiscardGlxPixmapRunnable::DiscardGlxPixmapRunnable(uint texture, QFunctionPointer deleteFunction, xcb_pixmap_t pixmap) + : QRunnable(), + m_texture(texture), + m_releaseTexImage(deleteFunction), + m_glxPixmap(pixmap) +{} + +void DiscardGlxPixmapRunnable::run() +{ + if (m_glxPixmap != XCB_PIXMAP_NONE) { + Display *d = QX11Info::display(); + ((glXReleaseTexImageEXT_func)(m_releaseTexImage))(d, m_glxPixmap, GLX_FRONT_LEFT_EXT); + glXDestroyPixmap(d, m_glxPixmap); + glDeleteTextures(1, &m_texture); + } +} +#endif //HAVE_GLX + +#if HAVE_EGL +class DiscardEglPixmapRunnable : public QRunnable { +public: + DiscardEglPixmapRunnable( + uint, + QFunctionPointer, + EGLImageKHR + ); + void run() Q_DECL_OVERRIDE; +private: + uint m_texture; + QFunctionPointer m_eglDestroyImageKHR; + EGLImageKHR m_image; +}; + +DiscardEglPixmapRunnable::DiscardEglPixmapRunnable(uint texture, QFunctionPointer deleteFunction, EGLImageKHR image) + : QRunnable(), + m_texture(texture), + m_eglDestroyImageKHR(deleteFunction), + m_image(image) +{} + +void DiscardEglPixmapRunnable::run() +{ + if (m_image != EGL_NO_IMAGE_KHR) { + ((eglDestroyImageKHR_func)(m_eglDestroyImageKHR))(eglGetCurrentDisplay(), m_image); + glDeleteTextures(1, &m_texture); + } +} +#endif//HAVE_EGL +#endif //HAVE_XCB_COMPOSITE + WindowTextureNode::WindowTextureNode() : QSGSimpleTextureNode() { @@ -101,6 +169,7 @@ WindowThumbnail::WindowThumbnail(QQuickItem *parent) connect(this, &QQuickItem::enabledChanged, [this]() { if (!isEnabled()) { stopRedirecting(); + releaseResources(); } else if (isVisible()) { startRedirecting(); } @@ -108,6 +177,7 @@ WindowThumbnail::WindowThumbnail(QQuickItem *parent) connect(this, &QQuickItem::visibleChanged, [this]() { if (!isVisible()) { stopRedirecting(); + releaseResources(); } else if (isEnabled()) { startRedirecting(); } @@ -138,10 +208,48 @@ WindowThumbnail::~WindowThumbnail() if (m_xcb) { QCoreApplication::instance()->removeNativeEventFilter(this); stopRedirecting(); - discardPixmap(); } } +void WindowThumbnail::releaseResources() +{ +#if HAVE_XCB_COMPOSITE + +#if HAVE_GLX && HAVE_EGL + //only one (or none) should be set, but never both + Q_ASSERT(m_glxPixmap == XCB_PIXMAP_NONE || m_image == EGL_NO_IMAGE_KHR); +#endif + + //data is deleted in the render thread (with relevant GLX calls) + //note runnable may be called *after* this is deleted + //but the pointer is held by the WindowThumbnail which is in the main thread +#if HAVE_GLX + if (m_glxPixmap != XCB_PIXMAP_NONE) { + window()->scheduleRenderJob(new DiscardGlxPixmapRunnable(m_texture, + m_releaseTexImage, + m_glxPixmap), + QQuickWindow::NoStage); + + m_glxPixmap = XCB_PIXMAP_NONE; + m_texture = 0; + } +#endif +#if HAVE_EGL + if (m_image != EGL_NO_IMAGE_KHR) { + window()->scheduleRenderJob(new DiscardEglPixmapRunnable(m_texture, + m_eglDestroyImageKHR, + m_image), + QQuickWindow::NoStage); + + m_image = EGL_NO_IMAGE_KHR; + m_texture = 0; + } +#endif +#endif +} + + + uint32_t WindowThumbnail::winId() const { return m_winId; @@ -227,8 +335,9 @@ bool WindowThumbnail::nativeEventFilter(const QByteArray &eventType, void *messa } } else if (responseType == XCB_CONFIGURE_NOTIFY) { if (reinterpret_cast(event)->window == m_winId) { - // TODO: only discard if size changes - discardPixmap(); + releaseResources(); + m_damaged = true; + update(); } } #else @@ -373,16 +482,7 @@ void WindowThumbnail::windowToTexture(WindowTextureNode *textureNode) if (!textureNode->texture()) { // the texture got discarded by the scene graph, but our mapping is still valid // let's discard the pixmap to have a clean state again -#if HAVE_GLX - if (m_glxPixmap != XCB_PIXMAP_NONE) { - discardPixmap(); - } -#endif -#if HAVE_EGL - if (m_image != EGL_NO_IMAGE_KHR) { - discardPixmap(); - } -#endif + releaseResources(); } if (m_pixmap == XCB_PIXMAP_NONE) { m_pixmap = pixmapForWindow(); @@ -592,7 +692,6 @@ void WindowThumbnail::stopRedirecting() return; } xcb_composite_unredirect_window(c, m_winId, XCB_COMPOSITE_REDIRECT_AUTOMATIC); - discardPixmap(); if (m_damage == XCB_NONE) { return; } @@ -635,36 +734,7 @@ void WindowThumbnail::startRedirecting() #endif } -void WindowThumbnail::discardPixmap() -{ - if (!m_xcb) { - return; - } -#if HAVE_XCB_COMPOSITE -#if HAVE_GLX - if (m_glxPixmap != XCB_PIXMAP_NONE) { - Display *d = QX11Info::display(); - ((glXReleaseTexImageEXT_func)(m_releaseTexImage))(d, m_glxPixmap, GLX_FRONT_LEFT_EXT); - glXDestroyPixmap(d, m_glxPixmap); - m_glxPixmap = XCB_PIXMAP_NONE; - glDeleteTextures(1, &m_texture); - } -#endif -#if HAVE_EGL - if (m_image != EGL_NO_IMAGE_KHR) { - ((eglDestroyImageKHR_func)(m_eglDestroyImageKHR))(eglGetCurrentDisplay(), m_image); - m_image = EGL_NO_IMAGE_KHR; - glDeleteTextures(1, &m_texture); - } -#endif - if (m_pixmap != XCB_WINDOW_NONE) { - xcb_free_pixmap(QX11Info::connection(), m_pixmap); - m_pixmap = XCB_PIXMAP_NONE; - } - m_damaged = true; - update(); -#endif -} + void WindowThumbnail::setThumbnailAvailable(bool thumbnailAvailable) { diff --git a/src/declarativeimports/core/windowthumbnail.h b/src/declarativeimports/core/windowthumbnail.h index 7276f95de..780f1d407 100644 --- a/src/declarativeimports/core/windowthumbnail.h +++ b/src/declarativeimports/core/windowthumbnail.h @@ -99,13 +99,15 @@ Q_SIGNALS: void paintedSizeChanged(); void thumbnailAvailableChanged(); +protected: + void releaseResources() Q_DECL_OVERRIDE; + private: void iconToTexture(WindowTextureNode *textureNode); void windowToTexture(WindowTextureNode *textureNode); void startRedirecting(); void stopRedirecting(); void resetDamaged(); - void discardPixmap(); void setThumbnailAvailable(bool thumbnailAvailable); bool m_xcb; @@ -121,6 +123,8 @@ private: uint8_t m_damageEventBase; xcb_damage_damage_t m_damage; xcb_pixmap_t m_pixmap; + +/*The following must *only* be used from the render thread*/ uint m_texture; #if HAVE_GLX bool windowToTextureGLX(WindowTextureNode *textureNode);