[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