How is the thread safety for the audio mixer ?

I feel like they are not safe but my limited rust knowledge is not enough to get deep into it.

@ladvoc, could you please help review the code and look at the thread safety ?

Great question. I merely rebased this code and did not write it, so I was not very familiar before you asked. The thread safety of livekit::AudioMixer on the C++ side is not good. livekit::AudioMixer stores its own std::vector<std::shared_ptr<AudioMixerSource>> sources_ member. I don't see the reason for this. libwebrtc's webrtc::AudioMixerImpl stores its own list of sources internally which are guarded by a mutex. So livekit::AudioMixer seems to add a redundant list on top of that but without the thread safety, despite the manual impl's for Send + Sync traits.

This seems to not have been a practical issue for Zed because Zed wraps its AudioMixer with Arc<Mutex> on the Rust side. However, just looking at the Rust API exposed by the libwebrtc crate here, there is no reason for anyone who didn't write this code to suspect that Arc<Mutex> is necessary without documentation. There are also no SAFETY comments in the higher level Rust wrappers in the libwebrtc crate for the unsafe blocks documenting why it is safe to call the unsafe lower level functions.

From a quick look, I think there are several other parts of these new APIs that are redundant or overcomplicated.

I will talk with @ConradIrwin about how to proceed here. We have a pair coding call scheduled for Friday.

I think I did it that way to make it easier to implement remove, but there are probably other approaches we could take.

Oh, my previous replies were lost.

I think std::vector<std::shared_ptr> sources_ is needed, livekit::AudioMixer AddSouce / RemoveSouce does call into webrtc::AudioMixerImpl, but they just pass the raw pointers there, thus WebRTC lib does not own the object. That is the reason that std::vector<std::shared_ptr> sources_ solve the problem as we need a owner to the AudioMixerSource.
And we need to keep them.

I think Zed solved the racing issue by using Arc.

My suggestion is to make the C++ livekit::AudioMixer thread safe, something like

std::mutex sources_mutex_;

void AudioMixer::add_source(rust::Box<AudioMixerSourceWrapper> source) {
  auto native_source = std::make_shared<AudioMixerSource>(std::move(source));
  {
    std::lock_guard<std::mutex> lock(sources_mutex_);
    audio_mixer_->AddSource(native_source.get());
    sources_.push_back(native_source);
  }
}

void AudioMixer::remove_source(int source_ssrc) {
  std::lock_guard<std::mutex> lock(sources_mutex_);
  auto it = std::find_if(
      sources_.begin(), sources_.end(),
      [source_ssrc](const auto& s) { return s->Ssrc() == source_ssrc; });

  if (it != sources_.end()) {
    audio_mixer_->RemoveSource(it->get());
    sources_.erase(it);
  }
}

Good point about the lifetime; AudioMixerImpl in libwebrtc does keep its own vector of sources internally, but those are AudioMixerImpl::SourceStatus structs which have a plain pointer Source* member. So it is helpful to have this vector of std::shared_pointers above it.

We added a mutex guarding add/remove_source to resolve the thread safety issue with that. Unless you see any more thread safety issues, I think it's okay now.