Refactor/communication between audio and js threads #864
Conversation
packages/react-native-audio-api/common/cpp/audioapi/core/utils/AudioEventScheduler.h
Outdated
Show resolved
Hide resolved
packages/react-native-audio-api/common/cpp/audioapi/core/AudioNode.cpp
Outdated
Show resolved
Hide resolved
...t-native-audio-api/common/cpp/audioapi/HostObjects/sources/AudioBufferSourceNodeHostObject.h
Show resolved
Hide resolved
| bool AudioEventScheduler::scheduleEvent(std::function<void(BaseAudioContext &)> &&event) noexcept { | ||
| if (context_->isRunning()) { | ||
| return eventScheduler_.scheduleEvent(std::move(event)); | ||
| } else { | ||
| event(*context_); | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
It should be fine but one thing is missing. We drain the queue at the start of renderAudio callback so here look at this sequence
TA - audio thread
TJ - JS thread
- TA - drains the queue
- TJ - pushes E1 to queue (ctx running so pushes to queue)
- TJ - stops context
- TA - processes last frame
- TJ - pushes E2 to queue (ctx not running imediately executes)
We get inconsistent state because our operations are not executed in proper order that depends on the order of their schedule. So E1 should be executed before E2 for example these two events might depend on each other like connect disconnect or something like allowing these two things is not alright or even worse we can get race condition if last frames is processed after the flag is set to true
And draining queue on stop can cause a race cond on the other hand. So this is major problem that needs to be solved.
| } | ||
|
|
||
| bool AudioPlayer::start() { | ||
| if (mStream_) { |
There was a problem hiding this comment.
we can get rid of all implicit stream to bool casts in this pr
| for (int i = 0; i < framesToProcess; i++) { | ||
| for (int channel = 0; channel < channelCount_; channel += 1) { |
There was a problem hiding this comment.
| for (int i = 0; i < framesToProcess; i++) { | |
| for (int channel = 0; channel < channelCount_; channel += 1) { | |
| for (int i = 0; i < framesToProcess; i++) { | |
| for (int channel = 0; channel < channelCount_; channel++) { |
consistency
| audioBufferSourceNode->setLoop(value.getBool()); | ||
| auto loop = value.getBool(); | ||
|
|
||
| auto event = [audioBufferSourceNode, loop](BaseAudioContext &context) { |
There was a problem hiding this comment.
do we need reference to context here and in all of the lambdas that they do not use context inside it? if not change sfinae definition of a function so more types can be passed in event scheduler
| : nodeManager_(std::make_shared<AudioNodeManager>()), | ||
| : sampleRate_ {sampleRate}, | ||
| nodeManager_(std::make_shared<AudioNodeManager>()), | ||
| audioEventScheduler_(std::make_unique<CrossThreadEventScheduler<BaseAudioContext>>(1024)), |
There was a problem hiding this comment.
magic number, put it in some constant
Closes #
Introduced changes
CrossThreadEventSchedulerwrapper to be executed on Audio thread.HostObjectplays aShadowAudioNoderole - it stores a copy of node's state.Read: Simply read
HostObjectstateWrite: Schedule modification and modify
Shadow StateLock free approach!!!
TODO:
Checklist