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

Manage logging callback lifetime

See merge request videolan/vlc-rs!24

src/core.rs

@@ -8,6 +8,7 @@ use std::marker::PhantomData;
 use std::ffi::CString;
 use std::i32;
 use std::convert::TryInto;
+use std::sync::Mutex;
 use libc::{c_void, c_char, c_int};
 use vlc_sys as sys;
 use crate::tools::{to_cstr, from_cstr, from_cstr_ref};
@@ -29,7 +30,7 @@ pub fn compiler() -> String {
 
 pub struct Instance {
     pub(crate) ptr: *mut sys::libvlc_instance_t,
-
+    log_callback: Mutex<CallbackSlot<Box<LoggingCallback>>>,
 }
 
 unsafe impl Send for Instance {}
@@ -61,7 +62,10 @@ impl Instance {
                 return None;
             }
 
-            Some(Instance{ptr: p})
+            Some(Instance{
+                ptr: p,
+                log_callback: Mutex::new(CallbackSlot::new()),
+            })
         }
     }
 
@@ -133,11 +137,20 @@ impl Instance {
 
     /// Set logging callback
     pub fn set_log<F: Fn(LogLevel, Log, Cow<str>) + Send + 'static>(&self, f: F) {
-        let cb: Box<Box<dyn Fn(LogLevel, Log, Cow<str>) + Send + 'static>> = Box::new(Box::new(f));
+        let callback = Box::new(LoggingCallback::new(f));
+        let mut callbacks = self.log_callback.lock().unwrap();
 
         unsafe{
-            sys::libvlc_log_set(self.ptr, Some(logging_cb), Box::into_raw(cb) as *mut _);
+            // libVLC must stop calling the previous callback before its data is dropped.
+            sys::libvlc_log_unset(self.ptr);
         }
+        drop(callbacks.clear());
+
+        let replaced = callbacks.replace(callback);
+        debug_assert!(replaced.is_none());
+        drop(replaced);
+        let data = callbacks.callback().unwrap().data();
+        unsafe{ sys::libvlc_log_set(self.ptr, Some(logging_cb), data); }
     }
 
     /// Returns raw pointer
@@ -148,17 +161,60 @@ impl Instance {
 
 impl Drop for Instance {
     fn drop(&mut self) {
+        unsafe{
+            // The callback references data owned by `log_callback`, so unregister it first.
+            sys::libvlc_log_unset(self.ptr);
+        }
+        drop(self.log_callback.get_mut().unwrap().clear());
         unsafe{
             sys::libvlc_release(self.ptr);
         }
     }
 }
 
+type LogCallback = Box<dyn Fn(LogLevel, Log, Cow<str>) + Send + 'static>;
+
+struct LoggingCallback {
+    callback: LogCallback,
+}
+
+impl LoggingCallback {
+    fn new<F: Fn(LogLevel, Log, Cow<str>) + Send + 'static>(callback: F) -> LoggingCallback {
+        LoggingCallback { callback: Box::new(callback) }
+    }
+
+    fn data(&self) -> *mut c_void {
+        &self.callback as *const LogCallback as *mut c_void
+    }
+}
+
+struct CallbackSlot<T> {
+    callback: Option<T>,
+}
+
+impl<T> CallbackSlot<T> {
+    fn new() -> CallbackSlot<T> {
+        CallbackSlot { callback: None }
+    }
+
+    fn replace(&mut self, callback: T) -> Option<T> {
+        self.callback.replace(callback)
+    }
+
+    fn clear(&mut self) -> Option<T> {
+        self.callback.take()
+    }
+
+    fn callback(&self) -> Option<&T> {
+        self.callback.as_ref()
+    }
+}
+
 const BUF_SIZE: usize = 1024; // Write log message to the buffer by vsnprintf.
 unsafe extern "C" fn logging_cb(
     data: *mut c_void, level: c_int, ctx: *const sys::libvlc_log_t, fmt: *const c_char, args: sys::VaList) {
 
-    let f: &Box<dyn Fn(LogLevel, Log, Cow<str>) + Send + 'static> = ::std::mem::transmute(data);
+    let f: &LogCallback = ::std::mem::transmute(data);
     let mut buf: [c_char; BUF_SIZE] = [0; BUF_SIZE];
 
     sys::vsnprintf(buf.as_mut_ptr(), BUF_SIZE.try_into().unwrap(), fmt, args);
@@ -166,6 +222,34 @@ unsafe extern "C" fn logging_cb(
     f((level as u32).into(), Log{ptr: ctx}, from_cstr_ref(buf.as_ptr()).unwrap());
 }
 
+#[cfg(test)]
+mod tests {
+    use super::CallbackSlot;
+    use std::sync::Arc;
+    use std::sync::atomic::{AtomicUsize, Ordering};
+
+    struct DropCounter(Arc<AtomicUsize>);
+
+    impl Drop for DropCounter {
+        fn drop(&mut self) {
+            self.0.fetch_add(1, Ordering::SeqCst);
+        }
+    }
+
+    #[test]
+    fn callback_slot_releases_replaced_and_cleared_callbacks() {
+        let drops = Arc::new(AtomicUsize::new(0));
+        let mut callbacks = CallbackSlot::new();
+
+        assert!(callbacks.replace(DropCounter(Arc::clone(&drops))).is_none());
+        drop(callbacks.replace(DropCounter(Arc::clone(&drops))));
+        assert_eq!(drops.load(Ordering::SeqCst), 1);
+
+        drop(callbacks.clear());
+        assert_eq!(drops.load(Ordering::SeqCst), 2);
+    }
+}
+
 /// List of module description.
 pub struct ModuleDescriptionList {
     ptr: *mut sys::libvlc_module_description_t,
@@ -579,4 +663,3 @@ impl Log {
         self.ptr
     }
 }
-