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

GStreamer (bugzilla.gnome.org) bugzilla at gnome.org
Tue Aug 3 09:56:58 PDT 2010


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

--- Comment #5 from Edward Hervey <bilboed at gmail.com> 2010-08-03 16:56:55 UTC ---
(In reply to comment #2)
> Review of attachment 167044 [details]:
> 
> I'd prefer to have this in pbutils or one of the other existing libraries
> instead of introducing just another micro library

  No problem with that. I haven't changed it in the following patch, will do it
in another version.

> 
> ::: 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 ;)

  FIXED

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

  FIXED

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

  Good point, will ponder that.

> 
> ::: 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() :)

  FIXED

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

  FIXED

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

  what exactly can be removed ?

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

  FIXED

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

  True, will fix that in a future patch.

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

  It is documented in the discoverer docs that using the async API requires a
running mainloop (see top of the file).

> 
> @@ +1126,3 @@
> +  DISCO_LOCK (discoverer);
> +  if (discoverer->running) {
> +    /* FIXME : Stop any ongoing discovery */
> 
> This seems to be a rather fundamental feature that is missing

  Yes, this would go along the timeout implementation.

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

  Switching to GstMiniObject wouldn't help that much, we'd still end up with
the inheritance problem. The only thing that could make it a *bit* better for
bindings would be to not inherit and just copy all fields from the parent
struct over to those *sub*structures.

> 
> @@ +286,3 @@
> +
> +  /* current items */
> +  /* FIXME : Protect all this with a lock */
> 
> This is protected with the disco lock in the code it seems

  FIXED

> 
> @@ +299,3 @@
> +  GstBus *bus;
> +
> +  GType decodebin2_type;
> 
> Padding missing in all public structs

  FIXED

> 
> @@ +310,3 @@
> +  void        (*discovered)      (GstDiscoverer *discoverer,
> +                                  GstDiscovererInformation *info,
> +                  GError *err);
> 
> Maybe the GError should be const?

  In the callback yes. FIXED

(In reply to comment #3)
> Also the starting signal is never emitted it seems ;)

  FIXED

(In reply to comment #4)
> And is it correct to call discoverer_collect() for every message on the bus?

  It's not called on all messages, it's only done when handle_message tells us
we should shut down.

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




More information about the Gstreamer-bugs mailing list