[vlc-commits] auhal: rework locking and avoid potential deadlock

David Fuhrmann git at videolan.org
Wed Jul 30 18:47:05 CEST 2014


vlc | branch: master | David Fuhrmann <dfuhrmann at videolan.org> | Wed Jul 30 18:45:04 2014 +0200| [2034ba88d6e1779f6717a16520d1a8b07759b2aa] | committer: David Fuhrmann

auhal: rework locking and avoid potential deadlock

hopefully closes #11675

> http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=2034ba88d6e1779f6717a16520d1a8b07759b2aa
---

 modules/audio_output/auhal.c |   72 +++++++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 25 deletions(-)

diff --git a/modules/audio_output/auhal.c b/modules/audio_output/auhal.c
index 6253052..9bdd4fd 100644
--- a/modules/audio_output/auhal.c
+++ b/modules/audio_output/auhal.c
@@ -106,8 +106,12 @@ struct aout_sys_t
     int                         i_bytes_per_sample;
 
     CFArrayRef                  device_list;
+    vlc_mutex_t                 device_list_lock;    /* protects access to device_list */
 
-    vlc_mutex_t                 var_lock;           /* protects access to device_list and i_selected_dev */
+    vlc_mutex_t                 selected_device_lock;/* Synchronizes access to i_selected_dev. This is only needed
+                                                        between VLCs audio thread and the core audio callback thread.
+                                                        The value is only changed in Start, further access to this variable
+                                                        within the audio thread (start, stop, close) needs no protection. */
 
     float                       f_volume;
     bool                        b_mute;
@@ -186,7 +190,8 @@ static int Open(vlc_object_t *obj)
 
     OSStatus err = noErr;
 
-    vlc_mutex_init(&p_sys->var_lock);
+    vlc_mutex_init(&p_sys->device_list_lock);
+    vlc_mutex_init(&p_sys->selected_device_lock);
     vlc_mutex_init(&p_sys->lock);
     vlc_cond_init(&p_sys->cond);
     p_sys->b_digital = false;
@@ -263,13 +268,21 @@ static void Close(vlc_object_t *obj)
     if (err != noErr)
         msg_Err(p_aout, "failed to remove listener for default audio device [%4.4s]", (char *)&err);
 
-    vlc_mutex_lock(&p_sys->var_lock);
+    /*
+     * StreamsChangedListener can rebuild the device list and thus held the device_list_lock.
+     * To avoid a possible deadlock, an array copy is created here.
+     * In rare cases, this can lead to missing StreamsChangedListener callback deregistration (TODO).
+     */
+    vlc_mutex_lock(&p_sys->device_list_lock);
+    CFArrayRef device_list_cpy = CFArrayCreateCopy(NULL, p_sys->device_list);
+    vlc_mutex_unlock(&p_sys->device_list_lock);
+
     /* remove streams callbacks */
-    CFIndex count = CFArrayGetCount(p_sys->device_list);
+    CFIndex count = CFArrayGetCount(device_list_cpy);
     if (count > 0) {
         for (CFIndex x = 0; x < count; x++) {
             AudioDeviceID deviceId = 0;
-            CFNumberRef cfn_device_id = CFArrayGetValueAtIndex(p_sys->device_list, x);
+            CFNumberRef cfn_device_id = CFArrayGetValueAtIndex(device_list_cpy, x);
             if (!cfn_device_id)
                 continue;
 
@@ -280,14 +293,15 @@ static void Close(vlc_object_t *obj)
         }
     }
 
+    CFRelease(device_list_cpy);
     CFRelease(p_sys->device_list);
-    vlc_mutex_unlock(&p_sys->var_lock);
 
     char *psz_device = aout_DeviceGet(p_aout);
     config_PutPsz(p_aout, "auhal-audio-device", psz_device);
     free(psz_device);
 
-    vlc_mutex_destroy(&p_sys->var_lock);
+    vlc_mutex_destroy(&p_sys->selected_device_lock);
+    vlc_mutex_destroy(&p_sys->device_list_lock);
     vlc_mutex_destroy(&p_sys->lock);
     vlc_cond_destroy(&p_sys->cond);
 
