RTCAudioSource - audio only sent to one Peer.

[havardholvik]

We have successfully integrated this library in our application, and currently use it to bridge PSTN phone calls to our video service. For this to work, we rely on the nodejs process being able to send audio to remote participants.

However, we stumbled upon an issue when joining p2p calls with more than one remote participant (SFU calls using Mediasoup works fine). Whenever the 2nd remote participant joins, audio is only sent from the Nodejs process to the last participant.

Looking at existing issues, seems like node-webrtc#745 is quite similar, although in our implementation, we only create a single local audio track.

Diving into the code, it seems like the RTCAudioSource implementation is a bit strange. Internally, the audio source only keeps track of one single _sink. Wouldnt this break if there are more than one sink, eg multiple RTP senders?

Im hoping to confirm this with a local fix and build, but thought Id share in advance.

[duvallj]

Hi @havardholvik! Thanks for bringing this up, that does sound pretty non-ideal, and it also seems like an issue inherited from node-webrtc#745 when we forked it.

I will not deny the RTCAudioSource (and the rest of the nonstandard APIs) are pretty strange :) I'd hoped to fix this at some point, but work hasn't allocated me any time to do deep-dives on this. If you find out a solution, that'd be great, happy to accept contributions! Let me know if you have any questions about the build process, it can be tricky at times.

[havardholvik]

So I implemented a change in the RTCAudioSource header and can verify my case of sending audio from node to multiple peers now work. Here is the patch

diff --git a/src/interfaces/rtc_audio_source.h b/src/interfaces/rtc_audio_source.h
index e8038c9..129d495 100644
--- a/src/interfaces/rtc_audio_source.h
+++ b/src/interfaces/rtc_audio_source.h
@@ -27,6 +27,7 @@ public:
   ~RTCAudioTrackSource() override {
     PeerConnectionFactory::Release();
     _factory = nullptr;
+    _sinks.clear();
   }

   SourceState state() const override {
@@ -36,25 +37,33 @@ public:
   bool remote() const override { return false; }

   void PushData(RTCOnDataEventDict dict) {
-    webrtc::AudioTrackSinkInterface *sink = _sink;
-    if (sink && dict.numberOfFrames.IsJust()) {
-      sink->OnData(dict.samples, dict.bitsPerSample, dict.sampleRate,
+    if (dict.numberOfFrames.IsJust()) {
+      for (auto sink : _sinks) {
+        sink->OnData(dict.samples, dict.bitsPerSample, dict.sampleRate,
                    dict.channelCount, dict.numberOfFrames.UnsafeFromJust());
+      }
     }
+
     // HACK(mroberts): I'd rather we use a smart pointer.
     delete[] dict.samples;
   }

-  void AddSink(webrtc::AudioTrackSinkInterface *sink) override { _sink = sink; }
+  void AddSink(webrtc::AudioTrackSinkInterface *sink) override {
+    _sinks.push_back(sink);
+  }
+
+  void RemoveSink(webrtc::AudioTrackSinkInterface *sink) override {
+    auto it = std::find(_sinks.begin(), _sinks.end(), sink);

-  void RemoveSink(webrtc::AudioTrackSinkInterface *) override {
-    _sink = nullptr;
+    if (it != _sinks.end()) {
+        _sinks.erase(it);
+    }
   }

 private:
   PeerConnectionFactory *_factory = PeerConnectionFactory::GetOrCreateDefault();

-  std::atomic<webrtc::AudioTrackSinkInterface *> _sink = {nullptr};
+  std::vector<webrtc::AudioTrackSinkInterface *> _sinks;
 };

 class RTCAudioSource : public Napi::ObjectWrap<RTCAudioSource> {

Is it fine if I open a PR for this? Unsure if there are any tests for this functionality (should be really). I also had to apply these changes to 6d2395f (commit from your last binary build), as I got some exceptions using latest develop branch. Ill register a separate issue on this.

[duvallj]

Definitely to open a PR for this and another issue for the exceptions! I will note now, that changing from std::atomic to std::vector may be problematic, I think we'll want a std::mutex<std::vector<webrtc::AudioTrackSinkInterfact *>> instead

[havardholvik]

I opened a PR now, lets discuss the implementation details from there 👍

[havardholvik]

I see you just pushed the same(ish) changes right before I opened the PR 🙈