[gstreamer-bugs] [Bug 640985] Create a videosink that makes it possible to interact with QGraphicsView

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=640985
  GStreamer | qt-gstreamer | git

George Kiagiadakis <kiagiadakis.george> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #179763|none                        |reviewed
             status|                            |

--- Comment #4 from George Kiagiadakis <kiagiadakis.george at gmail.com> 2011-02-02 17:00:19 UTC ---
Review of attachment 179763:
 --> (https://bugzilla.gnome.org/review?bug=640985&attachment=179763)

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.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.