alsa: cleanup, fix deadlock

Change-Id: Ia395103ba2ec2ec85400cf710a07c40888568d68

src/media/audio/alsa/alsalayer.cpp

@@ -39,71 +39,14 @@
 
 namespace jami {
 
-class AlsaThread
-{
-public:
-    AlsaThread(AlsaLayer* alsa);
-    ~AlsaThread();
-    void start();
-    bool isRunning() const;
-
-private:
-    NON_COPYABLE(AlsaThread);
-    void run();
-    AlsaLayer* alsa_;
-    std::atomic<bool> running_;
-    std::thread thread_;
-};
-
-AlsaThread::AlsaThread(AlsaLayer* alsa)
-    : alsa_(alsa)
-    , running_(false)
-    , thread_()
-{}
-
-bool
-AlsaThread::isRunning() const
-{
-    return running_;
-}
-
-AlsaThread::~AlsaThread()
-{
-    running_ = false;
-    if (thread_.joinable())
-        thread_.join();
-}
-
-void
-AlsaThread::start()
-{
-    running_ = true;
-    thread_ = std::thread(&AlsaThread::run, this);
-}
-
-void
-AlsaThread::run()
-{
-    alsa_->run(running_);
-}
-
 AlsaLayer::AlsaLayer(const AudioPreference& pref)
     : AudioLayer(pref)
     , indexIn_(pref.getAlsaCardin())
     , indexOut_(pref.getAlsaCardout())
     , indexRing_(pref.getAlsaCardRingtone())
-    , playbackHandle_(nullptr)
-    , ringtoneHandle_(nullptr)
-    , captureHandle_(nullptr)
     , audioPlugin_(pref.getAlsaPlugin())
     , playbackBuff_(0, audioFormat_)
     , captureBuff_(0, audioFormat_)
-    , is_capture_prepared_(false)
-    , is_playback_running_(false)
-    , is_capture_running_(false)
-    , is_playback_open_(false)
-    , is_capture_open_(false)
-    , audioThread_(nullptr)
 {
     setHasNativeAEC(false);
 }
@@ -111,7 +54,7 @@ AlsaLayer::AlsaLayer(const AudioPreference& pref)
 AlsaLayer::~AlsaLayer()
 {
     status_ = Status::Idle;
-    audioThread_.reset();
+    stopThread();
 
     /* Then close the audio devices */
     closeCaptureStream();
@@ -123,14 +66,14 @@ AlsaLayer::~AlsaLayer()
  * Reimplementation of run()
  */
 void
-AlsaLayer::run(const std::atomic<bool>& isRunning)
+AlsaLayer::run()
 {
     if (playbackHandle_)
         playbackChanged(true);
     if (captureHandle_)
         recordChanged(true);
 
-    while (status_ == Status::Started and isRunning) {
+    while (status_ == Status::Started and running_) {
         playback();
         ringtone();
         capture();
@@ -148,8 +91,8 @@ AlsaLayer::openDevice(snd_pcm_t** pcm,
                       AudioFormat& format)
 {
     JAMI_DBG("Alsa: Opening %s device '%s'",
-        (stream == SND_PCM_STREAM_CAPTURE) ? "capture" : "playback",
-        dev.c_str());
+             (stream == SND_PCM_STREAM_CAPTURE) ? "capture" : "playback",
+             dev.c_str());
 
     static const int MAX_RETRIES = 10; // times of 100ms
     int err, tries = 0;
@@ -165,9 +108,9 @@ AlsaLayer::openDevice(snd_pcm_t** pcm,
 
     if (err < 0) {
         JAMI_ERR("Alsa: couldn't open %s device %s : %s",
-                 (stream == SND_PCM_STREAM_CAPTURE)
-                     ? "capture"
-                     : (stream == SND_PCM_STREAM_PLAYBACK) ? "playback" : "ringtone",
+                 (stream == SND_PCM_STREAM_CAPTURE)    ? "capture"
+                 : (stream == SND_PCM_STREAM_PLAYBACK) ? "playback"
+                                                       : "ringtone",
                  dev.c_str(),
                  snd_strerror(err));
         return false;
@@ -181,10 +124,12 @@ AlsaLayer::openDevice(snd_pcm_t** pcm,
     return true;
 }
 
-void AlsaLayer::startStream(AudioDeviceType type)
+void
+AlsaLayer::startStream(AudioDeviceType type)
 {
+    std::unique_lock<std::mutex> lk(mutex_);
     status_ = Status::Starting;
-    audioThread_.reset();
+    stopThread();
 
     bool dsnop = audioPlugin_ == PCM_DMIX_DSNOOP;
 
@@ -200,7 +145,8 @@ void AlsaLayer::startStream(AudioDeviceType type)
         startPlaybackStream();
     }
 
-    if (type == AudioDeviceType::RINGTONE and getIndexPlayback() != getIndexRingtone() and not ringtoneHandle_) {
+    if (type == AudioDeviceType::RINGTONE and getIndexPlayback() != getIndexRingtone()
+        and not ringtoneHandle_) {
         if (!openDevice(&ringtoneHandle_,
                         buildDeviceTopo(dsnop ? PCM_DMIX : audioPlugin_, indexRing_),
                         SND_PCM_STREAM_PLAYBACK,
@@ -221,14 +167,14 @@ void AlsaLayer::startStream(AudioDeviceType type)
     }
 
     status_ = Status::Started;
-    audioThread_.reset(new AlsaThread(this));
-    audioThread_->start();
+    startThread();
 }
 
 void
 AlsaLayer::stopStream(AudioDeviceType stream)
 {
-    audioThread_.reset();
+    std::unique_lock<std::mutex> lk(mutex_);
+    stopThread();
 
     if (stream == AudioDeviceType::CAPTURE && is_capture_open_) {
         closeCaptureStream();
@@ -245,13 +191,27 @@ AlsaLayer::stopStream(AudioDeviceType stream)
     }
 
     if (is_capture_open_ or is_playback_open_ or ringtoneHandle_) {
-        audioThread_.reset(new AlsaThread(this));
-        audioThread_->start();
+        startThread();
     } else {
         status_ = Status::Idle;
     }
 }
 
+void
+AlsaLayer::startThread()
+{
+    running_ = true;
+    audioThread_ = std::thread(&AlsaLayer::run, this);
+}
+
+void
+AlsaLayer::stopThread()
+{
+    running_ = false;
+    if (audioThread_.joinable())
+        audioThread_.join();
+}
+
 /*
  * GCC extension : statement expression
  *
@@ -284,7 +244,8 @@ AlsaLayer::closeCaptureStream()
         stopCaptureStream();
 
     JAMI_DBG("Alsa: Closing capture stream");
-    if (is_capture_open_ && ALSA_CALL(snd_pcm_close(captureHandle_), "Couldn't close capture") >= 0) {
+    if (is_capture_open_
+        && ALSA_CALL(snd_pcm_close(captureHandle_), "Couldn't close capture") >= 0) {
         is_capture_open_ = false;
         captureHandle_ = nullptr;
     }

src/media/audio/alsa/alsalayer.h

@@ -28,6 +28,7 @@
 #include <alsa/asoundlib.h>
 
 #include <memory>
+#include <thread>
 
 #define PCM_DMIX "plug:dmix" /** Alsa plugin for software mixing */
 
@@ -137,7 +138,7 @@ public:
      */
     virtual int getIndexRingtone() const { return indexRing_; }
 
-    void run(const std::atomic<bool>& isRunning);
+    void run();
 
 private:
     /**
@@ -188,6 +189,9 @@ private:
 
     bool alsa_set_params(snd_pcm_t* pcm_handle, AudioFormat& format);
 
+    void startThread();
+    void stopThread();
+
     /**
      * Copy a data buffer in the internal ring buffer
      * ALSA Library API
@@ -211,19 +215,19 @@ private:
      * Handles to manipulate playback stream
      * ALSA Library API
      */
-    snd_pcm_t* playbackHandle_;
+    snd_pcm_t* playbackHandle_ {nullptr};
 
     /**
      * Handles to manipulate ringtone stream
      *
      */
-    snd_pcm_t* ringtoneHandle_;
+    snd_pcm_t* ringtoneHandle_ {nullptr};
 
     /**
      * Handles to manipulate capture stream
      * ALSA Library API
      */
-    snd_pcm_t* captureHandle_;
+    snd_pcm_t* captureHandle_ {nullptr};
 
     /**
      * name of the alsa audio plugin used
@@ -234,13 +238,14 @@ private:
     AudioBuffer playbackBuff_;
     AudioBuffer captureBuff_;
 
-    bool is_capture_prepared_;
-    bool is_playback_running_;
-    bool is_capture_running_;
-    bool is_playback_open_;
-    bool is_capture_open_;
+    bool is_capture_prepared_ {false};
+    bool is_playback_running_ {false};
+    bool is_capture_running_ {false};
+    bool is_playback_open_ {false};
+    bool is_capture_open_ {false};
 
-    std::unique_ptr<AlsaThread> audioThread_;
+    std::atomic_bool running_ {false};
+    std::thread audioThread_;
 };
 
 } // namespace jami

src/media/audio/audiolayer.cpp

@@ -62,7 +62,6 @@ void
 AudioLayer::hardwareFormatAvailable(AudioFormat playback, size_t bufSize)
 {
     JAMI_DBG("Hardware audio format available : %s %zu", playback.toString().c_str(), bufSize);
-    std::unique_lock<std::mutex> lk(mutex_);
     audioFormat_ = Manager::instance().hardwareAudioFormatChanged(playback);
     urgentRingBuffer_.setFormat(audioFormat_);
     nativeFrameSize_ = bufSize;
@@ -89,7 +88,6 @@ AudioLayer::flushMain()
 void
 AudioLayer::flushUrgent()
 {
-    std::lock_guard<std::mutex> lock(mutex_);
     urgentRingBuffer_.flushAll();
 }
 

src/media/audio/jack/jacklayer.cpp

@@ -379,12 +379,10 @@ JackLayer::process_playback(jack_nframes_t frames, void* arg)
  */
 void JackLayer::startStream(AudioDeviceType)
 {
-    {
-        std::lock_guard<std::mutex> lock(mutex_);
-        if (status_ != Status::Idle)
-            return;
-        status_ = Status::Started;
-    }
+    std::lock_guard<std::mutex> lock(mutex_);
+    if (status_ != Status::Idle)
+        return;
+    status_ = Status::Started;
 
     dcblocker_.reset();
     if (jack_activate(playbackClient_) or jack_activate(captureClient_)) {
@@ -408,13 +406,11 @@ JackLayer::onShutdown(void* /* data */)
  */
 void JackLayer::stopStream(AudioDeviceType)
 {
-    {
-        std::lock_guard<std::mutex> lock(mutex_);
-        if (status_ != Status::Started)
-            return;
-        status_ = Status::Idle;
-        data_ready_.notify_one();
-    }
+    std::lock_guard<std::mutex> lock(mutex_);
+    if (status_ != Status::Started)
+        return;
+    status_ = Status::Idle;
+    data_ready_.notify_one();
 
     if (jack_deactivate(playbackClient_) or jack_deactivate(captureClient_)) {
         JAMI_ERR("JACK client could not deactivate");

src/media/audio/opensl/opensllayer.cpp

@@ -43,8 +43,7 @@ namespace jami {
 // Constructor
 OpenSLLayer::OpenSLLayer(const AudioPreference& pref)
     : AudioLayer(pref)
-{
-}
+{}
 
 // Destructor
 OpenSLLayer::~OpenSLLayer()
@@ -56,10 +55,11 @@ void
 OpenSLLayer::startStream(AudioDeviceType stream)
 {
     using namespace std::placeholders;
+
+    std::lock_guard<std::mutex> lock(mutex_);
     if (!engineObject_)
         initAudioEngine();
 
-    std::lock_guard<std::mutex> lock(mutex_);
     JAMI_WARN("Start OpenSL audio layer");
 
     if (stream == AudioDeviceType::PLAYBACK) {
@@ -97,7 +97,8 @@ OpenSLLayer::startStream(AudioDeviceType stream)
         if (not recorder_) {
             std::lock_guard<std::mutex> lck(recMtx);
             try {
-                recorder_.reset(new opensl::AudioRecorder(hardwareFormat_, hardwareBuffSize_, engineInterface_));
+                recorder_.reset(
+                    new opensl::AudioRecorder(hardwareFormat_, hardwareBuffSize_, engineInterface_));
                 recorder_->setBufQueues(&freeRecBufQueue_, &recBufQueue_);
                 recorder_->registerCallback(std::bind(&OpenSLLayer::engineServiceRec, this));
                 setHasNativeAEC(recorder_->hasNativeAEC());
@@ -163,7 +164,6 @@ OpenSLLayer::initAudioEngine()
     hardwareBuffSize_ = hw_infos[1];
     hardwareFormatAvailable(hardwareFormat_, hardwareBuffSize_);
 
-    std::lock_guard<std::mutex> lock(mutex_);
     SLASSERT(slCreateEngine(&engineObject_, 0, nullptr, 0, nullptr, nullptr));
     SLASSERT((*engineObject_)->Realize(engineObject_, SL_BOOLEAN_FALSE));
     SLASSERT((*engineObject_)->GetInterface(engineObject_, SL_IID_ENGINE, &engineInterface_));
@@ -254,10 +254,14 @@ OpenSLLayer::engineServicePlay()
                 break;
             }
             if (not dat->pointer()->data[0] or not buf->buf_) {
-                JAMI_ERR("null bufer %p -> %p %d", dat->pointer()->data[0], buf->buf_, dat->pointer()->nb_samples);
+                JAMI_ERR("null bufer %p -> %p %d",
+                         dat->pointer()->data[0],
+                         buf->buf_,
+                         dat->pointer()->nb_samples);
                 break;
             }
-            //JAMI_ERR("std::copy_n %p -> %p %zu", dat->pointer()->data[0], buf->buf_, dat->pointer()->nb_samples);
+            // JAMI_ERR("std::copy_n %p -> %p %zu", dat->pointer()->data[0], buf->buf_,
+            // dat->pointer()->nb_samples);
             std::copy_n((const AudioSample*) dat->pointer()->data[0],
                         dat->pointer()->nb_samples,
                         (AudioSample*) buf->buf_);
@@ -286,7 +290,10 @@ OpenSLLayer::engineServiceRing()
                 break;
             }
             if (not dat->pointer()->data[0] or not buf->buf_) {
-                JAMI_ERR("null bufer %p -> %p %d", dat->pointer()->data[0], buf->buf_, dat->pointer()->nb_samples);
+                JAMI_ERR("null bufer %p -> %p %d",
+                         dat->pointer()->data[0],
+                         buf->buf_,
+                         dat->pointer()->nb_samples);
                 break;
             }
             std::copy_n((const AudioSample*) dat->pointer()->data[0],

src/media/audio/pulseaudio/pulselayer.cpp

@@ -359,7 +359,8 @@ PulseLayer::getAudioDeviceName(int index, AudioDeviceType type) const
 }
 
 void
-PulseLayer::onStreamReady() {
+PulseLayer::onStreamReady()
+{
     if (--pendingStreams == 0) {
         JAMI_DBG("All streams ready, starting audio");
         // Flush outside the if statement: every time start stream is
@@ -376,23 +377,31 @@ PulseLayer::onStreamReady() {
 }
 
 void
-PulseLayer::createStream(std::unique_ptr<AudioStream>& stream, AudioDeviceType type, const PaDeviceInfos& dev_infos, bool ec, std::function<void(size_t)>&& onData)
+PulseLayer::createStream(std::unique_ptr<AudioStream>& stream,
+                         AudioDeviceType type,
+                         const PaDeviceInfos& dev_infos,
+                         bool ec,
+                         std::function<void(size_t)>&& onData)
 {
     if (stream) {
         JAMI_WARN("Stream already exists");
         return;
     }
     pendingStreams++;
-    const char* name = type == AudioDeviceType::PLAYBACK ? "Playback" :
-                      (type == AudioDeviceType::CAPTURE  ? "Record" :
-                      (type == AudioDeviceType::RINGTONE ? "Ringtone" : "?"));
-    stream.reset(new AudioStream(context_, mainloop_.get(),
-                                    name,
-                                    type,
-                                    audioFormat_.sample_rate,
-                                    dev_infos,
-                                    ec,
-                                    std::bind(&PulseLayer::onStreamReady, this), std::move(onData)));
+    const char* name = type == AudioDeviceType::PLAYBACK
+                           ? "Playback"
+                           : (type == AudioDeviceType::CAPTURE
+                                  ? "Record"
+                                  : (type == AudioDeviceType::RINGTONE ? "Ringtone" : "?"));
+    stream.reset(new AudioStream(context_,
+                                 mainloop_.get(),
+                                 name,
+                                 type,
+                                 audioFormat_.sample_rate,
+                                 dev_infos,
+                                 ec,
+                                 std::bind(&PulseLayer::onStreamReady, this),
+                                 std::move(onData)));
 }
 
 void
@@ -407,8 +416,10 @@ PulseLayer::disconnectAudioStream()
     startedCv_.notify_all();
 }
 
-void PulseLayer::startStream(AudioDeviceType type)
+void
+PulseLayer::startStream(AudioDeviceType type)
 {
+    std::lock_guard<std::mutex> lk(mutex_);
     waitForDevices();
     PulseMainLoopLock lock(mainloop_.get());
 
@@ -416,18 +427,25 @@ void PulseLayer::startStream(AudioDeviceType type)
     if (type == AudioDeviceType::PLAYBACK) {
         if (auto dev_infos = getDeviceInfos(sinkList_, getPreferredPlaybackDevice())) {
             bool ec = preference_.getEchoCanceller() == "system";
-            createStream(playback_, type, *dev_infos, ec, std::bind(&PulseLayer::writeToSpeaker, this));
+            createStream(playback_,
+                         type,
+                         *dev_infos,
+                         ec,
+                         std::bind(&PulseLayer::writeToSpeaker, this));
         }
     } else if (type == AudioDeviceType::RINGTONE) {
         if (auto dev_infos = getDeviceInfos(sinkList_, getPreferredRingtoneDevice()))
-            createStream(ringtone_, type, *dev_infos, false, std::bind(&PulseLayer::ringtoneToSpeaker, this));
+            createStream(ringtone_,
+                         type,
+                         *dev_infos,
+                         false,
+                         std::bind(&PulseLayer::ringtoneToSpeaker, this));
     } else if (type == AudioDeviceType::CAPTURE) {
         if (auto dev_infos = getDeviceInfos(sourceList_, getPreferredCaptureDevice()))
             createStream(record_, type, *dev_infos, true, std::bind(&PulseLayer::readFromMic, this));
     }
     pa_threaded_mainloop_signal(mainloop_.get(), 0);
 
-    std::lock_guard<std::mutex> lk(mutex_);
     status_ = Status::Started;
     startedCv_.notify_all();
 }
@@ -435,6 +453,7 @@ void PulseLayer::startStream(AudioDeviceType type)
 void
 PulseLayer::stopStream(AudioDeviceType type)
 {
+    std::lock_guard<std::mutex> lk(mutex_);
     waitForDevices();
     PulseMainLoopLock lock(mainloop_.get());
     auto& stream(getStream(type));
@@ -446,7 +465,6 @@ PulseLayer::stopStream(AudioDeviceType type)
     stream->stop();
     stream.reset();
 
-    std::unique_lock<std::mutex> lk(mutex_);
     if (not playback_ and not ringtone_ and not record_) {
         pendingStreams = 0;
         status_ = Status::Idle;
@@ -613,13 +631,14 @@ PulseLayer::waitForDeviceList()
         devicesChanged();
         auto playbackInfo = getDeviceInfos(sinkList_, getPreferredPlaybackDevice());
         playbackDeviceChanged = playback_
-                                and (!playbackInfo->name.empty() and playbackInfo->name
-                                            != stripEchoSufix(playback_->getDeviceName()));
+                                and (!playbackInfo->name.empty()
+                                     and playbackInfo->name
+                                             != stripEchoSufix(playback_->getDeviceName()));
 
         auto recordInfo = getDeviceInfos(sourceList_, getPreferredCaptureDevice());
         recordDeviceChanged = record_
-                                and (!recordInfo->name.empty() and recordInfo->name
-                                            != stripEchoSufix(record_->getDeviceName()));
+                              and (!recordInfo->name.empty()
+                                   and recordInfo->name != stripEchoSufix(record_->getDeviceName()));
 
         if (status_ != Status::Started)
             return;
@@ -661,7 +680,10 @@ PulseLayer::server_info_callback(pa_context*, const pa_server_info* i, void* use
     context->defaultSink_ = {};
     context->defaultSource_ = {};
     context->defaultAudioFormat_ = {i->sample_spec.rate, i->sample_spec.channels};
-    context->hardwareFormatAvailable(context->defaultAudioFormat_);
+    {
+        std::lock_guard<std::mutex> lk(context->mutex_);
+        context->hardwareFormatAvailable(context->defaultAudioFormat_);
+    }
     if (not context->sinkList_.empty())
         context->sinkList_.front().channel_map.channels = std::min(i->sample_spec.channels,
                                                                    (uint8_t) 2);