GNOME Bugzilla – Bug 738285
playbin: Sequential playback of the file list does not work
Last modified: 2018-05-05 15:37:47 UTC
Created attachment 288200 [details] [review] for inspect simple example (Sequential playback of the file list): == start ======================================== static GMainLoop *loop; static gint index = 0; static gchar* list[] = { "media_uri_1", "media_uri_2", NULL }; static void play_next(GObject *playbin, gpointer user_data) { const gchar *next = list[++index]; if (next == NULL) { g_main_loop_quit (loop); } g_object_set(playbin, "uri", next, NULL); } int main() { ... GstElement *playbin = gst_element_factory_make("playbin", "playbin"); g_signal_connect(G_OBJECT(playbin), "about-to-finish", (GCallback)play_next, NULL); g_object_set(G_OBJECT(playbin), "uri", list[index], NULL); gst_element_set_state(playbin, GST_STATE_PLAYING); ... } == end ========================================= To make it easier to explain, apply example.patch from attachment View log from attachemt: line 1540 (EOS from audio): 0:00:09.175147876 gstdecodebin2.c:4475:source_pad_event_probe:<decodebin0:src_1> we received EOS line 1551 (inspect result EOS event): 0:00:09.175320269 gstdecodebin2.c:4632:gst_decode_pad_event:<decodebin0:src_1> EOS result: unknown, 1 Why 'unknown'? This EOS event will be sticked (result != GST_FLOW_OK)! line 1601 (EOS from video): 0:00:10.165434115 gstdecodebin2.c:4475:source_pad_event_probe:<decodebin0:src_0> we received EOS line 1623 (inspect result EOS event, all right) 0:00:10.168051241 gstdecodebin2.c:4632:gst_decode_pad_event:<decodebin0:src_0> EOS result: ok, 0 PLAY next file call 'setup_next_source' form gstplaybin2.c line 1796 (reconfiguring & unblocking): 0:00:10.194371416 gstplaysink.c:3181:gst_play_sink_do_reconfigure:<playsink> reconfiguring line 1809 ... (audio pad): 0:00:10.199289077 gstdecodebin2.c:4616:gst_decode_pad_chain:<decodebin1:src_1> Buffer 0:00:00.000000000 0:00:10.199311819 gstdecodebin2.c:4618:gst_decode_pad_chain:<decodebin1:src_1> push result eos 0:00:10.199517290 gstdecodebin2.c:4616:gst_decode_pad_chain:<decodebin1:src_1> Buffer 0:00:00.024000000 0:00:10.199599560 gstdecodebin2.c:4618:gst_decode_pad_chain:<decodebin1:src_1> push result eos 0:00:10.199842973 gstdecodebin2.c:4616:gst_decode_pad_chain:<decodebin1:src_1> Buffer 0:00:00.048000000 0:00:10.199894467 gstdecodebin2.c:4618:gst_decode_pad_chain:<decodebin1:src_1> push result eos ... EOS sticky line 2655: 0:00:10.335347188 gststreamsynchronizer.c:348:gst_stream_synchronizer_sink_event:<streamsynchronizer0:sink_0> Stream 0 is waiting finish
Created attachment 288201 [details] log log
Does it work with gst-play-1.0 ?
use gstreamer from git. https://bugzilla.gnome.org/show_bug.cgi?id=738064 The drained signal must be fixed.
gst-play-1.0 media1 media2 --gapless
Created attachment 288495 [details] [review] patch My reasoning above were wrong. playsink->audio_tee_sink and all downstream pads have GST_PAD_FLAG_EOS. I do not know the correct solution, but sending flush_stop event solves the problem.
Created attachment 288496 [details] [review] patch My reasoning above were wrong. playsink->audio_tee_sink and all downstream pads have GST_PAD_FLAG_EOS. I do not know the correct solution, but sending flush_stop event solves the problem.
Sending flush-events here is wrong. There should not be an EOS event forwarded at all in this case. You'll have to check why decodebin does not wait for all streams to produce an EOS event before forwarding it. That's somewhere around the drained signal code you already know :)
Created attachment 288574 [details] [review] patch Yet another attempt
Created attachment 288885 [details] [review] patch 2 wait until all chains are drained
Review of attachment 288885 [details] [review]: ::: gst/playback/gstdecodebin2.c @@ +3752,2 @@ g_signal_emit (dbin, gst_decode_bin_signals[SIGNAL_DRAINED], 0, NULL); + g_cond_broadcast (&dbin->all_eos_cond); You should use a mutex here (the object lock), and also store the information that we're all-eos now... @@ +3782,3 @@ + if (last_group && !drained) { + GST_OBJECT_LOCK (dbin); + g_cond_wait (&dbin->all_eos_cond, GST_OBJECT_GET_LOCK (dbin)); ... and here check after locking before g_cond_wait() if all are EOS already. Otherwise you have a race condition here. Also note that g_cond_wait() can be triggered spuriously under certain conditions, so you need a loop like while (not_all_eos) g_cond_wait();
Created attachment 288921 [details] [review] fix Thanks for the review. I was a windows programmer
Review of attachment 288921 [details] [review]: Thanks for updating the patch :) ::: gst/playback/gstdecodebin2.c @@ +3787,3 @@ + GST_OBJECT_LOCK (dbin); + while (!dbin->all_eos) + g_cond_wait (&dbin->all_eos_cond, GST_OBJECT_GET_LOCK (dbin)); This here needs to unlock on FLUSH_START events, and when going from PAUSED to READY state (before chaining up to the parent class' change_state implementation) @@ +4781,3 @@ DYN_UNLOCK (dbin); dbin->have_type = FALSE; + dbin->all_eos = FALSE; The condition variable should probably also be reset on FLUSH_STOP
Created attachment 289005 [details] [review] fix
New problem. if audio disabled (flags=1) => deadlock. multiqueue wait eos from video 1544 if (mq->numwaiting > 0 && sq->srcresult == GST_FLOW_EOS) { 1545 compute_high_time (mq); 1546 compute_high_id (mq); 1547 wake_up_next_non_linked (mq); 1548 } video wait eos form not-linked audio (all_eos)
Review of attachment 289005 [details] [review]: ::: gst/playback/gstdecodebin2.c @@ +3754,2 @@ g_signal_emit (dbin, gst_decode_bin_signals[SIGNAL_DRAINED], 0, NULL); + DYN_LOCK (dbin); Why do you switch to the DYN_LOCK from the OBJECT_LOCK? @@ +4515,3 @@ + case GST_EVENT_FLUSH_STOP: + DYN_LOCK (dbin); + dbin->flushing = FALSE; Flushing is per pad, not globally
DYN_LOCK used for dbin->shutdown
Why not propably use a list eos_pending_pads from my first patch to send pending eos?
It seems not necessary for EOS, but for flushing you need to store the flushing state per pad (just store it inside the pad!)
https://bugzilla.gnome.org/show_bug.cgi?id=738285#c14 Did you read comment 14?
Ah, I missed that indeed. Yes, why not just remember which pads are EOS instead of blocking... and once all are EOS send an EOS event on all of them. That should solve all these problems and actually simplify the code. You don't have to store the EOS state in a list though, it's already stored elsewhere in decodebin in the decode chain or decode pad IIRC.
Created attachment 289012 [details] [review] fix
Created attachment 289013 [details] [review] fix
The problem remains: gst-play-1.0 --gapless only_video.avi audio_and_video.avi Almost always after the first media without audio, the following media with audio does not play.Your suggestions.
Not sure and I don't have time to debug that myself right now. What exactly happens after the first has finished? And is your fix fixing anything at all then?
fix should be commited. I will try to find a solution
Well, what exactly does it fix? :) As I see it, it would prevent sending EOS downstream while there are still unfinished chains... because otherwise e.g. the audio sink might go to EOS already, then we send the drained signal, then we switch to a next track. And then nothing works anymore because the audio sink is already done.
all right. EOS after drained signal. fix works. maybe error in streamsynchronizer.
There are several problems. fix solves one
Created attachment 289112 [details] graph pulsesink Comment 23. maybe bug in pulsesink. If use alsasink instead pulsesink then all well.
Created attachment 289113 [details] graph alsasink
Created attachment 289114 [details] pulsesink log GST_DEBUG=pulse:6 gst-play-1.0 --gapless noaudio.avi media.avi 2>pulse.log
pulsesink.c /* called from pulse thread with the mainloop lock */ static void mainloop_enter_defer_cb (pa_mainloop_api * api, void *userdata) { GstPulseSink *pulsesink = GST_PULSESINK (userdata); GstMessage *message; GValue val = { 0 }; GST_DEBUG_OBJECT (pulsesink, "posting ENTER stream status"); message = gst_message_new_stream_status (GST_OBJECT (pulsesink), GST_STREAM_STATUS_TYPE_ENTER, GST_ELEMENT (pulsesink)); g_value_init (&val, GST_TYPE_G_THREAD); g_value_set_boxed (&val, g_thread_self ()); gst_message_set_stream_status_object (message, &val); g_value_unset (&val); gst_element_post_message (GST_ELEMENT (pulsesink), message); ================================ g_return_if_fail (pulsesink->defer_pending); pulsesink->defer_pending--; pa_threaded_mainloop_signal (mainloop, 0); } "gst_element_post_message" function is called and the execution stops, therefore the "pa_threaded_mainloop_signal" function call was not occur. Sorry for my bad English
This is deadlock, "gst_element_post_message" try lock mutex GST_OBJECT_GET_LOCK is already locked by another thread. I need help! I need to play files sequentially.
The pulsesink deadlock is unrelated and is bug #736071
If cancel the #736071 fix, everything works
You mean with the patch remaining in this bug, and the pulsesink related fix everything works for you and there's nothing left to be fixed?
Yes, if commit my patch and cancel #736071 fix, everything works
Comment on attachment 289013 [details] [review] fix While this looks good, it causes a deadlock in the playbin-complex test. You can run it with make elements/playbin-complex.check in tests/check
+ Trace 234249
Thread 26 (Thread 0x7fffed598700 (LWP 23773))
Thread 25 (Thread 0x7fffee59a700 (LWP 23772))
Thread 24 (Thread 0x7fffedd99700 (LWP 23771))
Thread 23 (Thread 0x7ffff4b42700 (LWP 23770))
Thread 4 (Thread 0x7fffef5d9700 (LWP 23751))
Pause caused by the input-selector. The two audio streams connected to it. When the active stream ends (but the EOS was not sent), non-linked stream not dispatch buffer.
GST_DEBUG=input-selector:6 make elements/playbin-complex.check
Created attachment 289194 [details] [review] fix input-selector
Created attachment 289195 [details] [review] fix input-selectoe
Can you explain what the exact problem here is and how the commit fixes it?
I will try to speak correctly in English. The report can be seen that freezes the last buffer of non-linked audio stream (1). Its timestamp = timestamp last buffer of active audio stream(0). This timestamp = active_segment.position Correctly would increase the active_segment.position for the duration of the buffer when it successfully sent (and then would not have to make changes), but do not specify the duration of buffers in this test suite. Therefore, we replace the condition on GREAT not Great or equal
... Otherwise, we will wait for events/data from ended stream.
I think this is not correct then. We would run into the same problem if the non-linked streams are longer than the linked ones, e.g. if there are a few milliseconds more audio than video and audio is disabled.
You're right, but I think it is defect input-selector. I will try to find a solution
Also note that the worst of the correction will not be
The problem is that without the EOS event the input-selector does not know that a stream is finished, and that it can wake up the other streams. So maybe we should only delay EOS until all *linked* streams are EOS and then forward it, instead of also waiting for the not linked ones.
Created attachment 289245 [details] [review] fix pending eos waiting EOS only for the linked streams
Created attachment 289246 [details] [review] fix pending eos waiting EOS only for the linked streams
Created attachment 289248 [details] [review] fix pending eos waiting EOS only for the linked streams
Review of attachment 289248 [details] [review]: ::: gst/playback/gstdecodebin2.c @@ +3678,3 @@ + CHAIN_MUTEX_LOCK (chain); + chain->drained = TRUE; + chain->endpad->drained = TRUE; Shouldn't it these all be set to TRUE already at this point? @@ +3682,3 @@ + peer = gst_pad_get_peer (GST_PAD_CAST (chain->endpad)); + if (peer) { + gst_pad_send_event (peer, gst_event_new_eos ()); gst_pad_push_event (chain->endpad, gst_event_new_eos()) ? @@ +3722,2 @@ chain->drained = chain->endpad->drained; + *drained = chain->endpad->notlinked; This does not seem right. A not-linked pad is not automatically drained, it might be selected again later by input-selector. Only if all linked pads are at EOS and only the not-linked ones are not you can consider the not-linked pads as drained. @@ +3801,3 @@ } + // return last_group; + return FALSE; Don't use C99 comments, but also just delete that line :) @@ +4645,3 @@ + if (drained) { + gst_buffer_unref (buffer); + ret = GST_FLOW_EOS; This part is not needed, it will return EOS automatically from gst_proxy_pad_chain_default() already.
Review of attachment 289248 [details] [review]: ::: gst/playback/gstdecodebin2.c @@ +3678,3 @@ + CHAIN_MUTEX_LOCK (chain); + chain->drained = TRUE; + chain->endpad->drained = TRUE; If not linked stream longer then active, it will not be drained. In the future, if it will come the data we discard them. @@ +3682,3 @@ + peer = gst_pad_get_peer (GST_PAD_CAST (chain->endpad)); + if (peer) { + CHAIN_MUTEX_LOCK (chain); endpad is source pad, not sink pad. EOS is downstream event. @@ +3722,2 @@ chain->drained = chain->endpad->drained; + *drained = chain->endpad->notlinked; drained state not saved in the pad (chain->endpad->drained != TRUE), only now @@ +3801,3 @@ } + // return last_group; + return FALSE; Ok @@ +4645,3 @@ + if (drained) { + gst_buffer_unref (buffer); + gboolean drained; if not-linked stream longer then active and we already drained signal emit
Created attachment 289258 [details] [review] fix
offtopic: I want to improve the plugin inter (plugin for inter-pipeline communication), but it's not a bug. I believe that the idea is good, but the implementation is weak. What do you need me to do?
Please open a new bug for that or discuss it on the mailing list.
(In reply to comment #54) > Review of attachment 289248 [details] [review]: > > ::: gst/playback/gstdecodebin2.c > @@ +3678,3 @@ > + CHAIN_MUTEX_LOCK (chain); > + chain->drained = TRUE; > + chain->endpad->drained = TRUE; > > If not linked stream longer then active, it will not be drained. In the future, > if it will come the data we discard them. What would be the problem with that? > @@ +3682,3 @@ > + peer = gst_pad_get_peer (GST_PAD_CAST (chain->endpad)); > + if (peer) { > + CHAIN_MUTEX_LOCK (chain); > > endpad is source pad, not sink pad. EOS is downstream event. That's why I said gst_pad_push_event() on the endpad :) That should do the same as gst_pad_send_event() on the peer, but is a few lines shorter. > @@ +3722,2 @@ > chain->drained = chain->endpad->drained; > + *drained = chain->endpad->notlinked; > > drained state not saved in the pad (chain->endpad->drained != TRUE), only now Good point, missed that. That would need a comment there :) Or maybe use a separate variable with a speaking name for this. > @@ +4645,3 @@ > + if (drained) { > + gst_buffer_unref (buffer); > + gboolean drained; > > if not-linked stream longer then active and we already drained signal emit This would break if you try to seek to some position after EOS I think, you would also need to reset the drained flag then. Also what's the problem with letting GstPad worry about this?
(In reply to comment #58) > > > @@ +3682,3 @@ > > + peer = gst_pad_get_peer (GST_PAD_CAST (chain->endpad)); > > + if (peer) { > > + CHAIN_MUTEX_LOCK (chain); > > > > endpad is source pad, not sink pad. EOS is downstream event. > > That's why I said gst_pad_push_event() on the endpad :) That should do the same > as gst_pad_send_event() on the peer, but is a few lines shorter. endpad already has EOS state/flag (We're EOS), event will not be sent
Created attachment 289267 [details] [review] fix drain_and_switch_* broke my brain. gst_pad_push_event(endpad) can not be used because endpad already has EOS flag and any event will not be sent.
I change drain_and_switch_* because notlinked may become persistent.
Please excuse me for the disrespectful remarks. I did not want to offend.
I did not notice any disrespectful remarks :) I was just busy with other stuff, that's why I didn't get to take a look at this again. It's still on my list.
Maybe I found a bug, it takes time
You definitely found a bug :)
Created attachment 289637 [details] [review] fix and new tests If the EOS event is received from the *linked* pad, then pending EOS events are sent when all pads are drained or not linked. I also added tests: 1. video, video 2. video+audio, video+audio 3. video+audio, video+audio (audio disabled) 4. video, video+audio /* not used */ #if 0 static gboolean gst_decode_chain_is_drained (GstDecodeChain * chain); static gboolean gst_decode_group_is_drained (GstDecodeGroup * group); #endif
Review of attachment 289637 [details] [review]: Please explain what exactly you're doing here and why, in the commit message ::: gst/playback/gstdecodebin2.c @@ +471,3 @@ static gboolean gst_decode_chain_is_drained (GstDecodeChain * chain); +static gboolean gst_decode_group_is_drained (GstDecodeGroup * group); +#endif Where do you stop using them? @@ +4641,3 @@ + GstDecodePad *dpad = GST_DECODE_PAD (parent); + GstFlowReturn ret = gst_proxy_pad_chain_default (pad, parent, buffer); + gboolean notlinked = (ret == GST_FLOW_NOT_LINKED); Handling not-linked specially here seems wrong. If anything this should be (!= OK && != FLUSHING) or something like that ::: tests/check/elements/playbin-complex.c @@ +1036,3 @@ + gst_object_unref (bus); + + g_usleep (10000); Why do you sleep here? @@ +1037,3 @@ + + g_usleep (10000); + fail_unless (remaining == 0); You only check if the about-to-finish signal is emitted often enough... but that doesn't mean that the second URI is actually played. Maybe it just went EOS directly after the first? @@ +1101,3 @@ + + g_usleep (10000); + fail_unless (remaining == 0); Same two comments here @@ +1138,3 @@ + g_signal_connect (G_OBJECT (playbin), "about-to-finish", + (GCallback) play_next, &remaining); + g_object_set (G_OBJECT (playbin), "flags", 1, NULL); flags are usually set by first getting the current flags, and then unsetting or setting any flags you care about @@ +1165,3 @@ + + g_usleep (10000); + fail_unless (remaining == 0); And here
Review of attachment 289637 [details] [review]: ::: gst/playback/gstdecodebin2.c @@ +471,3 @@ static gboolean gst_decode_chain_is_drained (GstDecodeChain * chain); +static gboolean gst_decode_group_is_drained (GstDecodeGroup * group); +#endif These static functions call each other, and nowhere more (circular reference). @@ +4641,3 @@ + GstDecodePad *dpad = GST_DECODE_PAD (parent); + GstFlowReturn ret = gst_proxy_pad_chain_default (pad, parent, buffer); + gboolean notlinked = (ret == GST_FLOW_NOT_LINKED); You know better, but that's exactly what I need. No matter: success or failed, only not-linked (comment 39) ::: tests/check/elements/playbin-complex.c @@ +1036,3 @@ + gst_object_unref (bus); + + ("caps:video/x-raw, " if the about-to-finish signal is emitted several times (from different threads), then the next check "fail_unless (remaining == 0)" could be completed successfully, but could fail (fifty-fifty). Adding a delay we will be sure that all calls completed. @@ +1037,3 @@ + + g_usleep (10000); + "format=(string)I420, " What should I do? We can try to count the number of STREAM_START messages. @@ +1138,3 @@ + g_signal_connect (G_OBJECT (playbin), "about-to-finish", + (GCallback) play_next, &remaining); + gboolean done = FALSE; Ok
Created attachment 290073 [details] [review] fix and new tests according to the recommendations of part-seqnums.txt I copy the seqnum from the original EOS to the newly created. I think it is time to end our discussion :), if I'm somewhere is not right, you better do it yourself
It seems no one uses continuous playback. I need to broadcast on the network. It is not necessary all the time to interrupt the flow
approaching release of my project. I do not want to fork gstreamer %( Somebody do something that worked continuous playback or accept my commit
Essence of the problem: It is very important for continuous playback send all EOS events after the emission 'drained' signal to the playbin switched on the next media. If the container contains multiple audio streams, only one stream will be linked to audio sink. input-selector to pump buffer all streams synchronously with the active stream. When the active stream ends, the selector will not know and will wait for the next buffer even if there was only a last buffer in not linked stream. Not linked stream never send the EOS event. Playback stops. To prevent this need to wait for EOS events only from the connected streams. PLEASE, I spent a lot of time to understand the problem and find an acceptable solution. Help me finish the job. Thank you.
Hi, This issue is fixed with gapless playback in playbin3.