Fix media player deadlock with ffmpeg background

Briefly, this kind of deadlock occurs when one thread locks
mutex A and then mutex B, whereas other thread locks B and then A.
By the design, the problem in the code is following:
ClockController::setPlaybackRate (locks mutex A) =>
Renderer::setPlaybackRate (locks mutex B), renderer is a thread.

ClockController::setPaused (locks mutex A) =>
Renderer::setPaused (locks mutex B)

and

Thread::run (locks mutex B) =>
AudioRenderer::loop => ClockController::timeUpdated (locks mutex A)
Thread::run (locks mutex B) =>
AudioRenderer::loop => ClockController::currentTime (locks mutex A)
The deadlock was reproduced pretty easy,
it was enough to change the plaback rate a few times while playing.

The solution is to protect by mutex A only data
that can be accessed from threads.
Clocks creation, removal and iteration are expected from
the main thread, so we don't have to synchronize them.
Clock *m_master can be read from threads,
so it has been refactored as atomic.
A stress test for this case has been implemented,
see codereview 428720.
Note, the fix doesn't fix all threading problems
in this design (the mentioned test has found new ones),
so the design is to be refactored in order to make it safe and clear.
Current solution is sufficient for 6.4.

Task-number: QTBUG-105521
Change-Id: Ic8b9a158ecee3ee5f0d5249a283bbc024b57d31c
Reviewed-by: Lars Knoll <lars@knoll.priv.no>
(cherry picked from commit 483e8fc375cadaff414550585fcf64d6d29bd50c)

3 files changed, 92 insertions, 61 deletions

diff --git a/src/plugins/multimedia/ffmpeg/qffmpegclock.cpp b/src/plugins/multimedia/ffmpeg/qffmpegclock.cpp
index c1a9c9168..c080c55df 100644
--- a/src/plugins/multimedia/ffmpeg/qffmpegclock.cpp
+++ b/src/plugins/multimedia/ffmpeg/qffmpegclock.cpp
@@ -7,6 +7,17 @@ Q_LOGGING_CATEGORY(qLcClock, "qt.multimedia.ffmpeg.clock")
 
 QT_BEGIN_NAMESPACE
 
