From 6dc91b156a7e730e66315ee670ed08dbcfcf5fbd Mon Sep 17 00:00:00 2001 From: tux3 Date: Sat, 10 Oct 2015 01:24:04 +0200 Subject: [PATCH] Use actual mutexes for CameraSource That homemade spinlock thing was insane, VTune says we were spending 1.5s spinning for no reason when openning a device for example --- src/core/coreav.cpp | 3 ++ src/video/camerasource.cpp | 80 ++++++++------------------------------ src/video/camerasource.h | 3 +- 3 files changed, 21 insertions(+), 65 deletions(-) diff --git a/src/core/coreav.cpp b/src/core/coreav.cpp index 549fe6f85..336660ebb 100644 --- a/src/core/coreav.cpp +++ b/src/core/coreav.cpp @@ -395,6 +395,9 @@ void CoreAV::callCallback(ToxAV* toxav, uint32_t friendNum, bool audio, bool vid if (self->calls.contains(friendNum)) { + /// NOTE: Hanging up from a callback is supposed to be UB, + /// but since currently the toxav callbacks are fired from the toxcore thread, + /// we'll always reach this point through a non-blocking queud connection, so not in the callback. qWarning() << QString("Rejecting call invite from %1, we're already in that call!").arg(friendNum); toxav_call_control(toxav, friendNum, TOXAV_CALL_CONTROL_CANCEL, nullptr); return; diff --git a/src/video/camerasource.cpp b/src/video/camerasource.cpp index fa046877c..29da0b148 100644 --- a/src/video/camerasource.cpp +++ b/src/video/camerasource.cpp @@ -37,7 +37,6 @@ CameraSource* CameraSource::instance{nullptr}; CameraSource::CameraSource() : deviceName{"none"}, device{nullptr}, mode(VideoMode{0,0,0}), cctx{nullptr}, cctxOrig{nullptr}, videoStreamIndex{-1}, - biglock{false}, freelistLock{false}, _isOpen{false}, subscriptions{0} { subscriptions = 0; @@ -73,17 +72,10 @@ void CameraSource::open(const QString deviceName) void CameraSource::open(const QString DeviceName, VideoMode Mode) { - { - bool expected = false; - while (!biglock.compare_exchange_weak(expected, true)) - expected = false; - } + QMutexLocker l{&biglock}; if (DeviceName == deviceName && Mode == mode) - { - biglock = false; return; - } if (subscriptions) closeDevice(); @@ -94,8 +86,6 @@ void CameraSource::open(const QString DeviceName, VideoMode Mode) if (subscriptions && _isOpen) openDevice(); - - biglock = false; } void CameraSource::close() @@ -110,18 +100,10 @@ bool CameraSource::isOpen() CameraSource::~CameraSource() { - // Fast lock, in case our stream thread is running - { - bool expected = false; - while (!biglock.compare_exchange_weak(expected, true)) - expected = false; - } + QMutexLocker l{&biglock}; if (!_isOpen) - { - biglock = false; return; - } // Free all remaining VideoFrame // Locking must be done precisely this way to avoid races @@ -144,7 +126,7 @@ CameraSource::~CameraSource() device = nullptr; // Memfence so the stream thread sees a nullptr device std::atomic_thread_fence(std::memory_order_release); - biglock=false; + l.unlock(); // Synchronize with our stream thread while (streamFuture.isRunning()) @@ -153,24 +135,17 @@ CameraSource::~CameraSource() bool CameraSource::subscribe() { - // Fast lock - { - bool expected = false; - while (!biglock.compare_exchange_weak(expected, true)) - expected = false; - } + QMutexLocker l{&biglock}; if (!_isOpen) { ++subscriptions; - biglock = false; return true; } if (openDevice()) { ++subscriptions; - biglock = false; return true; } else @@ -181,7 +156,6 @@ bool CameraSource::subscribe() videoStreamIndex = -1; // Memfence so the stream thread sees a nullptr device std::atomic_thread_fence(std::memory_order_release); - biglock = false; return false; } @@ -189,31 +163,24 @@ bool CameraSource::subscribe() void CameraSource::unsubscribe() { - // Fast lock - { - bool expected = false; - while (!biglock.compare_exchange_weak(expected, true)) - expected = false; - } + QMutexLocker l{&biglock}; if (!_isOpen) { - subscriptions--; - biglock = false; + --subscriptions; return; } if (!device) { qWarning() << "Unsubscribing with zero subscriber"; - biglock = false; return; } if (subscriptions - 1 == 0) { closeDevice(); - biglock = false; + l.unlock(); // Synchronize with our stream thread while (streamFuture.isRunning()) @@ -222,7 +189,6 @@ void CameraSource::unsubscribe() else { device->close(); - biglock = false; } subscriptions--; @@ -230,6 +196,8 @@ void CameraSource::unsubscribe() bool CameraSource::openDevice() { + qDebug() << "Opening device "<open(); @@ -301,6 +269,8 @@ bool CameraSource::openDevice() void CameraSource::closeDevice() { + qDebug() << "Closing device "< vframe = std::make_shared(frame, frameFreeCb); freelist.append(vframe); - freelistLock = false; + freelistLock.unlock(); emit frameAvailable(vframe); } @@ -366,39 +331,28 @@ void CameraSource::stream() }; forever { - // Fast lock - { - bool expected = false; - while (!biglock.compare_exchange_weak(expected, true)) - expected = false; - } + biglock.lock(); // When a thread makes device null, it releases it, so we acquire here std::atomic_thread_fence(std::memory_order_acquire); if (!device) { - biglock = false; + biglock.unlock(); return; } streamLoop(); // Give a chance to other functions to pick up the lock if needed - biglock = false; + biglock.unlock(); QThread::yieldCurrentThread(); } } void CameraSource::freelistCallback(int freelistIndex) { - // Fast spinlock - { - bool expected = false; - while (!freelistLock.compare_exchange_weak(expected, true)) - expected = false; - } + QMutexLocker l{&freelistLock}; freelist[freelistIndex].reset(); - freelistLock = false; } int CameraSource::getFreelistSlotLockless() diff --git a/src/video/camerasource.h b/src/video/camerasource.h index 5b28b9e55..24ee5aada 100644 --- a/src/video/camerasource.h +++ b/src/video/camerasource.h @@ -91,8 +91,7 @@ private: VideoMode mode; ///< What mode we tried to open the device in, all zeros means default mode AVCodecContext* cctx, *cctxOrig; ///< Codec context of the camera's selected video stream int videoStreamIndex; ///< A camera can have multiple streams, this is the one we're decoding - /// TODO: Replace the spinlocks with QMutexS, which spin and sleep - std::atomic_bool biglock, freelistLock; ///< True when locked. Faster than mutexes for video decoding. + QMutex biglock, freelistLock; ///< True when locked. Faster than mutexes for video decoding. std::atomic_bool _isOpen; std::atomic_int subscriptions; ///< Remember how many times we subscribed for RAII