Created attachment 219209 [details] [review]
Add initial wrapping of GstDiscoverer

Created attachment 219210 [details] [review]
Discoverer: Update gen.cpp for GstDiscoverer wrapping

Created attachment 219211 [details] [review]
tests: Teach QCOMPARE() printing certain GStreamer

Created attachment 219212 [details] [review]
tests: Add equality operators to QGst::Fraction

Created attachment 219213 [details] [review]
tests: Add tests for QGst::Discoverer

Any progress on reviewing this changes? Can I merge them?

Review of attachment 219207 [details] [review]:

::: CMakeLists.txt
@@ +55,3 @@
 endif()
 
+find_package(GStreamer 0.10.33 COMPONENTS base pbutils)

You don't need to search for this component if you are not going to use it. If you *are* going to use it, though, you need a macro_log_feature() below like it is done for the other libraries.

::: cmake/modules/FindGStreamer.cmake
@@ +82,3 @@
         _find_gst_component(NET gstnet.h)
+    elseif (${_component} STREQUAL "pbutils")
+        _find_gst_component(PBUTILS pbutils.h)

This is in the wrong file, isn't it? According to the web, pbutils is part of gst-plugins-base, not gstreamer core.

Yes, and that pbutils check was already in the FindGStreamerBase.cmake file. I have updated this
https://github.com/openismus/qt-gstreamer/commits/discoverer-contrib
(with a force push) in the commit here:
https://github.com/openismus/qt-gstreamer/commit/ad69261de04e826a1fde9233b9f34e9d9f5b1f6a

Sorry for the delay by us responding to your review, but could you continue with the review please? It would be great to get this accepted upstream.

Please?

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

I have made some of those changes in the branch.

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

That leads to some ambiguous calls. I'll see if I can still make it work.

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

You mean, to a new file?

(In reply to comment #11)
> I have made some of those changes in the branch.
> 
> > +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.
> 
> That leads to some ambiguous calls. I'll see if I can still make it work.

Hm, I don't see why that would be ambiguous.

> > +// FIXME: move to fraction.h
> > +static QDebug operator<<(QDebug debug, const Fraction &f)
> > 
> > Do move it to fraction.h please
> 
> You mean, to a new file?

Sorry, I meant structs.h, together with the Fraction struct definition.

(In reply to comment #10)

> +    /*! 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).
> 

I agree that GLib must be mentioned explicitly. In bold font even maybe. I don't entirely agree with the conclusions about requiring X11: We are using GStreamer already, so whatever imaginary non-X11 platform we deal with, you already have a fully functional GLib event loop. Only thing that's missing is the integration with the Qt event loop. So it should be sufficient to take QEventDispatcherGlib and adopt it for the non-X11 platform.

http://qt.gitorious.org/qt/qt/blobs/4.8/src/corelib/kernel/qeventdispatcher_glib.cpp
http://qt.gitorious.org/qt/qt/blobs/4.8/src/corelib/kernel/kernel.pri

Actually I don't see any references to X11 in that code. So it might be sufficient to just take this code and plug it.

I have made all the changes, apart from that documentation change, in the branch:
https://github.com/openismus/qt-gstreamer/commits/discoverer-contrib

Thanks, I have commited everything in master, together with some additional fixes that were needed.

(In reply to comment #13)
> I agree that GLib must be mentioned explicitly. In bold font even maybe. I
> don't entirely agree with the conclusions about requiring X11: We are using
> GStreamer already, so whatever imaginary non-X11 platform we deal with, you
> already have a fully functional GLib event loop. Only thing that's missing is
> the integration with the Qt event loop. So it should be sufficient to take
> QEventDispatcherGlib and adopt it for the non-X11 platform.
> 
> http://qt.gitorious.org/qt/qt/blobs/4.8/src/corelib/kernel/qeventdispatcher_glib.cpp
> http://qt.gitorious.org/qt/qt/blobs/4.8/src/corelib/kernel/kernel.pri
> 
> Actually I don't see any references to X11 in that code. So it might be
> sufficient to just take this code and plug it.

Yes, of course, I didn't mean it depends on X11, but just that Qt (4.x at least) doesn't come with this feature on non-X11 platforms. In any case, I've added this documentation note myself, without mentioning X11 explicitly.