+static bool compareClocks(const QFFmpeg::Clock *a, const QFFmpeg::Clock *b)
+{
+    if (!b)
+        return false;
+
+    if (!a)
+        return true;
+
+    return a->type() < b->type();
+}
+
 QFFmpeg::Clock::Clock(ClockController *controller)
     : controller(controller)
 {
@@ -22,7 +33,7 @@ QFFmpeg::Clock::~Clock()
 
 qint64 QFFmpeg::Clock::currentTime() const
 {
-    return controller ? controller->currentTime() : 0.;
+    return controller ? controller->currentTime() : 0;
 }
 
 void QFFmpeg::Clock::syncTo(qint64 time)
@@ -54,7 +65,7 @@ qint64 QFFmpeg::Clock::usecsTo(qint64 currentTime, qint64 displayTime)
 {
     if (!controller || controller->m_isPaused)
         return -1;
-    int t = qRound64((displayTime - currentTime)/playbackRate());
+    const qint64 t = qRound64((displayTime - currentTime) / playbackRate());
     return t < 0 ? 0 : t;
 }
 
@@ -65,7 +76,6 @@ QFFmpeg::Clock::Type QFFmpeg::Clock::type() const
 
 QFFmpeg::ClockController::~ClockController()
 {
-    QMutexLocker l(&m_mutex);
     for (auto *p : qAsConst(m_clocks))
         p->setController(nullptr);
 }
@@ -73,7 +83,7 @@ QFFmpeg::ClockController::~ClockController()
 qint64 QFFmpeg::ClockController::timeUpdated(Clock *clock, qint64 time)
 {
     QMutexLocker l(&m_mutex);
-    if (clock != m_master) {
+    if (!isMaster(clock)) {
         // If the clock isn't the master clock, simply return the current time
         // so we can make adjustments as needed
         return currentTimeNoLock();
@@ -81,7 +91,7 @@ qint64 QFFmpeg::ClockController::timeUpdated(Clock *clock, qint64 time)
 
     // if the clock is the master, adjust our base timing
     m_baseTime = time;
-    m_timer.restart();
+    m_elapsedTimer.restart();
 
     // Avoid posting too many updates to the notifyObject, or we can overload
     // the event queue with too many notifications
@@ -96,40 +106,41 @@ qint64 QFFmpeg::ClockController::timeUpdated(Clock *clock, qint64 time)
 
 void QFFmpeg::ClockController::addClock(Clock *clock)
 {
-    QMutexLocker l(&m_mutex);
     qCDebug(qLcClock) << "addClock" << clock;
     Q_ASSERT(clock != nullptr);
 
     if (m_clocks.contains(clock))
         return;
 
-    if (!m_master)
-        m_master = clock;
-
     m_clocks.append(clock);
-    clock->syncTo(currentTimeNoLock());
-    clock->setPaused(m_isPaused);
+    m_master = std::max(m_master.loadAcquire(), clock, compareClocks);
 
-    // update master clock
-    if (m_master != clock && clock->type() > m_master->type())
-        m_master = clock;
+    clock->syncTo(currentTime());
+    clock->setPaused(m_isPaused);
 }
 
 void QFFmpeg::ClockController::removeClock(Clock *clock)
 {
-    QMutexLocker l(&m_mutex);
     qCDebug(qLcClock) << "removeClock" << clock;
     m_clocks.removeAll(clock);
     if (m_master == clock) {
         // find a new master clock
-        m_master = nullptr;
-        for (auto *c : qAsConst(m_clocks)) {
-            if (!m_master || m_master->type() < c->type())
-                m_master = c;
-        }
+        m_master = m_clocks.empty()
+                ? nullptr
+                : *std::max_element(m_clocks.begin(), m_clocks.end(), compareClocks);
     }
 }
 
+bool QFFmpeg::ClockController::isMaster(const Clock *clock) const
+{
+    return m_master.loadAcquire() == clock;
+}
+
+qint64 QFFmpeg::ClockController::currentTimeNoLock() const
+{
+    return m_isPaused ? m_baseTime : m_baseTime + m_elapsedTimer.elapsed() / m_playbackRate;
+}
+
 qint64 QFFmpeg::ClockController::currentTime() const
 {
     QMutexLocker l(&m_mutex);
@@ -138,41 +149,61 @@ qint64 QFFmpeg::ClockController::currentTime() const
 
 void QFFmpeg::ClockController::syncTo(qint64 usecs)
 {
-    QMutexLocker l(&m_mutex);
-    qCDebug(qLcClock) << "syncTo" << usecs;
-    m_baseTime = usecs;
-    m_seekTime = usecs;
-    m_timer.restart();
+    {
+        QMutexLocker l(&m_mutex);
+        qCDebug(qLcClock) << "syncTo" << usecs;
+        m_baseTime = usecs;
+        m_seekTime = usecs;
+        m_elapsedTimer.restart();
+    }
+
     for (auto *p : qAsConst(m_clocks))
         p->syncTo(usecs);
 }
 
-void QFFmpeg::ClockController::setPlaybackRate(float s)
+void QFFmpeg::ClockController::setPlaybackRate(float rate)
 {
-    QMutexLocker l(&m_mutex);
-    qCDebug(qLcClock) << "setPlaybackRate" << s;
-    m_baseTime = currentTimeNoLock();
-    m_timer.restart();
-    m_playbackRate = s;
+    qint64 baseTime = 0;
+    {
+        qCDebug(qLcClock) << "setPlaybackRate" << rate;
+
+        QMutexLocker l(&m_mutex);
+
+        m_baseTime = baseTime = currentTimeNoLock();
+        m_elapsedTimer.restart();
+        m_playbackRate = rate;
+    }
+
     for (auto *p : qAsConst(m_clocks))
-        p->setPlaybackRate(s, m_baseTime);
+        p->setPlaybackRate(rate, baseTime);
 }
 
 void QFFmpeg::ClockController::setPaused(bool paused)
 {
-    QMutexLocker l(&m_mutex);
-    if (m_isPaused == paused)
-        return;
-    qCDebug(qLcClock) << "setPaused" << paused;
-    m_isPaused = paused;
-    if (m_isPaused) {
-        m_baseTime = currentTimeNoLock();
-        m_seekTime = m_baseTime;
-    } else {
-        m_timer.restart();
+    {
+        QMutexLocker l(&m_mutex);
+        if (m_isPaused == paused)
+            return;
+        qCDebug(qLcClock) << "setPaused" << paused;
+        m_isPaused = paused;
+        if (m_isPaused) {
+            m_baseTime = currentTimeNoLock();
+            m_seekTime = m_baseTime;
+        } else {
+            m_elapsedTimer.restart();
+        }
     }
+
     for (auto *p : qAsConst(m_clocks))
         p->setPaused(paused);
 }
 
+void QFFmpeg::ClockController::setNotify(QObject *object, QMetaMethod method)
+{
+    QMutexLocker l(&m_mutex);
+
+    notifyObject = object;
+    notify = method;
+}
+
 QT_END_NAMESPACE
diff --git a/src/plugins/multimedia/ffmpeg/qffmpegclock_p.h b/src/plugins/multimedia/ffmpeg/qffmpegclock_p.h
index 2abc53666..b09f7bd6d 100644
--- a/src/plugins/multimedia/ffmpeg/qffmpegclock_p.h
+++ b/src/plugins/multimedia/ffmpeg/qffmpegclock_p.h
@@ -5,6 +5,7 @@
 
 #include "qffmpeg_p.h"
 
+#include <qatomic.h>
 #include <qelapsedtimer.h>
 #include <qlist.h>
 #include <qmutex.h>
@@ -59,9 +60,9 @@ class ClockController
 {
     mutable QMutex m_mutex;
     QList<Clock *> m_clocks;
-    Clock *m_master = nullptr;
+    QAtomicPointer<Clock> m_master = nullptr;
 
-    QElapsedTimer m_timer;
+    QElapsedTimer m_elapsedTimer;
     qint64 m_baseTime = 0;
     qint64 m_seekTime = 0;
     float m_playbackRate = 1.;
@@ -70,15 +71,17 @@ class ClockController
     qint64 m_lastMasterTime = 0;
     QObject *notifyObject = nullptr;
     QMetaMethod notify;
-    qint64 currentTimeNoLock() const { return m_isPaused ? m_baseTime : m_baseTime + m_timer.elapsed()/m_playbackRate; }
+    qint64 currentTimeNoLock() const;
 
     friend class Clock;
     qint64 timeUpdated(Clock *clock, qint64 time);
     void addClock(Clock *provider);
     void removeClock(Clock *provider);
+    bool isMaster(const Clock *clock) const;
+
 public:
-    // max 25 msecs tolerance for the clock
-    enum { ClockTolerance = 25000 };
+    // max 5 msecs tolerance for the clock
+    enum { NotificationTolerance = 5000 };
     ClockController() = default;
     ~ClockController();
 
@@ -91,11 +94,7 @@ public:
     float playbackRate() const { return m_playbackRate; }
     void setPaused(bool paused);
 
-    void setNotify(QObject *object, QMetaMethod method)
-    {
-        notifyObject = object;
-        notify = method;
-    }
+    void setNotify(QObject *object, QMetaMethod method);
 };
 
 inline float Clock::playbackRate() const
@@ -105,7 +104,7 @@ inline float Clock::playbackRate() const
 
 inline bool Clock::isMaster() const
 {
-    return controller ? controller->m_master == this : false;
+    return controller && controller->isMaster(this);
 }
 
 inline qint64 Clock::seekTime() const
diff --git a/src/plugins/multimedia/ffmpeg/qffmpegdecoder.cpp b/src/plugins/multimedia/ffmpeg/qffmpegdecoder.cpp
index ad6ddc53d..66e2c28c3 100644
--- a/src/plugins/multimedia/ffmpeg/qffmpegdecoder.cpp
+++ b/src/plugins/multimedia/ffmpeg/qffmpegdecoder.cpp
@@ -731,8 +731,9 @@ void VideoRenderer::loop()
         nextFrameTime = startTime + duration;
     streamDecoder->unlockAndReleaseFrame();
     qint64 mtime = timeUpdated(startTime);
-    timeOut = usecsTo(mtime, nextFrameTime)/1000;
-//    qDebug() << "    next video frame in" << startTime << nextFrameTime << currentTime() << timeOut;
+    timeOut = usecsTo(mtime, nextFrameTime) / 1000;
+    //    qCDebug(qLcVideoRenderer) << "    next video frame in" << startTime << nextFrameTime <<
+    //    currentTime() << timeOut;
 }
 
 AudioRenderer::AudioRenderer(Decoder *decoder, QAudioOutput *output)
@@ -745,6 +746,8 @@ AudioRenderer::AudioRenderer(Decoder *decoder, QAudioOutput *output)
 
 void AudioRenderer::syncTo(qint64 usecs)
 {
+    QMutexLocker locker(&mutex);
+
     Clock::syncTo(usecs);
     audioBaseTime = usecs;
     processedBase = processedUSecs;
@@ -752,6 +755,8 @@ void AudioRenderer::syncTo(qint64 usecs)
 
 void AudioRenderer::setPlaybackRate(float rate, qint64 currentTime)
 {
+    QMutexLocker locker(&mutex);
+
     audioBaseTime = currentTime;
     processedBase = processedUSecs;
     Clock::setPlaybackRate(rate, currentTime);
@@ -893,8 +898,8 @@ void AudioRenderer::loop()
 //        qDebug() << ">>>>>>>>>>>>>>>>>>>>>>>> could not write all data" << (bufferedData.size() - bufferWritten);
 //    qDebug() << "Audio: processed" << processedUSecs << "written" << writtenUSecs
 //             << "delta" << (writtenUSecs - processedUSecs) << "timeOut" << timeOut;
-//    qDebug() << "    updating time to" << currentTimeNoLock();
-    timeUpdated(audioBaseTime + (processedUSecs - processedBase)*playbackRate());
+//    qCDebug(qLcAudioRenderer) << "    updating time to" << currentTimeNoLock();
+    timeUpdated(audioBaseTime + qRound((processedUSecs - processedBase) * playbackRate()));
 }
 
 void AudioRenderer::streamChanged()
@@ -1324,11 +1329,7 @@ void Decoder::seek(qint64 pos)
 
 void Decoder::setPlaybackRate(float rate)
 {
-    if (m_state == QMediaPlayer::PlayingState)
-        setPaused(true);
     clockController.setPlaybackRate(rate);
-    if (m_state == QMediaPlayer::PlayingState)
-        setPaused(false);
 }
 
 void Decoder::updateCurrentTime(qint64 time)