[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 #31 from Edward Hervey <bilboed at gmail.com> 2010-09-16 13:52:54 UTC ---
Anything I haven't commented about is fixed in the git branch.

(In reply to comment #29)
> Nitpicks / thoughts based on version from this afternoon:
> 
> 
> ========== gst-libs/gst/gstdiscoverer.h:
> 
>  - 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?

  It applies per-URI. And it is the result of the discovery of the given URI.


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

  I'm just convinced about one thing. Provided the docs are clear... the naming
doesn't matter *that* much. Will flip a coin later on to decide on the naming.

> 
> 
> ========== gst-libs/gst/gstdiscoverer.c:
> 
>  - does GstDiscovererResult really need to be
>    flags? This seems awkward to me, and quite
>    unnecessary too.

  Without suggestions and rationale for what it should be instead it'll stay a
flag.

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

  The rationale is for providing UI a way to indicate processing is going on:
   * "ready" => remove/deactivate any 'busy'/'processing' icons
   * "starting" => add/activate the 'busy'/'processing' icons

  The progress of the discovery should be handled by the user/app imho.
Discoverer has no knowledge of whether more URI's will be added or not and just
result in some crufted progress report a-la-debian update (wee 100%, oh no 30%,
oh wait 40%, ah no back to 100%, ...).

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

  OK

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

  I'd prefer to stick with nanoseconds for consistency.

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

  If it can't create those elements (which if you don't have them you're
already massively screwed up)... it will fail as such for all pads.. and you'll
end up with error/not-linked on the bus. I did move the checks though to avoid
segfaults :)

(In reply to comment #30)
> Last thing: we're adding yet another microlib here, is that really necessary?
> Can't we stuff this into libgstpbutils or libgsttag ?

I seriously don't care about that :) Either way is fine by me.

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