GNOME Bugzilla – Bug 650724
baseaudiosink: delay the resyncing of timestamp vs ringbuffertime
Last modified: 2014-12-23 11:39:31 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!
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! :)
Created attachment 188283 [details] [review] patch A small mistake had somehow crept into the last patch, making it not build...
Created attachment 188286 [details] [review] baseaudiosink: trivial comment fixes
Created attachment 188287 [details] [review] baseaudiosink: split drift-tolerance into alignment-threshold
Created attachment 188288 [details] [review] baseaudiosink: decrease alignment-threshold to 20ms
Created attachment 188289 [details] [review] baseaudiosink: use gst_util_uint64_scale_int when appropriate
Created attachment 188290 [details] [review] baseaudiosink: rename some variables
Created attachment 188291 [details] [review] baseaudiosink: delay the resyncing of timestamp vs ringbuffertime
(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.
(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.
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/
(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. :)
Created attachment 188306 [details] [review] patch Fixes the typos found by Felipe, changes the author and fixes "since" for the added setters and getters
*** This bug has been marked as a duplicate of bug 640859 ***