Merge branch 'fix/audio-callback-lifecycle' into 'master'

Manage audio callback lifetime

See merge request videolan/vlc-rs!27

src/media_player.rs

@@ -9,10 +9,12 @@ use crate::EventManager;
 use libc::{c_void, c_uint};
 use crate::enums::{State, Position};
 use std::mem::transmute;
+use std::sync::Mutex;
 
 /// A LibVLC media player plays one media (usually in a custom drawable).
 pub struct MediaPlayer {
     pub(crate) ptr: *mut sys::libvlc_media_player_t,
+    audio_callbacks: Mutex<AudioCallbackSlot<Box<AudioCallbacksData>>>,
 }
 
 unsafe impl Send for MediaPlayer {}
@@ -26,7 +28,10 @@ impl MediaPlayer {
             if p.is_null() {
                 return None;
             }
-            Some(MediaPlayer{ptr: p})
+            Some(MediaPlayer{
+                ptr: p,
+                audio_callbacks: Mutex::new(AudioCallbackSlot::new()),
+            })
         }
     }
 
@@ -101,13 +106,17 @@ impl MediaPlayer {
         let flag_flush = flush.is_some();
         let flag_drain = drain.is_some();
 
-        let data = AudioCallbacksData {
+        let data = Box::new(AudioCallbacksData {
             play: Box::new(play), pause: pause, resume: resume,
             flush: flush, drain: drain,
-        };
-        let data = Box::into_raw(Box::new(data));
+        });
+        let mut callbacks = self.audio_callbacks.lock().unwrap();
 
         unsafe{
+            // libVLC must stop using the previous opaque data before it is dropped.
+            callbacks.replace_after_unregistration(|| unset_audio_callbacks(self.ptr), data);
+
+            let data = callbacks.callback().unwrap().opaque();
             sys::libvlc_audio_set_callbacks(
                 self.ptr,
                 Some(audio_cb_play),
@@ -115,7 +124,7 @@ impl MediaPlayer {
                 if flag_resume {Some(audio_cb_resume)} else {None},
                 if flag_flush {Some(audio_cb_flush)} else {None},
                 if flag_drain {Some(audio_cb_drain)} else {None},
-                data as *mut c_void);
+                data);
         }
     }
 
@@ -322,10 +331,48 @@ impl MediaPlayer {
 
 impl Drop for MediaPlayer {
     fn drop(&mut self) {
+        self.audio_callbacks.get_mut().unwrap().clear_after_unregistration(|| {
+            // The callback data is owned by `audio_callbacks`, so unregister it first.
+            unset_audio_callbacks(self.ptr);
+        });
         unsafe{ sys::libvlc_media_player_release(self.ptr) };
     }
 }
 
+fn unset_audio_callbacks(player: *mut sys::libvlc_media_player_t) {
+    unsafe{
+        sys::libvlc_audio_set_callbacks(player, None, None, None, None, None, ::std::ptr::null_mut());
+    }
+}
+
+struct AudioCallbackSlot<T> {
+    callback: Option<T>,
+}
+
+impl<T> AudioCallbackSlot<T> {
+    fn new() -> AudioCallbackSlot<T> {
+        AudioCallbackSlot { callback: None }
+    }
+
+    fn replace_after_unregistration<F>(&mut self, unregister: F, callback: T)
+        where F: FnOnce(),
+    {
+        unregister();
+        drop(self.callback.replace(callback));
+    }
+
+    fn clear_after_unregistration<F>(&mut self, unregister: F)
+        where F: FnOnce(),
+    {
+        unregister();
+        drop(self.callback.take());
+    }
+
+    fn callback(&self) -> Option<&T> {
+        self.callback.as_ref()
+    }
+}
+
 // For audio_set_callbacks
 struct AudioCallbacksData {
     play: Box<dyn Fn(*const c_void, u32, i64) + Send + 'static>,
@@ -335,6 +382,12 @@ struct AudioCallbacksData {
     drain: Option<Box<dyn Fn() + Send + 'static>>,
 }
 
+impl AudioCallbacksData {
+    fn opaque(&self) -> *mut c_void {
+        self as *const AudioCallbacksData as *mut c_void
+    }
+}
+
 unsafe extern "C" fn audio_cb_play(
     data: *mut c_void, samples: *const c_void, count: c_uint, pts: i64) {
     let data: &AudioCallbacksData = transmute(data as *mut AudioCallbacksData);
@@ -362,9 +415,48 @@ unsafe extern "C" fn audio_cb_drain(data: *mut c_void) {
     (data.drain.as_ref().unwrap())();
 }
 
+#[cfg(test)]
+mod tests {
+    use super::AudioCallbackSlot;
+    use std::sync::Arc;
+    use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering};
+
+    struct DropCounter {
+        unregistered: Arc<AtomicBool>,
+        drops: Arc<AtomicUsize>,
+    }
+
+    impl Drop for DropCounter {
+        fn drop(&mut self) {
+            assert!(self.unregistered.load(Ordering::SeqCst));
+            self.drops.fetch_add(1, Ordering::SeqCst);
+        }
+    }
+
+    #[test]
+    fn audio_callback_slot_unregisters_before_replacing_and_clearing_callbacks() {
+        let unregistered = Arc::new(AtomicBool::new(false));
+        let drops = Arc::new(AtomicUsize::new(0));
+        let mut callbacks = AudioCallbackSlot::new();
+
+        callbacks.replace_after_unregistration(
+            || unregistered.store(true, Ordering::SeqCst),
+            DropCounter { unregistered: Arc::clone(&unregistered), drops: Arc::clone(&drops) });
+        unregistered.store(false, Ordering::SeqCst);
+
+        callbacks.replace_after_unregistration(
+            || unregistered.store(true, Ordering::SeqCst),
+            DropCounter { unregistered: Arc::clone(&unregistered), drops: Arc::clone(&drops) });
+        assert_eq!(drops.load(Ordering::SeqCst), 1);
+        unregistered.store(false, Ordering::SeqCst);
+
+        callbacks.clear_after_unregistration(|| unregistered.store(true, Ordering::SeqCst));
+        assert_eq!(drops.load(Ordering::SeqCst), 2);
+    }
+}
+
 #[derive(Clone, PartialEq, Eq, Hash, Debug)]
 pub struct TrackDescription {
     pub id: i32,
     pub name: Option<String>,
 }
-