GNOME Bugzilla – Bug 707605
streamiddemux: New "reverse-funnel" element
Last modified: 2015-03-12 14:41:17 UTC
Created attachment 254223 [details] [review] This patch file contains the diff information with reverse funnel. Overview: For the multi-stream support, I had failed to find any of opposite elements(1:N) to funnel element(N:1) http://gstreamer-devel.966125.n4.nabble.com/Is-there-any-proper-element-that-is-opposited-of-funnel-td4661732.html Recentely, I found that it is possible to confirm stream-id by pad, so I decided to use output-selector element from version of gst-core 1.1.2. Actual Results: in normal case, output-selector is just possible to 1:N streams. But, they could not guarantee that each streams flow their's src pad. Expected Results: 1. 'reverse-funnel-mode' of property is set as TRUE. (it is not permitted to create src pads by requesting from external) 2. When a GST_EVENT_CAPS event is received, create a src pad. And then maps it with stream-id. 3. When chain function is called, gets a src pad matching with stream-id. And then calls gst_pad_push().
Review of attachment 254223 [details] [review]: IMHO it's semantically a bit weird to have this in output-selector because output-selector has REQUEST pads and not SOMETIMES pads. Apart from that you should collect the stream-start events and get the stream ids from there, and whenever a stream-start event with a new stream id arrives you would create a new pad. All events/buffer after a stream-start event belong to that single stream id until there comes a new stream-start event with a different stream id.
So yes, after asking on IRC others agree that this should be a separate element. Should also make the code quite a bit easier overall. Keeping this assigned to gstreamer core as it should be there next to the funnel element.
Reverse funnel means stream-id-demultiplexer ?
Yes
According you guys comments, now I have been creating a new element for reverse-funnel. But, I don't get a good name for this element. 'reversefunnel' is a proper name? If I get a proper name, I will upload my patches.
reverse-funnel sounds good enough for now, we can always rename it easily before integrating it :)
Created attachment 254672 [details] this is a reversefunnel element. This is my reversefunnel element. I just upload reversefunnel.h and reversefunnel.c files. Please review this element that I upload. And I am not sure that this is proper way to upload. If it is weird, please let me know.
The best way would be a git patch against gstreamer core to integrate it, plus a unit test (see tests/check)
You could call it streamiddemux. I'm curious, what's the use-case for this? Why do do funnel then separate the streams ? You should probably create the new pad either on the stream-start event or the segment event, the CAPS event is not always present. You probably want g_str_hash/equal and copy the stream-id string into the hash table. Why do you have special handling for the caps/stream-start? You can just forward all sticky events when you switch pads, and just listen to the stream-start event do to the pad switch. Why do you update a GstSegment but never use it?
Yes, this code should and can be simplified a lot. Just trigger everything based on the stream-start events, which contain the actual stream id... as mentioned in comment 1. No handling of caps or segments or anything else is needed, can all just be passthrough to the currently active pad, which would be selected based on the last stream-start event.
Thanks for your comments. This is a kind of example for use-case. src - convert - capsfilter - enc - mux \ / filesink funnel - rfunnel src - convert - capsfilter - enc - mux / \ filesink src means autiotestsrc convert means audioconvert enc means vorbisenc mux means oggmux rfunnel means reversefunnel As I mentioned first, I want to support multiple-stream and use multiple audiosink and videosink at the same time. I think that output-selector could not guarantee that each streams flow their's src pad. So I want to guarantee it using stream-id.
Some other case like this. there is a file that contains 1-stream of video, 1-stream of audio and 8 subtitles. / input-selector - audiosink uridecodebin - input-selector - videosink `- funnel - rfunnel - appsink `- appsink `- appsink `- appsink `- appsink `- appsink `- appsink `- appsink
What's between funnel and rfunnel in your examples? If there's nothing in between it doesn't make much sense imho
I know that stream-start event has actual stream-id and I can know that when another streams are coming from stream-start or segment events. But If there are over 2 streams, How can I choice a proper srcpad imho?
The same way you do it now, just do it based on the stream-start events. Get the stream-id, if it belongs to an already created srcpad use that srcpad, otherwise create a new one.
I am a little bit confusing about your comment. sorry... Anyway according to your comments, I will use stream-start or segment event, not caps event. And you means that hash table is required, right?
The segment event does not matter at all for you. A hash table or some data structure to map the stream ids to the srcpads is required, yes
Thank you for your comment. I think that you are a little bit misunderstanding in my use-case. there is a file that contains 1-stream of video, 1-stream of audio and 8 subtitles. In case of multiple-stream, use the funnel instead of input-selector. And each streams flow there's sink element at the same time using reverse-funnel. [ this scope is like playsink bin] / input-selector - audiosink uridecodebin - input-selector - videosink `- funnel - rfunnel - appsink `- appsink `- appsink `- appsink `- appsink `- appsink `- appsink `- appsink So, I am not sure that which is required between funnel and reversefunnel? Please let me know.
Yes, that makes sense. Thanks
Created attachment 254906 [details] [review] I upload the stream-id-demultiplexer element with test code. Thanks for you guys help. I changed the element name from reversefunnel to streamiddemux. Because I think this works by stream-id. I upload the stream-id-demultiplexer element with test code. please check and review my patch. As you said to me, I checked stream-id when stream-start event came. But At the first time, stream-id was empty. but It is working fine from next time. Obviously segment event always has stream-id. I am not sure... But, Is this a kind of bug? So I just decided to use segment event.
Review of attachment 254906 [details] [review]: Just took a short look, but: ::: plugins/elements/gststreamiddemux.c @@ +331,3 @@ + case GST_EVENT_STREAM_START: + { + if (gst_pad_get_stream_id (pad) != NULL) { gst_event_parse_stream_start (event, &stream_id) will give you the stream-id. You can then create a pad if necessary, and select it as the active one. From this point on it will stay the active pad until the next stream-start event arrives @@ +341,3 @@ + { + /* Send caps to all src pads */ + res = gst_pad_event_default (pad, parent, event); Only to the active pad, probably for all events except flush-start, flush-stop and EOS @@ +351,3 @@ + } + /* Send newsegment to all src pads */ + res = gst_pad_event_default (pad, parent, event); No, only to the active pad @@ +396,3 @@ + } + default: + res = gst_pad_query_default (pad, parent, query); Probably more queries should go to the currently active pad
thanks for your comments. I will check that you said and I will upload new patch.
(In reply to comment #21) > Review of attachment 254906 [details] [review]: > Only to the active pad, probably for all events except flush-start, flush-stop > and EOS Why would EOS not go to all pads? Nothing will come after it.
Forget my last comment, misread the negation there
Created attachment 255075 [details] [review] streamiddemux with event handling
I attached a patch from HoonHeee who is a reporter.
Unfortunately, I did not understand exactly about your comments. Anyway, Justin Joy had uploaded new patch, so are there more changes need? Please check new patch if you did not review.
Review of attachment 255075 [details] [review]: ::: plugins/elements/gststreamiddemux.c @@ +5,3 @@ + * @author: Jeongseok Kim <jeongseok.kim@lge.com> + * @author: Wonchul Lee <wonchul86.lee@lge.com> + * Copyright 2013 LEG Corp. LGE Corp, also it's twice here @@ +57,3 @@ +enum +{ + SIGNAL_SRC_PAD_ADDED, Remove this signal, GstElement::pad-added does the same already @@ +63,3 @@ +static guint gst_streamid_demux_signals[LAST_SIGNAL] = { 0 }; + +static GRWLock lock; Put this either in the instance struct or just use the GstObject lock instead. No global lock for per-instance things @@ +171,3 @@ + } + + GST_OBJECT_LOCK (demux); Locking shouldn't be necessary here as this will always be calling from the streaming thread @@ +189,3 @@ + g_hash_table_insert (demux->stream_id_pairs, g_strdup (stream_id), + (gpointer) srcpad); + g_rw_lock_writer_unlock (&lock); If you just keep the currently active pad in the instance struct, you don't need to lock anything for this hash table as it will only be called from the streaming thread. @@ +192,3 @@ + + g_signal_emit (G_OBJECT (demux), + gst_streamid_demux_signals[SIGNAL_SRC_PAD_ADDED], 0, srcpad); Remove this signal @@ +211,3 @@ + active_srcpad = + gst_streamid_demux_get_srcpad_by_stream_id (demux, + gst_pad_get_stream_id (pad)); You can just store the currently active srcpad in the instance structure. And update that whenever you receive a stream-start event @@ +217,3 @@ + + if (active_srcpad) + res = gst_pad_push (active_srcpad, buf); else gst_buffer_unref (buf); @@ +236,3 @@ + g_rw_lock_writer_lock (&lock); + srcpad = g_hash_table_lookup (demux->stream_id_pairs, stream_id); + g_rw_lock_writer_unlock (&lock); This lock should go away but you're actually doing a reader lock here @@ +254,3 @@ + GST_DEBUG_OBJECT (demux, "stream_id = %s", stream_id); + + g_rw_lock_writer_lock (&lock); Also reader lock, see above (and should go away) @@ +283,3 @@ + if (stream_id != NULL) { + caps = gst_pad_get_current_caps (pad); + gst_streamid_demux_srcpad_create (demux, pad, caps, stream_id); You don't need the caps here, and often you don't even have them yet. Also if the pad already exists you will need to update the sticky events on it from the srcpad @@ +285,3 @@ + gst_streamid_demux_srcpad_create (demux, pad, caps, stream_id); + } + res = gst_pad_event_default (pad, parent, event); Only send to the active pad @@ +291,3 @@ + { + /* Send caps to all src pads */ + res = gst_pad_event_default (pad, parent, event); Only send to the active pad, but as you don't have any code here just handle it in the default case @@ +303,3 @@ +#endif + /* Send newsegment to all src pads */ + res = gst_pad_event_default (pad, parent, event); Only send to the active pad, but as you don't have any code here just handle it in the default case @@ +309,3 @@ + { + if (GST_EVENT_IS_STICKY (event)) { + res = gst_pad_event_default (pad, parent, event); Only send to the active pad @@ +348,3 @@ + } + default: + res = gst_pad_query_default (pad, parent, query); All queries should only be done on the active pad ::: plugins/elements/gststreamiddemux.h @@ +6,3 @@ + * @author: JeongSeok Kim <jeongseok.kim@lge.com> + * @author: WonChul Lee <wonchul86.lee@lge.com> + * Copyright 2013 LEG Corp. LGE Corp, right? Why twice here? :) @@ +60,3 @@ + GstElementClass parent_class; + + void (*src_pad_added) (GstElement * element, GstPad * pad); Not needed, GstElement::pad-added signal
Created attachment 256158 [details] [review] update streamid-demuxer element According to the your comments, I did update the streamid-demuxer element. please check and review my patch.
Created attachment 256254 [details] [review] Additionally, some properties are added for source pad Upper layer can know that number of source pads from "num-src-pads" and currently active src pad from "active-src-pad"
Review of attachment 256254 [details] [review]: ::: plugins/elements/gststreamiddemux.c @@ +46,3 @@ +{ + PROP_0, + PROP_NUM_SRC_PADS, You can get this via the GstElement API, please remove the property @@ +47,3 @@ + PROP_0, + PROP_NUM_SRC_PADS, + PROP_ACTIVE_SRC_PAD, This one however is useful, like in input-selector @@ +140,3 @@ + + if (demux->stream_id_pairs) { + g_hash_table_remove_all (demux->stream_id_pairs); You are leaking the srcpad references and stream id strings here. And this should all also be done in GstElement::change_state() when going from PAUSED to READY. @@ +161,3 @@ + break; + case PROP_ACTIVE_SRC_PAD: + g_value_set_object (value, demux->active_srcpad); This needs locking, like all accesses to the active_srcpad not from the streaming thread (non-serialized events for example) @@ +190,3 @@ + + if (gst_streamid_demux_srcpad_has_stream_id (demux, stream_id)) { + GST_DEBUG_OBJECT (demux, "already created src pad"); maybe just return the pad from here? @@ +201,3 @@ + gst_object_ref (srcpad); + g_hash_table_insert (demux->stream_id_pairs, g_strdup (stream_id), + (gpointer) srcpad); If you use the rwlock for the hash table, use it consistently everywhere. Needs a write lock here @@ +247,3 @@ + + g_rw_lock_reader_lock (&demux->lock); + srcpad = g_hash_table_lookup (demux->stream_id_pairs, stream_id); Should probably return a new reference to the pad here. It might go away when you handle non-serialized events and the element is currently going from PAUSED->READY. @@ +258,3 @@ + +static gboolean +gst_streamid_demux_srcpad_has_stream_id (GstStreamidDemux * demux, This is basically the same as get_srcpad_by_stream_id(). Remove it @@ +293,3 @@ + if (GST_EVENT_TYPE (event) == GST_EVENT_STREAM_START) { + gst_event_parse_stream_start (event, &stream_id); + if (stream_id != NULL) { stream_id *must* be not-null. Otherwise fail here @@ +300,3 @@ + demux->active_srcpad = active_srcpad; + } + GST_PAD_STREAM_LOCK (demux->active_srcpad); Not needed, please remove
Created attachment 256604 [details] [review] Add streamid-demuxer element According to the your comment. I modify some codes in sstreamid-demuxer element. please check and review my patch. And I have a question below this. @@ +247,3 @@ + + g_rw_lock_reader_lock (&demux->lock); + srcpad = g_hash_table_lookup (demux->stream_id_pairs, stream_id); Should probably return a new reference to the pad here. It might go away when you handle non-serialized events and the element is currently going from PAUSED->READY. -> Could you explain the non-serialized events with some examples? because, I did not understand about this.
Events like flush-start are non-serialized. That means that you can get them from a thread that is not the streaming thread (for your element). Because of that your pads could go away at that time if timing is bad (e.g. when your element is set to READY). If you return a new reference there you can be sure that nothing destroys the pad until you're done with it.
Review of attachment 256604 [details] [review]: ::: plugins/elements/gststreamiddemux.c @@ +130,3 @@ +static void +gst_streamid_demux_dispose (GObject * object) +{ You should free/clear the rw lock here @@ +178,3 @@ + + gst_object_ref (srcpad); + demux->active_srcpad = srcpad; You leak any previously set active_srcpad here. Also you protected active_srcpad with the object lock above, but not here @@ +183,3 @@ + g_hash_table_insert (demux->stream_id_pairs, g_strdup (stream_id), + (gpointer) srcpad); + g_rw_lock_writer_unlock (&demux->lock); Note again that you can just use the object lock for protecting the hash table. RW locks only make sense if there are many readers at once, but this never happens here. @@ +262,3 @@ + gst_streamid_demux_srcpad_create (demux, pad, stream_id); + } else if (demux->active_srcpad != active_srcpad) { + demux->active_srcpad = active_srcpad; You leak any previously set active_srcpad here. Also you protected active_srcpad with the object lock above, but not here @@ +284,3 @@ +no_stream_id: + { + GST_WARNING_OBJECT (demux, "no stream_id found"); GST_ELEMENT_ERROR or maybe even make this an assertion. It should not happen. @@ +314,3 @@ + g_hash_table_destroy (demux->stream_id_pairs); + demux->stream_id_pairs = NULL; + } You should also unref and unset active_srcpad here
Created attachment 256684 [details] [review] Add streamid-demuxer element According to the your comment. I modify some codes in sstreamid-demuxer element. please check and review my patch.
Review of attachment 256684 [details] [review]: ::: plugins/elements/gststreamiddemux.c @@ +191,3 @@ + demux->active_srcpad = srcpad; + g_hash_table_insert (demux->stream_id_pairs, g_strdup (stream_id), + (gpointer) srcpad); Do you want to use the RW lock or the object lock for the hashtable now? You do both in inconsistent ways currently. @@ +287,3 @@ + res = gst_pad_event_default (pad, parent, event); + else if (demux->active_srcpad) + res = gst_pad_push_event (demux->active_srcpad, event); You need locking to access the active_srcpad as elsewhere. @@ +311,3 @@ + if (srcpad != NULL) { + if (GST_OBJECT_REFCOUNT_VALUE (srcpad) > 0) + gst_object_unref (srcpad); Never do something like this, if you need it you get the refcounting wrong elsewhere. @@ +358,3 @@ + gst_object_unref (demux->active_srcpad); + demux->active_srcpad = NULL; + } hashtable and active_srcpad need proper locking
::: plugins/elements/gststreamiddemux.c @@ +191,3 @@ + demux->active_srcpad = srcpad; + g_hash_table_insert (demux->stream_id_pairs, g_strdup (stream_id), + (gpointer) srcpad); Do you want to use the RW lock or the object lock for the hashtable now? You do both in inconsistent ways currently. => I did understand that you want me to use object lock instead of using g_rw_lock_writer_lock(). If I want to use object lock, I need to remove RW lock with g_rw_lock_reader_lock()? @@ +287,3 @@ + res = gst_pad_event_default (pad, parent, event); + else if (demux->active_srcpad) + res = gst_pad_push_event (demux->active_srcpad, event); You need locking to access the active_srcpad as elsewhere. => do you think object lock is needed above gst_pad_push_event (demux->active_srcpad, event)? @@ +311,3 @@ + if (srcpad != NULL) { + if (GST_OBJECT_REFCOUNT_VALUE (srcpad) > 0) + gst_object_unref (srcpad); Never do something like this, if you need it you get the refcounting wrong elsewhere. => Could you tell me why i am not using GST_OBJECT_REFCOUNT_VALUE() ? I think that if I could know refcount of srcpad, then I could call gst_object_unref(srcpad) Is there any idea for this? @@ +358,3 @@ + gst_object_unref (demux->active_srcpad); + demux->active_srcpad = NULL; + } hashtable and active_srcpad need proper locking => before release hashtable and active_srcpad, object lock is needed, right?
(In reply to comment #37) > ::: plugins/elements/gststreamiddemux.c > @@ +191,3 @@ > + demux->active_srcpad = srcpad; > + g_hash_table_insert (demux->stream_id_pairs, g_strdup (stream_id), > + (gpointer) srcpad); > > Do you want to use the RW lock or the object lock for the hashtable now? You do > both in inconsistent ways currently. > => I did understand that you want me to use object lock instead of using > g_rw_lock_writer_lock(). > If I want to use object lock, I need to remove RW lock with > g_rw_lock_reader_lock()? Use one of them, but make the usage consistent. > @@ +287,3 @@ > + res = gst_pad_event_default (pad, parent, event); > + else if (demux->active_srcpad) > + res = gst_pad_push_event (demux->active_srcpad, event); > > You need locking to access the active_srcpad as elsewhere. > => do you think object lock is needed above gst_pad_push_event > (demux->active_srcpad, event)? Yes, code like this: lock(); pad = gst_object_ref (demux->active_srcpad); unlock(); res = gst_pad_push_event (pad, event); gst_object_unref (pad); > @@ +311,3 @@ > + if (srcpad != NULL) { > + if (GST_OBJECT_REFCOUNT_VALUE (srcpad) > 0) > + gst_object_unref (srcpad); > > Never do something like this, if you need it you get the refcounting wrong > elsewhere. > => Could you tell me why i am not using GST_OBJECT_REFCOUNT_VALUE() ? > I think that if I could know refcount of srcpad, then I could call > gst_object_unref(srcpad) > Is there any idea for this? Read about the concepts of refcounting. Either you own a reference and need to unref or give it away, or you don't and then don't unref or give it away. > @@ +358,3 @@ > + gst_object_unref (demux->active_srcpad); > + demux->active_srcpad = NULL; > + } > > hashtable and active_srcpad need proper locking > => before release hashtable and active_srcpad, object lock is needed, right? Depends on which lock you want to use. IMHO you can just use the object lock for all of these things and just get rid of that RW lock.
(In reply to comment #38) > @@ +311,3 @@ > + if (srcpad != NULL) { > + if (GST_OBJECT_REFCOUNT_VALUE (srcpad) > 0) > + gst_object_unref (srcpad); > > Never do something like this, if you need it you get the refcounting wrong > elsewhere. > => Could you tell me why i am not using GST_OBJECT_REFCOUNT_VALUE() ? > I think that if I could know refcount of srcpad, then I could call > gst_object_unref(srcpad) > Is there any idea for this? Read about the concepts of refcounting. Either you own a reference and need to unref or give it away, or you don't and then don't unref or give it away. => I am a little bit confusing. you means that if I have refcount, then I need to unref and give it away? and if I don't have refcount, then I don't need to unref and give it away?
(In reply to comment #39) > (In reply to comment #38) > > @@ +311,3 @@ > > + if (srcpad != NULL) { > > + if (GST_OBJECT_REFCOUNT_VALUE (srcpad) > 0) > > + gst_object_unref (srcpad); > > > > Never do something like this, if you need it you get the refcounting wrong > > elsewhere. > > => Could you tell me why i am not using GST_OBJECT_REFCOUNT_VALUE() ? > > I think that if I could know refcount of srcpad, then I could call > > gst_object_unref(srcpad) > > Is there any idea for this? > > Read about the concepts of refcounting. Either you own a reference and need to > unref or give it away, or you don't and then don't unref or give it away. > > => I am a little bit confusing. > you means that if I have refcount, then I need to unref and give it away? > and if I don't have refcount, then I don't need to unref and give it away? No, I really meant it with "or" :) Example: You own a reference to a pad. If you don't give that reference away to other code (e.g. return it from a callback or pass it to gst_element_add_pad()) you'll have to unref it at some point, otherwise you'll leak memory.
Created attachment 256875 [details] [review] Add streamid-demuxer element According to the your comment. I decided to use object lock. please check and review my patch.
Review of attachment 256875 [details] [review]: ::: plugins/elements/gststreamiddemux.c @@ +173,3 @@ + + if (demux->stream_id_pairs == NULL) + demux->stream_id_pairs = g_hash_table_new (g_str_hash, g_str_equal); Consider using https://developer.gnome.org/glib/unstable/glib-Hash-Tables.html#g-hash-table-new-full here, which allows you to free elements easier :) @@ +198,3 @@ + + if (padname) + g_free (padname); padname will always be != NULL here, and g_free() is NULL-safe anyway @@ +214,3 @@ + + if (demux->active_srcpad) { + GST_OBJECT_LOCK (demux); You need to lock *before* checking if active_srcpad is NULL @@ +220,3 @@ + gst_object_unref (srcpad); + } else + gst_buffer_unref (buf); I think this should be an error. There should always be an active srcpad @@ +243,3 @@ + GST_OBJECT_UNLOCK (demux); + if (srcpad) { + GST_DEBUG_OBJECT (demux, "srcpad = %s matched", gst_pad_get_name (srcpad)); GST_DEBUG_OBJECT (demux, "srcpad = %s:%s matched", GST_DEBUG_PAD_NAME (srcpad)) @@ +279,3 @@ + } + demux->active_srcpad = active_srcpad; + GST_OBJECT_UNLOCK (demux); If the active_srcpad changed, call g_object_notify() for the active-srcpad property @@ +287,3 @@ + || GST_EVENT_TYPE (event) == GST_EVENT_EOS) + res = gst_pad_event_default (pad, parent, event); + else if (demux->active_srcpad) { Use curly braces for the first if, and the else too. Easier to read :) @@ +355,3 @@ + g_hash_table_foreach (demux->stream_id_pairs, + gst_streamid_demux_free_stream_id_pairs, NULL); + g_hash_table_destroy (demux->stream_id_pairs); Use g_hash_table_unref() here for sanity ::: tests/check/elements/streamiddemux.c @@ +63,3 @@ +_fake_chain (GstPad * pad, GstObject * parent, GstBuffer * buffer) +{ + gst_buffer_unref (buffer); Probably makes sense here to check if you only get only the buffers meant for this stream here (maybe remove the randomness above and just send the same number of buffers to each stream?) @@ +102,3 @@ + while (mysink_cnt < NUM_SUBSTREAMS) { + gst_check_setup_events_with_stream_id (td.mysrc, td.demux, caps, + GST_FORMAT_BYTES, g_strdup_printf ("test%d", mysink_cnt++)); You forget cleanup here, all pads and elements need to be freed.
Thanks for your comment. ::: tests/check/elements/streamiddemux.c @@ +63,3 @@ +_fake_chain (GstPad * pad, GstObject * parent, GstBuffer * buffer) +{ + gst_buffer_unref (buffer); Probably makes sense here to check if you only get only the buffers meant for this stream here (maybe remove the randomness above and just send the same number of buffers to each stream?) -> I did not understand. You want me not to use gst_buffer_unref (buffer); and then?
No, just must unref the buffer there. But you should check if you only get the buffers for the stream you expected instead of for another stream. This test currently does not test for correct behaviour.
Created attachment 257087 [details] [review] Add streamid-demuxer element Most of all, I just updated streamiddemux element. please check and review my patch. I have an issue for test with streamiddemux.c I added release code for all pads and elements in streamiddemux.c but, when I called gst_element_set_state (td->demux, GST_STATE_NULL), below message printed always. "GLib (gthread-posix.c): Unexpected error from C library during 'pthread_mutex_lock': Invalid argument. Aborting." below message is debug level 5 for this. -------------------------------------------------------------- 0:00:00.505914613 21392 0x11ae830 DEBUG GST_STATES gstelement.c:2639:gst_element_change_state:<streamiddemux0> element changed state SUCCESS 0:00:00.505936000 21392 0x11ae830 INFO GST_STATES gstelement.c:2306:gst_element_continue_state:<streamiddemux0> committing state from PAUSED to READY, pending NULL, next NULL 0:00:00.505960300 21392 0x11ae830 INFO GST_STATES gstelement.c:2236:_priv_gst_element_state_changed:<streamiddemux0> notifying about state-changed PAUSED to READY (NULL pending) 0:00:00.505986289 21392 0x11ae830 DEBUG GST_MESSAGE gstelement.c:1700:gst_element_post_message_default:<streamiddemux0> not posting message 0x11bab40: no bus 0:00:00.506010445 21392 0x11ae830 INFO GST_STATES gstelement.c:2313:gst_element_continue_state:<streamiddemux0> continue state change READY to NULL, final NULL 0:00:00.506034076 21392 0x11ae830 DEBUG GST_ELEMENT_PADS gstelement.c:2743:gst_element_pads_activate:<streamiddemux0> deactivate pads 0:00:00.506059348 21392 0x11ae830 DEBUG GST_PADS gstpad.c:962:gst_pad_set_active:<streamiddemux0:sink> pad was inactive 0:00:00.506081905 21392 0x11ae830 DEBUG GST_ELEMENT_PADS gstelement.c:2762:gst_element_pads_activate:<streamiddemux0> pad deactivation successful 0:00:00.506104708 21392 0x11ae830 DEBUG GST_STATES gstelement.c:2639:gst_element_change_state:<streamiddemux0> element changed state SUCCESS 0:00:00.506125717 21392 0x11ae830 INFO GST_STATES gstelement.c:2331:gst_element_continue_state:<streamiddemux0> completed state change to NULL 0:00:00.506147680 21392 0x11ae830 INFO GST_STATES gstelement.c:2236:_priv_gst_element_state_changed:<streamiddemux0> notifying about state-changed READY to NULL (VOID_PENDING pending) 0:00:00.506173579 21392 0x11ae830 DEBUG GST_MESSAGE gstelement.c:1700:gst_element_post_message_default:<streamiddemux0> not posting message 0x11bacc0: no bus 0:00:00.506198098 21392 0x11ae830 DEBUG GST_STATES gstelement.c:2566:gst_element_set_state_func:<streamiddemux0> returned SUCCESS 0:00:00.506314582 21392 0x11ae830 DEBUG GST_REFCOUNTING gstpad.c:605:gst_pad_dispose:<'':src_3> dispose 0:00:00.506339599 21392 0x11ae830 DEBUG GST_PADS gstpad.c:5001:gst_pad_send_event_unchecked:<streamiddemux0:sink> sent event, ret ok 0:00:00.506362813 21392 0x11ae830 DEBUG GST_PADS gstpad.c:4545:store_sticky_event:<streamiddemux0:sink> pad is flushing 0:00:00.506384161 21392 0x11ae830 INFO GST_EVENT gstpad.c:5033:gst_pad_send_event_unchecked:<streamiddemux0:sink> Received event on flushing pad. Discarding GLib (gthread-posix.c): Unexpected error from C library during 'pthread_mutex_lock': Invalid argument. Aborting. 0%: Checks: 1, Failures: 0, Errors: 1 elements/streamiddemux.c:58:E:streamiddemux simple:test_streamiddemux_simple:0: (after this point) Received signal 6 (Aborted) ------------------------------------------------------------------ So, I am not sure that stremiddemux element has problem or my test code (streamiddemux.c) has problem. And another thet code is working fine without streamsynchronizer. (http://gstreamer-devel.966125.n4.nabble.com/I-am-wondering-that-streamsynchronizer-is-only-for-1-video-and-1-audio-and-1-text-tc4662414.html) Also, I am wondering that my test code (streamiddemux.c) is proper approach or not for unit test? please give me some advice or information.
Review of attachment 257087 [details] [review]: ::: plugins/elements/gststreamiddemux.c @@ +199,3 @@ + demux->stream_id_pairs = + g_hash_table_new_full (g_str_hash, g_str_equal, (GDestroyNotify) g_free, + (GDestroyNotify) gst_object_unref); Any reason you don't create the hashtable in the _init method ? @@ +213,3 @@ + demux->active_srcpad = gst_object_ref (srcpad); + g_hash_table_insert (demux->stream_id_pairs, g_strdup (stream_id), + (gpointer) gst_object_ref (srcpad)); gst_object_ref() already returns a gpointer @@ +324,3 @@ + } else if (demux->active_srcpad) { + GST_OBJECT_LOCK (demux); + GstPad *srcpad = gst_object_ref (demux->active_srcpad); Please declare the variable at the start of the block please @@ +337,3 @@ +no_stream_id: + { + GST_ELEMENT_ERROR (demux, STREAM, DEMUX, (NULL), ("no stream_id found")); Please add a translatable user friendly error string as the 4th parameter. @@ +364,3 @@ + gst_object_unref (demux->active_srcpad); + GST_OBJECT_UNLOCK (demux); + demux->active_srcpad = NULL; Why do you unlock before setting the pointer to NULL ? @@ +375,3 @@ + + it = gst_element_iterate_src_pads (GST_ELEMENT_CAST (demux)); + GstIteratorResult itret = GST_ITERATOR_OK; Please declare all variables at the beginning of the block, C89-style
Created attachment 257343 [details] [review] Add streamid-demuxer element According to Olivier's comment. I updated some codes in streamid-demuxer element. please check and review my patch.
I Have a question for output-selector element. In their event function, "stream-start" event with sticky event are sent to their's all of linked src pads. Also, "segment" event is sent to all src pads. So, I am not sure, "stream-start" and some other sticky-events have to be sent to only active-srcpad in streamiddemux. Because, I have a test code below this. => (http://gstreamer-devel.966125.n4.nabble.com/I-am-wondering-that-streamsynchronizer-is-only-for-1-video-and-1-audio-and-1-text-tc4662414.html) When I deployed streamsynchronizer element behind streamiddemuxer, there was a blocking issue. But I changed my code that "stream-start" event sent to all src pads in streamiddemuxer. After that, there was no blocking issue and 2 ogg files was made well.
And I have an another question. I found an issue. When I tested about mJpeg file, there was an especial occasion. "caps" event has been arrived earlier than "stream-start" event. This case is a normal case or not? Please give me some information.
(In reply to comment #50) > "caps" event has been arrived earlier than "stream-start" event. > This case is a normal case or not? That means there is a bug in some other element, the order of events should always be "stream-start", "caps" (optional), "segment" before any buffer can be sent.
(In reply to comment #51) Olivier, Thanks for your comment. It was our demuxer issue. > (In reply to comment #50) > > "caps" event has been arrived earlier than "stream-start" event. > > This case is a normal case or not? > > That means there is a bug in some other element, the order of events should > always be "stream-start", "caps" (optional), "segment" before any buffer can be > sent.
(In reply to comment #52) Sorry, Olivier. It was my demuxer issue. > (In reply to comment #51) > Olivier, Thanks for your comment. > It was our demuxer issue. > > > (In reply to comment #50) > > > "caps" event has been arrived earlier than "stream-start" event. > > > This case is a normal case or not? > > > > That means there is a bug in some other element, the order of events should > > always be "stream-start", "caps" (optional), "segment" before any buffer can be > > sent.
Created attachment 258551 [details] [review] Add streamid-demuxer element I updated streamiddemux with test code. please check and review my patch.
Review of attachment 258551 [details] [review]: ::: plugins/elements/gststreamiddemux.c @@ +44,3 @@ +enum +{ + SIGNAL_ACTIVE_SRCPAD_CHANGED, No need for this signal, call g_object_notify(self, "active-srcpad") instead. @@ +103,3 @@ + + g_object_class_install_property (gobject_class, PROP_ACTIVE_SRC_PAD, + g_param_spec_object ("active-src-pad", "Active src pad", Should probably just be called active-pad for consistency with input-selector @@ +172,3 @@ + switch (prop_id) { + case PROP_ACTIVE_SRC_PAD: + GST_PAD_STREAM_LOCK (demux->active_srcpad); This is not correct, nothing is guaranteeing you here that active_srcpad might not change at the time you try to lock it. You need to use another mutex, e.g. the object lock as before and in the code below. @@ +237,3 @@ + + GST_PAD_STREAM_LOCK (demux->active_srcpad); + if (demux->active_srcpad) { Wrong lock here too @@ +301,3 @@ + } else if (demux->active_srcpad != active_srcpad) { + GST_PAD_STREAM_LOCK (demux->active_srcpad); + if (demux->active_srcpad != NULL) { wrong lock @@ +322,3 @@ + GstPad *srcpad = NULL; + GST_PAD_STREAM_LOCK (demux->active_srcpad); + srcpad = gst_object_ref (demux->active_srcpad); wrong lock @@ +380,3 @@ + gst_iterator_foreach (it, + (GstIteratorForeachFunction) gst_streamid_demux_release_srcpads, demux); + gst_iterator_resync (it); You must only call resync() if the iterator returned RESYNC. Otherwise you'll loop forever here ::: tests/check/elements/streamiddemux.c @@ +188,3 @@ + gst_pad_get_current_caps (td.mysink[active_stream]), GST_FORMAT_BYTES, + g_strdup_printf ("test%d", active_stream)); + g_object_get (td.demux, "active-src-pad", &active_srcpad, NULL); You're leaking active_srcpad here, you probably also want to check if after the push the active_srcpad is the one you expect
Created attachment 258711 [details] [review] Add streamid-demuxer element According to your comment, I updated streamiddemux with test code. please check and review my patch.
Review of attachment 258711 [details] [review]: Generally looks good now, just some comments for the test ::: tests/check/elements/streamiddemux.c @@ +48,3 @@ + gst_object_unref (active_srcpad); + + g_object_get (td->demux, "active-pad", &active_srcpad, NULL); You're leaking this in the end of the tests @@ +190,3 @@ + set_active_srcpad (&td); + + fail_unless (gst_pad_push (td.mysrc, gst_buffer_new ()) == GST_FLOW_OK); Check that the active srcpad after pushing is the one you expect @@ +201,3 @@ + set_active_srcpad (&td); + + fail_unless (gst_pad_push (td.mysrc, gst_buffer_new ()) == GST_FLOW_OK); Check that the active srcpad after pushing is the one you expect @@ +206,3 @@ + GST_DEBUG ("Releasing mysink and mysrc"); + stream_cnt = 0; + while (stream_cnt < NUM_SUBSTREAMS) { Just make this a for loop, also elsewhere
Created attachment 258832 [details] [review] Add streamid-demuxer element According to your comment, I updated the test code. your comment : You're leaking this in the end of the tests. my solution : When release function is called, it is fixed to unref the active_srcpad. your comment : Check that the active srcpad after pushing is the one you expect my solution : In chain function, I already checked whether or not if peer-pad of active-srcpad equals to one of mysinkpad. Additionally, I check whether or not if stream-id of active-srcpad equals to stream-id of one of mysinkpad. please check and review my patch. your comment : Just make this a for loop, also elsewhere my solution : It is fixed to use for loop instead of while loop. please check and review my patch.
Review of attachment 258832 [details] [review]: Please run your test in valgrind :) ::: plugins/elements/gststreamiddemux.c @@ +274,3 @@ + + active_srcpad = + gst_streamid_demux_get_srcpad_by_stream_id (demux, stream_id); You're leaking active_srcpad here if active_srcpad == demux->active_srcpad. @@ +342,3 @@ + GST_OBJECT_UNLOCK (demux); + } else { + GST_OBJECT_UNLOCK (demux); Just put the unlock outside the if block ::: tests/check/elements/streamiddemux.c @@ +96,3 @@ + GstPad *peer_pad = NULL; + + peer_pad = gst_pad_get_peer (active_srcpad); You're leaking all the pads here @@ +99,3 @@ + fail_unless (pad == peer_pad); + fail_unless (g_strcmp0 (gst_pad_get_stream_id (active_srcpad), + gst_pad_get_stream_id (pad)) == 0); You're leaking the return value of gst_pad_get_stream_id() here. @@ +113,3 @@ + + GST_DEBUG ("Creating mysink"); + td.mysink[0] = gst_pad_new ("mysink0", GST_PAD_SINK); You're leaking this pad @@ +117,3 @@ + gst_pad_set_active (td.mysink[0], TRUE); + + td.mysink[1] = gst_pad_new ("mysink1", GST_PAD_SINK); You're leaking this pad @@ +176,3 @@ + for (stream_cnt = 0; stream_cnt < NUM_SUBSTREAMS; ++stream_cnt) { + td.mysink[stream_cnt] = + gst_pad_new (g_strdup_printf ("mysink%d", stream_cnt), GST_PAD_SINK); You're leaking the name string here @@ +189,3 @@ + for (stream_cnt = 0; stream_cnt < NUM_SUBSTREAMS; ++stream_cnt) { + gst_check_setup_events_with_stream_id (td.mysrc, td.demux, td.mycaps, + GST_FORMAT_BYTES, g_strdup_printf ("test%d", stream_cnt++)); You're leaking the g_strdup_printf() result @@ +200,3 @@ + gint active_stream = rand () % NUM_SUBSTREAMS; + gst_check_setup_events_with_stream_id (td.mysrc, td.demux, td.mycaps, + GST_FORMAT_BYTES, g_strdup_printf ("test%d", active_stream)); You're leaking the g_strdup_printf() result
Created attachment 260030 [details] [review] Add streamid-demuxer element Thanks for your comment. According your comment, I updated my code changed. I run the below command to check memory leak. => export G_SLICE=always-malloc => export G_DEBUG=gc-friendly => valgrind --tool=memcheck --leak-check=full --leak-resolution=high --num-callers=20 elements/streamiddemux But, I am not sure about valgrind with option. Could you let me know what is the exactly valgrind command with option? If I could once know exactly command with valgrind, I will be able to upload my patch again ASAP.
For test you can: cd tests/check make elements/streamiddemu.valgrind This will set the appropriate valgrind option and the suppression file, as certain allocation in GLib/GStreamer are are not freed in the end. This is to not waste time at quite for things that must be accessible during the life of your application (e.g. gstreamer registry, GObject object definitions, etc.) p.s. you should probably set to obsolete the old version of your patches
Created attachment 262802 [details] [review] Add streamid-demuxer element Thanks for your comment. I checked memory leak with valgrind, I updated some code changed to prevent memory leak. I used below command to check it. => make elements/streamiddemux.valgrind --------------------------------------------------------------- GStreamer has detected that it is running inside valgrind. It might now take different code paths to ease debugging. Of course, this may also lead to different bugs. Running suite(s): streamiddemux 100%: Checks: 2, Failures: 0, Errors: 0 -------------------------------------------------------------- I think that there is no memory leak. Please check and review my patch.
Sebastian, could you review my latest patch?
Review of attachment 262802 [details] [review]: Just quickly skimming through it ::: plugins/elements/gststreamiddemux.c @@ +189,3 @@ + if (demux->active_srcpad != NULL) + demux->active_srcpad = NULL; + Either there's something missing on that if block above or the next line makes it unneeded @@ +224,3 @@ + GST_OBJECT_UNLOCK (demux); + } + Drop the else clause and move the GST_OBJECT_UNLOCK outside the if block @@ +278,3 @@ + GST_OBJECT_LOCK (demux); + if (demux->active_srcpad != NULL) + demux->active_srcpad = NULL; See comment #1 ::: tests/check/elements/streamiddemux.c @@ +1,3 @@ +/* GStreamer unit tests for the streamiddemux + * + * Copyright (C) 2013 LGE You sure it's not LGE Corporation like on the rest of the files?
(In reply to comment #64) > @@ +224,3 @@ > + GST_OBJECT_UNLOCK (demux); > + } > + > > Drop the else clause and move the > GST_OBJECT_UNLOCK outside the if > block No, the lock must be taken for the if. It's for the access to demux->active_srcpad
Created attachment 264949 [details] [review] Add streamid-demuxer element As Sebastian's comment, I think that lock is required to access active-srcpad. And I modified the copyright as LGE Corporation. please review my patch.
Review of attachment 264949 [details] [review]: Good job, just have minor changes noted below. I'd also think that it should be GstStreamIdDemux instead of GstStreamidDemux. Thanks for your patch. ::: plugins/elements/gststreamiddemux.c @@ +147,3 @@ + GValue * value, GParamSpec * pspec) +{ + GstStreamidDemux *demux = GST_STREAMID_DEMUX (object); As you know for sure this object is a GstStreamidDemux, you can create a GST_STREAMID_DEMUX_CAST that is just a cast and avoids type checking that should always succeed. This applies to other occurrences, too. @@ +169,3 @@ + + return TRUE; +} It might be worth a note that this only works because they are forwarded right after a new stream-id is received, so that prevents it from sending mixed stream-ids. Also, can this handle different streams with different caps/segments? @@ +188,3 @@ + GST_OBJECT_LOCK (demux); + if (demux->active_srcpad != NULL) + demux->active_srcpad = NULL; This seems like a dead assignment @@ +222,3 @@ + gst_object_unref (srcpad); + } else { + GST_OBJECT_UNLOCK (demux); Should unref the buffer to prevent a leak @@ +278,3 @@ + GST_OBJECT_LOCK (demux); + if (demux->active_srcpad != NULL) + demux->active_srcpad = NULL; This is also a dead assignment @@ +284,3 @@ + g_object_notify (G_OBJECT (demux), "active-pad"); + } + } The locking in this block looks confusing to me, it works because this is inside the event handler, so only one event handler can run at a time (for the same pad). I'd prefer having the lock before the get_srcpad_by_stream_id (and make this function assume it should be called with the lock already) and then unlock inside the if/else branches. For the if, you likely will have to split the srcpad_create into 2 parts. The first part would just create the GstPad and can be run with the lock taken. The second one activates and adds the pad and should be run without the lock. On the else branch, it should be the same. Not sure if you agree with this, but it seems to make a more consistent locking. @@ +316,3 @@ +static void +gst_streamid_demux_release_srcpads (const GValue * item, + GstStreamidDemux * demux) Make the function name singular, it only releases 1 srcpad
Some further comments below... but most importantly this needs a decision if something like this should go into core, or better first into bad? Or if we want a different kind of element that can demux based on other aspects too? For myself this would be fine for core... once it has more tests for more use cases (see below). (In reply to comment #67) > @@ +169,3 @@ > + > + return TRUE; > +} > > It might be worth a note that this only works because they are forwarded right > after a new stream-id is received, so that prevents it from sending mixed > stream-ids. > > Also, can this handle different streams with different caps/segments? I think this should get a unit test, yes. > @@ +284,3 @@ > + g_object_notify (G_OBJECT (demux), "active-pad"); > + } > + } > > The locking in this block looks confusing to me, it works because this is > inside the event handler, so only one event handler can run at a time (for the > same pad). > > I'd prefer having the lock before the get_srcpad_by_stream_id (and make this > function assume it should be called with the lock already) and then unlock > inside the if/else branches. > > For the if, you likely will have to split the srcpad_create into 2 parts. The > first part would just create the GstPad and can be run with the lock taken. The > second one activates and adds the pad and should be run without the lock. > > On the else branch, it should be the same. > > Not sure if you agree with this, but it seems to make a more consistent > locking. Most importantly no lock must be taken while calling g_object_notify() :) Good catch.
Sebastian said, > if we want a different kind of element > that can demux based on other aspects too? Can I ask to discuss more about this thing? Actually, we've designed this element due to support a kind of strange feature in playbin. It could be weird, but the requirement was from the legacy services. The requirement is that each video and audio streams are given as element streams and the upper layer of pipeline should be compatible with normal playback. So we are designing a source element including multiple appsrc and funnel. To serialize element streams, we will use funnel in the source element, then we will connect the element to decodebin. In side of decodebin, we will deploy another "stream id demux". To archive this structure, I think we need real demuxer like element more than current streamiddemux. In this matter, how about using a name, "reverse-funnel" or "defunnel" for this patch, and then, reserve "stream-id-demux" for the future real demux element?
Thanks for your comment. (In reply to comment #67) > Review of attachment 264949 [details] [review]: > > Good job, just have minor changes noted below. > > I'd also think that it should be GstStreamIdDemux instead of GstStreamidDemux. > > Thanks for your patch. > > ::: plugins/elements/gststreamiddemux.c > @@ +147,3 @@ > + GValue * value, GParamSpec * pspec) > +{ > + GstStreamidDemux *demux = GST_STREAMID_DEMUX (object); > > As you know for sure this object is a GstStreamidDemux, you can create a > GST_STREAMID_DEMUX_CAST that is just a cast and avoids type checking that > should always succeed. > This applies to other occurrences, too. > > @@ +169,3 @@ > + > + return TRUE; > +} > > It might be worth a note that this only works because they are forwarded right > after a new stream-id is received, so that prevents it from sending mixed > stream-ids. > > Also, can this handle different streams with different caps/segments? => I don't have a unit test case for different caps/segments. Thus, I will share to you guys about the result of unit test after I've done for different caps/segments. > > @@ +188,3 @@ > + GST_OBJECT_LOCK (demux); > + if (demux->active_srcpad != NULL) > + demux->active_srcpad = NULL; > > This seems like a dead assignment => Thanks, I will remove dead assignment. > > @@ +222,3 @@ > + gst_object_unref (srcpad); > + } else { > + GST_OBJECT_UNLOCK (demux); > > Should unref the buffer to prevent a leak => Thanks. IMHO, I think unref is not required. You can reference below description for gst_pad_push() function. --------------------------------------------------------------------------- In all cases, success or failure, the caller loses its reference to @buffer after calling this function. --------------------------------------------------------------------------- > > @@ +278,3 @@ > + GST_OBJECT_LOCK (demux); > + if (demux->active_srcpad != NULL) > + demux->active_srcpad = NULL; > > This is also a dead assignment => Thanks, I will remove dead assignment. > > @@ +284,3 @@ > + g_object_notify (G_OBJECT (demux), "active-pad"); > + } > + } > > The locking in this block looks confusing to me, it works because this is > inside the event handler, so only one event handler can run at a time (for the > same pad). > > I'd prefer having the lock before the get_srcpad_by_stream_id (and make this > function assume it should be called with the lock already) and then unlock > inside the if/else branches. > > For the if, you likely will have to split the srcpad_create into 2 parts. The > first part would just create the GstPad and can be run with the lock taken. The > second one activates and adds the pad and should be run without the lock. > > On the else branch, it should be the same. > > Not sure if you agree with this, but it seems to make a more consistent > locking. => Thanks, Actually I am not clear to lock and unlock for above that. Thus, what I want to do is that someone helps me how to make for more consistent locking. > > @@ +316,3 @@ > +static void > +gst_streamid_demux_release_srcpads (const GValue * item, > + GstStreamidDemux * demux) > > Make the function name singular, it only releases 1 srcpad => I will modify function name as singular.
Dear all, please, ignore my last comment. I missed uridecodebin already supported the feature to deploy multiple decodebins. :)
Created attachment 270887 [details] [review] Add streamid-demuxer element As Sebastian and Thiago's comments, I updated streamiddemux element with more test case and example code. please review and test my patch.
Dear All I thought that this patch was reviewed well and is supposed to be merged soon. But, It is not merged yet to GStreamer core or bad plugin. Please check and review again. And please let me know, if I have to update patch.
commit ad161f7df51c05858be486af2c05c7923e9a4aa1 Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Mar 12 14:39:37 2015 +0000 streamiddemux: Reset pad counter after removing all pads commit fadabe8b787d53c90dc06e1fcdaff3f2f04e990c Author: HoonHee Lee <hoonhee.lee@lge.com> Date: Tue Mar 4 19:40:05 2014 +0900 streamiddemux: Add streamiddemux element Demultiplex a stream to multiple source pads based on the stream ids from the stream-start events. This basically reverses the behaviour of funnel. https://bugzilla.gnome.org/show_bug.cgi?id=707605