GNOME Bugzilla – Bug 648359
baseaudiosrc: ringbuffer: segbase/segdone not updated when ring buffer cleared leads to incorrect timestamps
Last modified: 2014-07-07 15:30:45 UTC
I've been working on an issue related to camerabin2 where between captures the audio src element is taken down to READY and back up to PLAYING. From what I understand, normal behaviour for sources is to timestamp their buffers using the running time. However, baseaudiosrc uses the number of samples processed in its ring buffer to calculate the buffer timestamp. This is fine, but the variables of interest inside the ring buffer (segbase and segdone) are not adjusted as appropriate when going down to READY and back up to PAUSED clearing the ring buffer indirectly through the gst_ring_buffer_set_flushing () calls. When in the GST_BASE_AUDIO_SRC_SLAVE_SKEW mode of operation and after hitting PLAYING again, baseaudiosrc uses segdone * samples per segment * GST_SECOND / sample rate to calculate a timestamp for new buffers. It also uses segbase, but segbase always remains at 0 as it is only altered in gst_ring_buffer_set_sample () which is never called anywhere in -base or -good. As such, I believe there are two issues in this code: 1) segbase and segdone need to be initialised/reset at appropriate points in the code paths. 2) segbase needs to be used when referring to segdone unless segdone without considering segbase is actually what one wants. For now, I will just take the element down to NULL and back as a workaround.
Looking at baseaudiosrc, it looks like it should reset segdone and call gst_audio_clock_reset in PAUSED_TO_READY
The idea of SLAVE_SKEW is to keep track of running-time, and resync to it if one drifts out from it. The two causes, (copied from the code) are: /* Resync the ringbuffer if: * * 1. We are more than the length of the ringbuffer behind. * The length of the ringbuffer then gets to dictate * the threshold for what is concidered "too late" * * 2. If this is our first buffer. * We know that we should catch up to running_time * the first time we are ran. */ And I am pretty sure the point 2. will be triggered if src->next_is sat to -1, which it is in: case GST_STATE_CHANGE_READY_TO_PAUSED: GST_DEBUG_OBJECT (src, "READY->PAUSED"); src->next_sample = -1; If not, that might be a bug.
You are correct that next_sample is set to -1, however the following is used to calculate the buffer timestamp before entering the slave mode switch: /* calculate the sequentially next sample we need to read. This can jump and * create a DISCONT. */ sample = gst_base_audio_src_get_offset (src); [some reading that increments sample by the amount read] /* get the normal timestamp to get the duration. */ timestamp = gst_util_uint64_scale_int (sample, GST_SECOND, spec->rate); duration = gst_util_uint64_scale_int (src->next_sample, GST_SECOND, spec->rate) - timestamp; In gst_base_audio_src_get_offset (): /* assume we can append to the previous sample */ sample = src->next_sample; ... /* get the currently processed segment */ segdone = g_atomic_int_get (&src->ringbuffer->segdone) - src->ringbuffer->segbase; if (sample != -1) { ... } else { /* no previous sample, go to the current position */ GST_DEBUG_OBJECT (src, "first sample, align to current %d", segdone); sample = ((guint64) (segdone)) * sps; readseg = segdone; } Looking at the audio ringbuffer code, segbase/segdone are never reset during the lifetime of the ringbuffer. When going down to READY and coming back up to PLAYING with the pipeline the base_time is updated and so the running time has been reset, but segbase/segdone were not reset meaning that the timestamp calculated using the ringbuffer offset as noted in the code path above will follow on from where it was before going down to READY. This means that the running time is _earlier_ than the calculated timestamp and as such neither of the two conditions quoted are hit in skew mode. The audio ringbuffer segbase/segdone are only reset if the source element is taken down to NULL, which is a current workaround. Wim has proposed that some unit tests be implemented to study the behaviour such that we can then figure out what the proper fix is. There are certainly many different ways to fix this and it seems that the audio ringbuffer code needs some love, but we could not decide as to what solution was correct.
Created attachment 193058 [details] [review] baseaudiosrc: Always resync the ringbuffer on the first buffer In SKEW mode, use next_sample == -1 to check for the first sample when starting to read samples so it resyncs the ringbuffer and timestamps are ok. Suggestion from Teemu Katajisto <teemu.katajisto@digia.com>
Created attachment 193061 [details] small test application Small test application that prints timestamps from buffers. It starts a pulsesrc ! fakesink pipeline and, after 2 secs, sets the source to READY, waits 1 sec, and back to PLAYING (while setting the base-time and clock back). Notice that without this patch it will continue the timestamps from where it had stopped, and with the patch it resyncs and advances the 1sec it was stopped.
Comment on attachment 193061 [details] small test application Please add a copyright notice and turn it into a patch for test/examples.
commit 929edc25728a45d889ca46f38ce62d421c072122 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Tue Aug 2 10:11:14 2011 -0300 audiobasesrc: Always resync the ringbuffer on the first buffer In SKEW mode, use next_sample == -1 to check for the first sample when starting to read samples so it resyncs the ringbuffer and timestamps are ok. Suggestion from Teemu Katajisto <teemu.katajisto@digia.com> https://bugzilla.gnome.org/show_bug.cgi?id=648359
Should we update camerabin2 now ?