Refactor/communication between audio and js thread#942
Refactor/communication between audio and js thread#942maciejmakowski2003 wants to merge 65 commits intomainfrom
Conversation
There was a problem hiding this comment.
It would be nice to update this doc in future and include few repeating solutions for most common problems that are used in our codebase aka:
- prealocating
- SPSC with worker thread
- how to communicate Audio <-> JSI
Also it is worth noting that we do not block syscalls or allocations on audio thread it is just strongly unrecommended to do but in case if someone is crafting custom node processor for his custom needs it might work fine
| /// @note Thread safe, because does not access state of the node. | ||
| void unregisterOnBufferEndedCallback(uint64_t callbackId); |
There was a problem hiding this comment.
it does not but might be inheritly unsafe through registry handler
| @@ -50,7 +50,7 @@ | |||
|
|
|||
| if (isConnected()) { | |||
| if (auto lock = Locker::tryLock(adapterNodeMutex_)) { | |||
| /// @note Thread safe, because does not access state of the node. | ||
| void unregisterOnBufferEndedCallback(uint64_t callbackId); |
There was a problem hiding this comment.
its thread safety depends on the thread safety of eventRegistryHandle. Besides these methods do not need to be thread safe as they are used on AudioThread through scheduled async events
| std::shared_ptr<IOSAudioPlayer> audioPlayer_; | ||
| #endif | ||
| bool isInitialized_; | ||
| std::atomic<bool> isInitialized_ = false; |
There was a problem hiding this comment.
it might be hard to swallow but i doubt it should be atomic as for node to be connected to graph thus processed it needs to be sent through spsc ... sooo its creation is done on the JS thread
| audioEventScheduler_( | ||
| std::make_unique<CrossThreadEventScheduler<BaseAudioContext>>(AUDIO_SCHEDULER_CAPACITY)) { | ||
| } |
There was a problem hiding this comment.
does it need to be unique_ptr?
There was a problem hiding this comment.
looks really good but few neatpicks and overall good job but write tests for it
There was a problem hiding this comment.
and i am kind of against puting something like this in such a big PR, cuz whole PR LGTM besides this part
| T buffers_[3]; | ||
| int frontIndex_ = 0; | ||
| std::atomic<State> state_{{1, false}}; | ||
| int backIndex_ = 2; |
There was a problem hiding this comment.
False sharing occurs here at alignas as in SPSC to prevent it. Also it might be better to use size_t instead of int but that is minor thing
| bool hasUpdate; | ||
| }; | ||
|
|
||
| T buffers_[3]; |
There was a problem hiding this comment.
buffers should also be alligned to cache line sizes, and u can maybe enchance it to use emplace new operator to allow T types which are not defaultly constructible
| struct State { | ||
| int index; | ||
| bool hasUpdate; | ||
| }; |
There was a problem hiding this comment.
use bitfields to fit this in 32 bits
| T *getForWriter() { | ||
| return &buffers_[backIndex_]; | ||
| } | ||
|
|
||
| void publish() { | ||
| State newState{backIndex_, true}; | ||
| auto prevState = state_.exchange(newState, std::memory_order_acq_rel); | ||
| backIndex_ = prevState.index; | ||
| } |
There was a problem hiding this comment.
it might have interface read(), write() instead of this it would be better for RAII i guess as u never forget to publish
mdydek
left a comment
mdydek
left a comment
There was a problem hiding this comment.
make sure to align with current formatter, because some files are wrongly formatted and git diff is going crazy
| @@ -0,0 +1,280 @@ | |||
| # How to create new AudioNode | |||
|
|
|||
| In this docs we present recommended design pattern for creating new AudioNode. | |||
There was a problem hiding this comment.
| In this docs we present recommended design pattern for creating new AudioNode. | |
| In this docs we present recommended patterns for creating new AudioNodes. |
|
|
||
| ## Layers | ||
|
|
||
| In common each AudioNode has three layers: |
There was a problem hiding this comment.
| In common each AudioNode has three layers: | |
| Usually, each AudioNode has three layers: |
| readonly gain: AudioParam; | ||
|
|
||
| constructor(context: BaseAudioContext, options?: TGainOptions) { | ||
| const gainNode: IGainNode = context.context.createGain(options || {}); |
There was a problem hiding this comment.
emphasize that context.context is underlying c++ object
| ``` | ||
| 1. **function** |
There was a problem hiding this comment.
| 1. **function** | |
| 3. **function** |
| ``` | ||
|
|
||
| ```cpp | ||
| SI_HOST_FUNCTION_IMPL(AnalyserNodeHostObject, getByteFrequencyData) { |
There was a problem hiding this comment.
| SI_HOST_FUNCTION_IMPL(AnalyserNodeHostObject, getByteFrequencyData) { | |
| JSI_HOST_FUNCTION_IMPL(AnalyserNodeHostObject, getByteFrequencyData) { |
| minDecibels_.store(minDecibels, std::memory_order_release); | ||
| } | ||
|
|
||
| void AnalyserNode::setMaxDecibels(float maxDecibels) { | ||
| maxDecibels_ = maxDecibels; | ||
| maxDecibels_.store(maxDecibels, std::memory_order_release); |
There was a problem hiding this comment.
I don't see where minDecibels_ and maxDecibels_ can be accessed outside of js thread, thus there is no necessity for atomic variables
| float getContextSampleRate() const { | ||
| if (std::shared_ptr<BaseAudioContext> context = context_.lock()) { | ||
| return context->getSampleRate(); | ||
| } | ||
|
|
||
| return DEFAULT_SAMPLE_RATE; | ||
| } |
There was a problem hiding this comment.
I don't like this idea, it can be very misleading since system can output correct amount even though context is not valid
| static FilterCoefficients | ||
| setNormalizedCoefficients(float b0, float b1, float b2, float a0, float a1, float a2); | ||
| FilterCoefficients | ||
| applyFilter(float frequency, float Q, float gain, float detune, BiquadFilterType type); |
There was a problem hiding this comment.
do we need type as parameter? In all use cases it is property of a class, so we can retrieve its value inside a function
| template <typename F> | ||
| bool inline scheduleAudioEvent(F &&event) noexcept { |
There was a problem hiding this comment.
add more constraints what can and what cannot be passed as an event, right now we can pass this event
auto event = []() {
return;
};but it will fail, because context will call this event like that
event(*this);| createAnalyser: (analyserOptions: AnalyserOptions) => IAnalyserNode; | ||
| createConvolver: (convolverOptions?: IConvolverOptions) => IConvolverNode; | ||
| createStreamer: (streamerOptions?: StreamerOptions) => IStreamerNode | null; // null when FFmpeg is not enabled | ||
| createStreamer: (streamerOptions: StreamerOptions) => IStreamerNode | null; // null when FFmpeg is not enabled |
3 participants
Closes RNAA-311
Closes RNAA-250
Closes RNAA-410
Part of RNAA-433
initializemethod fromStreamerNodewith passing url via ctor or factory method.AnalyserNodeIntroduced changes
CrossThreadEventSchedulerwrapper to be executed on Audio thread.HostObjectplays aShadowAudioNoderole - it stores a copy of node's state. There are some exceptions like using atomics ot triple buffer pattern.Checklist