-
Notifications
You must be signed in to change notification settings - Fork 44
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
Fix iOS compilation error and add macOS + iOS feedback example applications #72
Conversation
a65e643
to
cf875a7
Compare
@@ -525,7 +540,7 @@ impl AudioUnit { | |||
unsafe { | |||
// Retrieve the up-to-date stream format. | |||
let id = sys::kAudioUnitProperty_StreamFormat; | |||
let asbd = match super::get_property(audio_unit, id, Scope::Input, Element::Output) { | |||
let asbd = match super::get_property(audio_unit, id, Scope::Output, Element::Input) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are fetching the stream format for the audio unit being used to record the mic. Therefore we want to use the "input" bus/element, but the output scope. The input scope would give the hardware format, not the user requested format that will be rendered into.
As per #60
@@ -471,7 +474,7 @@ impl AudioUnit { | |||
// First, we'll retrieve the stream format so that we can ensure that the given callback | |||
// format matches the audio unit's format. | |||
let id = sys::kAudioUnitProperty_StreamFormat; | |||
let asbd = self.get_property(id, Scope::Input, Element::Input)?; | |||
let asbd = self.get_property(id, Scope::Output, Element::Input)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per #60 I think Input/Input gives the hardware stream format, not the user requested one.
@@ -398,7 +401,7 @@ impl AudioUnit { | |||
// First, we'll retrieve the stream format so that we can ensure that the given callback | |||
// format matches the audio unit's format. | |||
let id = sys::kAudioUnitProperty_StreamFormat; | |||
let asbd = try!(self.get_property(id, Scope::Output, Element::Output)); | |||
let asbd = try!(self.get_property(id, Scope::Input, Element::Output)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per #60 I think Output/Output gives the hardware stream format, not the user requested one.
cf875a7
to
489d6c3
Compare
1d8a55d
to
5a92876
Compare
@mitchmindtree I think this is ready for first pass of review now. I also updated 3 places where the stream formats are fetched to match comments from @plietar in #67 as I ran into issues writing the feedback example. Using signed integer i16 in the example doesn't work without making those tweaks. I'll need to verify that rodio/cpal still work with these changes. For that I still need to write/productionize the cpal ios coreaudio host. |
.travis.yml
Outdated
@@ -20,6 +20,10 @@ script: | |||
- cargo build --verbose | |||
- cargo test --verbose | |||
- cargo doc --verbose | |||
- cargo build --verbose --target aarch64-apple-ios | |||
- cargo test --verbose --target aarch64-apple-ios |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo test
won't work with different architectures. If you'd really want to run the tests in an iOS simulator, dinghy
is your friend but you'd still only be able to have CI run it with the x86_64-apple-ios
target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've revised the changes, now just doing a build without the test. How do I actually kick off a travis build to test it?
b4721fd
to
f1a26d0
Compare
Now added the iOS Xcode project sample |
f1a26d0
to
4812b0c
Compare
@MichaelHills any specific things to test? I could try testing it on weekend. |
4812b0c
to
a18f7da
Compare
@naithar I suppose testing that mac feedback example still works and the new iOS example works on device + simulator. Plus same thing on my cpal PR RustAudio/cpal#485 If both are good then can work towards getting them released and finally into Bevy. :) |
I gave this a test and found some compilation errors on my end. I had to add the ios example as a new workspace member and then make a I did manage to get the feedback example to run on my iPhone 7. I'm not sure what the example should do but it runs and I see some of the The iOS simulator crashes with this stacktrace:
|
Not sure if I'm doing everything correctly, but testing
|
@simlay it's meant to feedback the microphone with a 1 second delay, so best used with headphones. Try speaking and see if you hear your own voice play back. Not sure why our setups are different and simulator doesn't work. I'll cargo clean and re-test later tonight. What version of Xcode/iOS sim? I'll also setup the bevy ios example to have some audio playback to see if I can reproduce @naithar's issue. |
a18f7da
to
cafb94e
Compare
@simlay your simulator errors look more like some kind of system issue. Can you try the suggestion here? As for the workspace thing, sorry I forgot to git add @naithar I added audio to the bevy ios example project on this branch https://github.com/MichaelHills/bevy/tree/mike-ios-example give that a try? Was your error on device or simulator? |
Storing
and other compile errors. If I remove it there is no real difference between my previous testing implementation. Crash occurs on my device, but the reason for it seems to be in Clamping
Edit: After tweaking bevy some more (switching back to previous
|
@naithar are you using my updated bevy ios example or your own? https://github.com/MichaelHills/bevy/tree/mike-ios-example One issue might be that I've hard-coded the sample rate to 44100 in my cpal PR. I assumed that it should generally work everywhere. Possibly I'm wrong on that, or maybe the default on your device is something other than 44100 and that could be an issue. If we do need a sample rate other than 44100 to get this working, I might need to pull in some |
@MichaelHills your example resulted in first part of the errors
The last log with Edit: Sample rate log: Edit: Crash still happens even after rebasing to newest bevy.
|
The crash with |
Ok, so I've managed to make it work here: https://github.com/naithar/bevy/tree/feature/rodio-0.12, plus tweaked And since bevy has it's own PR for Crash log:
|
@naithar Do you know where the 48000 sample rate is coming from? Is that the default for your device? What phone is it? I hard-coded the cpal branch to 44100 as that seemed to be the default on my iPhone 6 and simulator. I will have an iPhone 11 soon :) and I have a couple of iPads can do more testing on them. Probably I should just set min/max to 8000/48000, and for default sample rate just query it off the audio unit default devices. I think to query the min and max I'd need to set it with Touch crashing is a bit weird, surely that's unrelated? Now that your touch PR has landed, I can update my ios-example branch to include touch and we'll have an all-in-one iOS Bevy example. Once audio works smoothly on all our devices then we're done! |
@MichaelHills no, no idea where
Well, touch also crashes if there is no sound to play. But without |
Ohh so apparently new iphones default to 48KHz based on this comment https://forum.audiob.us/discussion/comment/762478/#Comment_762478 I just checked my cpal branch does set min/max to the device default and 44100 is NOT hard-coded (it used to be earlier on), so that explains why your min/max is 48000. Something "wants" it to be 44100. That must be Bevy or Rodio? Sounds like I need to pull in some @simlay I noticed in that in |
@naithar It was actually my cpal branch defaulting to 44.1KHz. I've pushed an update where it sets the default to match that of your device. I think my ipad uses 44.1 though, might be a couple of weeks before my iPhone 11 arrives which should default to 48KHz. |
Great :) |
@naithar I'm going to assume that the touch issue in Bevy is not related to audio support, and if it is, we can address it in a follow up? If you can post me a Bevy repo/branch I can try it out. @simlay I still don't understand the whole workspace thing and why you need the change but I seemingly don't. In the original change you tested, I had forgotten to git add my If all looks good then I'll do a final round of testing. |
@MichaelHills yeah, I think we can ignore this issue for now. |
cf21764
to
741099d
Compare
In this PR I've tested macOS and the iOS example on multiple devices + simulator (see PR description). I think it's fine to merge as is. Testing from others using Bevy, audio output works on old devices and new once I sorted out the issue with new devices using 48KHz and old devices using 44.1KHz. |
Fixes #67
I started at bevyengine/bevy#87 and ended up here
Some prior discussion in #69
This enables building for iOS
aarch64-apple-ios
andx86_64-apple-ios
and I managed to get audio playback on device and simulator via rodio/cpal. The actual change here affects the mic recording callback so I decided to write a feedback example app for manual testing.feedback.rs
example for macOS to test recording + playback and manually test everything worksManually tested microphone + playback works on iOS
There seem to be some differences between macOS and iOS CoreAudio, in that the iOS can only enable mic recording on an uninitialized AudioUnit. Similarly the iOS one requires an active AVAudioSession to do some operations like reading out the current hardware buffer size.