GNOME Bugzilla – Bug 625944
[pbutils] GstDiscoverer - API to discover metadata and stream information
Last modified: 2010-10-15 16:07:03 UTC
This is a backport of GstDiscoverer from gst-convenience so everybody using -base can use it.
Created attachment 167044 [details] [review] libs: New GstDiscoverer library This provides an object to 'discover' various information about URIs.
Review of attachment 167044 [details] [review]: 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?
Also the starting signal is never emitted it seems ;)
And is it correct to call discoverer_collect() for every message on the bus?
(In reply to comment #2) > Review of attachment 167044 [details] [review]: > > 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.
Created attachment 167059 [details] [review] libs: New GstDiscoverer library This provides an object to 'discover' various information about URIs.
(In reply to comment #5) > > @@ +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 ? The decodebin2 special handling for older versions. > > > > @@ +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). Oh, right. But please add a parameter to use a non-default main context. Then it's possible to run it in another thread with a main loop ;) The GstBus unit test has an example how to get signal watches for a non-default main context btw... > > ::: 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. Well, for GstMiniObject bindings already have the required magic... for new stuff they don't. Is there any reason not to use GstMiniObject? :)
Created attachment 167176 [details] [review] libs: New GstDiscoverer library This provides an object to 'discover' various information about URIs.
Comment on attachment 167176 [details] [review] libs: New GstDiscoverer library Fixed: * removed the version #if chunks * renamed 'running' to 'processing' * properly stop running pipeline when calling _stop() Still to be fixed: * Implementing async timeout * parameter for non-default GMainContext
Comments, part 1: [looking at header file / API only for now] - dependency on video/video.h for GstVideoFormat is unfortunate. Is that info really needed? If yes, for what / why / where? - GstStreamType / GstStream*Information: I think it would be better if GstDiscoverer didn't use that rather generic namespace, but instead namespaced these things like GstDiscovererStream*. Makes things longer, but more consistent IMHO. Could also use nicer / more natural names then, like GstDiscovererAudioStreamInfo. - GstStream*Information: shorten Information to Info everywhere? (Or just drop the Info completely?) - GstStreamType: no type (types?) for subtitle streams (would want to differentiate text / bitmap somehow) - docs: "Audio media stream" => "Audio stream" etc.; "successfull" -l ; "happend" => "happened" - GstStream{Audio,Container,Video}Information: I am not very much in favour of these structures at all. I would make them completely opaque and just provide accessor functions, e.g.: gst_discoverer_audio_stream_info_get_sample_rate(), gst_discoverer_audio_stream_info_get_channels(), etc. I doubt there is a significant performance impact, and it makes for much nicer API (IMHO) and is much more bindings friendly. - GstDiscovererInformation: same as above, make opaque and provide accessors for everything - GstStreamVideoInformation: GValues are weird here - Does anyone ever need to go back to the previous info? - We should wait for the TOC API to land before finalising this API, so we can make sure it integrates well. - GstDiscoverer: should be entirely opaque, with at most a GstDiscovererPrivate * member in the structure (There's more, but it's probably easier to discuss this in chunks)
(In reply to comment #10) > Comments, part 1: > > [looking at header file / API only for now] > > - dependency on video/video.h for GstVideoFormat > is unfortunate. Is that info really needed? If > yes, for what / why / where? For converting the video caps information into end-user-friendly information (i.e. all the GstVideoStreamInformation fields). > > - GstStreamType / GstStream*Information: I think it > would be better if GstDiscoverer didn't use that > rather generic namespace, but instead namespaced > these things like GstDiscovererStream*. Makes > things longer, but more consistent IMHO. Could > also use nicer / more natural names then, like > GstDiscovererAudioStreamInfo. > > - GstStream*Information: shorten Information to Info > everywhere? (Or just drop the Info completely?) How about the following: * GstDiscovererInfo * GstDiscovererStreamInfo (for the generic part) * GstDiscovererVideoInfo * GstDiscovererAudioInfo * GstDiscovererContainerInfo And then: * GST_DISCOVERER_STREAM_* for GstStreamType ? > > - GstStreamType: no type (types?) for subtitle streams > (would want to differentiate text / bitmap somehow) Good point, I guess that could be easily addable * GST_DISCOVERER_STREAM_SUBTITLE * GstDiscovererSubtitleInfo * gboolean bitmap > > - docs: "Audio media stream" => "Audio stream" etc.; > "successfull" -l ; "happend" => "happened" FIXED > > - GstStream{Audio,Container,Video}Information: > I am not very much in favour of these structures > at all. I would make them completely opaque and > just provide accessor functions, e.g.: > > gst_discoverer_audio_stream_info_get_sample_rate(), > gst_discoverer_audio_stream_info_get_channels(), etc. > > I doubt there is a significant performance impact, > and it makes for much nicer API (IMHO) and is much more > bindings friendly. > > - GstDiscovererInformation: same as above, make opaque > and provide accessors for everything > > - GstStreamVideoInformation: GValues are weird here Sebastian made the same comment. I'll switch them to pairs of guint32. > > - Does anyone ever need to go back to the previous info? I don't understand what you mean. > > - We should wait for the TOC API to land before finalising > this API, so we can make sure it integrates well. I'd only care if you can show me one example of a URI with multiple chapters that changes media type between chapters. > > - GstDiscoverer: should be entirely opaque, with at most > a GstDiscovererPrivate * member in the structure > sure. > > (There's more, but it's probably easier to discuss this in chunks)
Created attachment 167195 [details] [review] tools: New gst-discoverer standalone tool
Created attachment 167196 [details] [review] libs: New GstDiscoverer library This provides an object to 'discover' various information about URIs.
Comment on attachment 167196 [details] [review] libs: New GstDiscoverer library Items fixed: * Using a custom GMainContext * Async timeout handling * Some doc fixups
What about accessor functions and opaque stream info structs?
> I'll switch them to pairs of guint32 Can we make that simple guints? IMHO sized integers in API should be avoided where possible. > > - Does anyone ever need to go back to the previous info? > > I don't understand what you mean. I was wondering if the 'previous' member in GstStreamInformation is necessary/useful. > > - We should wait for the TOC API to land before finalising > > this API, so we can make sure it integrates well. > > I'd only care if you can show me one example of a URI with > multiple chapters that changes media type between chapters. There are two things that may be expressed this way: - alternatives (ie. same title/chapter/whatever available in different resolutions) - titles/tracks/chapters/parts Alternatives may be of different format/resolution/bitrate etc. But different tracks may also have different characteristics even if they're the same format, at the very least tags/bitrate/duration. And the user might want to know that there are different tracks in this single file in the first place, so the question would still be if/how to expose that.
(In reply to comment #15) > What about accessor functions and opaque stream info structs? I'm fine with opaque stream info structs but I'd like to avoid having a gazillion methods. How about : GstDiscovererStreamType gst_discover_information_get_stream_type(GstDiscoverStreamInfo *i); gboolean gst_discoverer_information_parse(GstDiscoverStreamInfo *i, GstDiscovererStreamInfo **prev, GstDiscovererStreamInfo **next, GstCaps **caps, GstTagList **tags, GstStructure **misc); And then _parse for all the subclasses (having them parse the common info might be a good idea too for easy bindings, but then they're going to have a huge number of arguments). (In reply to comment #16) > > I'll switch them to pairs of guint32 > > Can we make that simple guints? IMHO sized integers in API should be avoided > where possible. Sure. > > > > > - Does anyone ever need to go back to the previous info? > > > > I don't understand what you mean. > > I was wondering if the 'previous' member in GstStreamInformation is > necessary/useful. You'd need it if you were looking at the GstDiscovererInformation stream_list in order to known the predecessor. > > > > > - We should wait for the TOC API to land before finalising > > > this API, so we can make sure it integrates well. > > > > I'd only care if you can show me one example of a URI with > > multiple chapters that changes media type between chapters. > > There are two things that may be expressed this way: > > - alternatives (ie. same title/chapter/whatever available in different > resolutions) > - titles/tracks/chapters/parts > > Alternatives may be of different format/resolution/bitrate etc. > > But different tracks may also have different characteristics even if they're > the same format, at the very least tags/bitrate/duration. And the user might > want to know that there are different tracks in this single file in the first > place, so the question would still be if/how to expose that. Will the TOC API allow different URI for the various track/angle/whatev ? If so I might be inclined to have a way to inform the user that the URI contains multiple angles/scenes/tracks/... with the individual track-specific URIs which he can then re-add to discoverer.
> (...) but I'd like to avoid having a gazillion methods. How about : > .... > gboolean > gst_discoverer_information_parse(GstDiscoverStreamInfo *i, > GstDiscovererStreamInfo **prev, GstDiscovererStreamInfo **next, GstCaps **caps, > GstTagList **tags, GstStructure **misc); > > And then _parse for all the subclasses (having them parse the common info might > be a good idea too for easy bindings, but then they're going to have a huge > number of arguments). I find this rather ugly tbh. I still prefer the 'gazillion' methods approach, at least for the audio/video/container stream structures, because that's just nicer and more straight-forward API IMHO, and also easier to extend when there will be more things to extract in future (ie. just add a new getter vs. a parse_more()). People often have problems with out arguments (see parsing functions in core), and this is supposed to be *nice* end-user API, the bar should be low here.
(In reply to comment #17) > > Alternatives may be of different format/resolution/bitrate etc. > > > > But different tracks may also have different characteristics even if they're > > the same format, at the very least tags/bitrate/duration. And the user might > > want to know that there are different tracks in this single file in the first > > place, so the question would still be if/how to expose that. > > Will the TOC API allow different URI for the various track/angle/whatev ? If > so I might be inclined to have a way to inform the user that the URI contains > multiple angles/scenes/tracks/... with the individual track-specific URIs which > he can then re-add to discoverer. What do you mean with different URIs? The URI will still be the same, you can just send SELECT events to the elements to select some part of the TOC and the elements give you TOC events/messages to inform you about the TOC. The TOC entries can be incomplete, i.e. there could be subentries but in the generic case you can only know that after selecting the TOC entry and at least pre-roll it (in the case of chapters being detected during playback when they appear you only know them during playback of course). (In reply to comment #18) > > (...) but I'd like to avoid having a gazillion methods. How about : > > .... > > gboolean > > gst_discoverer_information_parse(GstDiscoverStreamInfo *i, > > GstDiscovererStreamInfo **prev, GstDiscovererStreamInfo **next, GstCaps **caps, > > GstTagList **tags, GstStructure **misc); > > > > And then _parse for all the subclasses (having them parse the common info might > > be a good idea too for easy bindings, but then they're going to have a huge > > number of arguments). > > I find this rather ugly tbh. I still prefer the 'gazillion' methods approach, > at least for the audio/video/container stream structures, because that's just > nicer and more straight-forward API IMHO, and also easier to extend when there > will be more things to extract in future (ie. just add a new getter vs. a > parse_more()). > > People often have problems with out arguments (see parsing functions in core), > and this is supposed to be *nice* end-user API, the bar should be low here. I'd prefer the million methods too. In the end I thought we wanted opaque structures because they're better to extend later and if you now add a method for parsing it completely we again have the extension problem ;) You could make the info structs GObjects with properties maybe, then you don't have millions of functions... And again on the topic of making the structs GstMiniObjects (or GObjects if you want properties...)... it would allow people to check if the stream info they have is in reality a audio stream info, i.e. look at the exact type of it and walk the type hierarchy and things like that.
I've pushed all my pending patches to this git repository : http://cgit.freedesktop.org/~bilboed/gst-plugins-base/ * Switched all stream structures to being miniobjects * switch par/framerate to pairs of guint * Renamed all structures accordingly * Made contents of GstDiscoverer private
* the StreamType is gone (you just check via the GType or the provided macros) * there's no longer an IMAGE type, instead there is a "gboolean is_image" field on the video streams
Ok, what's missing so far? Making the info structs opaque and subtitle support?
Just a couple of comments since Sebastian and Time seem to have covered most of what I had in mind already: * We should s/Information/Info for GstDiscovererInformation as well * It would also be nice to also be able to easily get a list of audio/video/subtitle(?)/other streams, because most clients would probably be interested in quickly accessing these (this is the case in Rygel where we're using the interface now)
(In reply to comment #23) > Just a couple of comments since Sebastian and Time seem to have covered most of > what I had in mind already: > > * We should s/Information/Info for GstDiscovererInformation as well Yah, could do that. > > * It would also be nice to also be able to easily get a list of > audio/video/subtitle(?)/other streams, because most clients would probably be > interested in quickly accessing these (this is the case in Rygel where we're > using the interface now) something like gst_discoverer_info_get_streams (discoverer, type) ?
(In reply to comment #24) [...] > > * It would also be nice to also be able to easily get a list of > > audio/video/subtitle(?)/other streams, because most clients would probably be > > interested in quickly accessing these (this is the case in Rygel where we're > > using the interface now) > > something like gst_discoverer_info_get_streams (discoverer, type) ? I was thinking more along the lines of get_audio_streams, get_video_streams, and so on (translates neatly into a property when generating bindings), but this would be fine too.
Branch updated with: * conversion from GstDiscovererInformation to GstDiscovererInfo * doc fixes * methods for accessing GstDiscoverInfo streams
Branch updated with: * sealing of all structures * getter methods for all fields
Latest version looks good to me
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_@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
Last thing: we're adding yet another microlib here, is that really necessary? Can't we stuff this into libgstpbutils or libgsttag ?
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.
> > - 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. It didn't seem necessary to me to provide either a 'rationale' in favour of simple error/status enums, nor a suggestion of the obvious alternative, namely an enum. Both seemed rather obvious. Flags are awkward and unusual (and, imho, unnecessary here) - you need to justify why they are prefered/needed, not the other way around. As for the other things - ok/dunno, maybe Sebastian or others have opinions that make these things go in one direction or another.
- use _discoverer_uri() and not _discoverer_uri_sync(), that follows glib/gio style - really put it in pbutils, especially now that the video dependency is gone. all these micro libs are not going to reduce the memory footprint and performance of gstreamer (but the same goes for all the micro plugins which should better be grouped...) - IMHO GstDiscovererResult should be a simple enum and not flags. error is an unspecified error and the 4 other failures are specified errors. they feel disjoint - What about the compatiblity with the GstToc stuff? Edward, did you look at that patch and think about the relationship? :) It would be bad if discoverer caused problems for the toc stuff or the other way around and we have one of the APIs stable already. Other than that I think it's all ok and can be merged.
Oh, and: - Subtitle support? :) (can be added later but would still be nice) - ownership of the return values for accessors is not obvious, at least for gst_discoverer_container_info_get_streams, gst_discoverer_stream_info_get_misc, gst_discoverer_stream_info_get_tags, gst_discoverer_stream_info_get_caps, gst_discoverer_stream_info_get_next, gst_discoverer_stream_info_get_previous. Does the caller get a copy/ref? The code says "no" but the docs doesn't mention this and the return types are not marked as const either. It makes sense to have them still owned by the info object but it should be documented at least.
(In reply to comment #33) > - use _discoverer_uri() and not _discoverer_uri_sync(), that follows glib/gio > style > Makes sense. Maybe discoverer_uri_async() then for the alternative ? > - really put it in pbutils, especially now that the video dependency is gone. > all these micro libs are not going to reduce the memory footprint and > performance of gstreamer (but the same goes for all the micro plugins which > should better be grouped...) It will still need it for compilation though, the only thing removed is exporting libgstvideo API in the headers. Will move it in next commits. > > - IMHO GstDiscovererResult should be a simple enum and not flags. error is an > unspecified error and the 4 other failures are specified errors. they feel > disjoint Already changed that locally, will come in next push. > > - What about the compatiblity with the GstToc stuff? Edward, did you look at > that patch and think about the relationship? :) It would be bad if discoverer > caused problems for the toc stuff or the other way around and we have one of > the APIs stable already. We can add a _discoverer_uri_chapter{_async}(uri, chapter) later on. It would need to take subtlely different code paths since it would require prerolling and then going to the required chapter. > > > Other than that I think it's all ok and can be merged. (In reply to comment #34) > Oh, and: > > - Subtitle support? :) (can be added later but would still be nice) Argh, right. The advantage with having everything sealed in the new API is that we can trivially add it. I don't see any issues with that (it's just a different media type, but has behaviour similar to audio/video/container streams). I'm not that aware of the various properties of a subtitle stream, so would need some input about what would be needed to export in a GstDiscovererSubtitleStreamInfo. > > - ownership of the return values for accessors is not obvious, at least for > gst_discoverer_container_info_get_streams, gst_discoverer_stream_info_get_misc, > gst_discoverer_stream_info_get_tags, gst_discoverer_stream_info_get_caps, > gst_discoverer_stream_info_get_next, gst_discoverer_stream_info_get_previous. > Does the caller get a copy/ref? The code says "no" but the docs doesn't mention > this and the return types are not marked as const either. It makes sense to > have them still owned by the info object but it should be documented at least. I've been doing fixups for those in my local branch (basically const-ing when it would require a copy, or adding a ref when possible), will comme in my next push. Thanks for the reviews
All fixes/moves/... should now be in my discoverer branch.
commit d4b0274d89a116954b93130d469d84df613f7b3f Author: Edward Hervey <bilboed@bilboed.com> Date: Mon Sep 20 11:24:10 2010 +0200 tools: Standalone tool for discovering media file properties Fixes #625944 commit 440865f1272b48af11ddae0d8868f34e3994d72d Author: Edward Hervey <bilboed@bilboed.com> Date: Mon Sep 20 11:23:36 2010 +0200 win32: Update with symbols from GstDiscoverer Fixes #625944 commit 0fac936525b1ff8933414d49a88d64ca3820d88f Author: Edward Hervey <bilboed@bilboed.com> Date: Mon Sep 20 11:23:17 2010 +0200 docs: Documentation for new pbutils GstDiscoverer Fixes #625944 commit 30b3cf88231ffd06ac7b2a50bd9d5e54691da441 Author: Edward Hervey <bilboed@bilboed.com> Date: Mon Sep 20 11:22:32 2010 +0200 pbutils: New Discoverer utility Fixes #625944
I guess this can be closed? I don't see any open issues.