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

Refactor coreaudio host and add iOS support + ios-feedback example #485

Merged
merged 3 commits into from
Jan 2, 2021

Conversation

MichaelHills
Copy link
Contributor

@MichaelHills MichaelHills commented Oct 11, 2020

Fixes #224

Creating as a draft to get some early feedback while I clean it up / wait for dependency PRs to land. As per the linked issue, macOS + iOS coreaudio are different in many ways and the APIs for configuration are not the same. iOS uses AVAudioSession which doesn't exist on macOS.

Here's what I did to get iOS to work:

  • Refactored the coreaudio host to coreaudio/macos and coreaudio/ios with some shared code and re-export in coreaudio/mod.rs
  • The macOS split out is pretty much a straight move, the only code changes being removal of any iOS cfg switches / mentions in documentation
  • The iOS module doesn't do any enumeration, it just exposes a "default device" which is the RemoteIO audio unit and you're pretty much expected to use "default" configuration.
  • I created the ios-feedback example to show input + output working (tested on sim + iPhone 6)
  • I tested it on bevy as part of iOS Support bevyengine/bevy#87 (tested on sim + iPhone 6)

Dependencies

Testing

Following table for the feedback examples.

Device 3.5mm plug earphones Bluetooth earphones
MBP Late 2013 (Catalina) Underflow errors (same on master)
iPhone 11 (14.3)
iPhone 6 (12.4.8) Works but output stream fell behind and delay increased from 1s to ~2s
iPad 5th gen (14.1)
iPhone Simulator Can't hear anything, not sure why. (Verified correct system output device.)

I also tested that output works on Bevy on inbuilt speaker + bluetooth earphones on iPhone 11.

examples/feedback.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,109 @@
//! Feeds back the input stream directly into the output stream.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

more or less a straight copy of the feedback example, I changed latency to 1000ms as I found it easier to test

src/traits.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@MichaelHills MichaelHills force-pushed the mike-ios branch 3 times, most recently from 7121667 to ce7a2a0 Compare October 17, 2020 13:11
@MichaelHills
Copy link
Contributor Author

MichaelHills commented Oct 17, 2020

This should be mostly good to go except for new release in coreaudio-sys (and land + release in coreaudio-rs)

@MichaelHills
Copy link
Contributor Author

Oh maybe I need to add some kind of CI check for iOS?

@MichaelHills MichaelHills force-pushed the mike-ios branch 2 times, most recently from 246568f to d2f72f4 Compare October 24, 2020 13:52
@mitchmindtree
Copy link
Member

Exciting stuff @MichaelHills!

Just to clarify, the next step we're waiting on is RustAudio/coreaudio-rs#72 right?

Oh maybe I need to add some kind of CI check for iOS?

Yes that would be great, especially if it also included a build step for the iOS example to ensure that all the build-time linking etc continues to work too.

After these two steps and getting confirmation from another iOS user that they can get this working I'd be happy to see this land.

Then we should open issues for the implementations that are still incomplete, namely the device and supported config enumeration.

@MichaelHills
Copy link
Contributor Author

Exciting stuff @MichaelHills!

Just to clarify, the next step we're waiting on is RustAudio/coreaudio-rs#72 right?

I am still testing all the changes end-to-end across the stack (coreaudio-rs ios example, cpal ios example, bevy ios example) and getting a couple of others to help. On newer devices which default to 48KHz sample rate there was an issue. I think I have resolved it, but I also have an iPhone 11 now so I should be able to test this myself directly when I get a chance.

Once all issues have been ironed out, will push a new release on coreaudio-sys, merge the coreaudio-rs, then this PR will be ready to go.

Oh maybe I need to add some kind of CI check for iOS?

Yes that would be great, especially if it also included a build step for the iOS example to ensure that all the build-time linking etc continues to work too.

Yep I can add that. I think there's an example of doing an Xcode build from CLI in the bevy ios example I can follow.

After these two steps and getting confirmation from another iOS user that they can get this working I'd be happy to see this land.

Sounds good. :)

Then we should open issues for the implementations that are still incomplete, namely the device and supported config enumeration.

Yeah this stuff needs AVAudioSession, just adding AVFoundation.h to coreaudio-sys more than doubled the output size and compile times ballooned. So maybe a fine-grained bindgen would be nicer.

I'm also a bit unclear on actually the setting of sample rate for iOS. In the coreaudio-rs and cpal ios samples I can't seem to change the sample rate and have the feedback sample work properly. It underruns, the 1s latency evaporates and the feedback is staticy. It was my understanding that the RemoteIO device automatically employs sample rate converters when setting the ASBD, I need to go read the docs again. In any case, sticking to default (44.1 on old devices, 48 on new) seems to work fine.

@MichaelHills MichaelHills force-pushed the mike-ios branch 2 times, most recently from a2b2b5f to e250279 Compare December 5, 2020 12:26
@MichaelHills
Copy link
Contributor Author

I've rebased onto master, had some merge conflicts so I re-applied the differences manually. I compared my diff against the original diff (using vimdiff, diff of diffs!) and verified that I re-applied it correctly.

I added iOS to CI as well. All that remains is to publish coreaudio-rs, then get help to re-test one last time and merge. :)

