GNOME Bugzilla – Bug 702230
audioringbuffer: Don't access timestamps array if not acquired
Last modified: 2013-10-01 20:21:35 UTC
Created attachment 246788 [details] Log with extra debug prints for audiobasesrc and ringbuffer. The scenario is that we're trying to change a GstAlsaSrc PLAYING->READY. When doing so I sometimes se the following error: 0:08:44.558564170 16779 0xd72030 ERROR ringbuffer gst-plugins-base/gst-libs/gst/audio/gstaudioringbuffer.c:1990:gst_audio_ring_buffer_set_timestamp:<audiosrcringbuffer82> Could not store timestamp, no timestamps buffer I need some help in determining the correct way of solving this, but first let me explain my reasoning: I put a breakpoint on the GST_ERROR in gdb and obtained the following backtraces
+ Trace 232057
Thread 2 (Thread 3837)
Thread 1 (Thread 3645)
My analysis of what is happening is that by making GstAlsaSrc transition PLAYING->READY then gst_audio_base_src_change_state() will be called once for each transition PLAYING->PAUSED and PAUSED->READY: gst_audio_base_src_change_state() when going PLAYING->PAUSED calls gst_audio_ring_buffer_pause() which calls gst_audio_ring_buffer_pause_unlocked() which does not pause the sample-reading-thread audioringbuffer_thread_func() in gstaudiosrc.c because gstaudiosrc does not override the virtual pause member in GstAudioRingBuffer gst_audio_base_src_change_state() when going PAUSED->READY calls gst_audio_ring_buffer_release() which first calls gst_audio_ring_buffer_stop() which calls gst_audio_src_ring_buffer_stop() which does nothing with the sample-reading-thread audioringbuffer_thread_func() and then gst_audio_ring_buffer_release() frees buf->timestamps before calling gst_audio_src_ring_buffer_release() which for the sample-reading-thread audioringbuffer_thread_func() calls g_thread_join() All this the sample-reading-thread is busy doing this: audioringbuffer_thread_func() runs a loop reading sample data into the ringbuffer and calls gst_audio_ring_buffer_set_timestamp() which makes use of buf->timestamps The problem is that when changing states in GstAudioSrc there is nothing telling the sample-reading-thread to stop working on the ringbuffer (or more specifically the ringbufffers' timestamps array). There must be something in the code path somewhere to tell that thread to using the array before it is freed by gst_audio_ring_buffer_release(). There is some code in gst_audio_src_ring_buffer_stop() that Wim commented out in commit 265a494 in gst-plugins-base in 2008 #if 0 GST_DEBUG ("stop, waiting..."); GST_AUDIO_SRC_RING_BUFFER_WAIT (buf); GST_DEBUG ("stoped"); #endif With the following motivation: + * gst-libs/gst/audio/gstaudiosrc.c: (gst_audioringbuffer_stop): + Disable a code path that is now called but causes a deadlock for some + reason and is unneeded. This leads me to the tentative conclusion that either GstAudioSrcRingBuffer needs to implement GstAudioRingBuffer virtual pause() member or the WAIT Wim commented out _is_ necessary to make the sample-reading-thread stop reading samples from the device. Which solution is correct? I also obtained a more comprehensive log output by using GST_DEBUG=2,audiobasesrc:6,ringbuffer:6, though I'm unsure if this provides any relevant information. This is attached in log.txt
Wouldn't be enough to only free all these things (like the timestamp array) after shutting down the ringbuffer thread? Instead of the other way around?
Created attachment 255604 [details] [review] audioringbuffer: check if acquired in set_timestamp I can reproduce the GST_ERROR reported by Sebastian Rasmussen. As explained in his analysis, audioringbuffer_thread_func() in gstaudiosrc.c calls gst_audio_ring_buffer_set_timestamp() at a point where buf->timestamps has been set to NULL by gst_audio_ring_buffer_release() from another thread. My suggestion is to report this case using a GST_DEBUG in gst_audio_ring_buffer_set_timestamp(), not as a GST_ERROR. The GstAudioRingBuffer has just been released by a concurrent thread, and to me it seems best to not require that audioringbuffer_thread_func, as a user of the GstAudioRingBuffer API, handles that case itself in a thread-safe way. It should be no problem for the GstAudioRingBuffer to not save the timestamp when the buffer has been released. In the attached suggested patch the "acquired" flag is checked in gst_audio_ring_buffer_set_timestamp(), for handling the case where the buffer has been released. GST_OBJECT_LOCK is used for protecting object data.
commit 09d628f8f1a103a32b8c9d25581299f77480c4d5 Author: David Svensson Fors <davidsf@axis.com> Date: Mon Sep 23 11:35:43 2013 +0200 audioringbuffer: check if acquired in set_timestamp Also use GST_OBJECT_LOCK when accessing object data in set_timestamp. https://bugzilla.gnome.org/show_bug.cgi?id=702230