[Bug 680236] QtGStreamer doesn't wrap GstDiscoverer

[GStreamer (bugzilla.gnome.org)]

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

George Kiagiadakis <kiagiadakis.george> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
                 CC|                            |kiagiadakis.george at gmail.co
                   |                            |m
   Target Milestone|HEAD                        |0.10.3
     Ever Confirmed|0                           |1

--- Comment #10 from George Kiagiadakis <kiagiadakis.george at gmail.com> 2012-12-06 09:36:49 UTC ---
Yes, I'm sorry for the delay as well. I had started reviewing the other patches
as well, but got sidetracked and forgot about it. I have comments mainly for
the main feature patch
(https://github.com/openismus/qt-gstreamer/commit/98a845e9990ae5d8e2e7fe97452dcd9f1650a650).
Since splinter seems to be broken and does not allow me to make comments on
that patch, I will do it here:

discoverer.h:

+#if !QGLIB_HAVE_CXX0X
+# include <boost/preprocessor.hpp>
+#endif

This is useless, I don't see any boost preprocessor magic being used here.

+enum DiscovererResult {
+    DiscovererOk,
+    DiscovererUriInvalid,
+    DiscovererError,
+    DiscovererTimeout,
+    DiscovererBusy,
+    DiscovererMissingPlugins
+};

Enums should be in the enums.h file with the appropriate type registration
macros.

+    QList<DiscovererStreamInfoPtr> subTitleStreams() const;

subtitleStreams() would be better imho (small t) - keeps subtitle as one word,
like in other places in this API

+    /*! Allow asynchronous discovering of URIs to take place. An event loop
must be
+     * available for QGst::Discoverer to properly work in asynchronous mode.
+     * \ingroup async
+     */
+    void start();

This needs a big fat warning that it requires a *GLIB* event loop and therefore
it is only going to work on X11-based platforms (in Qt4 at least).

+QTGSTREAMER_EXPORT QDebug operator<<(QDebug debug, DiscovererStreamInfoPtr
info);
+QTGSTREAMER_EXPORT QDebug operator<<(QDebug debug, DiscovererInfoPtr info);

const FooPtr & info please. There is no reason in incrementing the reference
count for a debugging function.


discoverer.cpp:

+// FIXME: move to fraction.h
+static QDebug operator<<(QDebug debug, const Fraction &f)

Do move it to fraction.h please

+namespace Private {
+
+QGlib::RefCountedObject *wrapDiscovererInfo(void *info)
+{
+    return QGlib::constructWrapper(G_TYPE_FROM_INSTANCE(info), info);
+}
+
+} //namespace Private

This is not needed here. Remove it.


The test looks good, I trust it works.
I would also like the changes to qgsttest.h in
https://github.com/openismus/qt-gstreamer/commit/768342b3ee5816c0f5ada645d765f65d6599a004
to be moved to the earlier commit
https://github.com/openismus/qt-gstreamer/commit/d269da0501e50bb514f59b17a0230ad2093f0521
, to make sure the code compiles in all commits.

There are also some remaining comments to fix on
https://bugzilla.gnome.org/show_bug.cgi?id=680235 , which goes along with this
branch.

After all these have been fixed, this branch can be merged.

Thanks and sorry again for the delay from my side as well,
George

-- 
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.