[gstreamer-bugs] [Bug 625944] New GstDiscoverer helper library

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=625944
  GStreamer | gst-plugins-base | git

--- Comment #2 from Sebastian Dröge <slomo at circular-chaos.org> 2010-08-03 15:40:22 UTC ---
Review of attachment 167044:
 --> (https://bugzilla.gnome.org/review?bug=625944&attachment=167044)

I'd prefer to have this in pbutils or one of the other existing libraries
instead of introducing just another micro library

::: gst-libs/gst/discoverer/gstdiscoverer-types.c
@@ +41,3 @@
+gst_stream_information_new (void)
+{
+  GstStreamInformation *info = g_new0 (GstStreamInformation, 1);

Use GSlice here and everywhere else ;)

@@ +168,3 @@
+  static GType gst_stream_information_type = 0;
+
+  if (G_UNLIKELY (gst_stream_information_type == 0)) {

Not threadsafe, does this matter here? Better use g_once_init_enter/leave to be
on the safe side. Same for the other boxed type registrations

@@ +315,3 @@
+  info->parent.streamtype = GST_STREAM_VIDEO;
+  g_value_init (&info->frame_rate, GST_TYPE_FRACTION);
+  g_value_init (&info->pixel_aspect_ratio, GST_TYPE_FRACTION);

Maybe store the fractions as a numerator/denominator pair? That's easier to
handle than GstFractions, especially for bindings

::: gst-libs/gst/discoverer/gstdiscoverer.c
@@ +75,3 @@
+  GST_DEBUG_CATEGORY_INIT (discoverer_debug, "discoverer", 0, "Discoverer");
+
+  _INFORMATION_QUARK = g_quark_from_string ("information");

g_quark_from_static_string() :)

@@ +143,3 @@
+      g_param_spec_uint64 ("timeout", "timeout", "Timeout",
+          GST_SECOND, 3600 * GST_SECOND, DEFAULT_PROP_TIMEOUT,
+          G_PARAM_READWRITE | G_PARAM_CONSTRUCT));

G_PARAM_STATIC_STRINGS

@@ +233,3 @@
+  /* This is ugly. We get the GType of decodebin2 so we can quickly detect
+   * when a decodebin2 is added to uridecodebin so we can set the
+   * post-stream-topology setting to TRUE */

This can be removed, everywhere else too

@@ +444,3 @@
+  }
+
+  g_slice_free1 (sizeof (PrivateStream), ps);

g_slice_free (PrivateStream, ps)

@@ +847,3 @@
+handle_current_async (GstDiscoverer * dc)
+{
+  /* FIXME : TIMEOUT ! */

can't you just use a GTimeout as you require a running main loop anyway? This
seems to be an important missing feature

@@ +1100,3 @@
+  discoverer->async = TRUE;
+  /* Connect to bus signals */
+  gst_bus_add_signal_watch (discoverer->bus);

Of course this needs a main loop in the default main context. This should be
documented and maybe get an optional GMainContext* parameter

@@ +1126,3 @@
+  DISCO_LOCK (discoverer);
+  if (discoverer->running) {
+    /* FIXME : Stop any ongoing discovery */

This seems to be a rather fundamental feature that is missing

::: gst-libs/gst/discoverer/gstdiscoverer.h
@@ +62,3 @@
+ */
+struct _GstStreamInformation {
+  GstStreamType         streamtype;

Maybe this should be a GstMiniObject. The inheritance of the structs might be
complicated for bindings

@@ +286,3 @@
+
+  /* current items */
+  /* FIXME : Protect all this with a lock */

This is protected with the disco lock in the code it seems

@@ +299,3 @@
+  GstBus *bus;
+
+  GType decodebin2_type;

Padding missing in all public structs

@@ +310,3 @@
+  void        (*discovered)      (GstDiscoverer *discoverer,
+                                  GstDiscovererInformation *info,
+                  GError *err);

Maybe the GError should be const?

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