Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some unsoundess and crash issues #48

Closed
wants to merge 15 commits into from
Closed

Fix some unsoundess and crash issues #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
36c0e8a
Fix some unsoundess and crash issues
Rodrigodd Aug 26, 2022
1dad283
Update bindings for x86_64
katyo Aug 31, 2022
4dbf88f
CI: Update NDK version to 21.4.7075529 (LTS)
katyo Sep 1, 2022
06759f5
Update bindgen to 0.60
katyo Sep 1, 2022
7f2fbc2
Updated bindings
katyo Sep 1, 2022
1fe1e24
Add missing apk signing key for demo
katyo Sep 1, 2022
6259786
fix warning for unused result
Rodrigodd Aug 27, 2022
6367d3b
update ndk to 0.7
Rodrigodd Jan 11, 2023
d373232
Updated oboe library to 1.7.0
Rodrigodd Jan 12, 2023
e853223
Updated bindings
Rodrigodd Jan 12, 2023
5882266
Stop using deprecated AudioStreamCallback
Rodrigodd Jan 12, 2023
50894ec
Remove commented out code
Rodrigodd Jan 12, 2023
d9d5313
Use new API for setting the AudioStreamCallback
Rodrigodd Jan 12, 2023
ce797bc
Stop holding the audio callback in Rust side
Rodrigodd Jan 13, 2023
fea00f6
Update bindings
Rodrigodd Jan 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions .github/workflows/rust.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,14 @@ jobs:
uses: android-actions/setup-android@v2
- name: Setup Android NDK
env:
NDK_VERSION: 21.3.6528147
NDK_VERSION: 21.4.7075529
TRIPLE: x86_64-linux-android
run: |
rm -rf $ANDROID_SDK_ROOT/ndk-bundle
sdkmanager --sdk_root=$ANDROID_SDK_ROOT "ndk;$NDK_VERSION" | grep -v = || true
ln -s $ANDROID_SDK_ROOT/ndk/$NDK_VERSION $ANDROID_SDK_ROOT/ndk-bundle
echo "$ANDROID_SDK_ROOT/ndk-bundle/toolchains/llvm/prebuilt/linux-x86_64/bin" >> $GITHUB_PATH
sdkmanager --sdk_root=$ANDROID_SDK_ROOT --install "ndk;$NDK_VERSION" --channel=3 | grep -v = || true
echo "$ANDROID_SDK_ROOT/ndk/$NDK_VERSION/toolchains/llvm/prebuilt/linux-x86_64/bin" >> $GITHUB_PATH
for var in ANDROID_NDK ANDROID_NDK_HOME ANDROID_NDK_LATEST_HOME ANDROID_NDK_ROOT; do
echo "$var=$ANDROID_SDK_ROOT/ndk/$NDK_VERSION" >> $GITHUB_ENV
done
TRIPLE_ENV=$(echo $TRIPLE | tr '-' '_')
echo "CC_${TRIPLE_ENV}=${TRIPLE}21-clang" >> $GITHUB_ENV
echo "CXX_${TRIPLE_ENV}=${TRIPLE}21-clang++" >> $GITHUB_ENV
Expand Down Expand Up @@ -124,12 +125,13 @@ jobs:
uses: android-actions/setup-android@v2
- name: Setup Android NDK
env:
NDK_VERSION: 21.3.6528147
NDK_VERSION: 21.4.7075529
run: |
rm -rf $ANDROID_SDK_ROOT/ndk-bundle
sdkmanager --sdk_root=$ANDROID_SDK_ROOT "ndk;$NDK_VERSION" | grep -v = || true
ln -s $ANDROID_SDK_ROOT/ndk/$NDK_VERSION $ANDROID_SDK_ROOT/ndk-bundle
echo "$ANDROID_SDK_ROOT/ndk-bundle/toolchains/llvm/prebuilt/linux-x86_64/bin" >> $GITHUB_PATH
sdkmanager --sdk_root=$ANDROID_SDK_ROOT --install "ndk;$NDK_VERSION" --channel=3 | grep -v = || true
echo "$ANDROID_SDK_ROOT/ndk/$NDK_VERSION/toolchains/llvm/prebuilt/linux-x86_64/bin" >> $GITHUB_PATH
for var in ANDROID_NDK ANDROID_NDK_HOME ANDROID_NDK_LATEST_HOME ANDROID_NDK_ROOT; do
echo "$var=$ANDROID_SDK_ROOT/ndk/$NDK_VERSION" >> $GITHUB_ENV
done
- name: Setup Rust ${{ matrix.rust }} [${{ matrix.target }}]
uses: actions-rs/toolchain@v1
with:
Expand Down Expand Up @@ -242,6 +244,10 @@ jobs:
path: ~/.cargo/bin/cargo-apk
key: ${{ runner.os }}-cargo-apk
- uses: Swatinem/rust-cache@v1
- name: Create signing key
run: |
echo ${{ secrets.APK_SIGN_KEY_DATA }} | base64 -d > demo/release.keystore
sed -i 's/keystore_password = "android"/keystore_password = "${{ secrets.APK_SIGN_KEY_SECRET }}"/' demo/Cargo.toml
- name: Build demo apk
uses: actions-rs/cargo@v1
with:
Expand Down
6 changes: 1 addition & 5 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ version = "0.4.5"
path = "sys"

[dependencies.ndk]
version = "0.6"
version = "0.7"
optional = true

[dependencies.ndk-context]
Expand Down Expand Up @@ -64,7 +64,3 @@ codegen-units = 1
panic = 'unwind'
incremental = false
overflow-checks = false

[patch.crates-io]
winit = { git = "https://github.com/rust-windowing/winit" }
glutin = { git = "https://github.com/katyo/glutin", branch = "android-support" }
8 changes: 6 additions & 2 deletions demo/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ optional = true
version = "0.1.0"

