[expo-av][ios] Implement JSI Audio on iOS

[barthap]

Why

Part 1 of implementing the real-time audio streaming.

Implements the functionality of #13516 on iOS.

How

• Added .mm file extension and the ReactCommon dependency to podspec
• Modified EXAV module to be able to access the bridge - required for accessing the jsCallInvoker
• Audio samples are read from AudioMix using the MusicToolbox framework (MTAudioTap*)

  Note: this framework is not maintained since 2015 (no docs on website, only comment docs), but it's not a private API so it shouldn't be rejected by app store review.

• The audio sample callback is registered using JSI
• I needed to use CallbackWrapper and JS call invoker, because the code from the original PR worked on JSC, but crashed on Hermes here.
  • I got inspired by this function from RN
  • I am not sure if I'm not causing any memory leaks/race conditions. Honestly, I'm a bit confused with mixing smart pointers and ARC. Any tips/suggestions are welcome!
• Needed to copy some code from ReactCommon until we upgrade to RN 0.66
• The EXAVPlayerData now holds a reference to EXAudioSampleCallback which is an Obj-C holder for a C++ class AudioSampleCallbackWrapper. This class is responsible for calling and memory management of the JSI callbacks.

Test Plan

Tested manually in bare-expo NCL.
Will write tests/update NCL in a further PR (probably stacked).

• Works in Bare Expo (device + simulator)
• Works in Expo Go
• Works on JSC
• Works on Hermes

Checklist

• Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md).

  Will update it along with the changelog, when both iOS and Android are done.

• This diff will work correctly for expo build (eg: updated @expo/xdl).
• This diff will work correctly for expo prebuild & EAS Build (eg: updated a module plugin).

[barthap]

The buffer data pointer here is passed between threads - should I protect it somehow?
It is always created in one (audio) thread and read in the JS thread only, but a race condition might happen when JS is too slow with reading it, and updates are pretty frequent.

[wkozyra95]

it depends on underlying implementation if AudioBuffer might be reused after callback finishes or memory is freed it might fail. if that is the case you should copy all necessary data before switching to js thread or call that code in jsthread while still blocking current thread.

If callbacks are called to often we might need to handle it somehow, but it depends on what are they used for, if this is some headless processing then we need to have a way to slow down how often addSampleBufferCallback is called, if this some sort of live source we would need drop frames if can't keep up

just guessing I have no idea how this source works

[tsapeta]

A few comments in the first iteration 👇 I don't have any to JSI code, so good job! 👍 I hope Wojtek will have 😄
Can you also dispatch the versioning workflow to check if it compiles? Or test it locally. I guess some of the C++ stuff should be wrapped in expo namespace, but I'm not sure.

[wkozyra95]

it depends on underlying implementation if AudioBuffer might be reused after callback finishes or memory is freed it might fail. if that is the case you should copy all necessary data before switching to js thread or call that code in jsthread while still blocking current thread.

If callbacks are called to often we might need to handle it somehow, but it depends on what are they used for, if this is some headless processing then we need to have a way to slow down how often addSampleBufferCallback is called, if this some sort of live source we would need drop frames if can't keep up

just guessing I have no idea how this source works

[wkozyra95]

I don't have strong opinions on that, but maybe consider creating some global object for expo-av and attaching this method and any potential future methods to that object instead of directly to global

[wkozyra95]

I guess some of the C++ stuff should be wrapped in expo namespace, but I'm not sure.

I'm not sure what would be the point of wrapping that code in the namespace, it might be useful if

