From 98cfe9838fb13cdc8d9dca346bd13a71056afc94 Mon Sep 17 00:00:00 2001 From: sudden6 Date: Sat, 16 Nov 2019 23:16:51 +0100 Subject: [PATCH] refactor: properly lock against Core thread We we're calling toxav_* functions without synchronizing to any of the Tox threads. Additionally remove the call timeout, it creates timers from different threads, which causes errors. --- src/core/core.cpp | 2 +- src/core/coreav.cpp | 24 +++++++++--------------- src/core/coreav.h | 11 +++++++++-- src/core/toxcall.cpp | 23 ----------------------- src/core/toxcall.h | 7 ------- 5 files changed, 19 insertions(+), 48 deletions(-) diff --git a/src/core/core.cpp b/src/core/core.cpp index 194528c42..438bb5d12 100644 --- a/src/core/core.cpp +++ b/src/core/core.cpp @@ -628,7 +628,7 @@ ToxCorePtr Core::makeToxCore(const QByteArray& savedata, const ICoreSettings* co // toxcore is successfully created, create toxav // TODO(sudden6): don't create CoreAv here, Core should be usable without CoreAV - core->av = CoreAV::makeCoreAV(core->tox.get()); + core->av = CoreAV::makeCoreAV(core->tox.get(), core->coreLoopLock); if (!core->av) { qCritical() << "Toxav failed to start"; if (err) { diff --git a/src/core/coreav.cpp b/src/core/coreav.cpp index 9d453240f..1a6550a0a 100644 --- a/src/core/coreav.cpp +++ b/src/core/coreav.cpp @@ -68,11 +68,12 @@ * deadlock. */ -CoreAV::CoreAV(std::unique_ptr toxav) +CoreAV::CoreAV(std::unique_ptr toxav, QMutex& toxCoreLock) : audio{nullptr} , toxav{std::move(toxav)} , coreavThread{new QThread{this}} , iterateTimer{new QTimer{this}} + , coreLock{toxCoreLock} { assert(coreavThread); assert(iterateTimer); @@ -104,7 +105,7 @@ void CoreAV::connectCallbacks(ToxAV& toxav) * @param core pointer to the Tox instance * @return CoreAV instance on success, {} on failure */ -CoreAV::CoreAVPtr CoreAV::makeCoreAV(Tox* core) +CoreAV::CoreAVPtr CoreAV::makeCoreAV(Tox* core, QMutex &toxCoreLock) { TOXAV_ERR_NEW err; std::unique_ptr toxav{toxav_new(core, &err)}; @@ -124,7 +125,7 @@ CoreAV::CoreAVPtr CoreAV::makeCoreAV(Tox* core) assert(toxav != nullptr); - return CoreAVPtr{new CoreAV{std::move(toxav)}}; + return CoreAVPtr{new CoreAV{std::move(toxav), toxCoreLock}}; } /** @@ -235,9 +236,7 @@ bool CoreAV::isCallVideoEnabled(const Friend* f) const bool CoreAV::answerCall(uint32_t friendNum, bool video) { QMutexLocker locker{&callsLock}; - - // This callback should come from the Core thread - assert(QThread::currentThread() != coreavThread.get()); + QMutexLocker coreLocker{&coreLock}; qDebug() << QString("Answering call %1").arg(friendNum); auto it = calls.find(friendNum); @@ -260,9 +259,7 @@ bool CoreAV::answerCall(uint32_t friendNum, bool video) bool CoreAV::startCall(uint32_t friendNum, bool video) { QMutexLocker locker{&callsLock}; - - // This callback should come from the Core thread - assert(QThread::currentThread() != coreavThread.get()); + QMutexLocker coreLocker{&coreLock}; qDebug() << QString("Starting call with %1").arg(friendNum); auto it = calls.find(friendNum); @@ -280,17 +277,14 @@ bool CoreAV::startCall(uint32_t friendNum, bool video) assert(audio != nullptr); ToxFriendCallPtr call = ToxFriendCallPtr(new ToxFriendCall(friendNum, video, *this, *audio)); assert(call != nullptr); - auto ret = calls.emplace(friendNum, std::move(call)); - ret.first->second->startTimeout(friendNum); + calls.emplace(friendNum, std::move(call)); return true; } bool CoreAV::cancelCall(uint32_t friendNum) { QMutexLocker locker{&callsLock}; - - // This callback should come from the Core thread - assert(QThread::currentThread() != coreavThread.get()); + QMutexLocker coreLocker{&coreLock}; qDebug() << QString("Cancelling call with %1").arg(friendNum); if (!toxav_call_control(toxav.get(), friendNum, TOXAV_CALL_CONTROL_CANCEL, nullptr)) { @@ -380,6 +374,7 @@ void CoreAV::sendCallVideo(uint32_t callId, std::shared_ptr vframe) if (call.getNullVideoBitrate()) { qDebug() << "Restarting video stream to friend" << callId; + QMutexLocker coreLocker{&coreLock}; toxav_video_set_bit_rate(toxav.get(), callId, VIDEO_DEFAULT_BITRATE, nullptr); call.setNullVideoBitrate(false); } @@ -756,7 +751,6 @@ void CoreAV::stateCallback(ToxAV* toxav, uint32_t friendNum, uint32_t state, voi } else { // If our state was null, we started the call and were still ringing if (!call.getState() && state) { - call.stopTimeout(); call.setActive(true); emit self->avStart(friendNum, call.getVideoEnabled()); } else if ((call.getState() & TOXAV_FRIEND_CALL_STATE_SENDING_V) diff --git a/src/core/coreav.h b/src/core/coreav.h index 00597ab59..5289d7229 100644 --- a/src/core/coreav.h +++ b/src/core/coreav.h @@ -46,7 +46,7 @@ class CoreAV : public QObject public: using CoreAVPtr = std::unique_ptr; - static CoreAVPtr makeCoreAV(Tox* core); + static CoreAVPtr makeCoreAV(Tox* core, QMutex& coreLock); void setAudio(IAudioControl& newAudio); IAudioControl* getAudio(); @@ -112,7 +112,7 @@ private: } }; - explicit CoreAV(std::unique_ptr tox); + explicit CoreAV(std::unique_ptr tox, QMutex &toxCoreLock); void connectCallbacks(ToxAV& toxav); void process(); @@ -149,6 +149,13 @@ private: // protect 'calls' and 'groupCalls' from being modified by ToxAV and Tox threads mutable QMutex callsLock{QMutex::Recursive}; + + /** + * @brief needed to synchronize with the Core thread, some toxav_* functions + * must not execute at the same time as tox_iterate() + * @note This must be a recursive mutex as we're going to lock it in callbacks + */ + QMutex& coreLock; }; #endif // COREAV_H diff --git a/src/core/toxcall.cpp b/src/core/toxcall.cpp index c27091c59..2836c555b 100644 --- a/src/core/toxcall.cpp +++ b/src/core/toxcall.cpp @@ -190,29 +190,6 @@ void ToxFriendCall::onAudioSinkInvalidated() sink = std::move(newSink); } -void ToxFriendCall::startTimeout(uint32_t callId) -{ - if (!timeoutTimer) { - timeoutTimer = std::unique_ptr{new QTimer{}}; - // We might move, so we need copies of members. CoreAV won't move while we're alive - CoreAV* avCopy = av; - QObject::connect(timeoutTimer.get(), &QTimer::timeout, - [avCopy, callId]() { avCopy->timeoutCall(callId); }); - } - - if (!timeoutTimer->isActive()) - timeoutTimer->start(CALL_TIMEOUT); -} - -void ToxFriendCall::stopTimeout() -{ - if (!timeoutTimer) - return; - - timeoutTimer->stop(); - timeoutTimer.reset(); -} - TOXAV_FRIEND_CALL_STATE ToxFriendCall::getState() const { return state; diff --git a/src/core/toxcall.h b/src/core/toxcall.h index 1c50e1f61..80a23d6e6 100644 --- a/src/core/toxcall.h +++ b/src/core/toxcall.h @@ -95,17 +95,11 @@ public: ToxFriendCall& operator=(ToxFriendCall&& other) = delete; ~ToxFriendCall(); - void startTimeout(uint32_t callId); - void stopTimeout(); - TOXAV_FRIEND_CALL_STATE getState() const; void setState(const TOXAV_FRIEND_CALL_STATE& value); void playAudioBuffer(const int16_t* data, int samples, unsigned channels, int sampleRate) const; -protected: - std::unique_ptr timeoutTimer; - private: QMetaObject::Connection audioSinkInvalid; void onAudioSourceInvalidated(); @@ -113,7 +107,6 @@ private: private: TOXAV_FRIEND_CALL_STATE state{TOXAV_FRIEND_CALL_STATE_NONE}; - static constexpr int CALL_TIMEOUT = 45000; std::unique_ptr sink = nullptr; uint32_t friendId; };