* #14727: video: video receive thread should not call pjsip or touch the callmap

Also, even if tryEnterMutex returns false, we must call leaveMutex.
This was leading to a dead lock when hanging up and answering a call at
the same time.

daemon/src/sip/Makefile.am

@@ -14,9 +14,7 @@ libsiplink_la_SOURCES = \
 		sipvoiplink.h \
 		siptransport.h \
 		sip_utils.cpp \
-		sip_utils.h \
-		sip_thread_client.h \
-		sip_thread_client.cpp
+		sip_utils.h
 
 if BUILD_SDES
 libsiplink_la_SOURCES+= sdes_negotiator.cpp \

daemon/src/sip/sip_thread_client.cpp

-/*
- *  Copyright (C) 2012 Savoir-Faire Linux Inc.
- *  Author: Tristan Matthews <tristan.matthews@savoirfairelinux.com>
- *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 3 of the License, or
- *  (at your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
- *
- *  Additional permission under GNU GPL version 3 section 7:
- *
- *  If you modify this program, or any covered work, by linking or
- *  combining it with the OpenSSL project's OpenSSL library (or a
- *  modified version of that library), containing parts covered by the
- *  terms of the OpenSSL or SSLeay licenses, Savoir-Faire Linux Inc.
- *  grants you additional permission to convey the resulting work.
- *  Corresponding Source for a non-source form of such a combination
- *  shall include the source code for the parts of OpenSSL used as well
- *  as that of the covered work.
- */
-
-#include "sip_thread_client.h"
-#include "logger.h"
-#include <cstring>
-
-SIPThreadClient::SIPThreadClient() : thread_(0), desc_()
-{
-    memset(desc_, 0, sizeof(desc_));
-    // We have to register the external thread so it can access the pjsip frameworks
-    if (pj_thread_register(NULL, desc_, &thread_) != PJ_SUCCESS)
-        ERROR("Could not register thread");
-}
-
-SIPThreadClient::~SIPThreadClient()
-{
-    if (thread_) {
-        WARN("Destroying thread handle");
-        if (pj_thread_join(thread_) != PJ_SUCCESS) {
-            ERROR("Error on thread join");
-        } else if (pj_thread_destroy(thread_) != PJ_SUCCESS) {
-            ERROR("Error on thread destroy");
-        }
-        thread_ = 0;
-    }
-}

daemon/src/sip/sip_thread_client.h

-/*
- *  Copyright (C) 2011 Savoir-Faire Linux Inc.
- *  Author: Tristan Matthews <tristan.matthews@savoirfairelinux.com>
- *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 3 of the License, or
- *  (at your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
- *
- *  Additional permission under GNU GPL version 3 section 7:
- *
- *  If you modify this program, or any covered work, by linking or
- *  combining it with the OpenSSL project's OpenSSL library (or a
- *  modified version of that library), containing parts covered by the
- *  terms of the OpenSSL or SSLeay licenses, Savoir-Faire Linux Inc.
- *  grants you additional permission to convey the resulting work.
- *  Corresponding Source for a non-source form of such a combination
- *  shall include the source code for the parts of OpenSSL used as well
- *  as that of the covered work.
- */
-
-#ifndef SIP_THREAD_CLIENT_H_
-#define SIP_THREAD_CLIENT_H_
-
-#include "pj/os.h"
-#include "noncopyable.h"
-
-class pj_thread_t;
-
-class SIPThreadClient {
-    private:
-        NON_COPYABLE(SIPThreadClient);
-        pj_thread_t *thread_;
-        pj_thread_desc desc_;
-
-    public:
-        SIPThreadClient();
-        ~SIPThreadClient();
-};
-
-#endif // SIP_THREAD_CLIENT_H_

daemon/src/sip/sipvoiplink.cpp

@@ -415,6 +415,10 @@ pj_bool_t transaction_request_cb(pjsip_rx_data *rdata)
 
 SIPVoIPLink::SIPVoIPLink() : sipTransport(endpt_, cp_, pool_), sipAccountMap_(),
     sipCallMapMutex_(), sipCallMap_(), evThread_(this)
+#ifdef SFL_VIDEO
+    , keyframeRequestsMutex_()
+    , keyframeRequests_()
+#endif
 {
 #define TRY(ret) do { \
     if (ret != PJ_SUCCESS) \
@@ -561,6 +565,9 @@ bool SIPVoIPLink::getEvent()
 
     static const pj_time_val timeout = {0, 10};
     pjsip_endpt_handle_events(endpt_, &timeout);
+#ifdef SFL_VIDEO
+    dequeKeyframeRequests();
+#endif
     return handlingEvents_;
 }
 
@@ -1084,10 +1091,11 @@ SIPVoIPLink::getSipCall(const std::string& id)
 }
 
 SIPCall*
-SIPVoIPLink::tryGetSipCall(const std::string &id)
+SIPVoIPLink::tryGetSIPCall(const std::string &id)
 {
     if (not sipCallMapMutex_.tryEnterMutex()) {
         ERROR("Could not lock call map mutex");
+        sipCallMapMutex_.leaveMutex();
         return 0;
     }
     SipCallMap::iterator iter = sipCallMap_.find(id);
@@ -1278,13 +1286,35 @@ dtmfSend(SIPCall &call, char code, const std::string &dtmf)
 }
 
 #ifdef SFL_VIDEO
