From b7fc75c8835518d5cd705512690293fad0368066 Mon Sep 17 00:00:00 2001 From: Jan Schmidt Date: Wed, 12 Jul 2017 15:29:32 +1000 Subject: [PATCH] qt: Use a proxy object for access to the QML widget QML can destroy the video widget at any time, leaving us with a dangling pointer. Use a lock and a proxy object to cope with that, and block in the widget destructor if there are ongoing calls into the widget. --- ext/qt/gstqtsink.cc | 38 ++++++++--- ext/qt/gstqtsink.h | 4 +- ext/qt/qtitem.cc | 160 +++++++++++++++++++++++++++++++------------- ext/qt/qtitem.h | 42 +++++++++--- 4 files changed, 174 insertions(+), 70 deletions(-) diff --git a/ext/qt/gstqtsink.cc b/ext/qt/gstqtsink.cc index 2253156e2e..c7ac11ec6e 100644 --- a/ext/qt/gstqtsink.cc +++ b/ext/qt/gstqtsink.cc @@ -143,6 +143,7 @@ gst_qt_sink_class_init (GstQtSinkClass * klass) static void gst_qt_sink_init (GstQtSink * qt_sink) { + qt_sink->widget = QSharedPointer(); } static void @@ -152,9 +153,14 @@ gst_qt_sink_set_property (GObject * object, guint prop_id, GstQtSink *qt_sink = GST_QT_SINK (object); switch (prop_id) { - case PROP_WIDGET: - qt_sink->widget = static_cast (g_value_get_pointer (value)); + case PROP_WIDGET: { + QtGLVideoItem *qt_item = static_cast (g_value_get_pointer (value)); + if (qt_item) + qt_sink->widget = qt_item->getInterface(); + else + qt_sink->widget.clear(); break; + } case PROP_FORCE_ASPECT_RATIO: g_return_if_fail (qt_sink->widget); qt_sink->widget->setForceAspectRatio (g_value_get_boolean (value)); @@ -196,6 +202,8 @@ gst_qt_sink_finalize (GObject * object) _reset (qt_sink); + qt_sink->widget.clear(); + G_OBJECT_CLASS (parent_class)->finalize (object); } @@ -207,7 +215,13 @@ gst_qt_sink_get_property (GObject * object, guint prop_id, switch (prop_id) { case PROP_WIDGET: - g_value_set_pointer (value, qt_sink->widget); + /* This is not really safe - the app needs to be + * sure the widget is going to be kept alive or + * this can crash */ + if (qt_sink->widget) + g_value_set_pointer (value, qt_sink->widget->videoItem()); + else + g_value_set_pointer (value, NULL); break; case PROP_FORCE_ASPECT_RATIO: if (qt_sink->widget) @@ -287,16 +301,16 @@ gst_qt_sink_change_state (GstElement * element, GstStateChange transition) return GST_STATE_CHANGE_FAILURE; } - if (!qt_item_init_winsys (qt_sink->widget)) { + if (!qt_sink->widget->initWinSys()) { GST_ELEMENT_ERROR (element, RESOURCE, NOT_FOUND, ("%s", "Could not initialize window system"), (NULL)); return GST_STATE_CHANGE_FAILURE; } - qt_sink->display = qt_item_get_display (qt_sink->widget); - qt_sink->context = qt_item_get_context (qt_sink->widget); - qt_sink->qt_context = qt_item_get_qt_context (qt_sink->widget); + qt_sink->display = qt_sink->widget->getDisplay(); + qt_sink->context = qt_sink->widget->getContext(); + qt_sink->qt_context = qt_sink->widget->getQtContext(); if (!qt_sink->display || !qt_sink->context || !qt_sink->qt_context) { GST_ELEMENT_ERROR (element, RESOURCE, NOT_FOUND, @@ -321,7 +335,8 @@ gst_qt_sink_change_state (GstElement * element, GstStateChange transition) case GST_STATE_CHANGE_PLAYING_TO_PAUSED: break; case GST_STATE_CHANGE_PAUSED_TO_READY: - qt_item_set_buffer (qt_sink->widget, NULL); + if (qt_sink->widget) + qt_sink->widget->setBuffer(NULL); break; case GST_STATE_CHANGE_READY_TO_NULL: break; @@ -365,10 +380,10 @@ gst_qt_sink_set_caps (GstBaseSink * bsink, GstCaps * caps) if (!gst_video_info_from_caps (&qt_sink->v_info, caps)) return FALSE; - if (!qt_item_set_caps (qt_sink->widget, caps)) + if (!qt_sink->widget) return FALSE; - return TRUE; + return qt_sink->widget->setCaps(caps); } static GstFlowReturn @@ -380,7 +395,8 @@ gst_qt_sink_show_frame (GstVideoSink * vsink, GstBuffer * buf) qt_sink = GST_QT_SINK (vsink); - qt_item_set_buffer (qt_sink->widget, buf); + if (qt_sink->widget) + qt_sink->widget->setBuffer(buf); return GST_FLOW_OK; } diff --git a/ext/qt/gstqtsink.h b/ext/qt/gstqtsink.h index 9a1cfbece8..3ee20b7d5d 100644 --- a/ext/qt/gstqtsink.h +++ b/ext/qt/gstqtsink.h @@ -51,8 +51,6 @@ struct _GstQtSink /* */ GstVideoSink parent; - QtGLVideoItem *widget; - GstVideoInfo v_info; GstBufferPool *pool; @@ -60,7 +58,7 @@ struct _GstQtSink GstGLContext *context; GstGLContext *qt_context; - GstQtSinkPrivate *priv; + QSharedPointer widget; }; /** diff --git a/ext/qt/qtitem.cc b/ext/qt/qtitem.cc index be604683bb..5806bbcff3 100644 --- a/ext/qt/qtitem.cc +++ b/ext/qt/qtitem.cc @@ -30,6 +30,7 @@ #include "gstqtglutility.h" #include +#include #include #include #include @@ -123,11 +124,21 @@ QtGLVideoItem::QtGLVideoItem() connect(this, SIGNAL(windowChanged(QQuickWindow*)), this, SLOT(handleWindowChanged(QQuickWindow*))); + this->proxy = QSharedPointer(new QtGLVideoItemInterface(this)); + GST_DEBUG ("%p init Qt Video Item", this); } QtGLVideoItem::~QtGLVideoItem() { + /* 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 + * pointer */ + GST_INFO ("Destroying QtGLVideoItem and invalidating the proxy"); + proxy->invalidateRef(); + proxy.clear(); + g_mutex_clear (&this->priv->lock); if (this->priv->context) gst_object_unref(this->priv->context); @@ -237,18 +248,25 @@ _reset (QtGLVideoItem * qt_item) } void -qt_item_set_buffer (QtGLVideoItem * widget, GstBuffer * buffer) +QtGLVideoItemInterface::setBuffer (GstBuffer * buffer) { - g_return_if_fail (widget != NULL); - g_return_if_fail (widget->priv->negotiated); + QMutexLocker locker(&lock); - g_mutex_lock (&widget->priv->lock); + if (qt_item == NULL) + return; - gst_buffer_replace (&widget->priv->buffer, buffer); + if (!qt_item->priv->negotiated) { + GST_WARNING ("Got buffer on unnegotiated QtGLVideoItem. Dropping"); + return; + } - QMetaObject::invokeMethod(widget, "update", Qt::QueuedConnection); + g_mutex_lock (&qt_item->priv->lock); - g_mutex_unlock (&widget->priv->lock); + gst_buffer_replace (&qt_item->priv->buffer, buffer); + + QMetaObject::invokeMethod(qt_item, "update", Qt::QueuedConnection); + + g_mutex_unlock (&qt_item->priv->lock); } void @@ -280,50 +298,53 @@ QtGLVideoItem::onSceneGraphInvalidated () } gboolean -qt_item_init_winsys (QtGLVideoItem * widget) +QtGLVideoItemInterface::initWinSys () { + QMutexLocker locker(&lock); + GError *error = NULL; - g_return_val_if_fail (widget != NULL, FALSE); + if (qt_item == NULL) + return FALSE; - g_mutex_lock (&widget->priv->lock); + g_mutex_lock (&qt_item->priv->lock); - if (widget->priv->display && widget->priv->qt_context - && widget->priv->other_context && widget->priv->context) { + if (qt_item->priv->display && qt_item->priv->qt_context + && qt_item->priv->other_context && qt_item->priv->context) { /* already have the necessary state */ - g_mutex_unlock (&widget->priv->lock); + g_mutex_unlock (&qt_item->priv->lock); return TRUE; } - if (!GST_IS_GL_DISPLAY (widget->priv->display)) { + if (!GST_IS_GL_DISPLAY (qt_item->priv->display)) { GST_ERROR ("%p failed to retrieve display connection %" GST_PTR_FORMAT, - widget, widget->priv->display); - g_mutex_unlock (&widget->priv->lock); + qt_item, qt_item->priv->display); + g_mutex_unlock (&qt_item->priv->lock); return FALSE; } - if (!GST_IS_GL_CONTEXT (widget->priv->other_context)) { - GST_ERROR ("%p failed to retrieve wrapped context %" GST_PTR_FORMAT, widget, - widget->priv->other_context); - g_mutex_unlock (&widget->priv->lock); + if (!GST_IS_GL_CONTEXT (qt_item->priv->other_context)) { + GST_ERROR ("%p failed to retrieve wrapped context %" GST_PTR_FORMAT, qt_item, + qt_item->priv->other_context); + g_mutex_unlock (&qt_item->priv->lock); return FALSE; } - widget->priv->context = gst_gl_context_new (widget->priv->display); + qt_item->priv->context = gst_gl_context_new (qt_item->priv->display); - if (!widget->priv->context) { - g_mutex_unlock (&widget->priv->lock); + if (!qt_item->priv->context) { + g_mutex_unlock (&qt_item->priv->lock); return FALSE; } - if (!gst_gl_context_create (widget->priv->context, widget->priv->other_context, + if (!gst_gl_context_create (qt_item->priv->context, qt_item->priv->other_context, &error)) { GST_ERROR ("%s", error->message); - g_mutex_unlock (&widget->priv->lock); + g_mutex_unlock (&qt_item->priv->lock); return FALSE; } - g_mutex_unlock (&widget->priv->lock); + g_mutex_unlock (&qt_item->priv->lock); return TRUE; } @@ -403,68 +424,115 @@ _calculate_par (QtGLVideoItem * widget, GstVideoInfo * info) } gboolean -qt_item_set_caps (QtGLVideoItem * widget, GstCaps * caps) +QtGLVideoItemInterface::setCaps (GstCaps * caps) { + QMutexLocker locker(&lock); GstVideoInfo v_info; - g_return_val_if_fail (widget != NULL, FALSE); g_return_val_if_fail (GST_IS_CAPS (caps), FALSE); g_return_val_if_fail (gst_caps_is_fixed (caps), FALSE); - if (widget->priv->caps && gst_caps_is_equal_fixed (widget->priv->caps, caps)) + if (qt_item == NULL) + return FALSE; + + if (qt_item->priv->caps && gst_caps_is_equal_fixed (qt_item->priv->caps, caps)) return TRUE; if (!gst_video_info_from_caps (&v_info, caps)) return FALSE; - g_mutex_lock (&widget->priv->lock); + g_mutex_lock (&qt_item->priv->lock); - _reset (widget); + _reset (qt_item); - gst_caps_replace (&widget->priv->caps, caps); + gst_caps_replace (&qt_item->priv->caps, caps); - if (!_calculate_par (widget, &v_info)) { - g_mutex_unlock (&widget->priv->lock); + if (!_calculate_par (qt_item, &v_info)) { + g_mutex_unlock (&qt_item->priv->lock); return FALSE; } - widget->priv->v_info = v_info; - widget->priv->negotiated = TRUE; + qt_item->priv->v_info = v_info; + qt_item->priv->negotiated = TRUE; - g_mutex_unlock (&widget->priv->lock); + g_mutex_unlock (&qt_item->priv->lock); return TRUE; } GstGLContext * -qt_item_get_qt_context (QtGLVideoItem * qt_item) +QtGLVideoItemInterface::getQtContext () { - g_return_val_if_fail (qt_item != NULL, NULL); + QMutexLocker locker(&lock); - if (!qt_item->priv->other_context) + if (!qt_item || !qt_item->priv->other_context) return NULL; return (GstGLContext *) gst_object_ref (qt_item->priv->other_context); } GstGLContext * -qt_item_get_context (QtGLVideoItem * qt_item) +QtGLVideoItemInterface::getContext () { - g_return_val_if_fail (qt_item != NULL, NULL); + QMutexLocker locker(&lock); - if (!qt_item->priv->context) + if (!qt_item || !qt_item->priv->context) return NULL; return (GstGLContext *) gst_object_ref (qt_item->priv->context); } GstGLDisplay * -qt_item_get_display (QtGLVideoItem * qt_item) +QtGLVideoItemInterface::getDisplay() { - g_return_val_if_fail (qt_item != NULL, NULL); + QMutexLocker locker(&lock); - if (!qt_item->priv->display) + if (!qt_item || !qt_item->priv->display) return NULL; return (GstGLDisplay *) gst_object_ref (qt_item->priv->display); } + +void +QtGLVideoItemInterface::setDAR(gint num, gint den) +{ + QMutexLocker locker(&lock); + if (!qt_item) + return; + qt_item->setDAR(num, den); +} + +void +QtGLVideoItemInterface::getDAR(gint * num, gint * den) +{ + QMutexLocker locker(&lock); + if (!qt_item) + return; + qt_item->getDAR (num, den); +} + +void +QtGLVideoItemInterface::setForceAspectRatio(bool force_aspect_ratio) +{ + QMutexLocker locker(&lock); + if (!qt_item) + return; + qt_item->setForceAspectRatio(force_aspect_ratio); +} + +bool +QtGLVideoItemInterface::getForceAspectRatio() +{ + QMutexLocker locker(&lock); + if (!qt_item) + return FALSE; + return qt_item->getForceAspectRatio(); +} + +void +QtGLVideoItemInterface::invalidateRef() +{ + QMutexLocker locker(&lock); + qt_item = NULL; +} + diff --git a/ext/qt/qtitem.h b/ext/qt/qtitem.h index ae23eff107..b322068f3a 100644 --- a/ext/qt/qtitem.h +++ b/ext/qt/qtitem.h @@ -25,12 +25,40 @@ #include #include "gstqtgl.h" +#include #include #include #include typedef struct _QtGLVideoItemPrivate QtGLVideoItemPrivate; +class QtGLVideoItem; + +class QtGLVideoItemInterface : public QObject +{ + Q_OBJECT +public: + QtGLVideoItemInterface (QtGLVideoItem *w) : qt_item (w), lock() {}; + + void invalidateRef(); + + void setBuffer (GstBuffer * buffer); + gboolean setCaps (GstCaps *caps); + gboolean initWinSys (); + GstGLContext *getQtContext(); + GstGLContext *getContext(); + GstGLDisplay *getDisplay(); + QtGLVideoItem *videoItem () { return qt_item; }; + + void setDAR(gint, gint); + void getDAR(gint *, gint *); + void setForceAspectRatio(bool); + bool getForceAspectRatio(); +private: + QtGLVideoItem *qt_item; + QMutex lock; +}; + class InitializeSceneGraph; class QtGLVideoItem : public QQuickItem, protected QOpenGLFunctions @@ -45,6 +73,7 @@ public: void setForceAspectRatio(bool); bool getForceAspectRatio(); + QSharedPointer getInterface() { return proxy; }; /* private for C interface ... */ QtGLVideoItemPrivate *priv; @@ -57,22 +86,15 @@ protected: QSGNode * updatePaintNode (QSGNode * oldNode, UpdatePaintNodeData * updatePaintNodeData); private: + friend class InitializeSceneGraph; void setViewportSize(const QSize &size); void shareContext(); QSize m_viewportSize; bool m_openGlContextInitialized; + + QSharedPointer proxy; }; -extern "C" -{ -void qt_item_set_buffer (QtGLVideoItem * widget, GstBuffer * buffer); -gboolean qt_item_set_caps (QtGLVideoItem * widget, GstCaps * caps); -gboolean qt_item_init_winsys (QtGLVideoItem * widget); -GstGLContext * qt_item_get_qt_context (QtGLVideoItem * qt_item); -GstGLContext * qt_item_get_context (QtGLVideoItem * qt_item); -GstGLDisplay * qt_item_get_display (QtGLVideoItem * qt_item); -} - #endif /* __QT_ITEM_H__ */