@MichaelHills MichaelHills force-pushed the mike-ios branch 2 times, most recently from a7800b4 to fef7937 Compare December 5, 2020 12:32
@MichaelHills
Copy link
Contributor Author

I had to change the iOS CI to only build for iphonesimulator, it seems to want a development team for signing to build an iphoneos build.

@MichaelHills
Copy link
Contributor Author

Note that the CI does do cargo lipo inside so it will build a universal rust binary with both arm and x86 builds. Only the xcode project won't get built for arm because of the signing requirement. @simlay do you know if I can just do an iphoneos build for CI without it erroring out on missing a development team?

@MichaelHills MichaelHills force-pushed the mike-ios branch 2 times, most recently from 206b37a to 841ea2b Compare December 7, 2020 14:02
@MichaelHills MichaelHills marked this pull request as ready for review December 7, 2020 14:21
@MichaelHills
Copy link
Contributor Author

Okay this is ready for final review/testing. @naithar are you able to help me test the cpal-ios-example in this PR and bevyengine/bevy#1007 ?

For the cpal example you should be able to speak into the mic and hear your own voice play back with 1s delay.

Would be good to check macos still works as well:

  • cpal: cargo run --example beep + feedback
  • bevy: cargo run --example audio

Oh I think examples are broken in cpal atm so you'll need this to run them

diff --git a/Cargo.toml b/Cargo.toml
index de163a3..9eb11d6 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -55,23 +55,3 @@ oboe = { version = "0.3.0", features = [ "java-interface" ] }
 ndk = "0.2"
 ndk-glue = "0.2"
 jni = "0.17"
-
-[[example]]
-name = "beep"
-path = "examples/beep.rs"
-crate-type = ["cdylib"]
-
-[[example]]
-name = "enumerate"
-path = "examples/enumerate.rs"
-crate-type = ["cdylib"]
-
-[[example]]
-name = "feedback"
-path = "examples/feedback.rs"
-crate-type = ["cdylib"]
-
-[[example]]
-name = "record_wav"
-path = "examples/record_wav.rs"
-crate-type = ["cdylib"]

I'll re-do a full sweep of testing across macOS (Catalina), iPhone 6, 11, and an iPad soon as I can.

Copy link
Member

@simlay simlay left a comment

Choose a reason for hiding this comment

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

I'll test this example out on my Macbook and iPhone but from looking at the xcode setup, this looks good.

It might be obvious to the people working on cpal what to expect with a feedback example but users of cpal might be less aware. Maybe we could add a small README in the ios-feedback directory just saying what to expect (also maybe to install cargo-lipo as a dependency).

Great work @MichaelHills!

#[cfg(target_os = "macos")]
pub use self::macos::enumerate::{Devices, SupportedInputConfigs, SupportedOutputConfigs};
#[cfg(target_os = "macos")]
pub use self::macos::{Device, Host, Stream};
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what cargo fmt says but I think it would be more readable to have

#[cfg(target_os = "macos")]
mod macos;
#[cfg(target_os = "ios")]
mod ios;

#[cfg(target_os = "macos")]
pub use self::macos::{
    enumerate::{Devices, SupportedInputConfigs, SupportedOutputConfigs},
    Device, Host, Stream
};
// Same thing for the iOS pub use stuff below.

Double check the exact stuff here. From my experience with conditional compilation, fewer cfg(target_os = will make things easier for the next person.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes much better, thanks. cargo fmt seems to prefer to put ios first.

@enfipy
Copy link
Contributor

enfipy commented Dec 30, 2020

Shameless ping

Is it good enough to merge?

@MichaelHills MichaelHills force-pushed the mike-ios branch 5 times, most recently from 3379c5c to 986dc12 Compare January 1, 2021 14:08
@MichaelHills
Copy link
Contributor Author

@enfipy see the PR description, I just finished re-testing everything and put up the results. As far as I can tell we're good to go.

@simlay I addressed your comments, added README + cleaned up the imports.

@mitchmindtree originally requested confirmation from one other iOS user. I think it'd be good if someone else can independently verify that macOS still works fine and the iOS example works on a device. One device should be fine, I've done testing across the devices that I have with iPhone earphones (cord / 3.5mm plug) and some SoundPEATS TrueAir2 wireless earphones.

I'm not sure how cpal or users of cpal should be handling changes in devices. For example, turning off bluetooth on my iPhone 11 and switching back to inbuilt speaker seems to lead to underflow in the output and it becomes stuttery. Nothing that should block landing initial iOS support of course.

@enfipy
Copy link
Contributor

enfipy commented Jan 1, 2021

It works pretty well from my side. Tested on iPhone 5s, iPhone 12, and macOS 11.1 simulator.

@MichaelHills
Copy link
Contributor Author

Thanks for testing. Also macOS beep + feedback + record_wav all still fine for me. Sounds like we're good to merge then. 👍 I don't have permissions to merge so now up to the powers that be.

@mitchmindtree
Copy link
Member

Awesome stuff @MichaelHills, thanks for all your work on this!

@mitchmindtree mitchmindtree merged commit 1d72ae7 into RustAudio:master Jan 2, 2021
@MichaelHills
Copy link
Contributor Author

Excellent. :) Can we publish a new version? Then can finally use it in Bevy which was my original motivation all those months ago haha

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.

cpal uses macOS specific APIs not supported on iOS
4 participants