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 650724 - baseaudiosink: delay the resyncing of timestamp vs ringbuffertime
baseaudiosink: delay the resyncing of timestamp vs ringbuffertime
Status: RESOLVED DUPLICATE of bug 640859
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other All
: Normal major
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-05-21 10:15 UTC by Håvard Graff (hgr)
Modified: 2014-12-23 11:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (15.28 KB, patch)
2011-05-21 10:15 UTC, Håvard Graff (hgr)
none Details | Review
patch (15.51 KB, patch)
2011-05-21 11:01 UTC, Håvard Graff (hgr)
none Details | Review
baseaudiosink: trivial comment fixes (1.79 KB, patch)
2011-05-21 13:29 UTC, Felipe Contreras (banned)
none Details | Review
baseaudiosink: split drift-tolerance into alignment-threshold (7.35 KB, patch)
2011-05-21 13:29 UTC, Felipe Contreras (banned)
none Details | Review
baseaudiosink: decrease alignment-threshold to 20ms (1006 bytes, patch)
2011-05-21 13:29 UTC, Felipe Contreras (banned)
none Details | Review
baseaudiosink: use gst_util_uint64_scale_int when appropriate (1.06 KB, patch)
2011-05-21 13:30 UTC, Felipe Contreras (banned)
none Details | Review
baseaudiosink: rename some variables (2.48 KB, patch)
2011-05-21 13:30 UTC, Felipe Contreras (banned)
none Details | Review
baseaudiosink: delay the resyncing of timestamp vs ringbuffertime (10.71 KB, patch)
2011-05-21 13:31 UTC, Felipe Contreras (banned)
none Details | Review
patch (16.20 KB, patch)
2011-05-21 17:31 UTC, Håvard Graff (hgr)
none Details | Review

Description Håvard Graff (hgr) 2011-05-21 10:15:44 UTC
Created attachment 188281 [details] [review]
patch

Original ide and design by Felipe Contreras <felipe.contreras@gmail.com>

A common problem for audio-playback is that the timestamps might not be
completely linear. This is specially common when doing streaming over
a network, where you can have jittery and/or bursty packettransmission,
which again will often be reflected on the buffertimestamps.

Now, the current implementation have a threshold (changed in this patch to
"alignment-threshold", and made seperate from the property that set
the tolerance for clock-drift resync) that says how far the
buffertimestamp is allowed o drift from the ideal aligned time
in the ringbuffer. This was an instant reaction, and ment that if one
buffer arrived with a timestamp that would breach the aligment-threshold,
a resync would take place, and the result would be an audible gap for the
listener.

The annoying thing would be that in the case of a "timestamp-outlier", you
would first resync one way, say +100ms, and then, if the next timestamp
was "back on track", you would end up resyncing the other way (-100ms)
So in fact, when you had only one buffer with slightly off timestamping,
you would end up with *two* audible gaps. This is the problem this patch
addresses.

The way to "fix" this problem with the previous implementation, would
have been to increase the "alignment-threshold" to a value that was
greater than the largest timestamp-outlier one would normally expect.
The big problem with this approach, however, is that it will allow
normal operations with a huge offset timestamp vs running-time, which
is detrimental to lip-sync. If the alignment-threshold is set to 200ms, it
basically means that lip-sync can easily end up being off by that much.

This patch will basically start a timer when the first breach of
alignment-threshold is detected. If any following timestamp for the
next n nanoseconds gets "back on track" within the threshold, it has
basically eliminated the effect of an outlier, and the timer is stopped.
 If, however, all timestamps within this time-limit are breaching the
threshold, we are probably facing a more permanent offset in the
timestamps, and a resync is allowed to happen.

So basically this patch offers something as rare as both higher
accuracy, it terms of allowing smaller alignment-thresholds, as well
as much smoother, less glitchy playback!

Adds getters and setters for the configurable alignment-threshold
and discont-time, to allow derived implementations setting these values
upon construction. The defaults are set as to completely mimic the
behavior of the old implementation, only with fewer glitches!
Comment 1 Håvard Graff (hgr) 2011-05-21 10:31:19 UTC
Some hard numbers on the effect of this patch:

A sine wave is sent through the sink, and sampled real-life timing-data is applied to timestamps and clock. The resulting sine is analyzed for discontinuities and gaps, so the more of them, the more audible the sound-distortions are. Also note the "gap lenght" is the total size of all the silence in the gaps, so bigger number = more audible gaps.


20 ms alignment-threshold and 0 second discont-wait: (original behavior)

