GNOME Bugzilla – Bug 777073
streamsynchronizer: Can't complete preroll in case pause/seek is called after one of streams receive EOS
Last modified: 2018-11-03 11:53:32 UTC
Created attachment 343215 [details] Issue file (a/v duration is different) pipeline can not complete preroll when pause/seek is called in situations where one of the streams receives EOS and the other does not. The phenomenon occurs when playing multimedia files made of audio and video of different lengths. The root cause of the problem occurs because the segment position is set to out of segment. 1. Seek case - gststreamsynchronizer sink event handler update segment.stop to segment.position in normal eos case as below if (seen_data && stream->segment.position != -1) timestamp = stream->segment.position; else if (stream->segment.rate < 0.0 || stream->segment.stop == -1) timestamp = stream->segment.start; else timestamp = stream->segment.stop; stream->segment.position = timestamp; - gst_segment_clip function treats the start position of gap_evet as out of segment if it is greater than or equal to segment.stop. - gap_event is considered not_syncable then preroll can not be finished. 2. Pause case - gststreamersynchronzer sink_chain function update the position of eos stream using position of non eos stream. Therefore segment position of eos stream can be bigger than own segment stop value. /* Is there a 1 second lag? */ if (position != -1 && GST_CLOCK_TIME_IS_VALID (timestamp_end) && position + GST_SECOND < timestamp_end) { gint64 new_start; new_start = timestamp_end - GST_SECOND; GST_DEBUG_OBJECT (ostream->sinkpad, "Advancing stream %u from %" GST_TIME_FORMAT " to %" GST_TIME_FORMAT, ostream->stream_number, GST_TIME_ARGS (position), GST_TIME_ARGS (new_start)); ostream->segment.position = new_start; - gst_segment_clip function treats the start position of gap_evet as out of segment if it is greater than or equal to segment.stop. - gap_event is considered not_syncable then preroll can not be finished. I suggest below solution. 1. Seek case : change segment boundary. It would be aligned with comment, too. if (G_UNLIKELY (segment->stop != -1 && start != -1 && start >= segment->stop)) -> if (G_UNLIKELY (segment->stop != -1 && start != -1 && start > segment->stop)) 2. Pause case : add limitation of segment position when it is updated in sink_chain function of gststreamsynchronizer new_start = timestamp_end - GST_SECOND; + /* if new_start is bigger than segment.stop, it is out of segment. */ + if (new_start > ostream->segment.stop) + new_start = ostream->segment.stop; + It can be fixed another way, but I am trying to change as small as it can be. If there is better solution, please fix it or give me the advice about it.
Created attachment 343216 [details] [review] patch for seek I am attaching a patch for seek case.
Created attachment 343217 [details] [review] patch for pause I am attaching a patch for pause case. It is gst-plugins-base patch. If It is needed to separate from this one, please let me know.
I'm sure there's an existing bug for this btw (with lots of discussion)
(In reply to Tim-Philipp Müller from comment #3) > I'm sure there's an existing bug for this btw (with lots of discussion) Yes. This bug is obvious and easy to reproduce with the issue file. I made a file with ffmpeg to reproduce this issue because it is caused in our webOS TV set. This is not common, but this kind of situation can be happened, sometimes. Therefore I think it should be fixed with any way.
Aren't we going to fix it? Could you let me to know what can I do for fix it? For example, would you like me to approach another way or check another bug (test cases)?
Patience :) Yes, we should fix it of course. I was thinking of bug #633700 btw.
Comment on attachment 343217 [details] [review] patch for pause Thanks for the patch! Makes sense, independent of everything else already. Did you check if there are other instances of this same problem in streamsynchronizer? Please do so otherwise. Also please fix up the commit message. It should be in the following format: elementname: short description [empty line] longer description over multiple lines if needed [empty line] link to this bugzilla ticket There's also a typo: syncronizer should be synchronizer
Review of attachment 343216 [details] [review]: This would have to be solved differently somehow ::: gst/gstsegment.c @@ +889,1 @@ return FALSE; This is wrong: if the start of a buffer is == segment.stop, then the whole buffer is outside the segment.
Thanks for your comments. :) I understand that the range of the segment is start <= range < stop. I was thinking if start <= range <= stop was also reasonable for the segment range, but if it is wrong, I will remove the patch. And I checked a bit more about other file formats (avi / mkv), and the problem only occurred with mp4 (qtdemux). There seem to be two bugs in this issue. The first is that qecemux create a segment with segment.start == segment.stop == segment.position == segment.duration in the problem case but the others set segment.stop to GST_CLOCK_TIME_NONE or total duration. The second bug is that streamsynchronizer ignore segment when if segment.position is bigger or equal than segment.stop, and then it cause preroll can not be finished. It occurs because streamsynchronizer update segment.position of stream that gets eos already using stream of other stream position. I will update streamsynchronizer patch again after check your comment. (In reply to Sebastian Dröge (slomo) from comment #8) > Review of attachment 343216 [details] [review] [review]: > > This would have to be solved differently somehow > > ::: gst/gstsegment.c > @@ +889,1 @@ > return FALSE; > > This is wrong: if the start of a buffer is == segment.stop, then the whole > buffer is outside the segment.
Created attachment 344286 [details] [review] Fix to complete preroll after one of stream receives EOS I am uploading new patch without change segment range. I removed 1 second lag because I can not find why it is needed. If there is any reason to add 1 second lag, let me know. Also, I have checked that if there are other instances of this same problem in streamsynchronizer, but I can not find any more.
Review of attachment 344286 [details] [review]: There are various problems in the patch, but I also don't really understand how it solves the problem and what exactly the problem is. The problem is that for EOS streams we would have to send a GAP event (just like for lagging streams), so that downstream advances/prerolls? And we would set the GAP event timestamp to >= segment.stop, causing it to be just dropped downstream? I think that a buffer that has timestamp=segment.stop && duration=0 is considered inside the segment (can you confirm in gstsegment.c?), in which case the same should also apply to GAP events (might need fixing) and we should just send such a GAP event then. ::: gst/playback/gststreamsynchronizer.c @@ +573,3 @@ srcpad = gst_object_ref (stream->srcpad); + if (!seen_data || !GST_CLOCK_TIME_IS_VALID (stream->segment.position)) { You inverse this condition but the code below stays more or less the same. Why is this correct? @@ +585,3 @@ + if (GST_CLOCK_TIME_IS_VALID (stream->segment.stop) && + stream->segment.position >= stream->segment.stop) + stream->segment.position = stream->segment.stop - G_USEC_PER_SEC; Why G_USEC_PER_SEC? You probably want GST_SECOND but also why that? And if you want to subtract something, you need to ensure that you don't go below zero (i.e. underflow) @@ -738,3 @@ - if (!GST_CLOCK_TIME_IS_VALID (timestamp_end) && - GST_CLOCK_TIME_IS_VALID (timestamp)) { - timestamp_end = timestamp + GST_SECOND; Check "git blame" why this was added @@ -756,3 @@ - /* Is there a 1 second lag? */ - if (position != -1 && GST_CLOCK_TIME_IS_VALID (timestamp_end) && - position + GST_SECOND < timestamp_end) { You're completely removing the advancing of lagging streams here
Review of attachment 344286 [details] [review]: I added explanation about what you have questioned. Some could be wrong again, but I hope you kindly review it again. The buffer that has timestamp=segment.stop && duration=0 is considered outside the segment. That is why I made first patch for it (https://bugzilla.gnome.org/review?bug=777073&attachment=343216). You can reproduce it when you try seek or pause during playing the issue file after the position of 8.4 seconds. ::: gst/playback/gststreamsynchronizer.c @@ +573,3 @@ srcpad = gst_object_ref (stream->srcpad); + if (!seen_data || !GST_CLOCK_TIME_IS_VALID (stream->segment.position)) { In original code, when if stream->segment.position is valid and seen_data == true, timestamp is replaced with stream->segment.position and stream->segment.position is replaced timestamp again in line 583. The goal of this if condition is for updating stream->segment.position when if seen_data is false or stream->segment.position is not valid. Therefore, I fixed it for working only when if stream->segment.position need to be updated, and removed unnecessary copy and variable. If you implemented it on purpose to look simple, I will revert it. @@ +585,3 @@ + if (GST_CLOCK_TIME_IS_VALID (stream->segment.stop) && + stream->segment.position >= stream->segment.stop) + stream->segment.position = stream->segment.stop - G_USEC_PER_SEC; G_USEC_PER_SEC is substracted for updating stream->segment.position to smaller than stream->segment.stop, because of https://bug777073.bugzilla-attachments.gnome.org/attachment.cgi?id=343216. You rejected that patch because the segment valid range is smaller than segment.stop. If segment.position is same as segment.stop, it will be skipped because gst_segment_clip return FALSE. Therefore preroll(do_sync) can not be finished after one of stream receives EOS. GST_MSECOND is better than G_USEC_PER_SEC and Yes, it is needed checking the below zero case but I missed it. @@ -738,3 @@ - if (!GST_CLOCK_TIME_IS_VALID (timestamp_end) && - GST_CLOCK_TIME_IS_VALID (timestamp)) { - timestamp_end = timestamp + GST_SECOND; stream->segment.position is updated with timestamp_end when playback rate > 0 and updated with timestamp when playback rate <= 0, only when if those value is not -1. Therefore, I thought stream->segment.position is accurate than timestamp_end. Therefore, I think Advancing EOS can be done with stream->segment.position instead of timestamp_end and it is not needed to be checked again here. @@ -756,3 @@ - /* Is there a 1 second lag? */ - if (position != -1 && GST_CLOCK_TIME_IS_VALID (timestamp_end) && - position + GST_SECOND < timestamp_end) { Is this for sending GAP event only when if EOSed stream is 1 second behind non EOS stream? If so, when if the playback rate < 0 and the non EOS stream is one second ahead of the EOS stream? Could you let me know why 1 second lag is necessary? I dig git history and couldn't find why it is added.
-- 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/gst-plugins-base/issues/325.