Prevent data races in napaudio

CasimirGeelhoed commented 2 months ago

These changes prevent data races warnings that were reported by the Thread Sanitizer when running our 4DSOUND software.

Changes in napaudio:

Added atomics to members of LinearSmoothedValue and RampedValue that are often set by non-audio threads.
The LevelMeterNode is rewritten so it doesn't trigger data race warnings, by calculating on the audio thread and storing the result in a single atomic float. To keep the cpu load stable, the algorithms are rewritten so that calculations are happening continuously instead of in chuncks.
InputPinBase::connect(), InputPinBase::disconnect() and InputPinBase::disconnectAll() will now enqueue their connections, so they can be safely called form the main (or in our case: control) thread.

Change in napmath:

Made the std::mt19937 generator in mathutils 'thread_local', so math::random() can safely be called from multiple threads at the same time.

cklosters commented 1 month ago

Thanks for this change, looking good! Unfortunately all builds are currently failing (GCC & MSVC) with the following message, which should be trivial to fix:

[00:02:36]
[Step 1/1] [ 27%] Building CXX object system_modules/napaudio/CMakeFiles/napaudio.dir/src/audio/core/audionode.cpp.o
[00:02:37]
[Step 1/1] In file included from /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napaudio/src/audio/core/audionode.h:16,
[00:02:37]
[Step 1/1]                  from /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napaudio/src/audio/core/audionode.cpp:5:
[00:02:37]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napaudio/src/audio/core/audiopin.h:129:9: error: ‘void nap::audio::InputPin::disconnectAll()’ marked ‘override’, but does not override
[00:02:37]
[Step 1/1]   129 |    void disconnectAll() override;
[00:02:37]
[Step 1/1]       |         ^~~~~~~~~~~~~
[00:02:37]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napaudio/src/audio/core/audiopin.h:134:9: error: ‘bool nap::audio::InputPin::isConnected() const’ marked ‘override’, but does not override
[00:02:37]
[Step 1/1]   134 |    bool isConnected() const override { return mInput != nullptr; }
[00:02:37]
[Step 1/1]       |         ^~~~~~~~~~~
[00:02:37]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napaudio/src/audio/core/audiopin.h:175:9: error: ‘void nap::audio::MultiInputPin::connect(nap::audio::OutputPin&)’ marked ‘override’, but does not override
[00:02:37]
[Step 1/1]   175 |    void connect(OutputPin& input) override;
[00:02:37]
[Step 1/1]       |         ^~~~~~~
[00:02:37]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napaudio/src/audio/core/audiopin.h:181:9: error: ‘void nap::audio::MultiInputPin::disconnect(nap::audio::OutputPin&)’ marked ‘override’, but does not override
[00:02:37]
[Step 1/1]   181 |    void disconnect(OutputPin& input) override;
[00:02:37]
[Step 1/1]       |         ^~~~~~~~~~
[00:02:37]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napaudio/src/audio/core/audiopin.h:198:9: error: ‘void nap::audio::MultiInputPin::disconnectAll()’ marked ‘override’, but does not override
[00:02:37]
[Step 1/1]   198 |    void disconnectAll() override;
[00:02:37]
[Step 1/1]       |         ^~~~~~~~~~~~~
[00:02:37]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napaudio/src/audio/core/audiopin.h:203:9: error: ‘bool nap::audio::MultiInputPin::isConnected() const’ marked ‘override’, but does not override
[00:02:37]
[Step 1/1]   203 |    bool isConnected() const override { return !mInputs.empty(); }
[00:02:37]
[Step 1/1]       |         ^~~~~~~~~~~
[00:02:38]
[Step 1/1] make[2]: *** [system_modules/napaudio/CMakeFiles/napaudio.dir/build.make:90: system_modules/napaudio/CMakeFiles/napaudio.dir/src/audio/core/audionode.cpp.o] Error 1
[00:02:38]
[Step 1/1] make[1]: *** [CMakeFiles/Makefile2:4006: system_modules/napaudio/CMakeFiles/napaudio.dir/all] Error 2
[00:02:38]
[Step 1/1] make[1]: *** Waiting for unfinished jobs....

CasimirGeelhoed commented 1 month ago

Sorry, something went wrong merging audiopin.h during the git rebase, it should be fixed now.

There is one more change I should note: we decided to remove the InputPinBase::isConnected() method, as its result would be unpredictable when (dis)connections are enqueued on the audio thread (for example, calling isConnected() directly after calling disconnectAll() on the main thread could still return true). This is a potentially breaking change.

cklosters commented 1 month ago

Build keeps failing unfortunately, I recommend building your changes on a linux / windows machine to validate your changes:

