GNOME Bugzilla – Bug 724757
basesrc: Ignores seek made when the last buffer is pushed
Last modified: 2014-02-24 13:59:17 UTC
basesrc might ignore seeks made during the last buffer pushed because it has already decided to go eos and ignores the seek that was received. The attached patch fixes it and has a test case to reproduce the issue.
Created attachment 269742 [details] [review] basesrc: Do not send eos when seeking after last buffer If pushing the last buffer triggers a seek from downstream, do not go into EOS if a new segment was requested. Contains unit test
Comment on attachment 269742 [details] [review] basesrc: Do not send eos when seeking after last buffer I'm surprised that the seeking from the streaming thread in the test works at all and does not deadlock ;) Otherwise the change itself makes sense.
(In reply to comment #2) > (From update of attachment 269742 [details] [review]) > I'm surprised that the seeking from the streaming thread in the test works at > all and does not deadlock ;) > > Otherwise the change itself makes sense. About locking on seeks/segment in basesrc: 1) I was checking locking in basesrc and it seems that there isn't much thread safety around the segment for concurrent operations on the seeking and _loop functions. With this patch I believe the only portion that needs locking is around: switch (src->segment.format) { case GST_FORMAT_BYTES: { guint bufsize = gst_buffer_get_size (buf); /* we subtracted above for negative rates */ if (src->segment.rate >= 0.0) position += bufsize; break; } If the segment is changed we might advance the position on the new segment and that can compromise the stream by EOS'ing early or position query. 2) Good question about doing seeking on the same thread, I wrote the patch based on some experiments I was conducting with pushfilesrc ! oggdemux, but I'm not sure why it works as the seeking function seems to need the pad stream lock. Will try to understand this properly before pushing :)
The pad stream lock is recursive(In reply to comment #3) > (In reply to comment #2) > > 2) Good question about doing seeking on the same thread, I wrote the patch > based on some experiments I was conducting with pushfilesrc ! oggdemux, but I'm > not sure why it works as the seeking function seems to need the pad stream > lock. Will try to understand this properly before pushing :) The pad stream lock is recursive and the probes are called without the pad object lock.
Are right, and there's no queue in between here so you definitely call from the same thread and the recursive mutex can recurse ;) Tricky... I think you should add a big comment about this in the test to not lead people do the same in other code unless they know what they're doing :) For the locking problem, can you handle that in a separate patch? Also talk to Wim about that, he might have an opinion on how this should be fixed properly.
Double checked the threads and it seems that basesrc is correct, it shouldn't be possible to have a race when updating the position from the seek and loop functions. commit 2c2e55789d212e1e5a236930404af7902c475f2c Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Wed Feb 19 21:17:27 2014 -0300 basesrc: Do not send eos when seeking after last buffer If pushing the last buffer triggers a seek from downstream, do not go into EOS if a new segment was requested. Contains unit test https://bugzilla.gnome.org/show_bug.cgi?id=724757
Comment on attachment 269742 [details] [review] basesrc: Do not send eos when seeking after last buffer Committed with an added comment at the test.