+// Called from a video thread
+void
+SIPVoIPLink::enqueueKeyframeRequest(const std::string &id)
+{
+    ost::MutexLock m(instance_->keyframeRequestsMutex_);
+    instance_->keyframeRequests_.push(id);
+}
+
+// Called from SIP event thread
+void
+SIPVoIPLink::dequeKeyframeRequests()
+{
+    int max_requests = 20;
+    while (not keyframeRequests_.empty() and max_requests--) {
+        ost::MutexLock m(keyframeRequestsMutex_);
+        const std::string &id(keyframeRequests_.front());
+        requestKeyframe(id);
+        keyframeRequests_.pop();
+    }
+}
+
+// Called from SIP event thread
 void
-SIPVoIPLink::requestFastPictureUpdate(const std::string &callID)
+SIPVoIPLink::requestKeyframe(const std::string &callID)
 {
     SIPCall *call = 0;
     const int tries = 10;
     for (int i = 0; !call and i < tries; ++i)
-        call = SIPVoIPLink::instance()->tryGetSipCall(callID);
+        call = SIPVoIPLink::instance()->tryGetSIPCall(callID);
     if (!call)
         return;
 

daemon/src/sip/sipvoiplink.h

@@ -48,6 +48,9 @@
 #include <pjlib-util.h>
 #include <pjnath.h>
 #include <pjnath/stun_config.h>
+#ifdef SFL_VIDEO
+#include <queue>
+#endif
 
 #include "sipaccount.h"
 #include "voiplink.h"
@@ -283,9 +286,8 @@ class SIPVoIPLink : public VoIPLink {
         SipTransport sipTransport;
 
 #ifdef SFL_VIDEO
-        static void requestFastPictureUpdate(const std::string &callID);
+        static void enqueueKeyframeRequest(const std::string &callID);
 #endif
-
     private:
 
         NON_COPYABLE(SIPVoIPLink);
@@ -313,6 +315,13 @@ class SIPVoIPLink : public VoIPLink {
          */
         EventThread evThread_;
 
+#ifdef SFL_VIDEO
+        void dequeKeyframeRequests();
+        void requestKeyframe(const std::string &callID);
+        ost::Mutex keyframeRequestsMutex_;
+        std::queue<std::string> keyframeRequests_;
+#endif
+
         static bool destroyed_;
         static SIPVoIPLink *instance_;
 

daemon/src/video/video_receive_thread.cpp

@@ -32,7 +32,6 @@
 #include "video_receive_thread.h"
 #include "dbus/video_controls.h"
 #include "packet_handle.h"
-#include "sip/sip_thread_client.h"
 #include "check.h"
 
 // libav includes
@@ -157,10 +156,9 @@ void VideoReceiveThread::setup()
         ERROR("Could not remove %s", sdpFilename_.c_str());
 
     DEBUG("Finding stream info");
-    if (requestKeyFrameCallback_) {
-        sipThreadClient_.reset(new SIPThreadClient);
+    if (requestKeyFrameCallback_)
         requestKeyFrameCallback_(id_);
-    }
+
 #if LIBAVFORMAT_VERSION_INT < AV_VERSION_INT(53, 8, 0)
     ret = av_find_stream_info(inputCtx_);
 #else
@@ -223,8 +221,7 @@ VideoReceiveThread::VideoReceiveThread(const std::string &id, const std::map<str
     args_(args), frameNumber_(0), inputDecoder_(0), decoderCtx_(0), rawFrame_(0),
     scaledPicture_(0), streamIndex_(-1), inputCtx_(0), imgConvertCtx_(0),
     dstWidth_(0), dstHeight_(0), sink_(), threadRunning_(false),
-    sdpFilename_(), bufferSize_(0), id_(id), interruptCb_(), requestKeyFrameCallback_(0),
-    sipThreadClient_(0)
+    sdpFilename_(), bufferSize_(0), id_(id), interruptCb_(), requestKeyFrameCallback_(0)
 {
     interruptCb_.callback = interruptCb;
     interruptCb_.opaque = this;
@@ -299,7 +296,6 @@ void VideoReceiveThread::run()
 VideoReceiveThread::~VideoReceiveThread()
 {
     threadRunning_ = false;
-    sipThreadClient_.reset(0);
     Manager::instance().getVideoControls()->stoppedDecoding(id_, sink_.openedName());
     // this calls join, which waits for the run() method (in separate thread) to return
     ost::Thread::terminate();

daemon/src/video/video_receive_thread.h

@@ -35,7 +35,6 @@
 #include <map>
 #include <string>
 #include <climits>
-#include <memory>
 #include "shm_sink.h"
 #include "noncopyable.h"
 
@@ -48,8 +47,6 @@ class AVCodecContext;
 class AVStream;
 class AVFormatContext;
 class AVFrame;
-class SIPThreadClient;
-
 namespace sfl_video {
 
 class VideoReceiveThread : public ost::Thread {
@@ -84,8 +81,6 @@ class VideoReceiveThread : public ost::Thread {
         const std::string id_;
         AVIOInterruptCB interruptCb_;
         void (* requestKeyFrameCallback_)(const std::string &);
-        // XXX: This must be allocated in the video thread, not the main thread
-        std::auto_ptr<SIPThreadClient> sipThreadClient_;
 
         void setup();
         void openDecoder();

daemon/src/video/video_rtp_session.cpp

@@ -133,7 +133,7 @@ void VideoRtpSession::start()
         if (receiveThread_.get())
             WARN("restarting video receiver");
         receiveThread_.reset(new VideoReceiveThread(callID_, rxArgs_));
-        receiveThread_->setRequestKeyFrameCallback(&SIPVoIPLink::requestFastPictureUpdate);
+        receiveThread_->setRequestKeyFrameCallback(&SIPVoIPLink::enqueueKeyframeRequest);
         receiveThread_->start();
     } else {
         DEBUG("Video receiving disabled");