GNOME Bugzilla – Bug 760003
gst_parse_launch: warn if we're still waiting to plug sub-pipelines after no-more-pads
Last modified: 2016-01-03 22:19:34 UTC
A classic mistake I just made is to forget converter-elements in a pipeline like this: https://gist.github.com/ensonic/0e6a7cfa1b6492a675cf Unfortunately this is juts stuck then. I think we could check for no-more-pads in grammar.y where we also do: g_signal_connect_data (src, "pad-added", G_CALLBACK (gst_parse_found_pad), data, (GClosureNotify) gst_parse_free_delayed_link, (GConnectFlags) 0); We'd pass the same, DelayedLink data as user-data. If either signal happens we remove the other signal handler (in the case of pad-added, only if linking worked). If no-more-pads happens we send a warning to the bus. gst-launch could catch such warnings. Could this work? Makes sense?
Created attachment 318169 [details] [review] handle no-more-pads In no-more-pads I cancel the delayed link. When discussing this on IRC, Tim mentioned that despite no-more-pads an element could still add more pads. If that is the case, I could just warn and keep the delayed-link active. Opinions?
With the patch: gst-launch-1.0 filesrc location=DeathValley_test_uncompressed_384x216.AVI ! decodebin name=d mp4mux name=m ! filesink location=/dev/null d. ! queue ! progressreport ! x264enc ! queue ! m.video_0 d. ! queue max-size-buffers=0 max-size-bytes=0 max-size-time=0 ! voaacenc ! aacparse ! queue ! m.audio_0 Setting pipeline to PAUSED ... Pipeline is PREROLLING ... WARNING: from element /GstPipeline:pipeline0/GstDecodeBin:d: failed delayed linking d:(NULL) to queue0:(NULL) Additional debug info: ./grammar.y(503): gst_parse_no_more_pads (): /GstPipeline:pipeline0/GstDecodeBin:d: failed delayed linking d:(NULL) to queue0:(NULL)
Review of attachment 318169 [details] [review]: Makes sense to me, just some cosmetic comments ::: gst/parse/grammar.y @@ +498,3 @@ + ("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)), The user-facing error message should probably be something less detailed and technical :) @@ +768,3 @@ * (e.g. fakesrc ! sinkreferencename.padname) **************************************************************/ +chain: openchain { $$=$1; Why these whitespace changes? @@ +812,3 @@ $$->src.pads = NULL; $$->sink.pads = NULL; + $$->caps = NULL; and here and in various other places
Yes, having a better user-facing message would be nice. The question is: how far do we want to go: - We could look at the caps and recommend to insert converters in the case of raw caps. - We could try to store the string rep of the delayed link in the struct to print which parts of the launch line we could not link - We can check the unlinked pads of src to print the actual name (but what if several of them failed). And still the question. Do we want to cancel the pending delayed-link?
Created attachment 318179 [details] [review] handle no-more-pads Updated the patch: - dropped whitespace change - mark message for i18n and reword
(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')
Thanks for the better commit messaged. Applied. Regarding the error code, instead of GstParseError it would need to ge GstParseLaunchError and likewise GST_PARSE_LAUNCH_ERROR_DELAYED_LINKING => PARSE_LAUNCH, DELAYED_LINKING to not confuse this with parsing errors as such. It's a bit longish but doable. Regarding the message, I was thinking about it, but I am concerned with i18n. The message would become quite modular; we'd build 'name-fragments' for each side of the link, like: if (pad_name) { src_name = g_strdup_sprintf (_("pad %s of %s element named %s"), ...); } else src_name = g_strdup_sprintf (_("%s element named %s"), ...); } and then combine the fragments: _("Linking %s to %s failed. Check that caps are compatible."), src_name, sink_name); If done right these name helpers could be added to gst_util and used elsewhere too. Ideally we do this step by step though.
Don't really mind about the naming of the error code, as you prefer. Just thought we should add a specific one in the existing domain instead of using the generic CORE|PAD one. Regarding i18n: we don't translate debug strings, so only the first part would need to be translated, not the 2nd part with all the details.
Created attachment 318189 [details] [review] handle no-more-pads Thanks for the reviews. This should be good to go now. I also have another patch that will improve the debug message in several cases.
Looks good to me, thanks :)