GNOME Bugzilla – Bug 753196
audio/videoaggregator: Assumes that running time starts at seeking position after a seek (but it starts at 0 for flushing seeks)
Last modified: 2015-09-14 17:57:38 UTC
Basically, seek freeze for several seconds in presence of compositor or glvideomixer. This is a regression I believe. Reverting the patch the removes the clipping didn't help. To reproduce: time GST_GL_XINITTHREADS=1 DISPLAY=:0 GST_VALIDATE_SCENARIO=seek_forward gst-validate-1.0 compositor sink_1::alpha=0.5 sink_1::xpos=50 sink_1::ypos=50 name=_mixer ! deinterlace ! videoconvert ! xvimagesink videotestsrc pattern=snow timestamp-offset=3000000000 ! 'video/x-raw,format=AYUV,width=640,height=480,framerate=(fraction)30/1' ! timeoverlay ! _mixer. videotestsrc pattern=smpte ! 'video/x-raw,format=AYUV,width=800,height=600,framerate=(fraction)10/1' ! timeoverlay ! _mixer.
I tested with 1.4 and 1.5 and both had the same behavior, seeking hangs for some seconds.
I had a quick look at the issue and I can see that in GstVideoAggregator when seeking to 20secs (on that same pipeline), we end up dropping all buffers from 20s to 40s entering that condition: https://phabricator.freedesktop.org/diffusion/GSTBAD/browse/master/gst-libs/gst/video/gstvideoaggregator.c;6d1eda9391d3143b5fc633edfd94b89dbba209ab$1107 Getting the message: stvideoaggregator.c:1115:gst_videoaggregator_fill_queues:<_mixer:sink_1> replacing old buffer with a newer buffer, start 0:00:00.000000000 out end 0:00:20.033333333 I can see that after the seek, we output a segment with [start=20.0, duration=-1] and we get buffers with timestamp like: timestamp: 20.0 running time: 0 timestamp: 20.033 running time: 0.033 In gst_videoaggregator_fill_queues we have output_start_time=20.0 and output_end_time=20.0333333 and we expect the buffers running times to be inside those boundaries, which is why buffers are dropped until we get a buffer with timestamp=40.0, running_time=20.0 I do not completely understand the current algorithm and am not sure yet what is wrong exactly, maybe from those information you might have a better idea of where what we are doing is wrong.
Makes sense, looks like some segment handling confusion. We assume to output a buffer with the seek position as running time after a flushing seek, and assume to get buffers with that running time. But after a flushing seek, running times will start at 0 again. Needs some segment fixing, probably also in aggregator. Ideally by reusing things like gst_segment_do_seek() instead of manually modifying segments.
I have a fix, testing a bit more first
Created attachment 311131 [details] [review] aggregator: Document that get_next_time() should return running time
Created attachment 311132 [details] [review] videoaggregator: Fix mixup of running times and segment positions We have to queue buffers based on their running time, not based on the segment position. Also only update the segment position after we pushed a buffer, otherwise we're going to push down a segment event with the next position already.
Created attachment 311177 [details] [review] aggregator: Document that get_next_time() should return running time
Created attachment 311178 [details] [review] videoaggregator: Fix mixup of running times and segment positions We have to queue buffers based on their running time, not based on the segment position. Also return running time from GstAggregator::get_next_time() instead of a segment position, as required by the API. Also only update the segment position after we pushed a buffer, otherwise we're going to push down a segment event with the next position already.
Created attachment 311179 [details] [review] audioaggregator: Use stream time in the position query instead of segment position
Created attachment 311180 [details] [review] audioaggregator: Fix mixup of running times and segment positions We have to queue buffers based on their running time, not based on the segment position. Also return running time from GstAggregator::get_next_time() instead of a segment position, as required by the API. Also only update the segment position after we pushed a buffer, otherwise we're going to push down a segment event with the next position already.
gst-validate is happy with all these changes and unit tests too
Let's get the changes in and give them some wider testing?
(In reply to Sebastian Dröge (slomo) from comment #12) > Let's get the changes in and give them some wider testing? Ack. The patches look good to me.
Thanks! commit 637106e2873c3ee32eb168dde464dac9971c942b Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Sep 11 21:37:08 2015 +0200 audioaggregator: Fix mixup of running times and segment positions We have to queue buffers based on their running time, not based on the segment position. Also return running time from GstAggregator::get_next_time() instead of a segment position, as required by the API. Also only update the segment position after we pushed a buffer, otherwise we're going to push down a segment event with the next position already. https://bugzilla.gnome.org/show_bug.cgi?id=753196 commit 97fe89f3518596ea10a6cbe5b09848d0948683d0 Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Sep 11 16:56:40 2015 +0200 audioaggregator: Use stream time in the position query instead of segment position https://bugzilla.gnome.org/show_bug.cgi?id=753196 commit eb97bb0adbd24f06c6bb32155b27bce995cd9a2d Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Sep 11 12:22:51 2015 +0200 videoaggregator: Fix mixup of running times and segment positions We have to queue buffers based on their running time, not based on the segment position. Also return running time from GstAggregator::get_next_time() instead of a segment position, as required by the API. Also only update the segment position after we pushed a buffer, otherwise we're going to push down a segment event with the next position already. https://bugzilla.gnome.org/show_bug.cgi?id=753196 commit 336ca3207a2ba591107ceba8921ed73957ddd235 Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Sep 11 12:21:50 2015 +0200 aggregator: Document that get_next_time() should return running time https://bugzilla.gnome.org/show_bug.cgi?id=753196