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

Mon Sep 13 10:07:45 PDT 2010

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

--- Comment #29 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2010-09-13 17:07:37 UTC ---
Nitpicks / thoughts based on version from this afternoon:

========== tools/gst-discoverer.c:

 - needs updating for _start() API change (no more main context argument)

 - the _FOO_QUARK stuff is (thankfully) unused and can be removed

 - should _gst_stream_type_to_string() be turned into
   public _get_nick()-type API? (if yes, should return
   lower-case results like other _get_nick() functions)

 - should _gst_video_format_to_string() be turned
   into public _get_nick()-type API?

 - vararg macros are not portable:
   #define my_g_string_append_printf(str, format, ...) \
   g_string_append_printf (str, "%*s" format, 2*depth, " ", ##__VA_ARGS__)

========== gst-libs/gst/discoverer/Makefile.am:

 - this looks wrong (builddir vs. srcdir), it's
   there for the video/video.h include, no?

     --add-include-path=$(builddir)/../video \

   (two occurances)

 - this also looks wrong:

     nodist_libgstdiscoverer_ at GST_MAJORMINOR@include_HEADERS = \
    gstdiscoverer-enumtypes.h    \
    gstdiscoverer-private.h

   I think the -private.h header needs to be disted,
   but not installed, so maybe noinst_* target instead?   

========== gst-libs/gst/gstdiscoverer-private.h:

 - I think all the GST_PADDING is no longer needed

========== gst-libs/gst/gstdiscoverer.h:

 - all gtk-doc chunks should have 'Since: 0.10.31' markers

 - GstDiscovererResult flags: see comments below

 - gst_discoverer_info_get_result(info):
   why is this on the *Info object, and not on the main
   GstDiscoverer object? It's not really a 'result', is
   it, more like a status or somesuch? Because it
   applies per-uri?

 - GstDiscoverer: instance struct should still
   have some padding padding

 - typedef GstMiniObjectClass GstDiscovererStreamInfoClass;
   etc. - I think that should be done with
   struct _GstDiscovererStreamInfoClass and so on just for
   consistency, even if it's not needed.

 - could use some formatting love (function decls
   alignment, few spaces/newlines here and ther)

 - is gst_discoverer_info_copy() needed? What for?
   (aren't these objects immutable basically?)

 - There's a gst_discoverer_info_unref() but not _ref()

 - gst_discoverer_append_uri() should take a const gchar *

 - gst_discoverer_discover_uri() should take a const gchar *

 - API naming - random suggestion: how about:

   _discover_uri() => _discover_uri_sync()

   _append_uri()   => _discover_uri_async()
   _start()        => _start_async()
   _stop()         => _stop_async()

   Not entirely convinced these are better myself
   yet, just think the current naming could be
   improved upon.

========== gst-libs/gst/gstdiscoverer.c:

 - does GstDiscovererResult really need to be
   flags? This seems awkward to me, and quite
   unnecessary too.

 - signals are a bit counterintuitive IMHO:
   the "ready" signal is emitted "when all pending
   URIs have been processed", whereas in GStreamer
   READY state is before anything happens, so
   that's confusing. And I don't quite see the point
   of the "starting" signal, since it's the user
   who's calling gst_discoverer_start() anyway, no?

 - I think those three signals should be folded
   into two signals: for example: keep the
   "discovered" signal as it is now, but add a
   "progress" signal, which would have as argument
   the URI about to be discovered, and some kind of
   progress indication, e.g. X out of N or a
   percentage value or number of items left to
   discover. When discoverer is done, it could
   call this with a NULL URI and 100% progress,
   or something like that?

 - if not something like the above, then "ready"
   should at least be renamed to "finished" or
   so IMHO.

 - _INFORMATION_QUARK is unused

 - looks like gst_discoverer_init() can fail if
   "uridecodebin" isn't available, or couldn't be
   created for some reason. The code should at the
   very least not crash if this is the case, but
   better also throw an error then later. We can't
   use GInitable here yet, but we can either fix
   up the API so we can add that after the next
   release (meaning: add a GError ** argument to
   gst_discoverer_new() and making it return
   NULL on error), or move that into _start()
   and allow _start() to fail maybe?

 - elements, pipelines, pads and GstBus should
   be unreffed with gst_object_unref() really
   (it's just what we do everywhere else)

 - timeout: how about we make the timeout a
   normal unsigned integer and denominate it
   in milliseconds instead? (just an idea,
   don't know if we need nanosecond precision
   here, it seems to confuse people. But of
   course using nanosecs is more consistent
   with other low-level GStreamer API)

 - gst_discoverer_get_property(): if you lock
   in _set_timeout(), then you should probably
   also lock when reading the timeout, no?

 - uridecodebin_pad_added_cb():
     - handle ps->sink == NULL correctly (ie.
       move the NULL check before the
       g_object_set())
     - in the error handler free the GSlice with
       g_slice_free() and not g_free()
     - is an error here propagated somewhere or
       will we just time out, or rely on a
       not-linked internal stream error?

 - gst_structure_id_get ((GstStructure *) st, ...)
   cast not / no longer needed

 - still dislike the dependency on <video/video.h>
   just for GstVideoFormat, which doesn't even
   provide particularly useful information (just
   whatever the decoder defaults to as output
   format, not even the actual subsampling/format
   of the underlying video/image data). Especially
   in the API. A video format 'nick' would do just
   as well, no? (could add _to/from nick to gstvideo
   if it's actually useful in any way other than
   printing something). The two _parse() functions
   for video caps can trivially done without the lib
   too, but that's implementation anyway, so care
   less about that.

 - handle_message(): would be nice to log
   gerr->message as well

 - g_list_append() to priv->pending_uris is not
   exactly great, but implementation detail that
   can be fixed later

========== gst-libs/gst/gstdiscoverer-types.c:

 - gst_discoverer_info_get_*_streams():

    * Returns: A #GList of matching #GstDiscovererStreamInfo.
    * The caller should free it with #g_list_free.

   Shouldn't we add a ref to the items in the list, and add
   a gst_discoverer_info_free_streams() function that does
   the g_list_foreach(unref)+g_list_free() ?

 - similar to the above: accessors like
   gst_discoverer_stream_info_get_caps() etc.:

     * Returns: the #GstCaps of the stream.

   it's not clear from the API+docs whether this
   adds a ref or not, and if the caller should unref/free
   afterwards or not. Maybe sprinkle some const if not?

 - should include "config.h" as first thing

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