GNOME Bugzilla – Bug 736655
basesink: preroll issue for some clips which audio is shorter than video
Last modified: 2018-11-03 12:23:22 UTC
I have some clips which audio is shorter than video. The player can't change state from PLAYING to PAUSE state after audio is finish and video is playing. The root cause is audio sink hasn't received GAP event and can't finish preroll and video sink changed to PAUSE state. It is ok if send EOS when audio finish, but streamsynchronizer send GAP event. I haven't seem streamsynchronizer send GAP event when player change state from PLAYING to PAUSE state. Can give me some advice or do you need more information?
Below is my understand: o Change EOS event to GAP event is used to keep sink alive when one track is shorter than other. So can select another track and sink can work rightly. o Gststreamsynchronizer will send GAP event when one track reach EOS and another track still has data. And will send all tracks EOS when all track reach EOS. o The shorter track sink can’t get buffer for preroll as another track is prerolled and blocked on gst_pad_push() function. So can’t finish state change to PAUSE.
My solution is send GAP when PLAYING to PAUSE in gststreamsynchronizer to let sink finish preroll. I will try it.
All elements that don't sync to the clock have to handle PLAYING and PAUSED the same usually, so this seems wrong. (In reply to comment #1) > o The shorter track sink can’t get buffer for preroll as another track is > prerolled and blocked on gst_pad_push() function. So can’t finish state change > to PAUSE. Why is it blocked? Are you missing queues before the video input and audio input of streamsynchronizer and after it? Can you show a backtrace of all threads in that situation?
I used playbin. below is the thread info. (gdb) info th Id Target Id Frame 13 Thread 0x6c2f3460 (LWP 13074) "aqueue:src" 0x76fa0c98 in __pthread_cond_wait (cond=0x74b487f8, mutex=0x0) at pthread_cond_wait.c:187 12 Thread 0x6cb42460 (LWP 13073) "vqueue:src" 0x76fa0c98 in __pthread_cond_wait (cond=0x1ba650, mutex=0x0) at pthread_cond_wait.c:187 11 Thread 0x6d342460 (LWP 13072) "compositor thre" 0x76fa0c98 in __pthread_cond_wait (cond=0x74b42848, mutex=0x0) at pthread_cond_wait.c: 187 10 Thread 0x720d3460 (LWP 13068) "threaded-ml" 0x76f20c00 in poll () at ../sysdeps/unix/syscall-template.S:81 8 Thread 0x72afa460 (LWP 13065) "multiqueue0:src" 0x76fa0c98 in __pthread_cond_wait (cond=0x19f1a0, mutex=0x0) at pthread_cond_wait.c:18 7 7 Thread 0x738cd460 (LWP 13064) "multiqueue0:src" 0x76fa0c98 in __pthread_cond_wait (cond=0x74b453a0, mutex=0x0) at pthread_cond_wait.c: 187 6 Thread 0x740cd460 (LWP 13063) "aiurdemux0:sink" 0x76fa0c98 in __pthread_cond_wait (cond=0x74200d98, mutex=0x0) at pthread_cond_wait.c: 187 5 Thread 0x74aff460 (LWP 13062) "typefind:sink" 0x76fa0c98 in __pthread_cond_wait (cond=0x198bc8, mutex=0x0) at pthread_cond_wait.c:187 4 Thread 0x754ff460 (LWP 13061) "gplay-1.0" 0x76fa0c98 in __pthread_cond_wait (cond=cond@entry=0x1c860, mutex=0x0, mutex@entry=0x1c834) at pthread_cond_wait.c:187 3 Thread 0x75eaa460 (LWP 13060) "gplay-1.0" 0x76fa4c10 in nanosleep () at ../sysdeps/unix/syscall-template.S:81 2 Thread 0x766aa460 (LWP 13059) "gplay-1.0" 0x76efab70 in nanosleep () ---Type <return> to continue, or q <return> to quit--- at ../sysdeps/unix/syscall-template.S:81 * 1 Thread 0x76ff8000 (LWP 13055) "gplay-1.0" 0x76fa1050 in __pthread_cond_timedwait (cond=0x162fa0, mutex=0x0, abstime=0x7efff628) at pthread_cond_timedwait.c:194
which thread back trace do you need or you need all thread back trace?
Of all threads :) "thread apply all bt" in gdb
(gdb) info th Id Target Id Frame 14 Thread 0x63b25460 (LWP 1223) "pool" 0x76fa1050 in __pthread_cond_timedwait (cond=cond@entry=0x26780, mutex=0x0, abstime=abstime@entry= 0x63b24d98) at pthread_cond_timedwait.c:194 13 Thread 0x6c383460 (LWP 1222) "aqueue:src" 0x76fa0c98 in __pthread_cond_wait (cond=cond@entry=0x74b499a8, mutex=0x0) at pthread_cond_wa it.c:187 12 Thread 0x6cbd2460 (LWP 1221) "vqueue:src" 0x76fa0c98 in __pthread_cond_wait (cond=cond@entry=0x11a8a0, mutex=0x0) at pthread_cond_wait .c:187 11 Thread 0x6d3d2460 (LWP 1220) "compositor thre" 0x76fa0c98 in __pthread_cond_wait (cond=cond@entry=0x74b48800, mutex=0x0) at pthread_co nd_wait.c:187 10 Thread 0x72163460 (LWP 1218) "threaded-ml" 0x76f20c00 in poll () at ../sysdeps/unix/syscall-template.S:81 8 Thread 0x72b95460 (LWP 1216) "multiqueue0:src" 0x76fa0c98 in __pthread_cond_wait (cond=cond@entry=0x163988, mutex=0x0) at pthread_cond _wait.c:187 7 Thread 0x739b8460 (LWP 1215) "multiqueue0:src" 0x76fa0c98 in __pthread_cond_wait (cond=cond@entry=0x74b465c8, mutex=0x0) at pthread_co nd_wait.c:187 6 Thread 0x741cf460 (LWP 1214) "aiurdemux0:sink" 0x76fa0c98 in __pthread_cond_wait (cond=cond@entry=0x74c00d98, mutex=0x0) at pthread_co nd_wait.c:187 5 Thread 0x74aff460 (LWP 1213) "typefind:sink" 0x76fa0c98 in __pthread_cond_wait (cond=cond@entry=0x136600, mutex=0x0) at pthread_cond_w ait.c:187 4 Thread 0x754ff460 (LWP 1212) "gplay-1.0" 0x76fa0c98 in __pthread_cond_wait (cond=cond@entry=0x1c860, mutex=0x0, mutex@entry=0x1c834) at pthread_cond_wait.c:187 ---Type <return> to continue, or q <return> to quit--- 3 Thread 0x75eb5460 (LWP 1211) "gplay-1.0" 0x76fa4c10 in nanosleep () at ../sysdeps/unix/syscall-template.S:81 2 Thread 0x766b5460 (LWP 1210) "gplay-1.0" 0x76efab70 in nanosleep () at ../sysdeps/unix/syscall-template.S:81 * 1 Thread 0x76ff8000 (LWP 1205) "gplay-1.0" 0x76fa1050 in __pthread_cond_timedwait (cond=cond@entry=0x10fc88, mutex=0x0, abstime=abstime@ entry=0x7efff688) at pthread_cond_timedwait.c:194 (gdb) t 14 [Switching to thread 14 (Thread 0x63b25460 (LWP 1223))]
+ Trace 234097
The problem here seems that the videodecoder goes directly into the streamsynchronizer without a queue between them.
I don't understand. Why queue between video decoder and streamsynchronizer can effect sink preroll?
Actually you're right, it shouldn't. And I assume it doesn't make a difference? Where is the backtrace of the audio related threads? There should be one from the multiqueue through the decoder to streamsynchronizer, and another to the audio sink (and I only see one thread with a sink). What exactly is the situation at this point? I see that the video is trying to pre-roll but there is nothing from the audio anywhere. Did streamsynchronizer receive EOS for the audio already and that's why nothing is to be seen for the audio? If so it should've sent a GAP event to the audio sink, so the audio sink prerolls and the video sink can finish prerolling.
The test clip is audio shorter than video. user pause the player when audio already EOS and video is playing. My understand is: 1. sink will finish pre-roll if received GAP event; 2. streamsynchronizer should send GAP event when PLAYING to PAUSE state as it is blocked by gst_pad_push() function in chain function and can't send GAP event; 3. video already reach PAUSE state as video sink already received one buffer; Is my understand right?
1) and 3) is correct, 2) is not. streamsynchronizer is not blocked anywhere for the audio pad, but it should've received the EOS event there before. As result of the EOS event it should've sent a GAP event downstream to pre-roll the audio sink.
Any chance you could make the clip available that triggers this problem?
(In reply to comment #12) > 1) and 3) is correct, 2) is not. streamsynchronizer is not blocked anywhere for > the audio pad, but it should've received the EOS event there before. As result > of the EOS event it should've sent a GAP event downstream to pre-roll the audio > sink. Which element should send the GAP event and how? (In reply to comment #13) > Any chance you could make the clip available that triggers this problem? I found same issue on: https://bugzilla.gnome.org/show_bug.cgi?id=633700 maybe you can get test clip on that thread.
(In reply to comment #14) > (In reply to comment #12) > > 1) and 3) is correct, 2) is not. streamsynchronizer is not blocked anywhere for > > the audio pad, but it should've received the EOS event there before. As result > > of the EOS event it should've sent a GAP event downstream to pre-roll the audio > > sink. > > Which element should send the GAP event and how? streamsynchronizer, when it receives EOS on a stream without a buffer before that and when not forwarding EOS (because not all streams are EOS yet).
streamsynchronizer already send the GAP event when received EOS and not forwarding EOS from the code.
Then the question would be why the audio sink does not pre-roll because of that GAP event. Can you check what happens there?
I can check it. But below is my concern: 1) streamsynchronizer audio sink pad chain will never be called after audio reach EOS. 2) streamsynchronizer will send GAP to audio sink after received video buffer and pushed to video sink. 3) audio sink need GAP event to pre-roll every time when PLAYING to PAUSE. so streamersynchronizer should send GAP every time when PLAYING to PAUSE. 4) streamersynchronizer video pad chain will be block after video sink received one buffer and changed to PAUSE state. the reason is video sink is PAUSED and queue between streamsynchronizer and video sink is full. Is it right? Can the GAP event send by streamsynchronizer when audio EOS cause audio sink pre-rolled all the time?
I see your concern. Maybe streamsynchronizer really has to send a GAP event whenever going from PLAYING to PAUSED if a stream is at EOS but the others are not yet. Does this fix all the problems for you?
I have tried it, but found audio sink till has issue. I will try to fix it in base sink and send out the patch for your review.
Created attachment 286349 [details] [review] basesink need call do_preroll when received GAP event.
Created attachment 286350 [details] [review] streamsynchronizer need send GAP event when state change from PLAYING to PAUSE.
Some relevant discussion from IRC: <slomo_> wtay_: https://bugzilla.gnome.org/show_bug.cgi?id=736655 what do you think of these patches? <wtay_> slomo_, I think in general we agreed that a GAP event should preroll a sink and we can do that now because caps are not on the buffer so the sink should be negotiated and ready to go <wtay_> but audiosink should play silence for the GAP event, I doubt it does that now <slomo_> yes to both <slomo_> but sending the GAP event in PLAYING->PAUSED in streamsynchronizer? :) <wtay_> that does not seem right <slomo_> good, i agree ;) do you have another idea? <wtay_> I can only think of sending a gap event with unlimited duration, which should make basesink go to play<->paused without ASYNC <wtay_> theoretically, I doubt it does that currently <slomo_> and make it switch between those states forever without need for a further GAP event? <wtay_> I know audiosink goes to paused SYNC when it still has unplayed audio (unplayed as in not yet sent to the device)
So the solution is like below: 1) streamsynchronizer will send GAP event with unlimited duration when received short track EOS. 2 ) basesink will treat the GAP event with unlimited duration specially and needn't pre-roll when change to PAUSE state. 3 ) streamsynchronizer needn't send GAP event when process pad chain function for the short track. I checked the latest GST 1.x code, 1) is ready, 2) basesink hasn't set needn't pre-roll flag when received GAP event which has unlimited duration. 3) streamsynchronizer still send GAP in pad chain function when one track reach EOS. If your agree above method, I will generate patch to fix those issue.
That sounds correct, yes. 2) also needs to go back into normal pre-rolling mode if receiving a buffer or finite GAP event.
I checked gstaudiobasesink, the GAP event with unlimited duration and EOS have same handle code. So, if we use above method, streamsynchronizer send GAP event with unlimited duration is totally same with EOS event. Do we miss something? What is the reason of change EOS event to GAP event?
Do your gays has any idea? I have environment to reproduce the issue and try any idea.
What's the status here now and the summary of the situation? Also isn't this the same or closely related to bug #727074?
Will check it on Gstreamer 1.4.1 later and feedback.
*** Bug 727074 has been marked as a duplicate of this bug. ***
*** Bug 726854 has been marked as a duplicate of this bug. ***
I checked the code, propose below method to fix the issue. Please help to review: 1. Audio sink will generate unlimited length mute audio when received GAP event with -1 duration and write audio device until flush, state change. gst_base_sink_wait_preroll() will call when interrupt and needn't preroll buffer to finish preroll. 2. Video sink will enter GAP_EOS states after received GAP event with -1 duration and will call gst_base_sink_wait_preroll() when flush, state change, so needn't buffer to finish preroll. 3. Streamersynchronizor needn't send GAP event in chain() function. Only need send GAP event when received EOS event. Is it reasonable? Will try the method and upload patch if the method is ok.
No, that won't work unfortunately. A GAP event with duration=-1 is supposed to indicate a GAP of unknown length that will end at the timestamp of the next buffer or GAP event.
My proposal also need end if buffer with timestamp or GAP event come. I think streamsynchronizor send GAP in chain() function has much limitation. We also has use case to send audio EOS when rewind. Can it support rewind? If the proposals all reject, can you give some suggestion?
I don't see the relation between EOS and rewind. What exactly are you doing there? In your proposal you said that the audio sink will generate unlimited length silence audio if a GAP event with duration=-1 is received. However it should only generate silence until the timestamp of the next buffer or GAP event. Same goes for video, it should not change any state but continue showing the previous frame (or black if there was none) until the timestamp of the next buffer instead of forever.
If one file has big gap in the middle of file, you can get the next valid audio packet. So you don't know the next valid audio time stamp.
As I checked before, streamsynchroinzor can't send GAP event in chain() function when rewind after audio track send EOS and video is rewinding.
For video sink change, the purpose is finish pre-roll when change to PAUSE state. Not actural change state.
(In reply to comment #36) > If one file has big gap in the middle of file, you can get the next valid audio > packet. So you don't know the next valid audio time stamp. Yes, you only know it once it is there. Which is why I'm not sure yet what the correct solution here would be. If you just generate infinite amount of silence, you will never play anything after such a GAP event. (In reply to comment #37) > As I checked before, streamsynchroinzor can't send GAP event in chain() > function when rewind after audio track send EOS and video is rewinding. What do you mean with rewind here?
The infinite amount of silence should be interrupt by buffer or GAP event. Some thing like 20 ms one time. If still no buffer or GAP event, playback another 20 ms. Rewind case is our demux will send EOS to audio track when rewind. Video will only send key frame. I remember streamsynchronizer can't send GAP event to audio sink. Should be a bug. The root cause of the issue in the thread is audio sink can't finish pre-roll as GAP event can't be send as streamsynchronizer blocked on video push. And the GAP event send in chain function has many conditions (so it will can't be send). So is there more good resolution to handle this?
(In reply to comment #40) > The infinite amount of silence should be interrupt by buffer or GAP event. Some > thing like 20 ms one time. If still no buffer or GAP event, playback another 20 > ms. That's what would automatically happen in GstAudioBaseSink if the ringbuffer is started. It would output silence all the time unless some real data is written into the ringbuffer. So there shouldn't be much necessary for this change, other than starting the ringbuffer and prerolling on GAP events (and EOS too, I remember seeing a bug about that in Bugzilla recently). > Rewind case is our demux will send EOS to audio track when rewind. Video will > only send key frame. I remember streamsynchronizer can't send GAP event to > audio sink. Should be a bug. > > The root cause of the issue in the thread is audio sink can't finish pre-roll > as GAP event can't be send as streamsynchronizer blocked on video push. And the > GAP event send in chain function has many conditions (so it will can't be > send). So is there more good resolution to handle this? I don't understand what you mean here.
If audio is shorter and reach EOS, audio GAP event will send after video pad pushed video buffer in streamsynchronizer. Video pipeline should be full with data in normal playback. Video pad push will blocked when change to PAUSE state and audio GAP event can't be send out. Audio sink can't finish pre-roll. The attachment patch will send GAP event during PLAYING to PAUSE state change. But the patch reject. Can put send GAP event before video pad push? If one stream seek to only video part, audio will send Segment and EOS. But as audio decoder no output, so Segment can't send out of audio decoder. Below code in streamsynchronizer will bypass send GAP. How to handle this? if (!ostream->is_eos || ostream->segment.format != GST_FORMAT_TIME) continue;
I have verified the issue on Gstreamer 1.4.1 and read whole thread to refresh my head. Still has the issue on Gstreamer 1.4.1. But the second attached patch can fix the issue, don't need the first patch any more. But the method has been reject base on comment #23. The root cause of the issue is video pad push buffer blocked when state change from PLAYING to PAUSE and can't send out audio GAP event in streamsynchronizer to finish audio sink pre-roll. Based on comment #24 and #25. You agree below method. 1) streamsynchronizer will send GAP event with unlimited duration when received short track EOS. 2 ) basesink will treat the GAP event with unlimited duration specially and needn't pre-roll when change to PAUSE state. basesink will change to normal pre-roll after received buffer or GAP with duration or flush. 3 ) streamsynchronizer needn't send GAP event when process pad chain function for the short track. Do you still think the method is ok? if so, I will try it and upload to review.
1) yes 2) GAP events should *always* preroll a sink 3) what do you mean with that?
The key point is audiosink can't received GAP to finish pre-roll. The root cause is video pad push has been blocked. Don't know how to fix it. Can you advice?
Why can't the audiosink receive the GAP event because something in the video part of the pipeline is blocked?
As talked on IM. EOS event will blocked on event function and will send GAP event after post one GCond and than block again. Should unblock after flush or shutdown. Should post GCond when change state from PLAYING to PAUSE. Do need post when PAUSE to PLAYING? sink hasn't any dependence when change state from PAUSE to PLAYING.
Summary of IRC dicussion is to change streamsynchronizer to: a) Wait in the event function when EOS is received until all are EOS on a GCond, and forward EOS from there then (currently we send EOS from the event function of the last EOS pad) b) Change the GAP events that are currently sent from the chain function of a pad to all other pads that are late. They should be sent from the event function of that specific pad instead by waking up the GCond shortly. c) Send a new GAP event from the event function whenever going from PAUSED->PLAYING and PLAYING->PAUSED by shortly waking up the GCond. d) Wake up the GCond and forward the EOS event when either the last pad went EOS, or a flush-start event is received on that pad (or its corresponding srcpad), or the streamsynchronizer goes from PAUSED->READY. a) and b) also fix potential deadlocks.
This bug is also related (or the same?) as bug #633700.
For b), do need add position and duration into the GAP event? How to get it? How to get it if no segment? Should it same as current code in chain function? Lag one second?
Yes, the same values as are used right now. You need to pass them over to the other thread so it can be used after waking up from the GCond.
I mentioned streamsynchronizer hasn't received segment when EOS is fixed by below commit. So should be not the concern. But I can't see same process in video decoder base. Do we need add send pending event when received EOS in video decoder base? commit b2413d46edb7c97d1ad4e2535dd5fe737b0c04bc Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Fri Nov 28 15:06:27 2014 +0100 audiodecoder: Push pending events before sending EOS. Segments are added to the pending events, and pushing a segment is mandatory before sending EOS. + Adds a test. https://bugzilla.gnome.org/show_bug.cgi?id=740853
Element should send pending sticky events before send EOS event? Is it right? We have clips which has valid media information for audio track, but haven't any audio data, we can't get audio TAG information on playbin. Audio and video decoder base should send pending sticky events before send EOS event. This is another issue, should submit one ticket for it. But segment event must send before EOS, or streamsynchrinozer can't work.
> Element should send pending sticky events before send EOS event? Is it right? I think so, yes. Please file a separate bug for that.
filed one bug with below link: https://bugzilla.gnome.org/show_bug.cgi?id=742789
Created attachment 294326 [details] [review] patch to fix the issue.
The patch haven't include below statement: b) Change the GAP events that are currently sent from the chain function of a pad to all other pads that are late. They should be sent from the event function of that specific pad instead by waking up the GCond shortly. Will do it later. Note: Will send eos immediately if not seen any data. As decoder can't get output caps if can't decode any valid audio data. Sink will report error (not negotiate) if send GAP event.
Created attachment 294330 [details] [review] Patch for item b)
Created attachment 294382 [details] [review] update the patch for minor change. seen_data changed to mean received data in the whole time. not received data after seek.
Created attachment 294383 [details] [review] update the patch
Will write unit test for the case and upload patch later.
Review of attachment 294382 [details] [review]: I think in general you need to call gst_pad_get_element_private() again whenever you have unlocked the mutex and locked it again. in the meantime the application could've removed the pad from streamsynchronizer, and freed the stream. ::: gst/playback/gststreamsynchronizer.c @@ +493,3 @@ + if (!seen_data) { + /* no buffer form the pad anytime, not negotiate, send eos */ + stream->eos_sent = TRUE; Should probably also only be set when it happens below @@ +504,3 @@ timestamp = stream->segment.stop; + stream->segment.position = timestamp; Why? @@ +520,3 @@ GstStream *ostream = l->data; /* local snapshot of current pads */ + ostream->eos_sent = TRUE; Should probably be done when it actually happened, not before @@ +556,3 @@ + if (self->eos) { + GST_DEBUG_OBJECT (pad, "send EOS event"); + ret = gst_pad_push_event (srcpad, gst_event_new_eos ()); stream->eos_sent = TRUE now? @@ +561,3 @@ + /* send eos as sink can't handle if haven't negotiate */ + GST_DEBUG_OBJECT (pad, "send EOS event"); + ret = gst_pad_push_event (srcpad, gst_event_new_eos ()); The problem here is that we can't preroll the sink again later here. So after you forwarded EOS, then went to PAUSED and the PLAYING again... nothing will ever be sent to that sink again. Not even an EOS. Maybe another EOS should be sent?
Review of attachment 294383 [details] [review]: ::: gst/playback/gststreamsynchronizer.c @@ +229,3 @@ + event = + gst_event_new_gap (stream->segment.position, stream->gap_duration); Access to GAP duration should probably be protected by the mutex too
(In reply to comment #62) > Review of attachment 294382 [details] [review]: > > I think in general you need to call gst_pad_get_element_private() again > whenever you have unlocked the mutex and locked it again. in the meantime the > application could've removed the pad from streamsynchronizer, and freed the > stream. > Will fix it in function gst_stream_synchronizer_wait(). Call gst_pad_get_element_private() in while loop. > ::: gst/playback/gststreamsynchronizer.c > @@ +493,3 @@ > + if (!seen_data) { > + /* no buffer form the pad anytime, not negotiate, send eos */ > + stream->eos_sent = TRUE; > > Should probably also only be set when it happens below > Will set it after pushed EOS event. > @@ +504,3 @@ > timestamp = stream->segment.stop; > > + stream->segment.position = timestamp; > > Why? > Record current position and use the position when push GAP event. stream->segment.position also been updated in chain(). > @@ +520,3 @@ > GstStream *ostream = l->data; > /* local snapshot of current pads */ > + ostream->eos_sent = TRUE; > > Should probably be done when it actually happened, not before > will set it after pushed EOS event. > @@ +556,3 @@ > + if (self->eos) { > + GST_DEBUG_OBJECT (pad, "send EOS event"); > + ret = gst_pad_push_event (srcpad, gst_event_new_eos ()); > > stream->eos_sent = TRUE now? > Yes, get lock and set it as the lock has been released. > @@ +561,3 @@ > + /* send eos as sink can't handle if haven't negotiate */ > + GST_DEBUG_OBJECT (pad, "send EOS event"); > + ret = gst_pad_push_event (srcpad, gst_event_new_eos ()); > > The problem here is that we can't preroll the sink again later here. So after > you forwarded EOS, then went to PAUSED and the PLAYING again... nothing will > ever be sent to that sink again. Not even an EOS. Maybe another EOS should be > sent? This only will be happened when not received data in anytime. So the track is all corrupted. sink don't know how to initialize the sink device. Sink needn't preroll after received EOS. and streamsynchronizer can't send anything through pad after send EOS event. So pipeline can PAUSE again. Currently, we use one workaround for the issue. The workaround is send EOS immediately after received EOS. And not found issue with the workaround. Demux will send EOS again after seek. So seek is ok.
(In reply to comment #63) > Review of attachment 294383 [details] [review]: > > ::: gst/playback/gststreamsynchronizer.c > @@ +229,3 @@ > > + event = > + gst_event_new_gap (stream->segment.position, stream->gap_duration); > > Access to GAP duration should probably be protected by the mutex too Those code is in gst_stream_synchronizer_wait(). gst_stream_synchronizer_wait() is called in below code. gst_stream_synchronizer_wait() should be called with GST_STREAM_SYNCHRONIZER_LOCK. + GST_STREAM_SYNCHRONIZER_LOCK (self); + self->send_gap_event = TRUE; + ret = gst_stream_synchronizer_wait (self, srcpad); + GST_STREAM_SYNCHRONIZER_UNLOCK (self);
(In reply to comment #62) > Review of attachment 294382 [details] [review] [details]: > > I think in general you need to call gst_pad_get_element_private() again > whenever you have unlocked the mutex and locked it again. in the meantime the > application could've removed the pad from streamsynchronizer, and freed the > stream. > Will fix it in function gst_stream_synchronizer_wait(). Call gst_pad_get_element_private() in while loop. > ::: gst/playback/gststreamsynchronizer.c > @@ +493,3 @@ > + if (!seen_data) { > + /* no buffer form the pad anytime, not negotiate, send eos */ > + stream->eos_sent = TRUE; > > Should probably also only be set when it happens below > Will set it after pushed EOS event. > @@ +504,3 @@ > timestamp = stream->segment.stop; > > + stream->segment.position = timestamp; > > Why? > Record current position and use the position when push GAP event. stream->segment.position also been updated in chain(). > @@ +520,3 @@ > GstStream *ostream = l->data; > /* local snapshot of current pads */ > + ostream->eos_sent = TRUE; > > Should probably be done when it actually happened, not before > will set it after pushed EOS event. > @@ +556,3 @@ > + if (self->eos) { > + GST_DEBUG_OBJECT (pad, "send EOS event"); > + ret = gst_pad_push_event (srcpad, gst_event_new_eos ()); > > stream->eos_sent = TRUE now? > Yes, get lock and set it as the lock has been released. > @@ +561,3 @@ > + /* send eos as sink can't handle if haven't negotiate */ > + GST_DEBUG_OBJECT (pad, "send EOS event"); > + ret = gst_pad_push_event (srcpad, gst_event_new_eos ()); > > The problem here is that we can't preroll the sink again later here. So after > you forwarded EOS, then went to PAUSED and the PLAYING again... nothing will > ever be sent to that sink again. Not even an EOS. Maybe another EOS should be > sent? This only will be happened when haven't received data in anytime. So the track is all corrupted. sink don't know how to initialize the sink device. Sink needn't preroll after received EOS. So pipeline can PAUSE again. Demux will send EOS again after seek. So seek is ok.
Created attachment 294947 [details] [review] Updated patch based on review.
Created attachment 294948 [details] [review] Updated patch based on review.
(In reply to comment #64) > > > + stream->segment.position = timestamp; > > > > Why? > > > Record current position and use the position when push GAP event. > stream->segment.position also been updated in chain(). This probably also has to be protected with a mutex then > > @@ +561,3 @@ > > + /* send eos as sink can't handle if haven't negotiate */ > > + GST_DEBUG_OBJECT (pad, "send EOS event"); > > + ret = gst_pad_push_event (srcpad, gst_event_new_eos ()); > > > > The problem here is that we can't preroll the sink again later here. So after > > you forwarded EOS, then went to PAUSED and the PLAYING again... nothing will > > ever be sent to that sink again. Not even an EOS. Maybe another EOS should be > > sent? > This only will be happened when not received data in anytime. So the track is > all corrupted. sink don't know how to initialize the sink device. > Sink needn't preroll after received EOS. and streamsynchronizer can't send > anything through pad after send EOS event. So pipeline can PAUSE again. > Currently, we use one workaround for the issue. The workaround is send EOS > immediately after received EOS. And not found issue with the workaround. > Demux will send EOS again after seek. So seek is ok. Not sure I can follow :) You mean you have a custom workaround somewhere for the case of PLAYING->PAUSED->PLAYING if you get into this situation? But with just your patch it would never go to PLAYING again?
(In reply to comment #65) > > Access to GAP duration should probably be protected by the mutex too > > Those code is in gst_stream_synchronizer_wait(). gst_stream_synchronizer_wait() > is called in below code. gst_stream_synchronizer_wait() should be called with > GST_STREAM_SYNCHRONIZER_LOCK. > > + GST_STREAM_SYNCHRONIZER_LOCK (self); > + self->send_gap_event = TRUE; > + ret = gst_stream_synchronizer_wait (self, srcpad); > + GST_STREAM_SYNCHRONIZER_UNLOCK (self); But that means that you send the GAP event while holding that mutex. That's never a good idea :) Don't send any events, queries or buffers downstream while holding the mutex.
(In reply to comment #69) > (In reply to comment #64) > > > > > + stream->segment.position = timestamp; > > > > > > Why? > > > > > Record current position and use the position when push GAP event. > > stream->segment.position also been updated in chain(). > > This probably also has to be protected with a mutex then > > > > @@ +561,3 @@ > > > + /* send eos as sink can't handle if haven't negotiate */ > > > + GST_DEBUG_OBJECT (pad, "send EOS event"); > > > + ret = gst_pad_push_event (srcpad, gst_event_new_eos ()); > > > > > > The problem here is that we can't preroll the sink again later here. So after > > > you forwarded EOS, then went to PAUSED and the PLAYING again... nothing will > > > ever be sent to that sink again. Not even an EOS. Maybe another EOS should be > > > sent? > > This only will be happened when not received data in anytime. So the track is > > all corrupted. sink don't know how to initialize the sink device. > > Sink needn't preroll after received EOS. and streamsynchronizer can't send > > anything through pad after send EOS event. So pipeline can PAUSE again. > > Currently, we use one workaround for the issue. The workaround is send EOS > > immediately after received EOS. And not found issue with the workaround. > > Demux will send EOS again after seek. So seek is ok. > > Not sure I can follow :) You mean you have a custom workaround somewhere for > the case of PLAYING->PAUSED->PLAYING if you get into this situation? But with > just your patch it would never go to PLAYING again? And that it will only go to PLAYING again after a seek, but not when pausing without a seek?
Summary of IRC talk: Should unlock before send GAP event and hold the lock again after send GAP event. Should call gst_pad_get_element_private() after take the lock after send GAP and EOS event.
Unit test: 1. Source pad should received GAP event after one track reach EOS. 2. Source pad should received GAP event when state change from PLAYING to PAUSE after one track reach EOS. 3. Source pad should received EOS event when one track haven't any data and reach EOS.
Created attachment 295067 [details] [review] Update based on IRC talk.
Created attachment 295068 [details] [review] Update based on IRC talk.
Streamsynchronizer Unit Test: 1. Streamsynchronizer sink pad should send GAP event after one track reach EOS when normal playing. 2. Streamsynchronizer sink pad should send GAP event when state change from PLAYING to PAUSE after one track reach EOS. 3. Streamsynchronizer sink pad should send EOS event when one track haven't any data and reach EOS. Is it ok?
Also check for PLAYING->PAUSED->PLAYING, otherwise seems ok. Maybe also don't check for GAP or EOS events, but if a pipeline with a sink after streamsynchronizer works properly in these scenarios (i.e. it prerolls instead of waiting forever). That would seem more future-proof, and check for what we wanted to fix here instead of checking for the solution.
So streamsynchronizer unit test should be below: A pipeline with a sink after streamsynchronizer can change state from PLAYING->PAUSED->PLAYING in below scenarios: 1. One track reach EOS after pushed some buffers while another track still push buffer. 2. One track reach EOS without buffer while another track still push buffer. After the pipeline test， do need below test? 1. Streamsynchronizer sink pad should send GAP event after one track reach EOS when normal playing. 2. Streamsynchronizer sink pad should send GAP event when state change from PLAYING to PAUSE after one track reach EOS. 3. Streamsynchronizer sink pad should send EOS event when one track haven't any data and reach EOS.
(In reply to comment #78) > So streamsynchronizer unit test should be below: > > A pipeline with a sink after streamsynchronizer can change state from > PLAYING->PAUSED->PLAYING in below scenarios: > > 1. One track reach EOS after pushed some buffers while another track still push > buffer. > 2. One track reach EOS without buffer while another track still push buffer. Yes, and not only test if PLAYING->PAUSED->PLAYING works but also if it prerolls when one track goes EOS immediately, while the other one produces data > After the pipeline test， do need below test? > [...] No, those are not needed IMHO. They are testing an implementation detail, not the actual behaviour we're interested in.
Created attachment 295290 [details] [review] Update based on IRC talk.
Created attachment 295291 [details] [review] Unit test for streamsynchronizer handle eos.
Upload source code for review firstly.
Created attachment 295329 [details] [review] Update patch for patch format
Created attachment 295330 [details] [review] Update patch for patch format
Created attachment 295409 [details] [review] Send GAP event in event thread.
Created attachment 295410 [details] [review] Don't send GAP event in chain function.
Created attachment 295411 [details] [review] Unit test for streamsynchronizer handle eos.
Ready for all patches. Please help to review. Thanks.
Thanks, I'll try to go through all this later today :)
Thanks, let me know if need improve the patch.
Sorry for still not getting to this, I didn't forget about it :)
My latest patches for audiobasesink in Bug 735666 help with the prerolling here, but only up to a point - audiobasesink will process a GAP with timestamp & duration as if it were an audio buffer of the same time+duration and successfully go PLAYING->PAUSED->PLAYING. That means that streamsynchroniser needs to make sure to send a GAP (or GAPs) that fill the entire time range until it is ready to either start a new stream or send EOS. It looks like these patches will achieve that, but sending a new GAP when changing state in stream synchroniser, but wouldn't it be better to wake the streams that are looping on EOS every time data passes on another pad that 'moves time forward' and have them send another GAP event then? With the approach in these patches, new GAP events are only sent when streamsynchroniser changes state, on the presumption that that's also when the audio sinks change state. Maybe there's pipelines where that assumption isn't true?
(In reply to comment #92) > My latest patches for audiobasesink in Bug 735666 help with the prerolling > here, but only up to a point - audiobasesink will process a GAP with timestamp > & duration as if it were an audio buffer of the same time+duration and > successfully go PLAYING->PAUSED->PLAYING. > How about video sink? Do video sink need GAP event to finish pre-roll when PLAYING->PAUSED? > That means that streamsynchroniser needs to make sure to send a GAP (or GAPs) > that fill the entire time range until it is ready to either start a new stream > or send EOS. > streamsynchronizer will seng GAP with duration is GST_CLOCK_TIME_NONE. Do streamsychronizer need search all segment to get to biggest time range? Or audio sink can handle GST_CLOCK_TIME_NONE. > It looks like these patches will achieve that, but sending a new GAP when > changing state in stream synchroniser, but wouldn't it be better to wake the > streams that are looping on EOS every time data passes on another pad that > 'moves time forward' and have them send another GAP event then? > streamsynchronizer will send GAP event in chain function. But pad push will be blocked when sink changed to PAUSED state and then can't send GAP event. So need send GAP event when state change in streamsynchronizer. > With the approach in these patches, new GAP events are only sent when > streamsynchroniser changes state, on the presumption that that's also when the > audio sinks change state. Maybe there's pipelines where that assumption isn't > true?
(In reply to comment #93) > (In reply to comment #92) > > My latest patches for audiobasesink in Bug 735666 help with the prerolling > > here, but only up to a point - audiobasesink will process a GAP with timestamp > > & duration as if it were an audio buffer of the same time+duration and > > successfully go PLAYING->PAUSED->PLAYING. > > > How about video sink? Do video sink need GAP event to finish pre-roll when > PLAYING->PAUSED? The same thing applies to video sinks - they need a GAP to preroll too. > > That means that streamsynchroniser needs to make sure to send a GAP (or GAPs) > > that fill the entire time range until it is ready to either start a new stream > > or send EOS. > > > streamsynchronizer will seng GAP with duration is GST_CLOCK_TIME_NONE. Do > streamsychronizer need search all segment to get to biggest time range? Or > audio sink can handle GST_CLOCK_TIME_NONE. Duration GST_CLOCK_TIME_NONE is handled as duration = 0 in the sink, so it will effectively 'fill time' up to the start timestamp. > > It looks like these patches will achieve that, but sending a new GAP when > > changing state in stream synchroniser, but wouldn't it be better to wake the > > streams that are looping on EOS every time data passes on another pad that > > 'moves time forward' and have them send another GAP event then? > > > streamsynchronizer will send GAP event in chain function. But pad push will be > blocked when sink changed to PAUSED state and then can't send GAP event. So > need send GAP event when state change in streamsynchronizer. If pad_push() is blocked, you can't send a GAP event downstream there anyway - the STREAM_LOCK() will be held by the pad_push(). That case should work. The one that is broken is where you sent a GAP downstream from teh EOS event handler, advancing time to some time 'timestamp', and then time advanced past that point, and your pad_push_event (pad, gap_event) returned - you need to either push a new gap, or push a buffer then.
(In reply to comment #94) > (In reply to comment #93) > > (In reply to comment #92) > > > My latest patches for audiobasesink in Bug 735666 help with the prerolling > > > here, but only up to a point - audiobasesink will process a GAP with timestamp > > > & duration as if it were an audio buffer of the same time+duration and > > > successfully go PLAYING->PAUSED->PLAYING. > > > > > How about video sink? Do video sink need GAP event to finish pre-roll when > > PLAYING->PAUSED? > > The same thing applies to video sinks - they need a GAP to preroll too. > If video is shorter than audio, streamsynchronizer will send GAP event when received video EOS. How to set the GAP duration? If pipeline change state from PLAYING to PAUSED, do streamsynchronizer need send GAP event again to finish video pre-roll? do streamsynchronizer need send GAP event every time when pipeline change state from PLAYING to PAUSED? > > > That means that streamsynchroniser needs to make sure to send a GAP (or GAPs) > > > that fill the entire time range until it is ready to either start a new stream > > > or send EOS. > > > > > streamsynchronizer will seng GAP with duration is GST_CLOCK_TIME_NONE. Do > > streamsychronizer need search all segment to get to biggest time range? Or > > audio sink can handle GST_CLOCK_TIME_NONE. > > Duration GST_CLOCK_TIME_NONE is handled as duration = 0 in the sink, so it will > effectively 'fill time' up to the start timestamp. > So streamsynchronizer should send GAP with duration is GST_CLOCK_TIME_NONE when received EOS for shorter track? and send GAP event for the shorter track to 'moves time forward' in other track chain function? And send all tracks EOS event after received all tracks EOS event. > > > It looks like these patches will achieve that, but sending a new GAP when > > > changing state in stream synchroniser, but wouldn't it be better to wake the > > > streams that are looping on EOS every time data passes on another pad that > > > 'moves time forward' and have them send another GAP event then? > > > > > streamsynchronizer will send GAP event in chain function. But pad push will be > > blocked when sink changed to PAUSED state and then can't send GAP event. So > > need send GAP event when state change in streamsynchronizer. > > If pad_push() is blocked, you can't send a GAP event downstream there anyway - > the STREAM_LOCK() will be held by the pad_push(). That case should work. The > one that is broken is where you sent a GAP downstream from teh EOS event > handler, advancing time to some time 'timestamp', and then time advanced past > that point, and your pad_push_event (pad, gap_event) returned - you need to > either push a new gap, or push a buffer then. If pad_push() is blocked, streamsynchronizer can't send GAP event for shorter track in chain function. The shorter track is reach EOS, and no any buffer reach streamsynchronizer, so basesink can't finished pre-roll for the shorter track. The longer track is pro-rolled and blocked. So streamsynchronizer need send GAP event for shorter track when state change, and send the GAP event in event handle thread.
(In reply to comment #95) > (In reply to comment #94) > > (In reply to comment #93) > > > (In reply to comment #92) > > > > My latest patches for audiobasesink in Bug 735666 help with the prerolling > > > > here, but only up to a point - audiobasesink will process a GAP with timestamp > > > > & duration as if it were an audio buffer of the same time+duration and > > > > successfully go PLAYING->PAUSED->PLAYING. > > > > > > > How about video sink? Do video sink need GAP event to finish pre-roll when > > > PLAYING->PAUSED? > > > > The same thing applies to video sinks - they need a GAP to preroll too. > > > If video is shorter than audio, streamsynchronizer will send GAP event when > received video EOS. How to set the GAP duration? The duration can be 0 or GST_CLOCK_TIME_NONE, it's fine - the key is that a pad that reaches EOS early should loop in EOS sending gap events downstream with advancing timestamps taken from the other non-EOS stream(s), to keep downstream updated on progress until everything reaches EOS. The gap events it sends should be based on the maximum position of the other stream(s) - buffer times + duration. The existing logic that calculates a start time for the gap event it sends now is probably fine. > If pipeline change state from PLAYING to PAUSED, do streamsynchronizer need > send GAP event again to finish video pre-roll? do streamsynchronizer need send > GAP event every time when pipeline change state from PLAYING to PAUSED? > > > > That means that streamsynchroniser needs to make sure to send a GAP (or GAPs) > > > > that fill the entire time range until it is ready to either start a new stream > > > > or send EOS. > > > > > > > streamsynchronizer will seng GAP with duration is GST_CLOCK_TIME_NONE. Do > > > streamsychronizer need search all segment to get to biggest time range? Or > > > audio sink can handle GST_CLOCK_TIME_NONE. > > > > Duration GST_CLOCK_TIME_NONE is handled as duration = 0 in the sink, so it will > > effectively 'fill time' up to the start timestamp. > > > So streamsynchronizer should send GAP with duration is GST_CLOCK_TIME_NONE when > received EOS for shorter track? and send GAP event for the shorter track to > 'moves time forward' in other track chain function? And send all tracks EOS > event after received all tracks EOS event. Yes, if I've understood what you're trying to say correctly. > > > > It looks like these patches will achieve that, but sending a new GAP when > > > > changing state in stream synchroniser, but wouldn't it be better to wake the > > > > streams that are looping on EOS every time data passes on another pad that > > > > 'moves time forward' and have them send another GAP event then? > > > > > > > streamsynchronizer will send GAP event in chain function. But pad push will be > > > blocked when sink changed to PAUSED state and then can't send GAP event. So > > > need send GAP event when state change in streamsynchronizer. > > > > If pad_push() is blocked, you can't send a GAP event downstream there anyway - > > the STREAM_LOCK() will be held by the pad_push(). That case should work. The > > one that is broken is where you sent a GAP downstream from teh EOS event > > handler, advancing time to some time 'timestamp', and then time advanced past > > that point, and your pad_push_event (pad, gap_event) returned - you need to > > either push a new gap, or push a buffer then. > > If pad_push() is blocked, streamsynchronizer can't send GAP event for shorter > track in chain function. The shorter track is reach EOS, and no any buffer > reach streamsynchronizer, so basesink can't finished pre-roll for the shorter > track. The longer track is pro-rolled and blocked. So streamsynchronizer need > send GAP event for shorter track when state change, and send the GAP event in > event handle thread. There are 2 threads / streams in stream synchroniser: * Short stream, has reached EOS. It pushes a gap downstream using gst_pad_push_event(), which won't return until the end time (timestamp + duration) of the gap event. Once that returns, it is free to push another gap event - or stop pushing them if it's time to switch to another input. * The longer stream, buffers are still passing, and can be used to set timestamps on the gap events being sent in the other thread. Is that clearer?
Very clear. Thank you very much.
Review of attachment 295409 [details] [review]: Looks good, just some minor things that should be fast to update :) Then this should be ready to be merged ::: gst/playback/gststreamsynchronizer.c @@ +207,3 @@ +/* must be called with the STREAM_SYNCHRONIZER_LOCK */ +static gboolean +gst_stream_synchronizer_wait (GstStreamSynchronizer * self, GstPad * pad) Can this function also be used for the g_cond_wait() in the SEGMENT event handler? I think it should be used there too for consistency (and modified to work in both cases). @@ +445,3 @@ GST_STREAM_SYNCHRONIZER_LOCK (self); stream = gst_pad_get_element_private (pad); + stream->flushing = TRUE; See the "if (stream)" some lines below :) You need to check for NULL @@ +550,3 @@ + + /* send eos if haven't seed data as sink can't handle if haven't negotiate */ + if (!seen_data || self->eos) { The only case when we get here and this is not true, is when we're flushing? Document that and maybe even add an assertion :) @@ +904,3 @@ + if (stream->is_eos && !stream->eos_sent) { + self->send_gap_event = TRUE; + g_cond_broadcast (&stream->stream_finish_cond); Please document in a comment here why a GAP event should be sent, also why not another one is required when going from PAUSED to PLAYING
Review of attachment 295410 [details] [review]: ::: gst/playback/gststreamsynchronizer.c @@ +909,3 @@ if (stream->is_eos && !stream->eos_sent) { self->send_gap_event = TRUE; + stream->gap_duration = GST_CLOCK_TIME_NONE; This should probably also be reset in PAUSED->READY (where the segment is already reset currently).
Review of attachment 295411 [details] [review]: ::: tests/check/pipelines/streamsynchronizer.c @@ +37,3 @@ + + g_mutex_lock (&test_mutex); + if (gst_event_parse_group_id (event, &group_id)) { This should always succeed here, just add an assertion instead of the else case @@ +126,3 @@ + GST_STATE_CHANGE_FAILURE); + + g_usleep (1 * G_USEC_PER_SEC); Why the sleeps? @@ +131,3 @@ + + g_object_unref (bus); + g_object_unref (G_OBJECT (pipeline)); Use gst_object_unref() and gst_object_ref(), and the G_OBJECT() casts are not needed then @@ +144,3 @@ + run_streamsynchronizer_handle_eos ("videotestsrc name=videosrc ! " + "streamsynchronizer name=streamsync audiotestsrc name=audiosrc num-buffers=0 ! " + "streamsync. streamsync. ! fakesink sync=true streamsync. ! fakesink sync=true"); Make a separate testcase for each of these pipelines. That will make the test runner output more informative :) @@ +156,3 @@ + + suite_add_tcase (s, tc_chain); + tcase_add_test (tc_chain, test_streamsynchronizer_handle_eos); Maybe we should also add some tests for the normal behaviour of streamsynchronizer while we're at it. Testing that it passes through data correctly, and especially aligns stream starts when they're switched to a new group.
Thanks, I will updated based on the review comments and upload patch later. Do you think aligns stream starts when they're switched to a new grout has some link with: https://bugzilla.gnome.org/show_bug.cgi?id=743973
Maybe, but for normal streams that should currently work... and having tests for that would be good :)
(In reply to Sebastian Dröge (slomo) from comment #98) > Review of attachment 295409 [details] [review] [review]: > > Looks good, just some minor things that should be fast to update :) Then > this should be ready to be merged > > ::: gst/playback/gststreamsynchronizer.c > @@ +207,3 @@ > +/* must be called with the STREAM_SYNCHRONIZER_LOCK */ > +static gboolean > +gst_stream_synchronizer_wait (GstStreamSynchronizer * self, GstPad * pad) > > Can this function also be used for the g_cond_wait() in the SEGMENT event > handler? I think it should be used there too for consistency (and modified > to work in both cases). > Accept, will do it in another patch. > @@ +445,3 @@ > GST_STREAM_SYNCHRONIZER_LOCK (self); > stream = gst_pad_get_element_private (pad); > + stream->flushing = TRUE; > > See the "if (stream)" some lines below :) You need to check for NULL > Accept. > @@ +550,3 @@ > + > + /* send eos if haven't seed data as sink can't handle if haven't > negotiate */ > + if (!seen_data || self->eos) { > > The only case when we get here and this is not true, is when we're flushing? > Document that and maybe even add an assertion :) > Add more comments for seen_data. > @@ +904,3 @@ > + if (stream->is_eos && !stream->eos_sent) { > + self->send_gap_event = TRUE; > + g_cond_broadcast (&stream->stream_finish_cond); > > Please document in a comment here why a GAP event should be sent, also why > not another one is required when going from PAUSED to PLAYING Add comments on why send GAP event. PAUSED to PLAYING needn't pre-roll, needn't send GAP event in change_state (). chain () will send GAP event in PLAYING state.
(In reply to Sebastian Dröge (slomo) from comment #99) > Review of attachment 295410 [details] [review] [review]: > > ::: gst/playback/gststreamsynchronizer.c > @@ +909,3 @@ > if (stream->is_eos && !stream->eos_sent) { > self->send_gap_event = TRUE; > + stream->gap_duration = GST_CLOCK_TIME_NONE; > > This should probably also be reset in PAUSED->READY (where the segment is > already reset currently). Do you mean set value to stream->gap_duration? The stream->gap_duration only used when self->send_gap_event is TRUE. So need set value to stream->gap_duration when set TRUE to self->send_gap_event. Not accept currently:)
(In reply to Sebastian Dröge (slomo) from comment #100) > Review of attachment 295411 [details] [review] [review]: > > ::: tests/check/pipelines/streamsynchronizer.c > @@ +37,3 @@ > + > + g_mutex_lock (&test_mutex); > + if (gst_event_parse_group_id (event, &group_id)) { > > This should always succeed here, just add an assertion instead of the else > case > Accepted. > @@ +126,3 @@ > + GST_STATE_CHANGE_FAILURE); > + > + g_usleep (1 * G_USEC_PER_SEC); > > Why the sleeps? > Don't need it. Removed. > @@ +131,3 @@ > + > + g_object_unref (bus); > + g_object_unref (G_OBJECT (pipeline)); > > Use gst_object_unref() and gst_object_ref(), and the G_OBJECT() casts are > not needed then > Accepted. > @@ +144,3 @@ > + run_streamsynchronizer_handle_eos ("videotestsrc name=videosrc ! " > + "streamsynchronizer name=streamsync audiotestsrc name=audiosrc > num-buffers=0 ! " > + "streamsync. streamsync. ! fakesink sync=true streamsync. ! fakesink > sync=true"); > > Make a separate testcase for each of these pipelines. That will make the > test runner output more informative :) > Accepted. > @@ +156,3 @@ > + > + suite_add_tcase (s, tc_chain); > + tcase_add_test (tc_chain, test_streamsynchronizer_handle_eos); > > Maybe we should also add some tests for the normal behaviour of > streamsynchronizer while we're at it. Testing that it passes through data > correctly, and especially aligns stream starts when they're switched to a > new group. Maybe can add the test when fix segment start which from different source is different:)
Created attachment 296857 [details] [review] streamsynchronizer: Send GAP event to finish preroll when change state from PLAYING to PAUSED
Created attachment 296858 [details] [review] streamsynchronizer: Send GAP event in event process thread, not data thread
Created attachment 296859 [details] [review] streamsynchronizer: Unit test for streamsynchronizer handle eos
Created attachment 296860 [details] [review] streamsynchronizer: Optimize wait when handle SEGMENT event
I don't understand the group in streamsynchronizer. Don't know the scenarios which group used. Don't know when need wait in SEGMENT event. So not sure if the last patch is OK.
Review of attachment 296860 [details] [review]: ::: gst/playback/gststreamsynchronizer.c @@ -403,3 @@ - stream = gst_pad_get_element_private (pad); - if (stream) - stream->wait = FALSE; I think this code was backwards, and would be broken with your changes (and a bit broken before already). It probably should be something like while (stream->wait && !self->flushing) { g_cond_wait(...); stream = gst_pad_get_element_private (pad); } And the code that calls g_cond_broadcast() would set wait=FALSE before the broadcast. Can you fix that when moving the code? This waiting is used you gaplessly play multiple files one after another in playbin. It would cause streamsynchronizer to wait playing the next file until all streams of the previous one are finished.
(In reply to kevin from comment #104) > (In reply to Sebastian Dröge (slomo) from comment #99) > > Review of attachment 295410 [details] [review] [review] [review]: > > > > ::: gst/playback/gststreamsynchronizer.c > > @@ +909,3 @@ > > if (stream->is_eos && !stream->eos_sent) { > > self->send_gap_event = TRUE; > > + stream->gap_duration = GST_CLOCK_TIME_NONE; > > > > This should probably also be reset in PAUSED->READY (where the segment is > > already reset currently). > > Do you mean set value to stream->gap_duration? The stream->gap_duration only > used when self->send_gap_event is TRUE. So need set value to > stream->gap_duration when set TRUE to self->send_gap_event. > Not accept currently:) Yes, it has no real effect... but it would seem cleaner to me to reset that variable at that point too :)
(In reply to Sebastian Dröge (slomo) from comment #112) > (In reply to kevin from comment #104) > > (In reply to Sebastian Dröge (slomo) from comment #99) > > > Review of attachment 295410 [details] [review] [review] [review] [review]: > > > > > > ::: gst/playback/gststreamsynchronizer.c > > > @@ +909,3 @@ > > > if (stream->is_eos && !stream->eos_sent) { > > > self->send_gap_event = TRUE; > > > + stream->gap_duration = GST_CLOCK_TIME_NONE; > > > > > > This should probably also be reset in PAUSED->READY (where the segment is > > > already reset currently). > > > > Do you mean set value to stream->gap_duration? The stream->gap_duration only > > used when self->send_gap_event is TRUE. So need set value to > > stream->gap_duration when set TRUE to self->send_gap_event. > > Not accept currently:) > > Yes, it has no real effect... but it would seem cleaner to me to reset that > variable at that point too :) Ok.
(In reply to Sebastian Dröge (slomo) from comment #111) > Review of attachment 296860 [details] [review] [review]: > > ::: gst/playback/gststreamsynchronizer.c > @@ -403,3 @@ > - stream = gst_pad_get_element_private (pad); > - if (stream) > - stream->wait = FALSE; > > I think this code was backwards, and would be broken with your changes (and > a bit broken before already). > It probably should be something like > > while (stream->wait && !self->flushing) { > g_cond_wait(...); > stream = gst_pad_get_element_private (pad); > } > > And the code that calls g_cond_broadcast() would set wait=FALSE before the > broadcast. Can you fix that when moving the code? > > > This waiting is used you gaplessly play multiple files one after another in > playbin. It would cause streamsynchronizer to wait playing the next file > until all streams of the previous one are finished. Ok, Have fixed it. Will update patch after some test.
You can also use gst-play-1.0 with the --gapless and multiple files for testing this btw. If you do that, first test with a few audio-only files. And only then with audio/video files. audio/video files doesn't seem to work reliable currently.
Created attachment 296915 [details] [review] streamsynchronizer: Send GAP event to finish preroll when change state from PLAYING to PAUSED
Created attachment 296916 [details] [review] streamsynchronizer: Send GAP event in event process thread, not data thread
Created attachment 296917 [details] [review] streamsynchronizer: Unit test for streamsynchronizer handle eos
Created attachment 296918 [details] [review] streamsynchronizer: Optimize wait when handle SEGMENT event
I will go home now. And will have a holiday for Chinese New Year. So can't test gapless playback currently. And will go back to work on Feb. 26th:)
Thanks for the patches and your patience, I've merged them now and did some minor cleanup on the way. Enjoy your holidays :) commit fb9ca25f7f0ee850f73b2c323a2c3a8e7d2d92fc Author: Song Bing <b06498@freescale.com> Date: Sun Feb 15 13:51:36 2015 +0800 streamsynchronizer: Use the same waiting function for EOS and stream switches Also improve the waiting condition for stream switches, which was assuming before that the condition variable will only stop waiting once when it is signaled. But the documentation says that there might be spurious wakeups. https://bugzilla.gnome.org/show_bug.cgi?id=736655 commit eeea911c2e477cc51ed72ac5ce2da34caa4115e3 Author: Song Bing <b06498@freescale.com> Date: Mon Jan 26 11:14:13 2015 +0800 streamsynchronizer: Unit test for streamsynchronizer's EOS handling Test that a pipeline can change from PLAYING to PAUSED and back in the following scenarios: 1. One track reach EOS after pushed some buffers while another track still pushes buffers 2. One track reach EOS without buffers while another track still pushes buffers https://bugzilla.gnome.org/show_bug.cgi?id=736655 commit 2614f80309bf37e65b30f0ea76d2acefba1d57c4 Author: Song Bing <b06498@freescale.com> Date: Mon Jan 12 17:40:25 2015 +0800 streamsynchronizer: Send GAP events from the pads' streaming threads Change the GAP events that are currently sent from the chain function of the current pad to all other EOS pads. They should instead be sent from their own streaming threads. https://bugzilla.gnome.org/show_bug.cgi?id=736655 commit 9f81931716b7f8fd29862f0bfa1a3891cb414bdc Author: Song Bing <b06498@freescale.com> Date: Mon Jan 12 16:08:33 2015 +0800 streamsynchronizer: Send GAP event to finish preroll when change state from PLAYING to PAUSED Wait in the event function when EOS is received until all pads are EOS and then forward the EOS event from each pads own event function. Also send a new GAP event for EOS pads from the event function whenever going from PLAYING->PAUSED by shortly waking up the GCond. This is needed to allow sinks to pre-roll again, as they did not receive EOS yet because we blocked that, but also will never get data again. https://bugzilla.gnome.org/show_bug.cgi?id=736655
The streamsynchronizer unit test sometimes fails for me now. I don't know if it's related to these patches or not, but I don't remember noticing it before. tpm@xps:~/gst/master/gst-plugins-base/tests/check$ make pipelines/streamsynchronizer.forever ... pipelines/streamsynchronizer.c:108:E:general:test_streamsynchronizer_track_without_data:0: (after this point) Test timeout expired 66%: Checks: 3, Failures: 0, Errors: 1
I add the unit test to test the patches. I will check the issue.
Unfortunately, I can't reproduce the fail after run the unit test hundreds times. I use Gstreamer latest code on master branch. run the test on i.MX6Q board.
Try running it on a normal Linux machine with a bit more CPU power :) make pipelines/streamsynchronizer.forever
Created attachment 298061 [details] [review] patch to workaround unit test fail issue. The root cause of unit test fail is basesink sink->have_preroll haven't set to false as PLAYING->PAUSED is right after PAUSED->PLAYING. The wait thread hasn's been scheduled. Attach patch to workaround it. I have little test the patch as my Linux work station has some issue:(. And I have to attend one meeting right now. Post the patch and will test more later.
I have test the workaround. It works. So the issue should be basesink issue.
Created attachment 298066 [details] [review] Remove unnecessary ERROR message
Do we need fix the unit test fail issue or use the workaround? The issue only appear when state change very quickly.
Instead of sleep() it would be better to actually wait for the condition to happen. Is there a bug in basesink here that this is working around, or is this just the unit test being racy?
Created attachment 298095 [details] [review] patch to fix unit test fail.
Why still need add sleep: 1. audio source will generate one audio buffer with 23ms. sink can finish preroll even if streamsynchronizer don't send GAP event when PLAYING->PAUSED if no sleep. 2. audio source will generate on audio buffer with 23ms. The buffer will consumed after sleep 100ms. sink can't finish preroll if streamsynchronizer don't send GAP event when PLAYING->PAUSED.
Comment on attachment 298095 [details] [review] patch to fix unit test fail. Adding random sleeps doesn't seem right. How do you know it will be long enough under all circumstances, on all kinds of machines?
Still met unit test fail even if wait state change complete with function gst_element_get_state((). Per my analysis, it is basesink issue, I really don't know how to fix it. Can ask basesink owner help to check it? Thanks.
Can you provide a detailed analysis of the problem? What steps are happening where, and why is that wrong or how should it be instead?
Created attachment 298368 [details] [review] Don't need preroll if have prerolled when state change from PLAYING to PAUSED
Created attachment 298369 [details] [review] Should wait state change complete before start another state change.
streamsynchronizer unit no fail anymore:) But I still wonder how to wait pipeline consumed the only one audio buffer and then start to state change from PLAYING to PAUSED. I want to test below pipeline. run_streamsynchronizer_handle_eos ("videotestsrc name=videosrc ! " "streamsynchronizer name=streamsync audiotestsrc name=audiosrc num-buffers=1 ! " "streamsync. streamsync. ! fakesink sync=true streamsync. ! fakesink sync=true");
The usual way to wait for one buffer to pass is a blocking pad probe on the sink's sinkpad.
The audio buffer has duration about 23ms. I want wait fakesink sync=TRUE to consume it. My mean is pipeline playing time is large then 23 ms. How to wait that?
Can help to review attached patch? So I can close it if the patch is ok.
The patch makes sense to me but I would like a basesink unit test to see what goes wrong.
The basesink unit test is ok with my patch in my side. How to reproduce basesink unit test fail in you side? kevin@kevin-Lenovo-B470e:~/dev/gstreamer/tests/check$ make libs/basesink.forever Running suite(s): GstBaseSink 100%: Checks: 4, Failures: 0, Errors: 0 Running suite(s): GstBaseSink 100%: Checks: 4, Failures: 0, Errors: 0
Sorry, I misunderstand. Ignore comment 143.
yes, I would like to understand how it fails without the patch and a unit test would show that. Or is there something else that shows failing behaviour?
Application call state change PAUSED->PLAYING and then PLAYING->PAUSED very quickly. state_change () in basesink will be called very quickly. pre-roll wait function seems haven't be scheduled between two state change call. The reproduce rate is low. Don't know other use case can show the same fail.
and what happens if it fails? you don't get a message? things lock up?
Yes. will hang in pre-roll wait () as async is FALSE and need_preroll is TRUE. Attached log for your reference.
Created attachment 298587 [details] Log for basesink hang issue.
Sorry. From the log, basesink seems has right behavior. It will be blocked on gst_base_sink_wait_preroll () when PLAYING to PAUSED. Needn't any change in basesink for the issue. The fix for synchronizer unit test fail is unit test code. Shouldn't wait for async-done event when the state change isn't async change. As some time PLAYING to PAUSED isn't async change.
commit 4b174b14f51b4d405674c0d1054e82f5dbc82094 Author: Wim Taymans <wtaymans@redhat.com> Date: Wed Mar 11 16:36:29 2015 +0100 basesink: clean up the need_preroll variable Based on patch from Song Bing <b06498@freescale.com> Don't just set the need_preroll flag to TRUE in all cases. When we are already prerolled it needs to be set to FALSE and when we go to READY we should not touch it. We should only set it to TRUE in other cases, like what the code above does. See https://bugzilla.gnome.org/show_bug.cgi?id=736655
commit 1942870257747290c0d64a09e1a0d25bd296f8c6 Author: Song Bing <b06498@freescale.com> Date: Tue Mar 3 15:18:04 2015 +0800 streamsynchronizer: Should wait state change complete before start another state change Should wait state change complete before start another state change. Can't ensure can received async-done message when state change from PLAYING to PAUSED. https://bugzilla.gnome.org/show_bug.cgi?id=736655 commit 7ce97c723c0d439770022167a00a02abea7a8c8c Author: Song Bing <b06498@freescale.com> Date: Fri Feb 27 16:40:23 2015 +0800 streamsynchronizer: Remove unnecessary ERROR message. Remove unnecessary ERROR message. Push GAP will fail as flushing. Needn't ERROR message. https://bugzilla.gnome.org/show_bug.cgi?id=736655
(In reply to Wim Taymans from comment #151) > commit 4b174b14f51b4d405674c0d1054e82f5dbc82094 > Author: Wim Taymans <wtaymans@redhat.com> > Date: Wed Mar 11 16:36:29 2015 +0100 > > basesink: clean up the need_preroll variable > > Based on patch from Song Bing <b06498@freescale.com> > > Don't just set the need_preroll flag to TRUE in all cases. When we > are already prerolled it needs to be set to FALSE and when we go to > READY we should not touch it. We should only set it to TRUE in other > cases, like what the code above does. > > See https://bugzilla.gnome.org/show_bug.cgi?id=736655 The patch has issue. sink can't pause after state change from PLAYING to PAUSED. Will upload basesink log later. Reproduce step: 1. revert below commit. so unit test will hang on wait async-done event. and then unit test timeout. 2. check log of basesink. basesink hasn't been paused. commit 1942870257747290c0d64a09e1a0d25bd296f8c6 Author: Song Bing <b06498@freescale.com> Date: Tue Mar 3 15:18:04 2015 +0800 streamsynchronizer: Should wait state change complete before start another state change Should wait state change complete before start another state change. Can't ensure can received async-done message when state change from PLAYING to PAUSED. https://bugzilla.gnome.org/show_bug.cgi?id=736655
Created attachment 299209 [details] basesink log for sink has been paused when state change from PLAYING to PAUSED.
Sorry, should be: basesink log for sink hasn't been paused when state change from PLAYING to PAUSED.
Reproduce: make pipelines/streamsynchronizer.forever until unit test timeout.
I have a similar problem, if not the same (1.12.0). It seems like the problem is a race condition when acting on the buffering messages emitted by queue2 and prerolling, in my case an appsink. In my case it sometimes, but not always, gets stuck. It happens when I get a buffering message from queue2 that the buffer dropped to 0%, and I take the pipeline to PAUSED state. However, it seems that the streamsynchronizer's queue is blocked (in gst_base_sink_wait_preroll of the appsink) pushing out a buffer, and at the same time it can't accept any new buffer because it's deemed full. But because it can't accept any more buffers, the queue2 is blocked as well, and so it never gets a change to send another buffering message, which then would trigger a state change back to PLAYING, which would unblock the preroll wait.
+ Trace 237493
Created attachment 352589 [details] [review] queue2: Add buffer-full-time, buffer-full-buffers, buffer-full-size properties I spent a good amount of time investigating this issue. I have come to conclusion that buffering cannot be implemented as documented, without potentially running into this lockup issue on the state change to PAUSED. It does not appear to be a bug, more a deficiency in gstreamer's architectural design, or assumptions made that shouldn't be made. I'll explain why, later. Here's what an application (in this case an audio player) will have to do to work around this issue reliably: 1. Create playbin as usual, but do NOT use GST_PLAY_FLAG_BUFFERING. Note however, that playbin2 may still emit buffering messages (bug in gstplaysink.c). 2. Create a bin that contains a queue2 element (let's call it "bufferqueue"). This element then is followed by whatever you need, in my case an appsink. If playing back (e.g. using some audio sink) you will most likely want the "sync" property of your sink is set to TRUE. Be sure to set the "async" property to FALSE, we do NOT want the state change to PAUSED to be asynchronous (I'll explain later, why)! Assign this bin to the "audio-sink" property of the playbin element. 3. The application should maintain a "target state" that indicates whether the user wants PLAYING or PAUSED. This is important because the pipeline state may be at PAUSED due to buffering, but the buffering logic will need to bring it back to PLAYING once buffered up, but leave it at PAUSED if the user paused playback during buffering. Be sure to handle the case where buffering was disabled (See #6 and #7.b) 4. Create a "begin_buffering()" function that will set your queue2 ("bufferqueue") properties to the following: use-buffering=TRUE, max-size-buffers=0, max-size-bytes=0, max-size-time=0, buffer-full-time=<some non-zero value indicating how much decoded audio you want to buffer>. This effectively changes the "bufferqueue" to an unbounded queue that will accept as much data as it is handed, but will report a 100% fill level once the decoded audio reaches what you set for the buffer-full-time property (attached patch required). The queue2 element should be in this state ONLY when the pipeline is at PAUSED state *AND* it is at PAUSED state due to buffering. 5. Create a "done_buffering()" function that will set your queue2 ("bufferqueue") properties to the following: use-buffering=TRUE, max-size-buffers=0, max-size-bytes=0, max-size-time=<non-zero value indicating how much decoded audio this queue should, max>, buffer-full-time=0. This effectively changes the "bufferqueue" to a bounded queue that will not hold on to more decoded audio data than max-size-time indicates. The queue2 element should be in this state whenever the pipeline is at any state other than PAUSED, or when it is at PAUSED due to the user requested pausing playback ("target state" indicating PAUSED) and if applicable, buffering completed. 6. Create a "disable_buffering()" function that will set your queue2 ("bufferqueue") properties to the following: user-buffering=FALSE, max-size-buffers=0, max-size-bytes=0, max-size-time=<non-zero value indicating how much decoded audio this queue should, max>, buffer-full-time=0. This effectively changes the "bufferqueue" to a bounded queue that will not hold on to more decoded audio data than max-size-time indicates, and it will no longer emit buffering messages. 7. When handling buffering messages, the application needs to do the following: 7.a Check if GST_MESSAGE_SRC() of the buffering message is equal to the queue2 element you created ("bufferqueue"), ignore the message if it's coming from any other element 7.b You might want to check whether this is a live pipeline (buffer message indicates GST_BUFFERING_LIVE or the last pipeline state change to PAUSED returned GST_STATE_CHANGE_NO_PREROLL). You might want to disable the buffering logic in this case and call your "disable_buffering()" function. Be sure to record that fact so that your application will proceed to PLAYING (See #3) once at PAUSED, because you're never going to get any more buffering messages. 7.b If your application determines that buffering should begin (e.g. when 0% is reported and 100% has never been reached), call your "begin_buffering()" function and take the pipeline to PAUSED. 7.c If your application determines that buffering completed (e.g. when 100% is reported), call your "done_buffering()" function, and if the user's target state indicates PLAYING, then take the pipeline back to PLAYING. 8. When initially starting playback, call your "begin_buffering()" function, and then take the pipeline to PAUSED (so that you can check whether it's a live pipeline by checking for GST_STATE_CHANGE_NO_PREROLL). Do *NOT* take it directly to PLAYING. Once at paused, if the user's "target state" indicates PLAYING and buffering has not started or been disabled, proceed to PLAYING. Essentially, what you need is a queue2 element that is unbounded during buffering and bounded otherwise. Also, you want this queue2 at the decoding phase. Currently, playbin2, uridecodebin, etc have queues that emit buffering messages, but these messages don't really make sense. E.g. uridecodebin has a queue2 that reports buffering of the original source (e.g. souphttpsrc), in my case that means this queue constantly jumps from 0% to 100% (depending on source) because it may read a large chunk of data (in one buffer) that instantly fills the queue2 and then once it pushed that buffer out it is instantly drained, reporting 0%. The other problem with playbin2's buffering is that there is a queue in the playsink that reports buffering based on the decoded data. And then there's also a multiqueue that may emit buffering messages. And the application gets all of those, but none of these really make sense (at least not to me). And here's where the design flaw comes into play. Let's say you get a buffering message of 0%. In my case it was because souphttpsrc is just pushing a e.g. 1 MB sized buffer of undecoded audio and the 500kb buffer it pushed before had just drained the queue2 emitting that message). When the application takes the pipeline to PAUSED, any push to the sink element will block due to prerolling. However, this 1 MB buffer that souphttpsrc is pushing is now producing a whole bunch of audio data, so it flows into the decoder and essentially fills up all the queues maintained by playbin2/playsink. Now the first buffers are finally making it out of playbin2 and they hit the sink element, but because it's in PAUSED this push is now blocked. So it's sitting there, waiting to goto PLAYING, but because the entire chain (up to and including the source) is blocked (because all the queues are filled), no more buffering messages get emitted. The problem really is that all theses queues are bounded in size, so they're set to not exceed a certain amount of data, which is not really a problem unless you're going to PAUSED and hit the prerolling wait. Then there's that chance that the entire chain all the way to the source element is blocked and it can't get out of this state. The only solution to solve this situation reliably is a dedicated queue2 for buffering purposes, and it should probably be operating on the decoded data, which needs to be unbounded during buffering phases and bounded otherwise. This allows everything prior to that queue to at least complete the push during buffering, regardless of how much data this raw source ballooned into. Because a unbounded queue2 could never emit buffering messages with values that make sense, I had to enhance the logic to allow defining a fill level that ought to be considered 100%, despite the queue being able to accept more data. This unbounded queue2 now may receive a large amount of buffer, but it allows it to emit a 100% e.g. in the middle of pushing that large 1 MB buffer downstream, which then kicks everything back to PLAYING, unblocking the preroll wait in the sink element. Also, if "async" on the sink element is not set to FALSE then the state changed from PLAYING back to PAUSED (e.g. due to buffering) is not going to complete because it expects to be taken to PLAYING again. This probably could be handled by the application, but it probably gets complicated/weird. I hope this all makes sense. I believe the documentation of how buffering should be implemented is completely insufficient and will not implement a buffering strategy that will work reliably. Depending on source, timing, etc. it's almost guaranteed you'll run into this "deadlock" unless you implement buffering as outlined above. Also, for me, I can make sense of this design problem easiest by thinking about a source element pushing a large amount of encoded raw data that will get decoded into a large amount of buffers and/or data (in time), enough to fill up the entire chain, blocking the push all the way up to the source while in the prerolling wait on the sink element that is in PAUSED.
This is a more generic issue regarding buffering and playbin/decodebin. Those new issues should be supported (and if not, fixed) with playbin3 and decodebin3. GstBlub, can you double-check if your issue is still present with playbin3/decodebin3 ? If so, let's determine the key issues to be fixed.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/75.