From 2052ae2ee7048a3c24cd3010f79f01f4beda5788 Mon Sep 17 00:00:00 2001 From: Dan Spencer Date: Sat, 31 Jan 2015 07:09:56 -0700 Subject: [PATCH 1/5] Avoid heap allocation in AudioCVT, convert() can never fail The size of SDL_AudioCVT is known at compile-time, so no heap allocation is necessary. convert() can only fail if buf is NULL, which, in this case, it never can be. Additionally, the function can still succeed if no conversion is needed; just do nothing to the buffer! AudioCVT.len unfortunately uses an int and not a size_t, so an overflow check is added to make sure the buffer length is within c_int. --- sdl2-sys/src/audio.rs | 3 +- src/sdl2/audio.rs | 68 ++++++++++++++++++++----------------------- 2 files changed, 33 insertions(+), 38 deletions(-) diff --git a/sdl2-sys/src/audio.rs b/sdl2-sys/src/audio.rs index 2ef384b8..b9510800 100644 --- a/sdl2-sys/src/audio.rs +++ b/sdl2-sys/src/audio.rs @@ -44,7 +44,8 @@ pub type SDL_AudioFilter = ::std::option::Option; -#[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, diff --git a/src/sdl2/audio.rs b/src/sdl2/audio.rs index b99cf1ac..fd248bbb 100644 --- a/src/sdl2/audio.rs +++ b/src/sdl2/audio.rs @@ -406,32 +406,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 { - + dst_format: ll::SDL_AudioFormat, dst_channels: u8, dst_rate: i32) -> SdlResult + { use std::mem; unsafe { - let c_cvt_p = libc::malloc(mem::size_of::() 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,35 +430,38 @@ impl AudioCVT { } #[unstable="Certain conversions may cause buffer overflows. See AngryLawyer/rust-sdl2 issue #270."] - pub fn convert(&self, mut src: Vec) -> SdlResult> { + pub fn convert(&self, mut src: Vec) -> Vec { //! 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 = (raw.len * raw.len_mult) as usize; + 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 } } } @@ -485,11 +479,11 @@ mod test { // 0,1,2,3, ... let buffer: Vec = 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 = 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(); + let new_buffer = cvt.convert(buffer); assert_eq!(new_buffer.len(), new_buffer_expected.len()); assert_eq!(new_buffer, new_buffer_expected); } From ce94ba52c337e46c19b8035424badaa55ee3063a Mon Sep 17 00:00:00 2001 From: Dan Spencer Date: Sat, 31 Jan 2015 07:39:41 -0700 Subject: [PATCH 2/5] Add is_conversion_needed() and get_capacity() to AudioCVT These methods can help the user improve performance. * is_conversion_needed() can be used to decide if convert() needs to be called. If not, the user can decide to omit the call. * get_capacity() can be used to avoid vector reallocations that are possible in the convert() method. --- src/sdl2/audio.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/sdl2/audio.rs b/src/sdl2/audio.rs index fd248bbb..eed94b57 100644 --- a/src/sdl2/audio.rs +++ b/src/sdl2/audio.rs @@ -442,7 +442,7 @@ impl AudioCVT { // calculate the size of the dst buffer raw.len = num::cast(src.len()).expect("Buffer length overflow"); - let dst_size = (raw.len * raw.len_mult) as usize; + let dst_size = self.get_capacity(src.len()); let needed = dst_size - src.len(); src.reserve_exact(needed); @@ -465,6 +465,17 @@ impl AudioCVT { } } } + + /// 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("Interger overflow") + } } @@ -483,6 +494,9 @@ mod test { let new_buffer_expected: Vec = range(0, 255).flat_map(|v| repeat(v).take(2)).collect(); let cvt = AudioCVT::new(AUDIOU8, 1, 44100, AUDIOU8, 2, 44100).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); From 7761d4710f45515844a796607753c947203e97a7 Mon Sep 17 00:00:00 2001 From: Dan Spencer Date: Sat, 31 Jan 2015 07:59:16 -0700 Subject: [PATCH 3/5] The AudioCallback trait must be transferable across threads (Send) Because the audio callback data is used by an audio thread, we should declare that said data can be transferred to other threads. The trait does not implement Sync; the data is not accessed concurrently by many threads. --- src/sdl2/audio.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sdl2/audio.rs b/src/sdl2/audio.rs index eed94b57..576efdc7 100644 --- a/src/sdl2/audio.rs +++ b/src/sdl2/audio.rs @@ -141,7 +141,7 @@ impl Drop for AudioSpecWAV { } } -pub trait AudioCallback { +pub trait AudioCallback: Send { fn callback(&mut self, &mut [T]); } From 1dbf5542d9641a90c665eeef40eedc06dd308766 Mon Sep 17 00:00:00 2001 From: Dan Spencer Date: Sat, 31 Jan 2015 08:23:44 -0700 Subject: [PATCH 4/5] Prevent the audio lock guard from being sent across threads std::sync::MutexGuard does the same thing. Classically, mutexes cannot be unlocked from another thread. `!Send` is a negative trait, and thus requires us to ungate the `optin_builtin_traits` feature. --- src/sdl2/audio.rs | 2 ++ src/sdl2/lib.rs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/sdl2/audio.rs b/src/sdl2/audio.rs index 576efdc7..1f2123d8 100644 --- a/src/sdl2/audio.rs +++ b/src/sdl2/audio.rs @@ -390,6 +390,8 @@ pub struct AudioDeviceLockGuard<'a, CB: 'a> { device: &'a mut AudioDevice } +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 } diff --git a/src/sdl2/lib.rs b/src/sdl2/lib.rs index a17bf871..02646b29 100644 --- a/src/sdl2/lib.rs +++ b/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; From c7c8295cdd0f8ee76bb7767f1337cee8cf6ac6d5 Mon Sep 17 00:00:00 2001 From: Dan Spencer Date: Sat, 31 Jan 2015 17:30:13 -0700 Subject: [PATCH 5/5] Interger? I believe it's spelled "Integer". --- src/sdl2/audio.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sdl2/audio.rs b/src/sdl2/audio.rs index 1f2123d8..1c3c6e84 100644 --- a/src/sdl2/audio.rs +++ b/src/sdl2/audio.rs @@ -476,7 +476,7 @@ impl AudioCVT { /// 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("Interger overflow") + src_len.checked_mul(self.raw.len_mult as usize).expect("Integer overflow") } }