Review of attachment 179671 [details] [review]:

This patch is wrong because you removed the events. Now, the updateBuffer() function is called from the streaming thread and the repaint() function from the main thread and there is no queue between them, and no mutex either! This is guaranteed to crash. You should re-add the trick with the events.

Plus, I explained last time that sending the image as the signal argument is also not ok, as it creates the need for doing an unnecessary copy of the image inside the widget/graphicsitem before it can be painted.

Created attachment 179763 [details] [review]
Patch v2

Here I added some of the things you requested. Hope it's better now.

Review of attachment 179763 [details] [review]:

Overall status: good. Please fix at least the issues in the test and I will take care of the GObject stuff in the sink.

::: elements/gstqimagevideosink.cpp
@@ +192,3 @@
+//BEGIN ******** WidgetProxy ********
+
+WidgetProxy::WidgetProxy(GObject *sink)

Ideally this object should have a better name

@@ +287,3 @@
+{
+    if(m_widget)
+        QCoreApplication::postEvent(m_widget.data(), new QEvent(QEvent::User));

This doesn't really need to send an event to the user's object. The event could be sent internally in WidgetProxy. No need to install an event filter on the external object.

@@ +399,3 @@
+        switch(event->type()) {
+        case QEvent::User:
+            QMetaObject::invokeMethod(m_widget.data(), "updateImage", Qt::QueuedConnection);

Qt::QueuedConenction is not needed here. Anyway, I think it would be better to use GObject signals to interract with the user, since GObject signals will appear on gst-inspect and the generated docs.

@@ +479,3 @@
+    g_object_class_install_property(gobject_class, PROP_WIDTH,
+        g_param_spec_boolean("width", "Suggest output width",
+                             "",

Description needed here. Plus, width & height should be in one property.

@@ +516,3 @@
+
+    switch (prop_id) {
+    case PROP_WIDGET:

If we turn the signals into GObject signals, there is no reason for this property to exist. It has a wrong name anyway... The expected object is not necessarily a widget.

@@ +647,3 @@
+
+/* entry point to initialize the plug-in */
+static gboolean plugin_init(GstPlugin *plugin)

Ideally this plugin should be merged with qwidgetvideosink and qwidgetvideosink should be turn into a bin that uses qimagevideosink internally.

::: tests/manual/qimagevideosinktest.cpp
@@ +22,3 @@
+#include <QtGui/QButtonGroup>
+#include <QtGui/QGraphicsPixmapItem>
+#include "ui_qimagevideosinktest.h"

local headers should always be included first.

@@ +31,3 @@
+    Q_PROPERTY(qreal rotation READ rotation WRITE setRotation)
+    Q_PROPERTY(QPointF position READ pos WRITE setPos)
+/*

Coding style: indent one level back. Access specifiers like "public:" should start at column 0 and functions should start at column 4.

@@ +36,3 @@
+    public Q_SLOTS:
+        void updateImage();
+        void setSink(QGst::ElementPtr elem);

const QGst::ElementPtr &

@@ +37,3 @@
+        void updateImage();
+        void setSink(QGst::ElementPtr elem);
+        QGst::ElementPtr sink() const { return m_sink; }

This doesn't need to be a slot, does it?

@@ +65,3 @@
+};
+
+VideoWidgetTest::VideoWidgetTest(QWidget *parent, Qt::WindowFlags f)

Shouldn't the items be initialized in the constructor? They seem to be initialized only when you change their count on the UI.

@@ +85,3 @@
+//     QPropertyAnimation* anim = new QPropertyAnimation(m_item, "rotation", m_item);
+//     anim->setStartValue(0);
+//     anim->setEndValue(360);

Cleanup the commented code please. Either remove or uncomment.

@@ +100,3 @@
+    videotest->link(tee);
+/*
+    m_src=tee;

shouldn't this be m_src = videotest; ?

@@ +101,3 @@
+    
+    m_src=tee;
+    if (!m_src) {

This check is useless here. If videotestsrc was not created, the app will crash above, when it is linked. Imho it should be removed completely, as in all the tests we assume that we have all the standard gstreamer plugins installed.

@@ +116,3 @@
+void VideoWidgetTest::updateItems(int items)
+{
+//     Q_ASSERT(items!=m_items.size());

Uncomment or remove

@@ +122,3 @@
+
+    if (!m_items.isEmpty()) {
+        m_pipeline->setState(QGst::State(QGst::State::StateNull));

setState(QGst::StateNull);

@@ +143,3 @@
+    
+    for(; items<m_items.size(); ) {
+        VideoGraphicsItem* toremove=m_items.takeLast();

This looks broken. You append items and then you remove them immediately? How does that work?

@@ +179,3 @@
+void VideoGraphicsItem::setSink(QGst::ElementPtr sink)
+{
+//     sink->setProperty<uint>("width", 320);

You should set width & height, and make sure that the values match the size of the graphics item.

This patch is not relevant anymore, I just merged qtvideosink/qtglvideosink, which are sinks that can paint anywhere (QWidget, QGraphicsView, QML and more), together with an API to use it from QML. I'm sorry that it took me so long.