[Bug 760003] gst_parse_launch: warn if we're still waiting to plug sub-pipelines after no-more-pads

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=760003

--- Comment #6 from Tim-Philipp Müller <t.i.m at zen.co.uk> ---
(Reviewing previous attachment, since I've started that, please ignore comments
that no longer apply :))

>Subject: [PATCH] parse-launch: warn when still waiting to plug sub-pipelines after no-more-pads

Nice! I think this patch is a good idea and should make gst-launch-1.0 a bit
more newbie friendly (at least you get some cryptic warning instead of it just
hanging).

I'm not sure if it should error out if it couldn't link all delayed links on
"no-more-pads".


>The parse-launch api performs delayed links, without any feedback on wheter the
>succeed ot not. If the fail the pipeline with most likely hang. Now we're also
>connecting to no-more pads. When this fires we post a element warning and
>cancel the delayed link.

Lots of typos, how about something like this :)

The parse-launch API automagically handles dynamic pads and performs delayed
linking as needed, without any feedback about whether the linking succeeded or
not however. If a delayed dynamic link can't be completed for whatever reason,
parse-launch will simply wait in case a suitable pad appears later. This may
never happen though, in which case the pipeline may just hang forever.

Try to improve this by connecting to the "no-more-pads" signal of any element
with dynamic pads and posting a warning message for any outstanding dynamic
links when "no-more-pads" is emitted."


Some minor suggestions for the code:

>+static void gst_parse_no_more_pads (GstElement *src, gpointer data)
>+{
>+  DelayedLink *link = data;
>+
>+  GST_ELEMENT_WARNING(src, CORE, PAD,

We should add a GstParseError code for that, like
GST_PARSE_ERROR_DELAYED_LINKING => PARSE, DELAYED_LINKING

>+    ("failed delayed linking %s:%s to %s:%s",
>+                   GST_STR_NULL (GST_ELEMENT_NAME (src)), GST_STR_NULL (link->src_pad),
>+                   GST_STR_NULL (GST_ELEMENT_NAME (link->sink)), GST_STR_NULL (link->sink_pad)),

What Sebastian said, this should just be a short and clear message without any
details, e.g. "Some delayed pad links could not be handled [or resolved or
whatever]."

>+    ("failed delayed linking %s:%s to %s:%s",
>+                   GST_STR_NULL (GST_ELEMENT_NAME (src)), GST_STR_NULL (link->src_pad),
>+                   GST_STR_NULL (GST_ELEMENT_NAME (link->sink)), GST_STR_NULL (link->sink_pad)));

Should try to make this a bit nicer too, since it's something newbies will see
in gst-launch. e.g. We should not have it print '(NULL)' here but instead just
not show the pad name. Providing the element factory name might be nice too for
the elements (i.e. 'matroskademux element named 'd'' instead of just 'd')

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.