GNOME Bugzilla – Bug 703405
parse: Allow creating plain elements instead of single-element bins
Last modified: 2013-08-19 09:36:58 UTC
In playbin's `pad_added_cb`, we do this to get sink pads from the stream combiners: sinkpad = gst_element_get_request_pad (combine->combiner, "sink_%u") The problem is that if `combine->combiner` is a bin, then it will always return NULL. These pipelines both stall because of this: gst-launch-1.0 playbin uri='https://dl.dropboxusercontent.com/s/x03ogh3epbd6lfq/example.mkv?token_hash=AAGW4CSrXYj9fNjD0CpO465zuXLRY0WEXcUy2lxQ5IOYJg&dl=1' audio-stream-combiner="adder" gst-launch-1.0 playbin uri=http://ftp.nluug.nl/ftp/graphics/blender/apricot/trailer/Sintel_Trailer1.480p.DivX_Plus_HD.mkv text-stream-combiner="funnel" This can be worked around in normal usage, but it's fairly annoying that the stream combiners can't be used in gst-launch. I'm not sure the best way to handle this though. Iterating through the elements in the bin until I find out that will give me a pad? Requesting sink pads from the first element and the src pad from the last? :\
You would need a GstBin subclass that elements GstElement::request_new_pad to implement the implicit interface that playbin expects from the stream-combiner elements. I can't think of any automagic that would look inside bins to do this without breaking anything or causing unintuitive behaviour. As such I think this is not a bug at all, just a limitation of gst-launch.
One could modify gst_parse_bin_from_description() so that it does not create a bin any more if there's just one single element and a bin hasn't been requested explicitly.
Created attachment 248179 [details] [review] Fail gracefully if custom combiner doesn't return a request pad This patch makes this case error out instead of hanging: $ gst-launch-1.0 playbin uri=http://ftp.nluug.nl/ftp/graphics/blender/apricot/trailer/Sintel_Trailer1.480p.DivX_Plus_HD.mkv text-stream-combiner="funnel" Setting pipeline to PAUSED ... Pipeline is PREROLLING ... Redistribute latency... 0:00:00.752194504 27788 0x7f48580bd940 ERROR playbin gstplaybin2.c:3178:pad_added_cb:<playbin0> failed to get request pad from combiner 0x1414120 0:00:00.752367212 27788 0x7f48580bd940 ERROR playbin gstplaybin2.c:3178:pad_added_cb:<playbin0> failed to get request pad from combiner 0x1414120 0:00:00.752535232 27788 0x7f48580bd940 ERROR playbin gstplaybin2.c:3178:pad_added_cb:<playbin0> failed to get request pad from combiner 0x1414120 0:00:00.752686087 27788 0x7f48580bd940 ERROR playbin gstplaybin2.c:3178:pad_added_cb:<playbin0> failed to get request pad from combiner 0x1414120 0:00:00.752889619 27788 0x7f48580bd940 ERROR playbin gstplaybin2.c:3178:pad_added_cb:<playbin0> failed to get request pad from combiner 0x1414120 0:00:00.753039442 27788 0x7f48580bd940 ERROR playbin gstplaybin2.c:3178:pad_added_cb:<playbin0> failed to get request pad from combiner 0x1414120 0:00:00.753218202 27788 0x7f48580bd940 ERROR playbin gstplaybin2.c:3178:pad_added_cb:<playbin0> failed to get request pad from combiner 0x1414120 0:00:00.753373756 27788 0x7f48580bd940 ERROR playbin gstplaybin2.c:3178:pad_added_cb:<playbin0> failed to get request pad from combiner 0x1414120 ERROR: from element /GstPlayBin:playbin0: Internal playbin error. Additional debug info: gstplaybin2.c(3350): no_more_pads_cb (): /GstPlayBin:playbin0: Failed to link combiner to sink. Error -1 ERROR: pipeline doesn't want to preroll. Setting pipeline to NULL ... Freeing pipeline ...
(In reply to comment #3) Or maybe it would be better to print the error message and then let it hang, or remove the combiner and retry with the default input-selector..
I think erroring out is the best solution, make it an GST_ELEMENT_ERROR() though. Additionally implementing what Tim said in comment #2 would be a good idea too
I also think erroring out is better. If someone specifies the selector, they probably want it used no matter what.
Created attachment 248180 [details] [review] Use GST_ELEMENT_ERROR instead I'll take a look at gst-launch also, but this at least "fixes" it in playbin. It's still kind of frustrating that commands like this won't work, even with gst-launch changes: gst-launch-1.0 playbin uri=... text-stream-combiner="funnel ! identity dump=true"
Created attachment 248182 [details] [review] (not working) When parsing a bin with a single element, just return the element instead of creating a bin So this changes the parser to return the element instead of a bin when parsing a single-element bin. It causes some warnings though: gst-launch-1.0 playbin uri='https://dl.dropboxusercontent.com/s/x03ogh3epbd6lfq/example.mkv?token_hash=AAGW4CSrXYj9fNjD0CpO465zuXLRY0WEXcUy2lxQ5IOYJg&dl=1' audio-stream-combiner="adder" (gst-launch-1.0:28056): GStreamer-CRITICAL **: gst_bin_find_unlinked_pad: assertion `GST_IS_BIN (bin)' failed (gst-launch-1.0:28056): GStreamer-CRITICAL **: gst_bin_find_unlinked_pad: assertion `GST_IS_BIN (bin)' failed
+ Trace 232176
I'll look into it more if I have time.
(In reply to comment #8) > It causes some warnings though: More specifically, the patch appears to work perfectly, it just has those warnings which I haven't figured out yet.
Changing gst_parse_bin_from_description() to not return a bin is an API break, it should be behind a new flag for gst_parse_bin_from_description_full().
Comment on attachment 248180 [details] [review] Use GST_ELEMENT_ERROR instead commit d3acb2b01a7a05dc0276f95f4830d5879bd0e42c Author: Brendan Long <self@brendanlong.com> Date: Mon Jul 1 12:52:43 2013 -0600 playbin: Post an error message if a stream combiner doesn't return a request pad.
(In reply to comment #10) > Changing gst_parse_bin_from_description() to not return a bin is an API break, > it should be behind a new flag for gst_parse_bin_from_description_full(). Agreed
(In reply to comment #8) > Created an attachment (id=248182) [details] [review] > (not working) When parsing a bin with a single element, just return the element > instead of creating a bin > > So this changes the parser to return the element instead of a bin when parsing > a single-element bin. It causes some warnings though: > > gst-launch-1.0 playbin > uri='https://dl.dropboxusercontent.com/s/x03ogh3epbd6lfq/example.mkv?token_hash=AAGW4CSrXYj9fNjD0CpO465zuXLRY0WEXcUy2lxQ5IOYJg&dl=1' > audio-stream-combiner="adder" > > (gst-launch-1.0:28056): GStreamer-CRITICAL **: gst_bin_find_unlinked_pad: > assertion `GST_IS_BIN (bin)' failed > (gst-launch-1.0:28056): GStreamer-CRITICAL **: gst_bin_find_unlinked_pad: > assertion `GST_IS_BIN (bin)' failed Well, check where this happens with gdb and G_DEBUG=fatal_warnings :) Something is still assuming that it always gets a bin from that function.
> Changing gst_parse_bin_from_description() to not return a bin is an API break, > it should be behind a new flag for gst_parse_bin_from_description_full(). Yes, you're right. What I actually meant was to make the GValue deserializer do that, which you could argue is also an API break strictly speaking, but I'm not sure I can think of a scenario where it would matter (can you?)
Created attachment 248247 [details] [review] Add GST_FLAG_NO_SINGLE_ELEMENT_BINS Is this what you had in mind? * Added flag GST_PARSE_FLAG_NO_SINGLE_ELEMENT_BINS * If that flag is passed to gst_parse_bin_from_description_full, it will return the element that would've been put in a single-element bin. * That flag is used in gstparse.c to avoid superfluous bins.
I'm not sure what you mean by changing the GValue deserializer though?
> I'm not sure what you mean by changing the GValue deserializer though? When in a gst-launch pipeline you do "playbin audio-stream-combiner=xyz" then the gst-launch parser will see the property assignment "audio-stream-combiner=xyz", and it will see that the property is of type GstElement *, and will need to figure out how to make a GstElement * of the string 'xyz'. That's done using a registered deserialization function for GstElement <-> string. That function will use gst_parse_bin_from_description() internally to do this conversion.
I checked gstvalue.c and there's nothing special for GstElement..
The special code for elements is in gst/parse/grammar.y
Oh ok. My patch already changes GST_BIN_MAKE in grammar.y.
Review of attachment 248247 [details] [review]: ::: gst/gstutils.c @@ +3068,3 @@ + } else { + bin = GST_BIN (gst_bin_new (NULL)); + if (!gst_bin_add (bin, element)) { Does this provide exactly the same behaviour as before? I remember that there is something in this code that would expose all unlinked pads inside the bin as ghostpads on the bin
(In reply to comment #21) > I remember that there > is something in this code that would expose all unlinked pads inside the bin as > ghostpads on the bin That happens right after the code you quoted: /* find pads and ghost them if necessary */ if (ghost_unlinked_pads) { if ((pad = gst_bin_find_unlinked_pad (bin, GST_PAD_SRC))) { gst_element_add_pad (GST_ELEMENT (bin), gst_ghost_pad_new ("src", pad)); gst_object_unref (pad); } if ((pad = gst_bin_find_unlinked_pad (bin, GST_PAD_SINK))) { gst_element_add_pad (GST_ELEMENT (bin), gst_ghost_pad_new ("sink", pad)); gst_object_unref (pad); } } There's one thing that may be different, which is that attributes used to get set on the bin and now get set on the element: (grammar.y, GST_BIN_MAKE) /* set the properties now */ \ for (walk = assign; walk; walk = walk->next) \ gst_parse_element_set ((gchar *) walk->data, element, graph); \
(In reply to comment #22) > (In reply to comment #21) > > I remember that there > > is something in this code that would expose all unlinked pads inside the bin as > > ghostpads on the bin > > That happens right after the code you quoted: Alright, thanks :) > There's one thing that may be different, which is that attributes used to get > set on the bin and now get set on the element: > > (grammar.y, GST_BIN_MAKE) > /* set the properties now */ \ > for (walk = assign; walk; walk = walk->next) \ > gst_parse_element_set ((gchar *) walk->data, element, graph); \ These are the properties that are set in the pipeline string? In that case it shouldn't matter :)
GST_BIN_MAKE() seems to be also used for some bin syntax I'm not even aware of... and don't really understand right now. There seems to be some syntax to create bins of a specific type and also assign properties of that bin. Something related to the () syntax that is used to create bins. That would of course break if we don't create a bin anymore if only a single element is contained in it. The explicit () syntax should always cause the creation of a bin.
Brendan?
I looked into it more and it looks like GST_BIN_MAKE is the wrong function to change. For bins as properties of elements, it uses gst_parse_bin_from_description(), which uses "gst_parse_launch_full". The real change that needed to be made was this: if (flags & GST_PARSE_FLAG_NO_SINGLE_ELEMENT_BINS) { desc = g_strdup_printf ("%s", bin_description); } else { desc = g_strdup_printf ("bin.( %s )", bin_description); }
Created attachment 251908 [details] [review] Patch which doesn't change GST_BIN_MAKE
Created attachment 251910 [details] [review] Don't duplicate the bin description unnecessarily Of course, I realized right after submitting this that it was doing useless g_strdup_printf's. Here's a better one.
commit 48319d4be2365559d785a55c5569ff9a629e9575 Author: Brendan Long <b.long@cablelabs.com> Date: Mon Jul 1 14:04:46 2013 -0600 parse: Add GST_FLAG_NO_SINGLE_ELEMENT_BINS This makes gst_parse_bin_from_description() return an element instead of a bin if there's only one element. Also changed gstparse.c to use this, so gst-launch won't create superfluous bins. https://bugzilla.gnome.org/show_bug.cgi?id=703405