[01:05:38]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napsequenceaudio/src/sequenceplayeraudiooutput.cpp: In member function ‘virtual bool nap::SequencePlayerAudioOutput::init(nap::utility::ErrorState&)’:
[01:05:38]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napsequenceaudio/src/sequenceplayeraudiooutput.cpp:57:41: error: ‘class nap::audio::InputPin’ has no member named ‘enqueueConnect’
[01:05:38]
[Step 1/1]    57 |                 output_node->audioInput.enqueueConnect(mix_nodes[i]->audioOutput);
[01:05:38]
[Step 1/1]       |                                         ^~~~~~~~~~~~~~
[01:05:38]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napsequenceaudio/src/sequenceplayeraudiooutput.cpp: In member function ‘void nap::SequencePlayerAudioOutput::registerAdapter(const nap::SequencePlayerAudioAdapter*)’:
[01:05:38]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napsequenceaudio/src/sequenceplayeraudiooutput.cpp:119:38: error: ‘class nap::audio::MultiInputPin’ has no member named ‘enqueueConnect’
[01:05:38]
[Step 1/1]   119 |                 mMixNodes[i]->inputs.enqueueConnect(*buffer_player->getOutputPins()[i]);
[01:05:38]
[Step 1/1]       |                                      ^~~~~~~~~~~~~~
[01:05:38]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napsequenceaudio/src/sequenceplayeraudiooutput.cpp: In member function ‘void nap::SequencePlayerAudioOutput::unregisterAdapter(const nap::SequencePlayerAudioAdapter*)’:
[01:05:38]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napsequenceaudio/src/sequenceplayeraudiooutput.cpp:149:42: error: ‘class nap::audio::MultiInputPin’ has no member named ‘enqueueDisconnect’
[01:05:38]
[Step 1/1]   149 |                     mMixNodes[i]->inputs.enqueueDisconnect(*output_pins[i]);
[01:05:38]
[Step 1/1]       |                                          ^~~~~~~~~~~~~~~~~
[01:05:38]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napsequenceaudio/src/sequenceplayeraudiooutput.cpp: In member function ‘void nap::SequencePlayerAudioOutput::connectInputPin(nap::audio::InputPin&, int)’:
[01:05:38]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napsequenceaudio/src/sequenceplayeraudiooutput.cpp:173:18: error: ‘class nap::audio::InputPin’ has no member named ‘enqueueConnect’
[01:05:38]
[Step 1/1]   173 |         inputPin.enqueueConnect(mMixNodes[channel]->audioOutput);
[01:05:38]
[Step 1/1]       |                  ^~~~~~~~~~~~~~
[01:05:38]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napsequenceaudio/src/sequenceplayeraudiooutput.cpp: In member function ‘void nap::SequencePlayerAudioOutput::disconnectInputPin(nap::audio::InputPin&, int)’:
[01:05:38]
[Step 1/1] /home/cklosters/agent/work/a62e0057f2f90802/nap/system_modules/napsequenceaudio/src/sequenceplayeraudiooutput.cpp:180:18: error: ‘class nap::audio::InputPin’ has no member named ‘enqueueDisconnect’
[01:05:38]
[Step 1/1]   180 |         inputPin.enqueueDisconnect(mMixNodes[channel]->audioOutput);
[01:05:38]
[Step 1/1]       |                  ^~~~~~~~~~~~~~~~~

cklosters commented 1 month ago

It has been 2 weeks and no changes - the build is still broken. Do you expect to fix the builld for this review to continue or should I close it?

Quick note on our review policy: We welcome changes but the changes must pass the build checks in order for them to be reviewed in the first place, that's what the build system is for: it ensures your code doesn't break core functionality, which it does. Simply letting the PR hang isn't very good practice either: close it if you don't wish to continue or notify us when you do.

If no changes are submitted and @stijnvanbeek hasn't reviewed this change in the next two weeks I will close this PR - feel free to open a new one when ready.

stijnvanbeek commented 1 month ago

Will look at it this week!

CasimirGeelhoed commented 1 month ago

It has been 2 weeks and no changes - the build is still broken. Do you expect to fix the builld for this review to continue or should I close it?

Quick note on our review policy: We welcome changes but the changes must pass the build checks in order for them to be reviewed in the first place, that's what the build system is for: it ensures your code doesn't break core functionality, which it does. Simply letting the PR hang isn't very good practice either: close it if you don't wish to continue or notify us when you do.

If no changes are submitted and @stijnvanbeek hasn't reviewed this change in the next two weeks I will close this PR - feel free to open a new one when ready.

Hey Coen, of course, totally understand this policy. Apologies for the delay & silence, @stijnvanbeek will review it this week. For next time, I'll make sure I can test all targets on Windows myself before making a PR.

cklosters commented 1 month ago

Hey Coen, of course, totally understand this policy. Apologies for the delay & silence, @stijnvanbeek will review it this week. For next time, I'll make sure I can test all targets on Windows myself before making a PR.

Thanks for taking the time to fix the build, all checks pass including package validation (where all demos are tested in every environment). So probably good if @stijnvanbeek reviews this code, I'll also take a look at it after he is done, merci!

stijnvanbeek commented 1 month ago

I understand the use of the sum buffer now, so all great to me.

stijnvanbeek commented 1 month ago

Only thing important to note is to mention the breaking changes:

enqueConnect() and enqueDisconnect() no loinger exist and need to be replaced by just connect() and disconnect()
the isConnected() function is removed as the result is unreliable now in a multithreading context and its existence is unnecessary because users can keep track of this themselves.

napframework / nap

Prevent data races in napaudio #31