Merge pull request #312 from nukep/master

Audio thread safety, improve AudioCVT

sdl2-sys/src/audio.rs

@@ -44,7 +44,8 @@ pub type SDL_AudioFilter =
     ::std::option::Option<extern "C" fn
                               (arg1: *const SDL_AudioCVT,
                                arg2: SDL_AudioFormat)>;
-#[allow(dead_code, missing_copy_implementations)]
+#[allow(dead_code, missing_copy_implementations, raw_pointer_derive)]
+#[derive(Copy)]
 #[repr(C)]
 pub struct SDL_AudioCVT {
     pub needed: c_int,

src/sdl2/audio.rs

@@ -141,7 +141,7 @@ impl Drop for AudioSpecWAV {
     }
 }
 
-pub trait AudioCallback<T> {
+pub trait AudioCallback<T>: Send {
     fn callback(&mut self, &mut [T]);
 }
 
@@ -390,6 +390,8 @@ pub struct AudioDeviceLockGuard<'a, CB: 'a> {
     device: &'a mut AudioDevice<CB>
 }
 
+impl<'a, CB: 'a> !Send for AudioDeviceLockGuard<'a, CB> {}
+
 impl<'a, CB: 'a> Deref for AudioDeviceLockGuard<'a, CB> {
     type Target = CB;
     fn deref(&self) -> &CB { &self.device.userdata.callback }
@@ -406,32 +408,23 @@ impl<'a, CB> Drop for AudioDeviceLockGuard<'a, CB> {
     }
 }
 
-#[derive(PartialEq)] #[allow(raw_pointer_derive)]
+#[derive(Copy)]
 pub struct AudioCVT {
-    raw: *mut ll::SDL_AudioCVT,
-    owned: bool,
-}
-
-impl Drop for AudioCVT {
-    fn drop(&mut self) {
-        if self.owned {
-            unsafe { libc::free(self.raw as *mut c_void) }
-        }
-    }
+    raw: ll::SDL_AudioCVT
 }
 
 impl AudioCVT {
     pub fn new(src_format: ll::SDL_AudioFormat, src_channels: u8, src_rate: i32,
-               dst_format: ll::SDL_AudioFormat, dst_channels: u8, dst_rate: i32) -> SdlResult<AudioCVT> {
-
+               dst_format: ll::SDL_AudioFormat, dst_channels: u8, dst_rate: i32) -> SdlResult<AudioCVT>
+    {
         use std::mem;
         unsafe {
-            let c_cvt_p = libc::malloc(mem::size_of::<ll::SDL_AudioCVT>() as size_t) as *mut ll::SDL_AudioCVT;
-            let ret = ll::SDL_BuildAudioCVT(c_cvt_p,
+            let mut raw: ll::SDL_AudioCVT = mem::uninitialized();
+            let ret = ll::SDL_BuildAudioCVT(&mut raw,
                                             src_format, src_channels, src_rate as c_int,
                                             dst_format, dst_channels, dst_rate as c_int);
             if ret == 1 || ret == 0 {
-                Ok(AudioCVT { raw: c_cvt_p, owned: true })
+                Ok(AudioCVT { raw: raw })
             } else {
                 Err(get_error())
             }
@@ -439,38 +432,52 @@ impl AudioCVT {
     }
 
     #[unstable="Certain conversions may cause buffer overflows. See AngryLawyer/rust-sdl2 issue #270."]
-    pub fn convert(&self, mut src: Vec<u8>) -> SdlResult<Vec<u8>> {
+    pub fn convert(&self, mut src: Vec<u8>) -> Vec<u8> {
         //! Convert audio data to a desired audio format.
         //!
         //! The `src` vector is adjusted to the capacity necessary to perform
         //! the conversion in place; then it is passed to the SDL library.
+        use std::num;
         unsafe {
-            if (*self.raw).needed != 1 {
-                return Err("no conversion needed!".to_owned())
-            }
+            if self.raw.needed != 0 {
+                let mut raw = self.raw;
 
-            // calculate the size of the dst buffer
-            (*self.raw).len = src.len() as c_int;
-            let dst_size = ( (*self.raw).len * (*self.raw).len_mult ) as usize;
-            let needed = dst_size - src.len();
-            src.reserve_exact(needed);
+                // calculate the size of the dst buffer
+                raw.len = num::cast(src.len()).expect("Buffer length overflow");
+                let dst_size = self.get_capacity(src.len());
+                let needed = dst_size - src.len();
+                src.reserve_exact(needed);
 
-            // perform the conversion in place
-            (*self.raw).buf = src.as_mut_ptr();
-            let ret = ll::SDL_ConvertAudio(self.raw);
+                // perform the conversion in place
+                raw.buf = src.as_mut_ptr();
+                let ret = ll::SDL_ConvertAudio(&mut raw);
+                // There's no reason for SDL_ConvertAudio to fail.
+                // The only time it can fail is if buf is NULL, which it never is.
+                if ret != 0 { panic!(get_error()) }
 
-            // return original buffer back to caller
-            if ret == 0 {
-                debug_assert!( (*self.raw).len_cvt > 0 );
-                debug_assert!( (*self.raw).len_cvt as usize <= src.capacity() );
+                // return original buffer back to caller
+                debug_assert!(raw.len_cvt > 0);
+                debug_assert!(raw.len_cvt as usize <= src.capacity());
 
-                src.set_len((*self.raw).len_cvt as usize);
-                Ok(src)
+                src.set_len(raw.len_cvt as usize);
+                src
             } else {
-                Err(get_error())
+                // The buffer remains unmodified
+                src
             }
         }
     }
+
+    /// Checks if any conversion is needed. i.e. if the buffer that goes
+    /// into `convert()` is unchanged from the result.
+    pub fn is_conversion_needed(&self) -> bool { self.raw.needed != 0 }
+
+    /// Gets the buffer capacity that can contain both the original and
+    /// converted data.
+    pub fn get_capacity(&self, src_len: usize) -> usize {
+        use std::num::Int;
+        src_len.checked_mul(self.raw.len_mult as usize).expect("Integer overflow")
+    }
 }
 
 
@@ -485,11 +492,14 @@ mod test {
         // 0,1,2,3, ...
         let buffer: Vec<u8> = range(0, 255).collect();
 
-        // 0,0,0,0,1,1,1,1,2,2,2,2,3,3,3,3, ...
+        // 0,0,1,1,2,2,3,3, ...
         let new_buffer_expected: Vec<u8> = range(0, 255).flat_map(|v| repeat(v).take(2)).collect();
 
         let cvt = AudioCVT::new(AUDIOU8, 1, 44100, AUDIOU8, 2, 44100).unwrap();
-        let new_buffer = cvt.convert(buffer).unwrap();
+        assert!(cvt.is_conversion_needed());
+        assert_eq!(cvt.get_capacity(255), 255*2);
+
+        let new_buffer = cvt.convert(buffer);
         assert_eq!(new_buffer.len(), new_buffer_expected.len());
         assert_eq!(new_buffer, new_buffer_expected);
     }

src/sdl2/lib.rs

@@ -1,7 +1,7 @@
 #![crate_name = "sdl2"]
 #![crate_type = "lib"]
 
-#![feature(slicing_syntax, unsafe_destructor)]
+#![feature(slicing_syntax, unsafe_destructor, optin_builtin_traits)]
 
 extern crate libc;
 extern crate collections;