From 291d4011d0682712f32b002d0729df4d969f14ca Mon Sep 17 00:00:00 2001 From: Matthew Waters Date: Mon, 3 Feb 2020 15:59:34 +1100 Subject: [PATCH 1/6] qtitem: fix leak of caps --- ext/qt/qtitem.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ext/qt/qtitem.cc b/ext/qt/qtitem.cc index 3c6722abe5..d92635f08a 100644 --- a/ext/qt/qtitem.cc +++ b/ext/qt/qtitem.cc @@ -150,6 +150,8 @@ QtGLVideoItem::~QtGLVideoItem() gst_object_unref(this->priv->other_context); if (this->priv->display) gst_object_unref(this->priv->display); + + gst_caps_replace (&this->priv->caps, NULL); g_free (this->priv); this->priv = NULL; } From 1db83ab4c3f75f91632469043d7a80d213c79424 Mon Sep 17 00:00:00 2001 From: Matthew Waters Date: Tue, 4 Feb 2020 19:40:45 +1100 Subject: [PATCH 2/6] qmlglsink: propagate the context up the the application Allows the application to be notified of the OpenGL context creation. --- ext/qt/gstqtsink.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ext/qt/gstqtsink.cc b/ext/qt/gstqtsink.cc index 2a4ad7ccc6..b447c2416b 100644 --- a/ext/qt/gstqtsink.cc +++ b/ext/qt/gstqtsink.cc @@ -320,6 +320,13 @@ gst_qt_sink_change_state (GstElement * element, GstStateChange transition) (NULL)); return GST_STATE_CHANGE_FAILURE; } + + GST_OBJECT_LOCK (qt_sink->display); + gst_gl_display_add_context (qt_sink->display, qt_sink->context); + GST_OBJECT_UNLOCK (qt_sink->display); + + gst_gl_element_propagate_display_context (GST_ELEMENT (qt_sink), qt_sink->display); + break; case GST_STATE_CHANGE_READY_TO_PAUSED: break; From bb5f86dc36eb8c6f837b0d0ba681c5f9ad233117 Mon Sep 17 00:00:00 2001 From: Matthew Waters Date: Tue, 25 Feb 2020 21:47:14 +1100 Subject: [PATCH 3/6] qt: don't always activate/deactivate our GstGLContext Techincally it is enough to activate at the beginning and then forget. --- ext/qt/gstqsgtexture.cc | 4 ---- ext/qt/qtitem.cc | 5 +++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/ext/qt/gstqsgtexture.cc b/ext/qt/gstqsgtexture.cc index ad1425e077..bfa79cda0f 100644 --- a/ext/qt/gstqsgtexture.cc +++ b/ext/qt/gstqsgtexture.cc @@ -99,8 +99,6 @@ GstQSGTexture::bind () if (!this->qt_context_) return; - gst_gl_context_activate (this->qt_context_, TRUE); - if (!this->buffer_) goto out; if (GST_VIDEO_INFO_FORMAT (&this->v_info) == GST_VIDEO_FORMAT_UNKNOWN) @@ -174,8 +172,6 @@ GstQSGTexture::bind () funcs->glBindTexture (GL_TEXTURE_2D, this->dummy_tex_id_); } - - gst_gl_context_activate (this->qt_context_, FALSE); } /* can be called from any thread */ diff --git a/ext/qt/qtitem.cc b/ext/qt/qtitem.cc index d92635f08a..6fdc96a0dd 100644 --- a/ext/qt/qtitem.cc +++ b/ext/qt/qtitem.cc @@ -203,7 +203,9 @@ QtGLVideoItem::updatePaintNode(QSGNode * oldNode, GstQSGTexture *tex; g_mutex_lock (&this->priv->lock); - gst_gl_context_activate (this->priv->other_context, TRUE); + + if (gst_gl_context_get_current() == NULL) + gst_gl_context_activate (this->priv->other_context, TRUE); GST_TRACE ("%p updatePaintNode", this); @@ -242,7 +244,6 @@ QtGLVideoItem::updatePaintNode(QSGNode * oldNode, texNode->setRect (QRectF (result.x, result.y, result.w, result.h)); - gst_gl_context_activate (this->priv->other_context, FALSE); g_mutex_unlock (&this->priv->lock); return texNode; From 7fd5ed2fa996dfb061fec2d6e975e080231491d3 Mon Sep 17 00:00:00 2001 From: Bastien Reboulet Date: Fri, 16 Oct 2020 16:05:45 -0700 Subject: [PATCH 4/6] qmlglsink: fix crash when created/destroyed in quick succession The crash is caused by a race condition where the render thread calls a method on the QtGLVideoItem instance that was previously destroyed by the main thread. Also, less frequently, QtGLVideoItem::onSceneGraphInitialized is called when QQuickItem::window is null, also causing a crash. Fixes #798 Part-of: --- ext/qt/qtitem.cc | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/ext/qt/qtitem.cc b/ext/qt/qtitem.cc index 6fdc96a0dd..2ee3686256 100644 --- a/ext/qt/qtitem.cc +++ b/ext/qt/qtitem.cc @@ -31,6 +31,7 @@ #include #include +#include #include #include #include @@ -91,7 +92,7 @@ class InitializeSceneGraph : public QRunnable void run(); private: - QtGLVideoItem *item_; + QPointer item_; }; InitializeSceneGraph::InitializeSceneGraph(QtGLVideoItem *item) : @@ -101,7 +102,8 @@ InitializeSceneGraph::InitializeSceneGraph(QtGLVideoItem *item) : void InitializeSceneGraph::run() { - item_->onSceneGraphInitialized(); + if(item_) + item_->onSceneGraphInitialized(); } QtGLVideoItem::QtGLVideoItem() @@ -292,7 +294,10 @@ QtGLVideoItem::onSceneGraphInitialized () QWindow* window = this->window(); #endif - GST_DEBUG ("scene graph initialization with Qt GL context %p", + if (this->window() == NULL) + return; + + GST_DEBUG ("%p scene graph initialization with Qt GL context %p", this, this->window()->openglContext ()); if (this->priv->qt_context == this->window()->openglContext ()) From 908d21a6ca2627fa73db1715f8bacf9e82b927b1 Mon Sep 17 00:00:00 2001 From: Matthew Waters Date: Thu, 3 Jun 2021 20:33:45 +1000 Subject: [PATCH 5/6] qtitem: don't potentially leak a large number of buffers The only other place where these queued buffers are removed, is in setCaps() but that is not called at all on shutdown so this list of buffers could not be removed. Part-of: --- ext/qt/qtitem.cc | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/ext/qt/qtitem.cc b/ext/qt/qtitem.cc index 2ee3686256..a93041bc36 100644 --- a/ext/qt/qtitem.cc +++ b/ext/qt/qtitem.cc @@ -137,6 +137,8 @@ QtGLVideoItem::QtGLVideoItem() QtGLVideoItem::~QtGLVideoItem() { + GstBuffer *tmp_buffer; + /* Before destroying the priv info, make sure * no qmlglsink's will call in again, and that * any ongoing calls are done by invalidating the proxy @@ -153,6 +155,17 @@ QtGLVideoItem::~QtGLVideoItem() if (this->priv->display) gst_object_unref(this->priv->display); + while ((tmp_buffer = (GstBuffer*) g_queue_pop_head (&this->priv->potentially_unbound_buffers))) { + GST_TRACE ("old buffer %p should be unbound now, unreffing", tmp_buffer); + gst_buffer_unref (tmp_buffer); + } + while ((tmp_buffer = (GstBuffer*) g_queue_pop_head (&this->priv->bound_buffers))) { + GST_TRACE ("old buffer %p should be unbound now, unreffing", tmp_buffer); + gst_buffer_unref (tmp_buffer); + } + + gst_buffer_replace (&this->priv->buffer, NULL); + gst_caps_replace (&this->priv->caps, NULL); g_free (this->priv); this->priv = NULL; From 074cb989533fb8d25541eeaaccd6fa3b9412ea4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Tue, 3 Nov 2020 15:58:30 +0200 Subject: [PATCH 6/6] qmlglsink: Keep old buffers around a bit longer if they were bound by QML We don't know exactly when QML will stop using them but it should be safe to unref them after at least 2 more buffers were bound. Part-of: --- ext/qt/gstqsgtexture.cc | 19 +++++++++++++++ ext/qt/gstqsgtexture.h | 2 ++ ext/qt/qtitem.cc | 54 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+) diff --git a/ext/qt/gstqsgtexture.cc b/ext/qt/gstqsgtexture.cc index bfa79cda0f..00e2ddad0e 100644 --- a/ext/qt/gstqsgtexture.cc +++ b/ext/qt/gstqsgtexture.cc @@ -47,6 +47,7 @@ GstQSGTexture::GstQSGTexture () gst_video_info_init (&this->v_info); this->buffer_ = NULL; + this->buffer_was_bound = FALSE; this->qt_context_ = NULL; this->sync_buffer_ = gst_buffer_new (); this->dummy_tex_id_ = 0; @@ -56,6 +57,7 @@ GstQSGTexture::~GstQSGTexture () { gst_buffer_replace (&this->buffer_, NULL); gst_buffer_replace (&this->sync_buffer_, NULL); + this->buffer_was_bound = FALSE; if (this->dummy_tex_id_ && QOpenGLContext::currentContext ()) { QOpenGLContext::currentContext ()->functions ()->glDeleteTextures (1, &this->dummy_tex_id_); @@ -80,11 +82,26 @@ GstQSGTexture::setBuffer (GstBuffer * buffer) if (!gst_buffer_replace (&this->buffer_, buffer)) return FALSE; + this->buffer_was_bound = FALSE; this->qt_context_ = gst_gl_context_get_current (); return TRUE; } +/* only called from the streaming thread with scene graph thread blocked */ +GstBuffer * +GstQSGTexture::getBuffer (gboolean * was_bound) +{ + GstBuffer *buffer = NULL; + + if (this->buffer_) + buffer = gst_buffer_ref (this->buffer_); + if (was_bound) + *was_bound = this->buffer_was_bound; + + return buffer; +} + /* only called from qt's scene graph render thread */ void GstQSGTexture::bind () @@ -142,6 +159,8 @@ GstQSGTexture::bind () * to use the dummy texture */ use_dummy_tex = FALSE; + this->buffer_was_bound = TRUE; + out: if (G_UNLIKELY (use_dummy_tex)) { QOpenGLContext *qglcontext = QOpenGLContext::currentContext (); diff --git a/ext/qt/gstqsgtexture.h b/ext/qt/gstqsgtexture.h index fdabe93a95..ec4a16f575 100644 --- a/ext/qt/gstqsgtexture.h +++ b/ext/qt/gstqsgtexture.h @@ -38,6 +38,7 @@ class GstQSGTexture : public QSGTexture, protected QOpenGLFunctions void setCaps (GstCaps * caps); gboolean setBuffer (GstBuffer * buffer); + GstBuffer * getBuffer (gboolean * was_bound); /* QSGTexture */ void bind (); @@ -48,6 +49,7 @@ class GstQSGTexture : public QSGTexture, protected QOpenGLFunctions private: GstBuffer * buffer_; + gboolean buffer_was_bound; GstBuffer * sync_buffer_; GstGLContext * qt_context_; GstMemory * mem_; diff --git a/ext/qt/qtitem.cc b/ext/qt/qtitem.cc index a93041bc36..f89d069121 100644 --- a/ext/qt/qtitem.cc +++ b/ext/qt/qtitem.cc @@ -83,6 +83,14 @@ struct _QtGLVideoItemPrivate QOpenGLContext *qt_context; GstGLContext *other_context; GstGLContext *context; + + /* buffers with textures that were bound by QML */ + GQueue bound_buffers; + /* buffers that were previously bound but in the meantime a new one was + * bound so this one is most likely not used anymore + * FIXME: Ideally we would use fences for this but there seems to be no + * way to reliably "try wait" on a fence */ + GQueue potentially_unbound_buffers; }; class InitializeSceneGraph : public QRunnable @@ -209,6 +217,9 @@ QSGNode * QtGLVideoItem::updatePaintNode(QSGNode * oldNode, UpdatePaintNodeData * updatePaintNodeData) { + GstBuffer *old_buffer; + gboolean was_bound = FALSE; + if (!m_openGlContextInitialized) { return oldNode; } @@ -236,6 +247,38 @@ QtGLVideoItem::updatePaintNode(QSGNode * oldNode, } tex = static_cast (texNode->texture()); + + if ((old_buffer = tex->getBuffer(&was_bound))) { + if (old_buffer == this->priv->buffer) { + /* same buffer */ + gst_buffer_unref (old_buffer); + } else if (!was_bound) { + GST_TRACE ("old buffer %p was not bound yet, unreffing", old_buffer); + gst_buffer_unref (old_buffer); + } else { + GstBuffer *tmp_buffer; + + GST_TRACE ("old buffer %p was bound, queueing up for later", old_buffer); + /* Unref all buffers that were previously not bound anymore. At least + * one more buffer was bound in the meantime so this one is most likely + * not in use anymore. */ + while ((tmp_buffer = (GstBuffer*) g_queue_pop_head (&this->priv->potentially_unbound_buffers))) { + GST_TRACE ("old buffer %p should be unbound now, unreffing", tmp_buffer); + gst_buffer_unref (tmp_buffer); + } + + /* Move previous bound buffers to the next queue. We now know that + * another buffer was bound in the meantime and will free them on + * the next iteration above. */ + while ((tmp_buffer = (GstBuffer*) g_queue_pop_head (&this->priv->bound_buffers))) { + GST_TRACE ("old buffer %p is potentially unbound now", tmp_buffer); + g_queue_push_tail (&this->priv->potentially_unbound_buffers, tmp_buffer); + } + g_queue_push_tail (&this->priv->bound_buffers, old_buffer); + } + old_buffer = NULL; + } + tex->setCaps (this->priv->caps); tex->setBuffer (this->priv->buffer); texNode->markDirty(QSGNode::DirtyMaterial); @@ -267,12 +310,23 @@ QtGLVideoItem::updatePaintNode(QSGNode * oldNode, static void _reset (QtGLVideoItem * qt_item) { + GstBuffer *tmp_buffer; + gst_buffer_replace (&qt_item->priv->buffer, NULL); gst_caps_replace (&qt_item->priv->caps, NULL); qt_item->priv->negotiated = FALSE; qt_item->priv->initted = FALSE; + + while ((tmp_buffer = (GstBuffer*) g_queue_pop_head (&qt_item->priv->potentially_unbound_buffers))) { + GST_TRACE ("old buffer %p should be unbound now, unreffing", tmp_buffer); + gst_buffer_unref (tmp_buffer); + } + while ((tmp_buffer = (GstBuffer*) g_queue_pop_head (&qt_item->priv->bound_buffers))) { + GST_TRACE ("old buffer %p should be unbound now, unreffing", tmp_buffer); + gst_buffer_unref (tmp_buffer); + } } void