• you are planning to incorporate those namespaces into versioning
• if the entire module would be in the same namespace(I assume it's not possible without converting the whole module to objc++)
• if well-defined part of the module was fully wrapped in the namespace and expose a minimal interface

otherwise doing that for just a single file does not really help with anything, but also there is no harm in adding that

[tsapeta]

@wkozyra95 Yeah, I was mostly thinking about versioning. Namespacing helps a lot in that, but on the other side if those things are not exposed in headers then it shouldn't cause any issues with versioning (I guess?).

[Kudo]

we can move the following !buffer or !reinterpret_cast<float *>(buffer->mData) here as earlier check. that will reduce unnecessary js thread tasks.

[barthap]

I think these checks have no purpose here, because buffer is created on this thread (this is audio tap thread) and it always exist here.

I added these checks to the JS thread, because I believe there may be some cases, when that JS-thread code gets called too late and something bad happens to the buffer and is no longer available.

This is the case that @wkozyra95 commented above. I think we can either copy the buffer and pass it to the JS thread or just skip if it's no longer available (like I do now).

[Kudo]

since LongLivedObjectCollection itself is a singleton, recommend not to keep it as a member here. just use LongLivedObjectCollection.get().doSomething()

[barthap]

I know it's a singleton. The problem was more complex here and I deliberately created a separate object here.

• Before RN 0.66 the TurboModules were using this singleton e.g. for promise callbacks.

• In RN 0.66 they replaced the singleton with separate instance: facebook/react-native@32bfd7a - that's why I copied some of the RN code here.

• If I was using the singleton, the collection would be shared with non expo-av places, e.g. RN internals and I don't want my code to affect them.

• The main problem is the global singleton collection is never cleared in main RN code - so the JSC complained about dangling objects (similar issue: Reloading Via React Native Debug Menu Causes Crash when Realm is installed software-mansion/react-native-reanimated#1424). So I want only to clear my own collection when jsc::Runtime is destroyed to avoid potential conflicts with recently quick evolution of RN code.

  Edit: according to the commit description above, the collection is actually cleared somewhere, however only in hidden fb-internal repo, but nowhere in public code ¯_(ツ)_/¯

[Kudo]

got it. thanks for your explicitly explanation 🔥

[bbarthec]

I'm more than happy that you're drilling through this module with such confidence! 👏
Left some minor remarks 👇

[bbarthec]

Is it possible to create EX_WEAKIFY/EX_STRONGIFY/EX_ENSURE_STRONGIFY_WITH_ERROR macros for c++? 🤔

[barthap]

I think it's not worth it:

Taking a look at the EX_WEAKIFY macro:

#define EX_WEAKIFY(var) __weak __typeof(var) EXWeak_##var = var;

I'd have to add 2 additional lines just to make them weak (instead of one __weak keyword):

auto weakCallInvoker = self.bridge.jsCallInvoker;
auto weakSoundDictionary = soundDictionary;
EX_WEAKIFY(weakCallInvoker);
EX_WEAKIFY(weakSoundDictionary);

---

Also for EX_ENSURE_STRONGIFY, that would not let me throw the jsi::JSError if they couldn't be strongified, because of the macro:

#define EX_STRONGIFY(var) __strong __typeof(var) var = EXWeak_##var; 

#define EX_ENSURE_STRONGIFY(var) \
EX_STRONGIFY(var); \
if (var == nil) { return; }

[expo-bot]

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing changelog entries

Your changes should be noted in the changelog. Read Updating Changelogs guide and consider (it's optional) adding an appropriate entry to the following changelogs:

• packages/expo-av/CHANGELOG.md

---

Generated by ExpoBot 🤖 against 237591e

[wkozyra95]

Suggested change

	- (id)initWithCallbackWrapper:(std::unique_ptr<AudioSampleCallbackWrapper>)wrapper {
	- (id)initWithCallbackWrapper:(std::unique_ptr<AudioSampleCallbackWrapper> &&)wrapper {

does not really matter with unique ptr but you should probably use rvalue here
note: I don't know if above suggestion is correct syntax for rvalue in objc++

[barthap]

it compiles so probably that's the right syntax, thanks.

But according to this answer, the preferred way is to pass that unique_ptr by value: https://stackoverflow.com/a/8114913

(A) By Value: If you mean for a function to claim ownership of a unique_ptr, take it by value.
(...)
(D) By r-value reference: If a function may or may not claim ownership (depending on internal code paths), then take it by &&. But I strongly advise against doing this whenever possible.

[mrousavy]

cool!

[tsheaff]

@barthap @mrousavy my understanding of the initial proposal was to allow for access to the raw audio sample data in realtime for audio playback or for a recording. This PR, to my read on the interface, only works on the Audio.Sound object, which is not available on a Audio.Recording object until after the recording completes. When I try to explicitly create an unplayed Audio.Sound object from the uri on the recording, it hangs on the await Audio.Sound.createAsync(...) call, never fulfilling. This means I can't access setOnAudioSampleReceived for a recording.

Am I understanding this interface correctly, and that there's still no way to access the sample in real time for a recording? I'd like to animate the waveform loudness to give the user feedback that they're being recorded, similar to the use-case described in the initial proposal #13404

[barthap]

That's right. There was planned support for recordings too, sound playback was supposed to be only the first step/part of this feature. Unfortunately, I don't have time for implementing it further. Community PRs are welcome, though.

[mrousavy]

yea I remember having the recording part working as well back then.