alsasink/audio(base)sink/audioringbuffer: clock skew due to improper accounting of frames written
I have tracked down a deficiency that can introduce incorrect clock skew due to a race condition accounting for frames/samples written vs buffered. For devices with large buffers and periods, this can cause clock skew corrections to be triggered that normally wouldn't be necessary, especially when a period size is close to the clock skew tolerance or even greater.
When this happens, I noticed the following:
Two consecutive calls to
gst_audio_base_sink_get_time showed in the log statement that the value of
raw remained unchanged, while
delay increased, resulting in backwards time. This happens when
gst_audio_sink_ring_buffer_delay is called when
gst_alsasink_write completed the call to
snd_pcm_writei but the ringbuffer thread hasn't called
gst_audio_ring_buffer_advance yet. The
segdone field that
gst_audio_ring_buffer_samples_done reads from isn't advanced until
gst_audio_ring_buffer_advance is called, but
gst_alsasink_write has written data already (e.g. a whole period) that should have been accounted for, which results in a window where the call to
gst_audio_ring_buffer_samples_done returns less data than that actually has been written, yet the call to
gst_audio_ring_buffer_delay returns the actual data buffered, leading to a backwards time swing. This is possible because the
GST_DELAY_SINK_LOCK lock is released (on purpose) between these calls, otherwise it could block
gst_audio_base_sink_get_time for way too long if no space is available to write.
The solution is actually quite simple: Account for the number of frames written immediately when they are written:
- Add a
GstAlsaSinkthat tracks the number of frames written. Whenever
snd_pcm_writeiis called, while the
GST_DELAY_SINK_LOCKis being held, add the number of frames written (assuming no error case).
- Add a
gst_alsasink_delay, as well as
delayvirtual method (and everywhere else that implements it). In
gst_alsasink_delay, while the
GST_DELAY_SINK_LOCKis being held, return the last stored value from that new field. This has to be done in the same call, we cannot add a separate method for this because the information must be retrieved at the same time as
- Add the same new argument to
gst_audio_ring_buffer_delaythat end up calling the
gst_audio_base_sink_get_timeso that it only calls
gst_audio_ring_buffer_samples_donein the case that
gst_audio_ring_buffer_delaydidn't return a value in the new
guint64*argument. If it did return a value, use it as
samples, otherwise call
gst_audio_ring_buffer_samples_doneas a fallback.
This dramatically reduces clock skew values, and eliminates many clock skew corrections I've been observing.