@@ -318,7 +332,7 @@ static int Start(audio_output_t *p_aout, audio_sample_format_t *restrict fmt)
     p_sys->b_paused = false;
     p_sys->i_device_latency = 0;
 
-    vlc_mutex_lock(&p_sys->var_lock);
+    vlc_mutex_lock(&p_sys->selected_device_lock);
     p_sys->i_selected_dev = p_sys->i_new_selected_dev;
 
     aout_FormatPrint(p_aout, "VLC is looking for:", fmt);
@@ -365,7 +379,7 @@ static int Start(audio_output_t *p_aout, audio_sample_format_t *restrict fmt)
         err = AudioObjectGetPropertyData(kAudioObjectSystemObject, &defaultDeviceAddress, 0, NULL, &propertySize, &defaultDeviceID);
         if (err != noErr) {
             msg_Err(p_aout, "could not get default audio device [%4.4s]", (char *)&err);
-            vlc_mutex_unlock(&p_sys->var_lock);
+            vlc_mutex_unlock(&p_sys->selected_device_lock);
             goto error;
         }
         else
@@ -374,7 +388,7 @@ static int Start(audio_output_t *p_aout, audio_sample_format_t *restrict fmt)
         p_sys->i_selected_dev = defaultDeviceID;
         p_sys->b_selected_dev_is_digital = var_InheritBool(p_aout, "spdif");
     }
-    vlc_mutex_unlock(&p_sys->var_lock);
+    vlc_mutex_unlock(&p_sys->selected_device_lock);
 
     // recheck if device still supports digital
     b_start_digital = p_sys->b_selected_dev_is_digital;
@@ -1286,7 +1300,7 @@ static void RebuildDeviceList(audio_output_t * p_aout)
         free(psz_name);
     }
 
-    vlc_mutex_lock(&p_sys->var_lock);
+    vlc_mutex_lock(&p_sys->device_list_lock);
     CFIndex count = 0;
     if (p_sys->device_list)
         count = CFArrayGetCount(p_sys->device_list);
@@ -1302,6 +1316,7 @@ static void RebuildDeviceList(audio_output_t * p_aout)
                 if (cfn_device_id) {
                     CFNumberGetValue(cfn_device_id, kCFNumberSInt32Type, &i_device_id);
                     msg_Dbg(p_aout, "Device ID %i is not found in new array, deleting.", i_device_id);
+
                     ReportDevice(p_aout, i_device_id, NULL);
                 }
             }
@@ -1311,7 +1326,7 @@ static void RebuildDeviceList(audio_output_t * p_aout)
         CFRelease(p_sys->device_list);
     p_sys->device_list = CFArrayCreateCopy(kCFAllocatorDefault, currentListOfDevices);
     CFRelease(currentListOfDevices);
-    vlc_mutex_unlock(&p_sys->var_lock);
+    vlc_mutex_unlock(&p_sys->device_list_lock);
 
     free(deviceIDs);
 }
@@ -1582,12 +1597,14 @@ static OSStatus DevicesListener(AudioObjectID inObjectID,  UInt32 inNumberAddres
     msg_Dbg(p_aout, "audio device configuration changed, resetting cache");
     RebuildDeviceList(p_aout);
 
-    vlc_mutex_lock(&p_sys->var_lock);
+    vlc_mutex_lock(&p_sys->selected_device_lock);
+    vlc_mutex_lock(&p_sys->device_list_lock);
     CFNumberRef selectedDevice = CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &p_sys->i_selected_dev);
     if(!CFArrayContainsValue(p_sys->device_list, CFRangeMake(0, CFArrayGetCount(p_sys->device_list)), selectedDevice))
         aout_RestartRequest(p_aout, AOUT_RESTART_OUTPUT);
     CFRelease(selectedDevice);
