search

RustAudio / coreaudio-rs

A friendly rust interface to Apple's Core Audio API.
Apache License 2.0
203 stars 38 forks source link

New features, modernize, format #82

Closed HEnquist closed 2 years ago

HEnquist commented 2 years ago

This PR contains a number of things:

There are some minor breaking changes, but mostly it's about added functionality.

It's a work in progress, I'm opening the PR now to get some early feedback. I don't know how to test things on iOS, so I have no idea if this currently works on iOS or not.

MichaelHills commented 2 years ago

Glad to see more contributors. :) I added the iOS support as part of my effort in contributing iOS support to the Bevy game engine so I can help test any changes. It's basically just opening the Xcode project and running the sample on the iOS simulator and/or an iPhone or iPad.

At the time I also noticed all of the open PRs for various issues, but didn't have the energy or time to address them all on top of the iOS changes I was doing. For example there are some issues around calculating buffer sizes properly regarding number of channels and interleaving (forget which issue), but it turns out this wasn't an immediate problem for what I was doing because CPAL was using the API in a way that meant it bypassed the issue. It would be nice to fix all this stuff up. I'll help review when I get a chance, time is scarce with family so no promises when.

HEnquist commented 2 years ago

The issue with broken interleaved mode was one of the reasons I started working on this. I believe I have sorted that out, but it's not very well tested yet. Would be great if someone takes a look at what I have done there!

I realized that it's very easy to run the iOS example, just copy the commands from the GitHub actions config! I have it working now, but needs some cleanup. I have fixed it by simply excluding all the new helper functions from iOS. Some of them are perhaps useful there too, but I think they would need separate implementations. This isn't something I'm planning on working on though, I know too little about iOS for that (and I don't have any iOS devices).

HEnquist commented 2 years ago

I added a few more helpers and did some other changes while working on integrating this in my DSP project. It seems to be working well now. Would appreciate some feedback :)

BTW this PR closes https://github.com/RustAudio/coreaudio-rs/issues/80 and the changes of https://github.com/RustAudio/coreaudio-rs/pull/70 are included.

HEnquist commented 2 years ago

Could someone please approve the workflow to run?

mitchmindtree commented 2 years ago

Hey @RustAudio/admins, if anyone who has access to macOS/iOS has a chance to review/test this awesome effort by @HEnquist that would be a big help!

Engid commented 2 years ago

Thank you, the check passed! I’m on mobile now but I can take a closer look tonight. If anyone else is following that can approve feel free to. Thanks @HEnquist

HEnquist commented 2 years ago

There is one thing I would like to ask about. Now I publish all the helper functions directly in audio_unit. Should they be there, or should I put them in a module of their own (like audio_unit::helpers)? Another option would be to implement them directly on the types, like audio_unit_from_device_id() (https://github.com/HEnquist/coreaudio-rs/blob/master/src/audio_unit/macos_helpers.rs#L82) could be implemented on AudioDeviceID as AudioDeviceID::to_audio_unit()

Opinions?

Engid commented 2 years ago

I'm asking around to see if anyone wants to comment. In the meantime feel free to come join the discord: https://discord.gg/jfXfeXkc

HEnquist commented 2 years ago

Would be great with some opinions on how to organize this stuff! BTW I joined discord but I don't find anything there about coreaudio, where would that be discussed?

HEnquist commented 2 years ago

Does anyone have an opinion on where to put the helper functions?

Engid commented 2 years ago

Hey @HEnquist, sorry no one is getting back to you. I've been a bit busy myself otherwise I would have responded sooner. Yeah long story on that discord's origin, but a bunch of folks in there have the same admin access as I do, so they might be able to help.

Anyway, to give you my humble opinion, I think putting the helper methods in a module might be the best.. but I'm wondering, do you see a need for making an AudioDeviceId type? (I didn't see one so I'm guessing it doesn't exist yet). If so then making a method on that type might be useful, but as far as I know the AudioDeviceId type might not be used much? So maybe put those methods in a audio_unit::macos::helpers module or something might be best, but I'd love to hear what you think!

HEnquist commented 2 years ago

Thanks for looking! I have also been thinking more about how organize it in the best way, and I agree that placing the helpers in their own module makes the most sense. Having them directly in audiounit may be a little confusing.

The AudioDeviceID comes from coreaudio-sys, and is just an alias for uint32. You are right in that it's nearly not used at all in coreaudio-rs. So then it probably doesn't make sense to implement methods on it (which IIUC will then also be implemented for any other uint32). I'll move the helpers to their own module, and keep them as they are otherwise.

HEnquist commented 2 years ago

The helpers are now in coreaudio::audio_unit::macos_helpers, please take a look :)

Engid commented 2 years ago

Awesome! All checks have passed. @mitchmindtree do you want to do any final review?

HEnquist commented 2 years ago

I think I have addressed all the review comments. Could we merge?

mitchmindtree commented 2 years ago

Ahh looks like something might have changed in the iOS CI between the last commit here and merging: https://github.com/RustAudio/coreaudio-rs/runs/4196505208?check_suite_focus=true

 ld: in target/universal/debug/libcoreaudio_ios_example.a(coreaudio_ios_example-95c4b4f56ee852ee.5tete5dijtjrp4r.rcgu.o), building for iOS Simulator, but linking in object file built for iOS, for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

** BUILD FAILED **

The following build commands failed:
    Ld /Users/runner/work/coreaudio-rs/coreaudio-rs/examples/ios/build/Build/Intermediates.noindex/coreaudio-ios-example.build/Debug-iphonesimulator/coreaudio-ios-example.build/Objects-normal/arm64/Binary/coreaudio-ios-example normal arm64
(1 failure)

Anyone have any ideas?

HEnquist commented 2 years ago

Thanks for merging! The default Xcode version on the runners got updated recently, I'm guessing that's the problem. I'll try to look into it.

HEnquist commented 2 years ago

I looked at the iOS ci issue, and quickly came to the conclusion that I don't have a clue.. @MichaelHills ?

MichaelHills commented 2 years ago

The only difference between the passing run and failing run that I can see is that the github actions runner version changed from v2.283.2 to v2.284.0. Both seem to use Xcode 12.4, so no difference there? Doesn't seem to be anything relevant in the github actions runner changelogs.

This answer seems reasonable though https://stackoverflow.com/a/64139830 I'll see if I have time on the weekend to give it a try.

MichaelHills commented 2 years ago

This fixes it... but still don't understand why it started failing https://github.com/RustAudio/coreaudio-rs/pull/84