GNOME Bugzilla – Bug 657076
audioringbuffer: may fail to provide running clock
Last modified: 2016-10-31 14:18:23 UTC
Created attachment 194363 [details] [review] ringbuffer: start ringbuffer if needed upon commit Consider some clip with very little audio (below a typical ringbuffer size). In this case, while the ringbuffer is allowed to start upon changing to PLAYING, it will never really start (since no new segment is needed, so no wait_segment is called, which otherwise starts ringbuffer). So, no advancing ringbuffer and no advancing clock, and then nothing much happens really (no video rendering either etc). Attached patch arranges to start ringbuffer upon commit. While it may be marginally less optimal (? in some cases ?) it makes sense to get going when there is something to do (i.e. data) and really arranges for a running clock which is often claimed to be provided. Note this is a sort-of counterpart to a recent fix that had to arrange for providing a running clock when going to PLAYING (after PAUSED) after EOS. Both seem to stem from no longer starting the ringbuffer upon PLAYING, but only allowing it to start, and it leads to wondering if that has provided more benefit (which ?) than issues ...
Created attachment 194364 [details] [review] pulsesink: uncork if needed upon commit And at least we are consistent, because something similar occurs in its own way in pulsesink ringbuffer handling ;)
Is this still relevant? It seems so, so we should do something about it. But shouldn't we just start the ringbuffer on EOS in that case?
There are 2 stages/notions of starting here; starting the ringbuffer on the one hand, and then really starting (uncorking) pulseaudio. Note that pulsesink does not really uncork when starting the ringbuffer. The ringbuffer is started on EOS, but that need not help as such (in view of the above). However, there are some other steps in place to arrange for an uncork when EOS or GAP is received. So, in overall, this is not a problem as long as upstream sends EOS or GAP. However, upstream (demuxer or whatever) may not be in a position (of knowing) to send EOS (or GAP), e.g. in some (live) streaming situation. And AFAIK, it is not spec'ed (is it?) to always send (spam?) GAP in absence of data (GAP being more a voluntary convenience in case of some sparse streams). As such, if we are sure to send GAP in lots of (upstream) places, this is not a problem. But it seems more robust (and sort of user-friendly) to handle this downstream in pulsesink, and really start/uncork when one would expect it (at the very l(e)ast when receiving data), rather than to over-optimize with potential drawbacks.
It's not strictly required for upstream to send GAP events, but it should send EOS when it's done (which it might only know after e.g. the video track is finished, so much later). I think something has to be fixed here then :) Mark, are your patches still valid as is or do they need changes?
Created attachment 294121 [details] [review] audioringbuffer: start ringbuffer if needed upon commit Patch as before, updated.
Created attachment 294122 [details] [review] audioringbuffer: start ringbuffer if needed upon commit Sigh; now really the updated version ...
Created attachment 294123 [details] [review] pulsesink: uncork if needed upon commit ... and also updated this one for good measure.
Committed [oops, with missing Fixes: annotations] in -good: commit 0dd46accf6d282ff07065852bd91c85c78af3394 Author: Mark Nauwelaerts <mnauw@users.sourceforge.net> Date: Thu Jan 8 21:07:05 2015 +0100 pulsesink: uncork if needed upon commit ... to provide for a running clock. in -base: commit 13ee94ef1091f8a8a90dbd395b39876c26c5188e Author: Mark Nauwelaerts <mnauw@users.sourceforge.net> Date: Thu Jan 8 21:20:14 2015 +0100 audioringbuffer: start ringbuffer if needed upon commit ... to provide for a running clock.
This patch starts the ringbuffer as soon as the first segment is committed by the writer/decode thread, and then the reader starts trying to read from the ringbuffer and feed things out to the audio device, but there's only 1 committed segment - so now we're in a race against the writer putting more segments in place before the reader tries to write them out. The frequent result is that the reader thread outputs a bit of audio, then glitches and has to produce silence for a while because the writer fell behind, then things start flowing out the audio device in realtime, and the writer gets a chance to catch up. The previous behaviour was to accumulate a ringbuffer's worth of audio before starting the ringbuffer and writing things out, giving the writer a (default) 200ms of grace before there'd be any glitch.
The reason noone ever complained about glitches before is because they don't happen with pulsesink where we map the pulseaudio ringbuffer directly and write into it. They only happen with all the other audio sinks that rely on the default ringbuffer. With pulseaudio, it'll start the ringbuffer, write 1 segment and then pulseaudio will start playing that segment. As long as the next segment is written within 10ms, there'll be no glitch - unlike the default ringbuffer where for the first $ringbuffersize milliseconds, segments will be read as quickly as the audio device accepts them.
Should we revert the changes then? Is there anything we can do about the original problem?
I think we should, yes.
+1
Reverted those 2 commits: commit 802eae296a95334412ce4a6ab26d36fba52f99cc Author: Jan Schmidt <jan@centricular.com> Date: Sat Apr 16 02:11:59 2016 +1000 Revert "audioringbuffer: start ringbuffer if needed upon commit" This reverts commit 13ee94ef1091f8a8a90dbd395b39876c26c5188e. Causes audio glitches at startup by starting to output segments from the ringbuffer before it has been filled / fully prerolled. https://bugzilla.gnome.org/show_bug.cgi?id=657076 commit 46a3c9ac8b2f8182ef8f211f4be09a67e4d42a61 Author: Jan Schmidt <jan@centricular.com> Date: Sat Apr 16 02:17:26 2016 +1000 Revert "pulsesink: uncork if needed upon commit" This reverts commit 0dd46accf6d282ff07065852bd91c85c78af3394. With some audiosinks, starting the ringbuffer on the first commit causes audio glitches at startup by starting to output segments from the ringbuffer before it has been filled / fully prerolled. This doesn't usually happen with pulsesink because we map the pulseaudio ringbuffer directly, but we should keep things consistent with other sinks with regards to startup latency, plus it gives more headway to avoid glitching, should the initial 2nd segment take more than 10ms to generate. https://bugzilla.gnome.org/show_bug.cgi?id=657076 Not sure if I should now just close this bug?
> Not sure if I should now just close this bug? I suppose so. Question is, do we understand the scenario this was supposed to fix originally properly? Are we sure it's fixed or should be fixed differently? Do we have a way of reproducing that? Or do we need more info from Mark?
We need info from Mark. I think his original problem has been invalidated by starting the ringbuffer on EOS and GAP events.
So, we are now again in the situation of Comment #3, that is, EOS and GAP events will indeed handle the situation assuming that upstream appropriately sends those, which is not definitely but probably (mostly) the case (mileage of interpretation may vary). As such, it is (equally) probably ok to close ...
I think we'll have to take an actual-definitely embarassingly-broken for years bug over a maybe-probably theoretical bug until proven otherwise.