GNOME Bugzilla – Bug 720661
audiobasesink: Fix locking bug accessing ring buffer time
Last modified: 2014-01-21 10:27:34 UTC
Created attachment 264467 [details] [review] Lock ring buffer acess from query function A position query from downstream could happen while the setcaps function was being called (this can happen when the query comes from a glib timeout function). The ring buffer could then be released between gst_audio_base_sink_get_time checking rate==0 and using it to scale samples to time, ending up dividing by 0. Note that I am not 100% sure it it always safe to get the stream lock here, it seems to work though. Relevant backtrace threads follow:
+ Trace 232936
Thread 6 (Thread 0x7fe9aa6dc700 (LWP 6383))
Thread 1 (Thread 0x7fe9c051b700 (LWP 6379))
Comment on attachment 264467 [details] [review] Lock ring buffer acess from query function I think taking the object lock there is fine... but gst_audio_base_sink_query_pad() does not call anything other than a ringbuffer function. So I would say that the locking should be done one level below inside the ringbuffer?
I think the audio sink would need to lock as a block a fair amount of the code in gst_audio_base_sink_setcaps, which would mean the audio sink itself would need adding lock/unlock calls (probably by adding gst_audio_ring_buffer_{,un}lock APIs). This seems more complex, unless I'm not seeing an easier way. When locking at the base sink level, we already get this lock for free.
Sounds sensible but please discuss with Wim before pushing.
I don't really want to block the whole object for all queries.. The problem is here that the get-time function of the clock calls into the basesink without locking for performance reasons. Essentially, you could have this problem as well if someone gets the current time and the ringbuffer is released. I'm thinking of doing the rate and ringbuffer checks only once in gst_audio_base_sink_get_time().
Do you think this is enough for now? commit 6a88d6f8cdf640d827f1082e4f498fc375fad781 Author: Wim Taymans <wtaymans@redhat.com> Date: Tue Jan 21 11:21:56 2014 +0100 audiobasesink: make _get_time more threadsafe We call the _get_time function from the provided clock and we don't lock the sink object for performance reasons. Make sure we only read and check variables once so that they don't change while we are executing the code. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=720661