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.

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

Android support via Oboe #468

Merged
merged 2 commits into from
Oct 27, 2020
Merged

Conversation

endragor
Copy link
Contributor

This code compiles (assuming my PR into oboe-rs gets merged), but has not been tested yet, because there are problems with including multiple .so in a single .apk (katyo/oboe-rs#5). In the meantime I'd like to ask for clarifications for a couple of questions that occurred during implementation:

  1. StreamInstant for callbacks.

It's unclear whether CPAL guarantees any relations between different StreamInstant values. For example, should callback and capture fields of InputStreamTimestamp be comparable in any way? Should InputStreamTimestamp values for different streams be comparable?

Oboe provides only the number of frames read/written from/to the stream. Based on the number of frames and sample rate it's possible to compute the duration. This is how I provide values for capture and playback fields of InputStreamTimestamp and OutputStreamTimestamp. For callback fields I simply use std::time::Instant, providing the time elapsed since the stream creation. Is that OK?

  1. There seems to be no good way to query maximum buffer size from Android, but methods for querying minimum buffer size exist. So I decided to return i32::MAX as the maximum buffer size. Providing this value should theoretically work, but the implementation may reduce the value to something more meaningful. Again, is this fine or is there a better approach?

@enfipy
Copy link
Contributor

enfipy commented Sep 4, 2020

What status of Android support here? Why this PR closed?

@endragor endragor reopened this Sep 4, 2020
@endragor
Copy link
Contributor Author

endragor commented Sep 4, 2020

Still need to figure out a way to include oboe library to the .apk builds that depend on it.

@endragor endragor force-pushed the android-support branch 2 times, most recently from e7f2ba8 to bd08f86 Compare September 6, 2020 05:53
@zmerp
Copy link

zmerp commented Oct 18, 2020

Is there any update on this?

@enfipy

This comment has been minimized.

@endragor endragor changed the title WIP: Android support via Oboe Android support via Oboe Oct 19, 2020
@endragor
Copy link
Contributor Author

This should in theory work and be ready for merge. I tested it in the past on Android 10 with the older version of oboe-rs. I'd appreciate if someone could perform more testing and see if this works on older Android version - ideally before Android 8.0, where AAudio was introduced.

@enfipy
Copy link
Contributor

enfipy commented Oct 19, 2020

@endragor I tried to add this to Bevy and I was able to build an app for Android. But when I add this system:

fn audio(asset_server: Res<AssetServer>, audio_output: Res<AudioOutput>) {
    let music = asset_server.load("sounds/Windless Slopes.mp3").unwrap();
    audio_output.play(music);
}

It breaks after rendering some Bevy graphics with the ndk_glue error here:

pub fn native_activity() -> &'static NativeActivity {
    unsafe { NATIVE_ACTIVITY.as_ref().unwrap() }
}

I'm not sure but maybe something wrong with get_activity or where it gets called. Any ideas?

Some of my changes:
creator-rs/bevy@23ccd4d
https://github.com/creator-rs/rodio/commit/738cab244c767be83f455251b757ee807614597e
And the project that I try this on:
https://github.com/creator-rs/creator/tree/master/examples/app

@mitchmindtree
Copy link
Member

Hey @endragor, thanks for such an epic PR!

As I don't have an Android device available to me, it's difficult to test this out locally. It would be great to have at list once other contributor be able to confirm this is running before landing this.

Also, it would be great if a CI job could be added for building and testing this backend (see .github/workflows/cpal.yml). This would greatly assist maintainers when they don't have a device to test on locally, and would help to reduce the chance of accidentally introducing errors. CI scripts can also act as a nice reference for maintainers or other users to see how they can setup an environment in which they can build for the desired target.

@enfipy
Copy link
Contributor

enfipy commented Oct 21, 2020

I'm trying to run it on my Mi Mix 2 and getting pretty weird errors. First, I wasn't able to proceed without updating ndk & ndk-glue in cpal to version 0.2 (was getting an error with native_activity()). But after I updated - I receive the following 2 kinds of errors randomly!

Screen Shot 2020-10-21 at 11 04 57 PM

I don't know when exactly errors appear but I'm sure that it panics during playback!

  1. FORTIFY: pthread_mutex_lock called on a destroyed mutex (0x77f01fe4d0)
  2. thread '<unnamed>' panicked at 'supplied instant is later than self', library/std/src/time.rs:285:48

Any ideas?

@enfipy
Copy link
Contributor

enfipy commented Oct 21, 2020

I placed my logs in Pastebin analog here.

I tried to clear them from gfx & bevy spam logs and it clearly shows what happens. But I'm not sure how to fix it.

@endragor
Copy link
Contributor Author

@enfipy I'm planning to do some testing on the weekend. At a glance the stack trace looks like rodio stream handle is being used when the stream itself was destroyed. So that is more about correct rodio usage; things look fine on CPAL side - the internal stream was opened and started successfully.

@endragor
Copy link
Contributor Author

ndk and ndk-glue versions indeed needed bumping, but otherwise this branch works for me.

I've added a repo for others to quickly test Android support: https://github.com/endragor/cpal-android-test
Here is a direct link to the prebuilt APK that you can share with others so they can test it: https://github.com/endragor/cpal-android-test/raw/master/CPALTest.apk

I've tested it on Honor 10i running Android 10. It'd be great to test on older Android versions. If Android emulator works for you (it does not for me), you may test it there, too.

@mitchmindtree I've added CI jobs to check clippy errors and compilation of examples for Android target. We can't actually run anything on an Android device from CI, so I don't think there is much else to do there. By the way, I noticed there are clippy warnings (unrelated to this PR) and yet the clippy job finishes successfully. Is that expected?

Copy link
Contributor

@enfipy enfipy left a comment

Choose a reason for hiding this comment

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

Works amazingly! Superb work!

@enfipy
Copy link
Contributor

enfipy commented Oct 26, 2020

@est31 Hey, may you consider to review this one so we can proceed with this?

@mitchmindtree
Copy link
Member

Awesome stuff @endragor, looking good to me! And thanks @enfipy for testing.

Maybe it would also be useful to have a cargo apk build step in the CI too, just to check that all the build time linking and packaging stays in a working state too? I think it's fine for this to be added in a follow-up PR though.

Happy to merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants