Prevent data races in napaudio #31

CasimirGeelhoed · 2024-07-09T10:54:47Z

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 · 2024-07-10T11:25:43Z

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 · 2024-07-10T16:35:07Z

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 · 2024-07-11T08:57:55Z

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 · 2024-07-22T12:28:50Z

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 · 2024-07-22T12:45:45Z

Will look at it this week!

CasimirGeelhoed · 2024-07-22T13:37:49Z

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 · 2024-07-24T08:56:03Z

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!

system_modules/napaudio/src/audio/node/levelmeternode.cpp

stijnvanbeek

I think the sum buffer in the LevelMeterNode can be refactored away.
For the rest all good!

I was wrooooong :)

stijnvanbeek · 2024-07-25T11:18:40Z

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

stijnvanbeek · 2024-07-25T12:35:23Z

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.

cklosters

Good change, thanks & congrats on your new contributor status ;) See comments for future PRs

cklosters · 2024-07-30T10:11:43Z

system_modules/napaudio/src/audio/node/levelmeternode.cpp

+					for (auto& sample : *inputBuffer)
+					{
+						// keep track of peak, sample by sample
+						float newValue = sample;


For future reference, local variables are never camelCase, they are separated using an underscore: float new_value , this allows you to differentiate between arguments and local variables, which is currently not the case. This is part of the official style guide.

cklosters · 2024-07-30T10:23:18Z

system_modules/napmath/src/mathutils.cpp

@@ -27,7 +27,7 @@ namespace nap

 		static std::mt19937& getGenerator()
 		{
-			static std::unique_ptr<std::mt19937> generator = nullptr;
+			static thread_local std::unique_ptr<std::mt19937> generator = nullptr;


I guess this is ok, assuming we want different random sequences for every thread. I can't think of a situation where this would be a problem atm, but considering the current implementation is not thread safe this solutions outweighs the other.

Interestingly, declaring something static in the cpp (such as the generator) is thread safe since C++ 11. Also, looking at the implementation it can be simplified to:

static std::mt19937& getGenerator() { static thread_local std::mt19937 generator(time(0)); return generator; }

I'll implement and test that some other time.

CasimirGeelhoed and others added 10 commits July 3, 2024 17:10

implement enqueueDisconnectAll

f0fd915

use atomics

438c0f4

make generator thread_local to prevent data races

7f78ad7

enqueue connection by default

608c4b6

use private (dis)connectNow() functions

9049909

safe LevelMeterNode with more constant cpu load

c1a4cb4

Whitespace fixes

f424b1f

whitespace fixes, add 'break'

721a0b7

Code style and no abs() LevelMeterNode

d30f7b6

add store() and load() calls

3f871c1

CasimirGeelhoed added 5 commits July 10, 2024 15:22

merging fix: keep virtual method, remove unimplemented functions

5bbfce1

merging fix: remove method definitions

e6575db

revert CMakeLists.txt change

1f79310

remove whitespace

0860663

comments improvements

ab4ee88

cklosters requested review from stijnvanbeek and cklosters July 10, 2024 14:28

fix calls to renamed functions

8d85052

Merge branch 'main' into data-races-to-parent

300c165

clean InputPin destructor

f5f6328

stijnvanbeek reviewed Jul 24, 2024

View reviewed changes

system_modules/napaudio/src/audio/node/levelmeternode.cpp Show resolved Hide resolved

stijnvanbeek previously requested changes Jul 24, 2024

View reviewed changes

cklosters added enhancement New feature or request audio Audio related Questions & Issues labels Jul 29, 2024

cklosters approved these changes Jul 30, 2024

View reviewed changes

cklosters merged commit 94321e6 into napframework:main Jul 30, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent data races in napaudio #31

Prevent data races in napaudio #31

CasimirGeelhoed commented Jul 9, 2024 •

edited

Loading

cklosters commented Jul 10, 2024 •

edited

Loading

CasimirGeelhoed commented Jul 10, 2024 •

edited

Loading

cklosters commented Jul 11, 2024

cklosters commented Jul 22, 2024

stijnvanbeek commented Jul 22, 2024

CasimirGeelhoed commented Jul 22, 2024

cklosters commented Jul 24, 2024

stijnvanbeek left a comment •

edited

Loading

stijnvanbeek commented Jul 25, 2024

stijnvanbeek commented Jul 25, 2024

cklosters left a comment •

edited

Loading

cklosters Jul 30, 2024

cklosters Jul 30, 2024

cklosters Jul 30, 2024

Prevent data races in napaudio #31

Prevent data races in napaudio #31

Conversation

CasimirGeelhoed commented Jul 9, 2024 • edited Loading

cklosters commented Jul 10, 2024 • edited Loading

CasimirGeelhoed commented Jul 10, 2024 • edited Loading

cklosters commented Jul 11, 2024

cklosters commented Jul 22, 2024

stijnvanbeek commented Jul 22, 2024

CasimirGeelhoed commented Jul 22, 2024

cklosters commented Jul 24, 2024

stijnvanbeek left a comment • edited Loading

Choose a reason for hiding this comment

stijnvanbeek commented Jul 25, 2024

stijnvanbeek commented Jul 25, 2024

cklosters left a comment • edited Loading

Choose a reason for hiding this comment

cklosters Jul 30, 2024

Choose a reason for hiding this comment

cklosters Jul 30, 2024

Choose a reason for hiding this comment

cklosters Jul 30, 2024

Choose a reason for hiding this comment

CasimirGeelhoed commented Jul 9, 2024 •

edited

Loading

cklosters commented Jul 10, 2024 •

edited

Loading

CasimirGeelhoed commented Jul 10, 2024 •

edited

Loading

stijnvanbeek left a comment •

edited

Loading

cklosters left a comment •

edited

Loading