-    vlc_mutex_unlock(&p_sys->var_lock);
+    vlc_mutex_unlock(&p_sys->device_list_lock);
+    vlc_mutex_unlock(&p_sys->selected_device_lock);
 
     return noErr;
 }
@@ -1624,6 +1641,8 @@ static OSStatus DefaultDeviceChangedListener(AudioObjectID inObjectID,  UInt32 i
     if (!p_aout)
         return -1;
 
+    aout_sys_t *p_sys = p_aout->sys;
+
     if (!p_aout->sys->b_selected_dev_is_default)
         return noErr;
 
@@ -1646,12 +1665,13 @@ static OSStatus DefaultDeviceChangedListener(AudioObjectID inObjectID,  UInt32 i
         return noErr;
     }
 
+    vlc_mutex_lock(&p_sys->selected_device_lock);
     /* Also ignore events which announce the same device id */
-    if(defaultDeviceID == p_aout->sys->i_selected_dev)
-        return noErr;
-
-    msg_Dbg(p_aout, "default device actually changed, resetting aout");
-    aout_RestartRequest(p_aout, AOUT_RESTART_OUTPUT);
+    if(defaultDeviceID != p_aout->sys->i_selected_dev) {
+        msg_Dbg(p_aout, "default device actually changed, resetting aout");
+        aout_RestartRequest(p_aout, AOUT_RESTART_OUTPUT);
+    }
+    vlc_mutex_unlock(&p_sys->selected_device_lock);
 
     return noErr;
 }
@@ -1680,37 +1700,39 @@ static OSStatus StreamsChangedListener(AudioObjectID inObjectID,  UInt32 inNumbe
     msg_Dbg(p_aout, "available physical formats for audio device changed");
     RebuildDeviceList(p_aout);
 
+    vlc_mutex_lock(&p_sys->selected_device_lock);
     /* In this case audio has not yet started. Below code will not work and is not needed here. */
-    if (p_sys->i_selected_dev == 0)
+    if (p_sys->i_selected_dev == 0) {
+        vlc_mutex_unlock(&p_sys->selected_device_lock);
         return 0;
+    }
 
     /*
      * check if changed stream id belongs to current device
      */
-    vlc_mutex_lock(&p_sys->var_lock);
     AudioObjectPropertyAddress streamsAddress = { kAudioDevicePropertyStreams, kAudioDevicePropertyScopeOutput, kAudioObjectPropertyElementMaster };
     err = AudioObjectGetPropertyDataSize(p_sys->i_selected_dev, &streamsAddress, 0, NULL, &i_param_size);
     if (err != noErr) {
         msg_Err(p_aout, "could not get number of streams for device %i [%4.4s]", p_sys->i_selected_dev, (char *)&err);
-        vlc_mutex_unlock(&p_sys->var_lock);
+        vlc_mutex_unlock(&p_sys->selected_device_lock);
         return VLC_EGENERIC;
     }
 
     i_streams = i_param_size / sizeof(AudioStreamID);
     p_streams = (AudioStreamID *)malloc(i_param_size);
     if (p_streams == NULL) {
-        vlc_mutex_unlock(&p_sys->var_lock);
+        vlc_mutex_unlock(&p_sys->selected_device_lock);
         return VLC_ENOMEM;
     }
 
     err = AudioObjectGetPropertyData(p_sys->i_selected_dev, &streamsAddress, 0, NULL, &i_param_size, p_streams);
     if (err != noErr) {
         msg_Err(p_aout, "could not get list of streams [%4.4s]", (char *)&err);
-        vlc_mutex_unlock(&p_sys->var_lock);
+        vlc_mutex_unlock(&p_sys->selected_device_lock);
         free(p_streams);
         return VLC_EGENERIC;
     }
-    vlc_mutex_unlock(&p_sys->var_lock);
+    vlc_mutex_unlock(&p_sys->selected_device_lock);
 
     for (int i = 0; i < i_streams; i++) {
         if (p_streams[i] == inObjectID) {



More information about the vlc-commits mailing list