diff --git a/src/audio_stream.rs b/src/audio_stream.rs index a11def3..3e2b933 100644 --- a/src/audio_stream.rs +++ b/src/audio_stream.rs @@ -411,9 +411,7 @@ impl AudioStream for T { } fn close(&mut self) -> Status { - wrap_status(unsafe { - ffi::oboe_AudioStream_close(self._raw_stream_mut() as *mut _ as *mut c_void) - }) + wrap_status(unsafe { ffi::oboe_AudioStream_close1(self._raw_stream_mut()) }) } fn start_with_timeout(&mut self, timeout_nanoseconds: i64) -> Status { @@ -537,24 +535,30 @@ pub(crate) fn audio_stream_fmt( '\n'.fmt(f) } -#[repr(transparent)] -struct AudioStreamHandle(*mut ffi::oboe_AudioStream); +struct AudioStreamHandle(*mut ffi::oboe_AudioStream, *mut c_void); -impl From<*mut ffi::oboe_AudioStream> for AudioStreamHandle { - fn from(raw: *mut ffi::oboe_AudioStream) -> Self { - Self(raw) +impl AudioStreamHandle { + fn new(raw: *mut ffi::oboe_AudioStream, shared_ptr: *mut c_void) -> Self { + Self(raw, shared_ptr) } -} -impl Default for AudioStreamHandle { - fn default() -> Self { - Self(null_mut()) + /// SAFETY: `self.0` and `self.1` must be valid pointers. + pub(crate) unsafe fn delete(&mut self) { + assert!(!self.0.is_null()); + assert!(!self.1.is_null()); + + // The error callback could be holding a shared_ptr, so don't delete AudioStream + // directly, but only its shared_ptr. + ffi::oboe_AudioStream_deleteShared(self.1); + + self.0 = null_mut(); + self.1 = null_mut(); } } -impl Drop for AudioStreamHandle { - fn drop(&mut self) { - unsafe { ffi::oboe_AudioStream_delete(self.0) } +impl Default for AudioStreamHandle { + fn default() -> Self { + Self(null_mut(), null_mut()) } } @@ -625,9 +629,6 @@ impl<'s> RawAudioOutputStream for AudioStreamRef<'s, Output> {} */ pub struct AudioStreamAsync { raw: AudioStreamHandle, - - // Needed to keep callback alive - #[allow(dead_code)] callback: AudioCallbackWrapper, } @@ -638,17 +639,37 @@ impl fmt::Debug for AudioStreamAsync { } impl AudioStreamAsync { - pub(crate) fn wrap_raw( + // SAFETY: `raw`, `shared_ptr` and `callback` must be valid. + pub(crate) unsafe fn wrap_raw( raw: *mut ffi::oboe_AudioStream, + shared_ptr: *mut c_void, callback: AudioCallbackWrapper, ) -> Self { Self { - raw: raw.into(), + raw: AudioStreamHandle(raw, shared_ptr), callback, } } } +impl Drop for AudioStreamAsync { + fn drop(&mut self) { + // SAFETY: As long as the conditions on Self::wrap_raw are guaranteed on the creation of + // self, this is safe. + unsafe { + self.close(); + self.raw.delete(); + + // NOTE: Currently there is no safe way to delete the AudioStreamCallback, so we are + // leaking it here. + // see https://github.com/google/oboe/issues/1610 and https://github.com/google/oboe/issues/1603 + + // replace this by `self.callback.delete()` when a fix upstream appear. + let _ = &self.callback; + } + } +} + impl RawAudioStreamBase for AudioStreamAsync { fn _raw_base(&self) -> &ffi::oboe_AudioStreamBase { unsafe { &*ffi::oboe_AudioStream_getBase(self.raw.0) } @@ -688,14 +709,29 @@ impl fmt::Debug for AudioStreamSync { } impl AudioStreamSync { - pub(crate) fn wrap_raw(raw: *mut ffi::oboe_AudioStream) -> Self { + // SAFETY: `raw`, `shared_ptr` must be valid, because they will be deleted on drop. + pub(crate) unsafe fn wrap_raw( + raw: *mut ffi::oboe_AudioStream, + shared_ptr: *mut c_void, + ) -> Self { Self { - raw: raw.into(), + raw: AudioStreamHandle::new(raw, shared_ptr), _phantom: PhantomData, } } } +impl Drop for AudioStreamSync { + fn drop(&mut self) { + // SAFETY: As long as the conditions on Self::wrap_raw are guaranteed on the creation of + // self, this is safe. + unsafe { + self.close(); + self.raw.delete(); + } + } +} + impl RawAudioStreamBase for AudioStreamSync { fn _raw_base(&self) -> &ffi::oboe_AudioStreamBase { unsafe { &*ffi::oboe_AudioStream_getBase(self.raw.0) } diff --git a/src/audio_stream_builder.rs b/src/audio_stream_builder.rs index 7f443ec..3e972c3 100644 --- a/src/audio_stream_builder.rs +++ b/src/audio_stream_builder.rs @@ -1,9 +1,10 @@ use num_traits::FromPrimitive; use oboe_sys as ffi; use std::{ + ffi::c_void, fmt, marker::PhantomData, - mem::MaybeUninit, + mem::{ManuallyDrop, MaybeUninit}, ops::{Deref, DerefMut}, }; @@ -49,10 +50,19 @@ impl DerefMut for AudioStreamBuilderHandle { */ #[repr(transparent)] pub struct AudioStreamBuilder { - raw: AudioStreamBuilderHandle, + raw: ManuallyDrop, _phantom: PhantomData<(D, C, T)>, } +impl Drop for AudioStreamBuilder { + fn drop(&mut self) { + // SAFETY: self.raw is only drop here, or taken in Self::destructs, which don't drop self. + unsafe { + ManuallyDrop::drop(&mut self.raw); + } + } +} + impl fmt::Debug for AudioStreamBuilder { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { audio_stream_base_fmt(self, f) @@ -81,16 +91,10 @@ impl Default for AudioStreamBuilder { } } -impl From> for AudioStreamBuilderHandle { - fn from(builder: AudioStreamBuilder) -> Self { - builder.raw - } -} - impl AudioStreamBuilder { fn convert(self) -> AudioStreamBuilder { AudioStreamBuilder { - raw: self.into(), + raw: ManuallyDrop::new(self.destructs()), _phantom: PhantomData, } } @@ -222,7 +226,7 @@ impl AudioStreamBuilder { * returns true. Otherwise __OpenSL ES__ will be used. */ pub fn get_audio_api(&self) -> AudioApi { - FromPrimitive::from_i32(unsafe { ffi::oboe_AudioStreamBuilder_getAudioApi(&*self.raw) }) + FromPrimitive::from_i32(unsafe { ffi::oboe_AudioStreamBuilder_getAudioApi(&**self.raw) }) .unwrap() } @@ -237,7 +241,7 @@ impl AudioStreamBuilder { * If the caller requests AAudio and it is supported then AAudio will be used. */ pub fn set_audio_api(mut self, audio_api: AudioApi) -> Self { - unsafe { ffi::oboe_AudioStreamBuilder_setAudioApi(&mut *self.raw, audio_api as i32) } + unsafe { ffi::oboe_AudioStreamBuilder_setAudioApi(&mut **self.raw, audio_api as i32) } self } @@ -444,6 +448,16 @@ impl AudioStreamBuilder { (audio_api == AudioApi::AAudio && Self::is_aaudio_supported()) || (audio_api == AudioApi::Unspecified && Self::is_aaudio_recommended()) } + + /// Descontructs self into its handle, without calling drop. + fn destructs(mut self) -> AudioStreamBuilderHandle { + // Safety: the std::mem::forget prevents `raw` from being dropped by Self::drop. + let raw = unsafe { ManuallyDrop::take(&mut self.raw) }; + + std::mem::forget(self); + + raw + } } impl AudioStreamBuilder { @@ -452,12 +466,23 @@ impl AudioStreamBuilder */ pub fn open_stream(self) -> Result> { let mut stream = MaybeUninit::<*mut ffi::oboe_AudioStream>::uninit(); - let Self { mut raw, .. } = self; - - wrap_status(unsafe { - ffi::oboe_AudioStreamBuilder_openStream(&mut *raw, stream.as_mut_ptr()) + let mut shared_ptr = MaybeUninit::<*mut c_void>::uninit(); + let mut raw = self.destructs(); + + let stream = wrap_status(unsafe { + ffi::oboe_AudioStreamBuilder_openStreamShared( + &mut *raw, + stream.as_mut_ptr(), + shared_ptr.as_mut_ptr(), + ) }) - .map(|_| AudioStreamSync::wrap_raw(unsafe { stream.assume_init() })) + .map(|_| unsafe { + AudioStreamSync::wrap_raw(stream.assume_init(), shared_ptr.assume_init()) + }); + + drop(raw); + + stream } } @@ -486,13 +511,13 @@ impl AudioStreamBuilder { (T, C): IsFrameType, { let mut callback = AudioCallbackWrapper::::wrap(stream_callback); - let Self { mut raw, .. } = self; + let mut raw = self.destructs(); unsafe { ffi::oboe_AudioStreamBuilder_setCallback(&mut *raw, callback.raw_callback()); } AudioStreamBuilderAsync { - raw, - callback, + raw: ManuallyDrop::new(raw), + callback: ManuallyDrop::new(callback), _phantom: PhantomData, } } @@ -523,13 +548,13 @@ impl AudioStreamBuilder { (T, C): IsFrameType, { let mut callback = AudioCallbackWrapper::::wrap(stream_callback); - let Self { mut raw, .. } = self; + let mut raw = self.destructs(); unsafe { ffi::oboe_AudioStreamBuilder_setCallback(&mut *raw, callback.raw_callback()); } AudioStreamBuilderAsync { - raw, - callback, + raw: ManuallyDrop::new(raw), + callback: ManuallyDrop::new(callback), _phantom: PhantomData, } } @@ -539,11 +564,25 @@ impl AudioStreamBuilder { * Factory for an audio stream. */ pub struct AudioStreamBuilderAsync { - raw: AudioStreamBuilderHandle, - callback: AudioCallbackWrapper, + raw: ManuallyDrop, + callback: ManuallyDrop>, _phantom: PhantomData<(D, F)>, } +impl Drop for AudioStreamBuilderAsync { + fn drop(&mut self) { + // SAFETY: the stream has not yet been open (Self::drop is not called after open_stream), + // so there is no data thread or error thread using this callback yet. + unsafe { + self.callback.delete(); + } + // SAFETY: self.raw is only drop here, or taken in Self::destructs, which don't drop self. + unsafe { + ManuallyDrop::drop(&mut self.raw); + } + } +} + impl fmt::Debug for AudioStreamBuilderAsync { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { audio_stream_base_fmt(self, f) @@ -560,20 +599,43 @@ impl RawAudioStreamBase for AudioStreamBuilderAsync { } } +impl AudioStreamBuilderAsync { + /// Descontructs self into its handle and audio callback, without calling drop. + fn destructs(mut self) -> (AudioStreamBuilderHandle, AudioCallbackWrapper) { + // Safety: the std::mem::forget prevents `raw` and `callback` from being dropped by + // Self::drop. + let raw = unsafe { ManuallyDrop::take(&mut self.raw) }; + let callback = unsafe { ManuallyDrop::take(&mut self.callback) }; + + std::mem::forget(self); + + (raw, callback) + } +} + impl AudioStreamBuilderAsync { /** * Create and open an asynchronous (callback-driven) input stream based on the current settings. */ pub fn open_stream(self) -> Result> { let mut stream = MaybeUninit::<*mut ffi::oboe_AudioStream>::uninit(); - let Self { - mut raw, callback, .. - } = self; - - wrap_status(unsafe { - ffi::oboe_AudioStreamBuilder_openStream(&mut *raw, stream.as_mut_ptr()) + let mut shared_ptr = MaybeUninit::<*mut c_void>::uninit(); + let (mut raw, callback) = self.destructs(); + + let stream = wrap_status(unsafe { + ffi::oboe_AudioStreamBuilder_openStreamShared( + &mut *raw, + stream.as_mut_ptr(), + shared_ptr.as_mut_ptr(), + ) }) - .map(|_| AudioStreamAsync::wrap_raw(unsafe { stream.assume_init() }, callback)) + .map(|_| unsafe { + AudioStreamAsync::wrap_raw(stream.assume_init(), shared_ptr.assume_init(), callback) + }); + + drop(raw); + + stream } } @@ -583,13 +645,22 @@ impl AudioStreamBuilderAsync { */ pub fn open_stream(self) -> Result> { let mut stream = MaybeUninit::<*mut ffi::oboe_AudioStream>::uninit(); - let Self { - mut raw, callback, .. - } = self; - - wrap_status(unsafe { - ffi::oboe_AudioStreamBuilder_openStream(&mut *raw, stream.as_mut_ptr()) + let mut shared_ptr = MaybeUninit::<*mut c_void>::uninit(); + let (mut raw, callback) = self.destructs(); + + let stream = wrap_status(unsafe { + ffi::oboe_AudioStreamBuilder_openStreamShared( + &mut *raw, + stream.as_mut_ptr(), + shared_ptr.as_mut_ptr(), + ) }) - .map(|_| AudioStreamAsync::wrap_raw(unsafe { stream.assume_init() }, callback)) + .map(|_| unsafe { + AudioStreamAsync::wrap_raw(stream.assume_init(), shared_ptr.assume_init(), callback) + }); + + drop(raw); + + stream } } diff --git a/src/audio_stream_callback.rs b/src/audio_stream_callback.rs index a9b099b..a9a3223 100644 --- a/src/audio_stream_callback.rs +++ b/src/audio_stream_callback.rs @@ -1,6 +1,7 @@ use std::{ ffi::c_void, marker::PhantomData, + mem::ManuallyDrop, ops::{Deref, DerefMut}, slice::{from_raw_parts, from_raw_parts_mut}, }; @@ -221,11 +222,10 @@ impl AudioStreamCallbackWrapperHandle { ffi::oboe_AudioStreamCallbackWrapper_new(audio_ready, before_close, after_close) }) } -} -impl Drop for AudioStreamCallbackWrapperHandle { - fn drop(&mut self) { - unsafe { ffi::oboe_AudioStreamCallbackWrapper_delete(self.0) } + /// SAFFETY: `self.0` must be a valid pointer. + pub(crate) unsafe fn delete(&mut self) { + ffi::oboe_AudioStreamCallbackWrapper_delete(self.0) } } @@ -245,7 +245,7 @@ impl DerefMut for AudioStreamCallbackWrapperHandle { pub(crate) struct AudioCallbackWrapper { raw: AudioStreamCallbackWrapperHandle, - callback: Box, + callback: ManuallyDrop>, _phantom: PhantomData, } @@ -253,6 +253,15 @@ impl AudioCallbackWrapper { pub(crate) fn raw_callback(&mut self) -> &mut ffi::oboe_AudioStreamCallbackWrapper { &mut *self.raw } + + /// SAFETY: `self.raw` and `self.callback` should be valid. Calling this twice results in a + /// double free. The AudioStream that owns this callback must not have being open. If the + /// AudioStream was open, there is currently no safe way of calling this function. + /// (see https://github.com/google/oboe/issues/1610) + pub(crate) unsafe fn delete(&mut self) { + self.raw.delete(); + ManuallyDrop::drop(&mut self.callback); + } } impl AudioCallbackWrapper @@ -267,11 +276,11 @@ where Some(on_error_before_close_input_wrapper::), Some(on_error_after_close_input_wrapper::), ), - callback, + callback: ManuallyDrop::new(callback), _phantom: PhantomData, }; unsafe { - (*wrapper.raw).setContext(&mut (*wrapper.callback) as *mut _ as *mut c_void); + (*wrapper.raw).setContext(&mut (**wrapper.callback) as *mut T as *mut c_void); } wrapper } @@ -289,11 +298,11 @@ where Some(on_error_before_close_output_wrapper::), Some(on_error_after_close_output_wrapper::), ), - callback, + callback: ManuallyDrop::new(callback), _phantom: PhantomData, }; unsafe { - (*wrapper.raw).setContext(&mut (*wrapper.callback) as *mut _ as *mut c_void); + (*wrapper.raw).setContext(&mut (**wrapper.callback) as *mut T as *mut c_void); } wrapper } diff --git a/sys/build.rs b/sys/build.rs index 111dc22..c26f08d 100644 --- a/sys/build.rs +++ b/sys/build.rs @@ -164,6 +164,7 @@ impl Builder { .allowlist_function("oboe::getSdkVersion") .blocklist_type("std::.*_ptr.*") .blocklist_type("oboe::ManagedStream") + .blocklist_function("oboe::AudioStreamBuilder_openStream") .blocklist_function("oboe::AudioStreamBuilder_openStream1") .blocklist_function("oboe::AudioStreamBuilder_openManagedStream") .blocklist_function("oboe::AudioStreamBuilder_setPackageName") diff --git a/sys/oboe-ext/include/oboe/OboeExt.h b/sys/oboe-ext/include/oboe/OboeExt.h index c89160c..be91241 100644 --- a/sys/oboe-ext/include/oboe/OboeExt.h +++ b/sys/oboe-ext/include/oboe/OboeExt.h @@ -59,9 +59,14 @@ namespace oboe { AudioApi AudioStreamBuilder_getAudioApi(const AudioStreamBuilder *builder); void AudioStreamBuilder_setAudioApi(AudioStreamBuilder *builder, AudioApi api); AudioStreamBase* AudioStreamBuilder_getBase(AudioStreamBuilder *builder); - + Result AudioStreamBuilder_openStreamShared(AudioStreamBuilder *builder, + AudioStream **stream, + void **shared_ptr); + void AudioStream_delete(AudioStream *oboeStream); + void AudioStream_deleteShared(void *shared_ptr); Result AudioStream_open(AudioStream *oboeStream); + Result AudioStream_close(AudioStream *oboeStream); Result AudioStream_requestStart(AudioStream *oboeStream); Result AudioStream_requestPause(AudioStream *oboeStream); Result AudioStream_requestFlush(AudioStream *oboeStream); diff --git a/sys/oboe-ext/src/AudioStreamBuilderWrapper.cpp b/sys/oboe-ext/src/AudioStreamBuilderWrapper.cpp index 283409f..23625b6 100644 --- a/sys/oboe-ext/src/AudioStreamBuilderWrapper.cpp +++ b/sys/oboe-ext/src/AudioStreamBuilderWrapper.cpp @@ -33,4 +33,14 @@ namespace oboe { AudioStreamBase* AudioStreamBuilder_getBase(AudioStreamBuilder *builder) { return static_cast(builder); } + + Result AudioStreamBuilder_openStreamShared(AudioStreamBuilder *builder, + AudioStream **stream, + void **shared_ptr) { + std::shared_ptr *s = new std::shared_ptr(); + Result res = builder->openStream(*s); + *stream = s->get(); + *shared_ptr = (void *)s; + return res; + } } diff --git a/sys/oboe-ext/src/AudioStreamWrapper.cpp b/sys/oboe-ext/src/AudioStreamWrapper.cpp index 8380ac6..e4e011b 100644 --- a/sys/oboe-ext/src/AudioStreamWrapper.cpp +++ b/sys/oboe-ext/src/AudioStreamWrapper.cpp @@ -1,14 +1,19 @@ #include "oboe/OboeExt.h" namespace oboe { - void AudioStream_delete(AudioStream *oboeStream) { - delete oboeStream; + void AudioStream_deleteShared(void *shared_ptr) { + std::shared_ptr *s = (std::shared_ptr *)shared_ptr; + delete s; } Result AudioStream_open(AudioStream *oboeStream) { return oboeStream->open(); } + Result AudioStream_close(AudioStream *oboeStream) { + return oboeStream->close(); + } + Result AudioStream_requestStart(AudioStream *oboeStream) { return oboeStream->requestStart(); }