/BaseAudioSinkSimulator/test_bug_84844_cyclic_audio_deterioration_and_dropout: 
Audio has 5 discontinuities and 16 gaps. Initial silence is 211181, and total gap length 18448 samples
OK
/BaseAudioSinkSimulator/test_henrikb_audioproblem: 
Audio has 251 discontinuities and 165 gaps. Initial silence is 35272, and total gap length 324168 samples
OK
/BaseAudioSinkSimulator/test_bug_84640_no_audio: 
Audio has 1 discontinuities and 3 gaps. Initial silence is 42523, and total gap length 110981 samples

---

20 ms alignment-threshold and 1 second discont-wait: (new default behavior)

/BaseAudioSinkSimulator/test_bug_84844_cyclic_audio_deterioration_and_dropout: 
Audio has 0 discontinuities and 10 gaps. Initial silence is 211181, and total gap length 12270 samples
OK
/BaseAudioSinkSimulator/test_henrikb_audioproblem: 
Audio has 34 discontinuities and 55 gaps. Initial silence is 35272, and total gap length 173651 samples
OK
/BaseAudioSinkSimulator/test_bug_84640_no_audio: 
Audio has 1 discontinuities and 1 gaps. Initial silence is 42523, and total gap length 2220 samples


Basically an enormous improvement in every way! :)
Comment 2 Håvard Graff (hgr) 2011-05-21 11:01:56 UTC
Created attachment 188283 [details] [review]
patch

A small mistake had somehow crept into the last patch, making it not build...
Comment 3 Felipe Contreras (banned) 2011-05-21 13:29:18 UTC
Created attachment 188286 [details] [review]
baseaudiosink: trivial comment fixes
Comment 4 Felipe Contreras (banned) 2011-05-21 13:29:37 UTC
Created attachment 188287 [details] [review]
baseaudiosink: split drift-tolerance into alignment-threshold
Comment 5 Felipe Contreras (banned) 2011-05-21 13:29:58 UTC
Created attachment 188288 [details] [review]
baseaudiosink: decrease alignment-threshold to 20ms
Comment 6 Felipe Contreras (banned) 2011-05-21 13:30:36 UTC
Created attachment 188289 [details] [review]
baseaudiosink: use gst_util_uint64_scale_int when appropriate
Comment 7 Felipe Contreras (banned) 2011-05-21 13:30:53 UTC
Created attachment 188290 [details] [review]
baseaudiosink: rename some variables
Comment 8 Felipe Contreras (banned) 2011-05-21 13:31:10 UTC
Created attachment 188291 [details] [review]
baseaudiosink: delay the resyncing of timestamp vs ringbuffertime
Comment 9 Felipe Contreras (banned) 2011-05-21 13:33:15 UTC
(In reply to comment #5)
> Created an attachment (id=188288) [details] [review]
> baseaudiosink: decrease alignment-threshold to 20ms

I don't think we should do this. At least not at this point.
Comment 10 Felipe Contreras (banned) 2011-05-21 13:38:11 UTC
(In reply to comment #8)
> Created an attachment (id=188291) [details] [review]
> baseaudiosink: delay the resyncing of timestamp vs ringbuffertime

This is basically my original patch, I think I should remain as the author.

The changes are basically:

 * Add gst_base_audio_sink_{set,get}_discont_wait
 * Change from guint64 to GstClockTime
 * Change some documentation
 * Shuffle some code

I could have made those changes if you asked.

I will rebase the patch and put it on the original bug.
Comment 11 Felipe Contreras (banned) 2011-05-21 13:41:20 UTC
Review of attachment 188291 [details] [review]:

s/ide/idea/

 * gst_base_audio_sink_set_discont_wait:
 * @sink: a #GstBaseAudioSink
 * @alignment_threshold: the new discont wait in nanoseconds

s/alignment_threshold/discont_wait/
Comment 12 Håvard Graff (hgr) 2011-05-21 16:24:34 UTC
(In reply to comment #10)
> (In reply to comment #8)
> > Created an attachment (id=188291) [details] [review] [details] [review]
> > baseaudiosink: delay the resyncing of timestamp vs ringbuffertime
> 
> This is basically my original patch, I think I should remain as the author.

I agree, and I am very happy to change the author for the patch to you, if the committing gentlemen prefers the one patch to several smaller ones. :)
Comment 13 Håvard Graff (hgr) 2011-05-21 17:31:45 UTC
Created attachment 188306 [details] [review]
patch

Fixes the typos found by Felipe, changes the author and fixes "since" for the added setters and getters
Comment 14 Wim Taymans 2011-11-07 11:05:18 UTC

*** This bug has been marked as a duplicate of bug 640859 ***