After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 657076 - audioringbuffer: may fail to provide running clock
audioringbuffer: may fail to provide running clock
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other Linux
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-08-22 13:40 UTC by Mark Nauwelaerts
Modified: 2016-10-31 14:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ringbuffer: start ringbuffer if needed upon commit (1.27 KB, patch)
2011-08-22 13:40 UTC, Mark Nauwelaerts
none Details | Review
pulsesink: uncork if needed upon commit (870 bytes, patch)
2011-08-22 13:45 UTC, Mark Nauwelaerts
none Details | Review
audioringbuffer: start ringbuffer if needed upon commit (1.26 KB, patch)
2015-01-08 20:18 UTC, Mark Nauwelaerts
none Details | Review
audioringbuffer: start ringbuffer if needed upon commit (1.26 KB, patch)
2015-01-08 20:23 UTC, Mark Nauwelaerts
committed Details | Review
pulsesink: uncork if needed upon commit (847 bytes, patch)
2015-01-08 20:33 UTC, Mark Nauwelaerts
committed Details | Review

Description Mark Nauwelaerts 2011-08-22 13:40:45 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 ...
Comment 1 Mark Nauwelaerts 2011-08-22 13:45:21 UTC
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 ;)
Comment 2 Sebastian Dröge (slomo) 2014-12-22 09:59:12 UTC
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?
Comment 3 Mark Nauwelaerts 2014-12-24 11:17:32 UTC
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.
Comment 4 Sebastian Dröge (slomo) 2015-01-08 12:28:50 UTC
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?
Comment 5 Mark Nauwelaerts 2015-01-08 20:18:09 UTC
Created attachment 294121 [details] [review]
audioringbuffer: start ringbuffer if needed upon commit

Patch as before, updated.
Comment 6 Mark Nauwelaerts 2015-01-08 20:23:26 UTC
Created attachment 294122 [details] [review]
audioringbuffer: start ringbuffer if needed upon commit

Sigh; now really the updated version ...
Comment 7 Mark Nauwelaerts 2015-01-08 20:33:51 UTC
Created attachment 294123 [details] [review]
pulsesink: uncork if needed upon commit

... and also updated this one for good measure.
Comment 8 Mark Nauwelaerts 2015-01-10 12:32:55 UTC
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.
Comment 9 Jan Schmidt 2016-04-08 16:21:08 UTC
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.
Comment 10 Jan Schmidt 2016-04-08 17:46:16 UTC
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.
Comment 11 Sebastian Dröge (slomo) 2016-04-11 06:17:39 UTC
Should we revert the changes then? Is there anything we can do about the original problem?
Comment 12 Jan Schmidt 2016-04-11 07:11:12 UTC
I think we should, yes.
Comment 13 Tim-Philipp Müller 2016-04-13 15:18:24 UTC
+1
Comment 14 Jan Schmidt 2016-04-15 16:24:56 UTC
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?
Comment 15 Tim-Philipp Müller 2016-04-15 16:32:55 UTC
> 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?
Comment 16 Jan Schmidt 2016-04-15 16:41:56 UTC
We need info from Mark. I think his original problem has been invalidated by starting the ringbuffer on EOS and GAP events.
Comment 17 Mark Nauwelaerts 2016-04-15 18:31:56 UTC
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 ...
Comment 18 Jan Schmidt 2016-04-15 21:56:26 UTC
I think we'll have to take an actual-definitely embarassingly-broken for years bug over a maybe-probably theoretical bug until proven otherwise.