GNOME Bugzilla – Bug 751450
parse/grammar.y: Allow multiple links to happen.
Last modified: 2016-11-02 01:11:49 UTC
In such a case : uridecodebin ! encodebin , if the encodebin has multiple profiles compatible with the decodebin, it seems legit to let them happen. Previously, after one delayed link was successfully done, the pad-added callback was disconnected.
Created attachment 306026 [details] [review] parse/grammar.y: Allow multiple links to happen.
I'm pretty wary of changing that - there's lots of gst-launch scripts out there, and I know I've written some that expect only one link. It might be better to add a new token to the lexer?
What token would you think about exactly ? "!!" maybe ? The thing is I filed that as enhancement, but it could nearly be considered a bug :) If we chose to add a new token, I'd be concerned about its discoverability, but I understand your concern, even though it kind of reminds me of https://xkcd.com/1172/ :)
What would happen if you had a capsfilter between the decodebin and encodebin? Would it detect that it should link all streams that match that caps?
(In reply to Mathieu Duponchelle from comment #3) > What token would you think about exactly ? "!!" maybe ? Existing pipeline operators are all single char. It's possible to match a regex instead, with a slightly bigger change to parse.l. I'm not sure what single character might be a good one, so '!!' is probably fine. Else, demux / multiqueue maybe? > The thing is I filed that as enhancement, but it could nearly be considered > a bug :) Not really - it does one thing, you want it to do another. Not really a bug - more a difference of opinion. > If we chose to add a new token, I'd be concerned about its discoverability, > but I understand your concern, even though it kind of reminds me of > https://xkcd.com/1172/ :) I'm more concerned about changing 10 year old behaviour causing even harder-to-discover bugs. (In reply to Thiago Sousa Santos from comment #4) > What would happen if you had a capsfilter between the decodebin and > encodebin? Would it detect that it should link all streams that match that > caps? Should do - a filtered link in the grammar just calls gst_element_link_pads_filtered(), which will create a new capsfilter element for each link.
"!!" will not work in bash actually : !! Refer to the previous command. This is a synonym for `!-1'.
Created attachment 306060 [details] [review] parse/grammar.y: Allow multiple links to happen. By introducing a new syntax : element1 ? element2 In such a case : uridecodebin ? encodebin , if the encodebin has multiple profiles compatible with the decodebin, multiple links will be created. With '!' , after one delayed link is successfully done, the pad-added callback is disconnected. The reason this isn't the default behaviour for '!' is legacy, as it would make previously-working scripts malfunction. This should be made the default behaviour at some point.
I attached a tentative patch with a new token, '?', if everyone is all right with it I'll update the documentation. It would also be comforting if someone with more flex / bison experience than I reviewed this patch.
For the record I don't think this should become the default behavior at some point. This is an interesting addition but being able to specify 'only link once' is very useful, specially because this is a test and debugging tool. Having 'multiple-linking' by default wouldn't allow having a single link and that defeats the flexibility of the tool for testing/debugging.
(In reply to Thiago Sousa Santos from comment #9) > For the record I don't think this should become the default behavior at some > point. Definitely not the default behavior, yes :)
Yep, didn't think this through :) Are you folks OK with introducing that new syntax otherwise ?
I think we should bike shed exactly which symbol to use until everyone gets bored. I vote for demux + multiqueue + muxer or demux : multiqueue : muxer (for constrast, vs demux ! multiqueue ! muxer)
I thought about * for a moment, but it expends in a shell. So I'm offcially removing this possibility ;-P If we think of regex, + would be one or more, ? usually means optional. I believe for a pipeline to work, one or more link will have to happen, makes + possibly a better choice.
I like either + or : over ?, but I'll survive with any. There's one thing in the patch that worth noting: with the new link operator, you can only put it on both sides of a capsfilter or neither. I don't think gst-launch-1.0 videotestsrc ! video/x-raw,format=I420 ? xvimagesink will be parsed. Semantically, it probably makes sense that if either side of the filter is "link_all", then it should link multiple times - it requires creating multiple capsfilter elements either way.
Created attachment 306122 [details] [review] parse-launch: Support linking all pads with new operator Introduce a new operator ':' - e.g. element1 ':' element2 For example, 'uridecodebin : encodebin' - if the encodebin has multiple profiles compatible with the decodebin, multiple links will be created. With '!' , after one delayed link is successfully done, the pad-added callback is disconnected.
Created attachment 306124 [details] [review] parse-launch: Support linking all pads with new operator Introduce a new operator ':' - e.g. element1 ':' element2 For example, 'uridecodebin : encodebin' - if the encodebin has multiple profiles compatible with the decodebin, multiple links will be created. With '!' , after one delayed link is successfully done, the pad-added callback is disconnected.
This sort of stalled out after the last patch. Any opinions on whether to apply it?
I would prefer to punt this until after 1.6
Going to push this after feature freeze is over.
Comment on attachment 306124 [details] [review] parse-launch: Support linking all pads with new operator Setting patch status accordingly, so we don't forget
Comment on attachment 306124 [details] [review] parse-launch: Support linking all pads with new operator Does not apply cleanly
Created attachment 338920 [details] [review] parse-launch: Support linking all pads with new operator Introduce a new operator ':' - e.g. element1 ':' element2 For example, 'uridecodebin : encodebin' - if the encodebin has multiple profiles compatible with the decodebin, multiple links will be created. With '!' , after one delayed link is successfully done, the pad-added callback is disconnected.
pushed: commit 5a5ae1be1fc22ab538fb74984a99ce9d8f708f80 Author: Jan Schmidt <jan@centricular.com> Date: Fri Jun 26 03:29:27 2015 +1000 parse-launch: Support linking all pads with new operator Introduce a new operator ':' - e.g. element1 ':' element2 For example, 'uridecodebin : encodebin' - if the encodebin has multiple profiles compatible with the decodebin, multiple links will be created. With '!' , after one delayed link is successfully done, the pad-added callback is disconnected. https://bugzilla.gnome.org/show_bug.cgi?id=751450