GNOME Bugzilla – Bug 777780
GstBaseParse incorrectly handles non-flush seek
Last modified: 2017-02-08 11:15:55 UTC
With a simple hello world app that basically just creates a playbin and sets its uri, it is possible to highlight a problem that prevents to properly loop over playbacks. When doing a gst_element_seek(flag=~flush, position=0) upon GST_MESSAGE_SEGMENT_DONE it waits the duration of the stream before looping. This is because segment.base is incremented by twice the duration of the stream. I encountered the problem when playing audio files like file.{aac, flac, mp3, ac3} where there is no proper demuxer involved. More generally the problem happens when this is GstBaseParse that handles the seek. The segment.base is first incremented here https://cgit.freedesktop.org/gstreamer/gstreamer/tree/libs/gst/base/gstbaseparse.c#n4551 and then here https://cgit.freedesktop.org/gstreamer/gstreamer/tree/libs/gst/base/gstbaseparse.c#n4649 (the memcpy just below actually writes it) In short it is like gst_segment_to_running_time is called recursively twice (output transfered to input). So I am wondering if the line 4649 pointed above should just be removed. Any comment ? Thx!
With git blame it is possible to find the commit bdbc0693 that changes gst_element_set_seek to gst_element_do_seek and also introduces the "base" field of GstSegement. But the memcpy was present before this change. This code is there since 2011 so I have the feeling the code is correct but the bug could have been hidden all this time. Because most of the time a real demuxer is used and also a flush seek allows to workaround that in the sense it is acceptable to do a flush in most of the situation. But in some cases it is required to do a non-flush seek.
Strange that gst-plugins-good/tests/icles/test-segment-seeks foo.flac works at all then.
Yes I'm not sure why 4649 is there, it shouldn't be needed. Maybe it was at some point during 0.11 though. Does your testcase work (and also Tim's) after removing that line? If so, go ahead, ideally with a test.
(In reply to Tim-Philipp Müller from comment #2) > Strange that gst-plugins-good/tests/icles/test-segment-seeks foo.flac works > at all then. Thx for pointing that test. I just tried it and it does not work as expected. You can hear a cut at each seek. With the following attached patch the sound is continuous.
Created attachment 344323 [details] [review] baseparse: correctly handle non-flush seek
Created attachment 344324 [details] Minimal test that does not loop playback with big gap. gcc -Wall -O0 main.c -o main $(pkg-config --cflags --libs gstreamer-1.0) ./main file.mp3 Without attached patch: At each end the pipeline waits for the duration of the stream doing nothing (just a clock_wait) With patch: it plays and restart continuously. Should I put this test in gstreamer/tests/examples ? Or do you want something which is run upon make check ?
Comment on attachment 344323 [details] [review] baseparse: correctly handle non-flush seek Makes sense to me
(In reply to Julien Isorce from comment #6) > Should I put this test in gstreamer/tests/examples ? Or do you want > something which is run upon make check ? If you can make it an automated test that would be great, but that doesn't seem trivial here
> Thx for pointing that test. I just tried it and it does not work as > expected. You can hear a cut at each seek. With the following attached patch > the sound is continuous. Ah, that's great. I never investigated what caused the crackle, I just assumed it was related to the audiosink resetting things on the DISCONT flag or such :)
Comment on attachment 344323 [details] [review] baseparse: correctly handle non-flush seek commit b2c05cac8e50204d9ded4a858618c8b8b5b00910 Author: Julien Isorce <jisorce@oblong.com> Date: Thu Jan 26 16:51:21 2017 +0000 baseparse: correctly handle non-flush seek Otherwise when seeking/looping to the start when reaching the end, the sink waits for the duration of the stream. So the user hears nothing for the duration of the stream before it actually loop again. See example attached to the bug for that. Existing test: gst-plugins-good/tests/icles/test-segment-seeks foo.flac Without the patch the user hears a crack/cut at each seek. https://bugzilla.gnome.org/show_bug.cgi?id=777780
Thx you both for the review and comments. I realized I cannot integrate my example in the patch since the example should go to -good, at least not to -core. So I updated commit message and I pushed it. Because the new example will go in a separate patch anyway and the exiting example is good enough to reproduce that issue. I keep the bug open till I push the new example and will think about an automated test.
Created attachment 344435 [details] WIP: non-flush seek unit test for gstbaseparse Added automated unit test tests/check/pipelines/seek.c::test_loopback (similar as basesrc ! baseparse ! fakesink sync=1). On going work, will try to finish next week.
Created attachment 344643 [details] [review] tests: add 2 unit tests for non-flush seek with gstbaseparse Please review this patch that contains 2 unit tests. Both would have fail without the fix above. The bug could manifest itself differently because it messed-up the "segment.base". In one case it could make the pipeline to wait (for sync) and on an other case it could make the pipeline to play out of sync. These 2 new unit tests verify these 2 cases.
Should I just merge the test then ?
Does it work reliably? If so, go ahead :)
(In reply to Sebastian Dröge (slomo) from comment #15) > Does it work reliably? If so, go ahead :) I designed it to be reliable and it looks to work reliably. I have not seen anything wrong for all the run I did. So I will push it and if anyone see any flakiness let me know (but it should not be flaky at all in principle :) )
Comment on attachment 344643 [details] [review] tests: add 2 unit tests for non-flush seek with gstbaseparse commit b66109abf7d4968164af30253b94582e1e835dff Author: Julien Isorce <jisorce@oblong.com> Date: Tue Jan 31 09:55:59 2017 +0000 tests: add 2 unit tests for non-flush seek with gstbaseparse The unit test defines a test parse element that inherit from GstBaseParse. The test pipeline is: fakesrc ! testparse ! fakesink sync=1 Before the fix b2c05cac8 the first new test would have fail because the pipeline would have wait doing nothing just after proceeded the seek event. The second new test would have fail because the pipeline would have played the media instantly just after proceeded the seek event (like if sync was FALSE on the sink). https://bugzilla.gnome.org/show_bug.cgi?id=777780
All done, closing. As a follow-up I think we should create a new debug element "testseek" to send a customizable seek event upon a selectable event/action. It would superseed existing navseek element.