[dependencies.ndk-glue]
version = "0.6.0"
version = "0.7.0"
features = ["logger"]

[dependencies.android_logger]
Expand All @@ -32,7 +32,7 @@ version = "0.10"
version = "0.4"

[dependencies.glutin]
version = "0.24"
version = "0.29"

[features]
default = ["audio"]
Expand Down Expand Up @@ -75,3 +75,7 @@ name = "android.permission.WRITE_EXTERNAL_STORAGE"

[[package.metadata.android.permission]]
name = "android.permission.RECORD_AUDIO"

[package.metadata.android.signing.release]
path = "release.keystore"
keystore_password = "android"
85 changes: 56 additions & 29 deletions src/audio_stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,9 @@ use std::{
};

use super::{
audio_stream_base_fmt, wrap_result, wrap_status, AudioApi, AudioCallbackWrapper,
AudioStreamBase, FrameTimestamp, Input, IsFrameType, Output, RawAudioInputStream,
RawAudioOutputStream, RawAudioStream, RawAudioStreamBase, Result, Status, StreamState,
NANOS_PER_MILLISECOND,
audio_stream_base_fmt, wrap_result, wrap_status, AudioApi, AudioStreamBase, FrameTimestamp,
Input, IsFrameType, Output, RawAudioInputStream, RawAudioOutputStream, RawAudioStream,
RawAudioStreamBase, Result, Status, StreamState, NANOS_PER_MILLISECOND,
};

/**
Expand Down Expand Up @@ -411,9 +410,7 @@ impl<T: RawAudioStream + RawAudioStreamBase> 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 {
Expand Down Expand Up @@ -537,24 +534,30 @@ pub(crate) fn audio_stream_fmt<T: AudioStreamSafe>(
'\n'.fmt(f)
}

#[repr(transparent)]
struct AudioStreamHandle(*mut ffi::oboe_AudioStream);
struct AudioStreamHandle(*mut ffi::oboe_AudioStream, *mut c_void);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about accessing to AudioStream via shared pointer here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean using something like *mut shared_ptr<oboe_AudioStream> instead of *mut c_void (and maybe ideally without the pointer indirection)?

I had no experience in creating bindings between Rust and C++, so I was not sure how to pass a shared_ptr between them. If you simply put std::shared_ptr<AudioStream> in oboe_AudioStreamBuilder_openStreamShared at C++ side, bindgen would use an opaque [u64; 2] type.

Maybe that was okay, or I could wrap the shared_ptr in another class or something (thinking back, that would allow accessing AudioStream through a getter, instead of keeping two pointers in Rust side). But I opted for allocating the shared_ptr on the heap, and passing a type-erased pointer to Rust side. This seemed to be less error-prone for me. But if anyone has a better idea on how to do it, I can change it.

Copy link
Owner

@katyo katyo Jan 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean I would like to avoid storing two pointers in AudioStreamHandle.

Typical shared_ptr<T> layout in C++ looks as follows:

|----------------------|
|   shared_ptr sPtr1   |
|----------------------|
|Pointer to the Object |---|-------> Object
|                      |   |           
|Pointer to the Ctr blk|---|---|
|----------------------|   |   |---> Ctl block
                           |   |       (Pointer to allocator, deleter, ...)
|----------------------|   |   |       (Shared reference counter)
|   shared_ptr sPtr2   |   |   |       (Weak reference counter + offset 1)
|----------------------|   |   |
|Pointer to the Object |---|   |
|                      |       |
|Pointer to the Ctr blk|-------|
|----------------------|

Seems opaque type that bindgen propose is that we need (actually it is a [*mut c_void; 2]).

To make things totally safe we may get raw pointer via function which we implement on C++ side.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated #49.


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())
}
}

Expand Down Expand Up @@ -625,10 +628,7 @@ impl<'s> RawAudioOutputStream for AudioStreamRef<'s, Output> {}
*/
pub struct AudioStreamAsync<D, F> {
raw: AudioStreamHandle,

// Needed to keep callback alive
#[allow(dead_code)]
callback: AudioCallbackWrapper<D, F>,
_phantom: PhantomData<(D, F)>,
}

impl<D, F> fmt::Debug for AudioStreamAsync<D, F> {
Expand All @@ -638,13 +638,25 @@ impl<D, F> fmt::Debug for AudioStreamAsync<D, F> {
}

impl<D, F> AudioStreamAsync<D, F> {
pub(crate) fn wrap_raw(
// SAFETY: `raw` and `shared_ptr` must be valid.
pub(crate) unsafe fn wrap_raw(
raw: *mut ffi::oboe_AudioStream,
callback: AudioCallbackWrapper<D, F>,
shared_ptr: *mut c_void,
) -> Self {
Self {
raw: raw.into(),
callback,
raw: AudioStreamHandle(raw, shared_ptr),
_phantom: PhantomData,
}
}
}

impl<D, F> Drop for AudioStreamAsync<D, F> {
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 {
let _ = self.close();
self.raw.delete();
}
}
}
Expand Down Expand Up @@ -688,14 +700,29 @@ impl<D, F> fmt::Debug for AudioStreamSync<D, F> {
}

impl<D, F> AudioStreamSync<D, F> {
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<D, F> Drop for AudioStreamSync<D, F> {
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 {
let _ = self.close();
self.raw.delete();
}
}
}

impl<D, T> RawAudioStreamBase for AudioStreamSync<D, T> {
fn _raw_base(&self) -> &ffi::oboe_AudioStreamBase {
unsafe { &*ffi::oboe_AudioStream_getBase(self.raw.0) }
Expand Down
Loading