Improve and prettify logic and usage of QAudioStateMachine

* both audio state + error now are handled atomically together.
* internal synchronization is not need for current purposes;
  it has been addressed.
* use acq/rel and relaxed memory order depending on the situation
  instead of the default non-optimal sequential order.
* add new unit tests and remove some of old ones that are not
  relevant o the refactoring.

Pick-to: 6.5 6.6
Change-Id: I644d8e67c933c0d98556a00d4a614fce879e37c2
Reviewed-by: Jøger Hansegård <joger.hansegard@qt.io>
Reviewed-by: Artem Dyomin <artem.dyomin@qt.io>

3 files changed, 113 insertions, 166 deletions

diff --git a/src/multimedia/audio/qaudiostatemachine.cpp b/src/multimedia/audio/qaudiostatemachine.cpp
index 83716efd2..2d42200e6 100644
--- a/src/multimedia/audio/qaudiostatemachine.cpp
+++ b/src/multimedia/audio/qaudiostatemachine.cpp
@@ -11,37 +11,7 @@ QT_BEGIN_NAMESPACE
 using Notifier = QAudioStateMachine::Notifier;
 using namespace AudioStateMachineUtils;
 
-struct QAudioStateMachine::Synchronizer {
-    QMutex m_mutex;
-    QWaitCondition m_condition;
-
-    template <typename Changer>
-    void changeState(Changer&& changer) {
-        {
-            QMutexLocker locker(&m_mutex);
-            changer();
-        }
-
-        m_condition.notify_all();
-    }
-
-    void waitForOperationFinished(std::atomic<RawState>& state)
-    {
-        QMutexLocker locker(&m_mutex);
-        while (isWaitingState(state))
-            m_condition.wait(&m_mutex);
-    }
-
-    void waitForDrained(std::atomic<RawState>& state, std::chrono::milliseconds timeout) {
-        QMutexLocker locker(&m_mutex);
-        if (isDrainingState(state))
-            m_condition.wait(&m_mutex, timeout.count());
-    }
-};
-
-QAudioStateMachine::QAudioStateMachine(QAudioStateChangeNotifier &notifier, bool synchronize) :
-    m_notifier(&notifier),
-    m_sychronizer(synchronize ? std::make_unique<Synchronizer>() : nullptr)
+QAudioStateMachine::QAudioStateMachine(QAudioStateChangeNotifier &notifier) : m_notifier(&notifier)
 {
 }
 
@@ -49,98 +19,70 @@ QAudioStateMachine::~QAudioStateMachine() = default;
 
 QAudio::State QAudioStateMachine::state() const
 {
-    return toAudioState(m_state);
+    return toAudioState(m_state.load(std::memory_order_acquire));
 }
 
 QAudio::Error QAudioStateMachine::error() const
 {
-    return m_error;
+    return toAudioError(m_state.load(std::memory_order_acquire));
 }
 
-Notifier QAudioStateMachine::changeState(std::pair<RawState, uint32_t> prevStatesSet,
-                                         RawState newState, QAudio::Error error, bool shouldDrain)
+template <typename StatesChecker, typename NewState>
+Notifier QAudioStateMachine::changeState(const StatesChecker &checker, const NewState &newState)
 {
-    auto checkState = [flags = prevStatesSet.second](RawState state) {
-        return (flags >> state) & 1;
-    };
-
-    if (!m_sychronizer) {
-        RawState prevState = prevStatesSet.first;
-        const auto exchanged = multipleCompareExchange(
-                m_state, prevState, newState, checkState);
+    if constexpr (std::is_same_v<RawState, NewState>)
+        return changeState(checker, [newState](RawState) { return newState; });
+    else {
+        RawState prevState = m_state.load(std::memory_order_relaxed);
+        const auto exchanged = multipleCompareExchange(m_state, prevState, checker, newState);
 
         if (Q_LIKELY(exchanged))
-            return { this, newState, prevState, error };
+            return { this, newState(prevState), prevState };
 
         return {};
     }
-
-    while (true) {
-        RawState prevState = prevStatesSet.first;
-
-        const auto newWaitingState = newState | (shouldDrain ? WaitingFlags : InProgressFlag);
-
-        const auto exchanged = multipleCompareExchange(
-                m_state, prevState, newWaitingState, [checkState](RawState state) {
-                    return !isWaitingState(state) && checkState(state);
-                });
-
-        if (Q_LIKELY(exchanged))
-            return { this, newState, prevState, error };
-
-        if (!isWaitingState(prevState))
-            return {};
-
-        if (!checkState(fromWaitingState(prevState)))
-            return {};
-
-        m_sychronizer->waitForOperationFinished(m_state);
-    }
 }
 
 Notifier QAudioStateMachine::stop(QAudio::Error error, bool shouldDrain, bool forceUpdateError)
 {
-    auto result = changeState(
-            makeStatesSet(QAudio::ActiveState, QAudio::IdleState, QAudio::SuspendedState),
-            QAudio::StoppedState, error, shouldDrain);
+    auto statesChecker =
+            makeStatesChecker(QAudio::ActiveState, QAudio::IdleState, QAudio::SuspendedState,
+                              forceUpdateError ? QAudio::StoppedState : QAudio::ActiveState);
 
-    if (!result && forceUpdateError)
-        setError(error);
+    const auto state = toRawState(QAudio::StoppedState, error);
+    auto getNewState = [&](RawState prevState) {
+        const bool shouldAddFlag = shouldDrain && toAudioState(prevState) == QAudio::ActiveState;
+        return shouldAddFlag ? addDrainingFlag(state) : state;
+    };
 
-    return result;
+    return changeState(statesChecker, getNewState);
 }
 
 Notifier QAudioStateMachine::start(bool active)
 {
-    return changeState(makeStatesSet(QAudio::StoppedState),
-                       active ? QAudio::ActiveState : QAudio::IdleState);
+    return changeState(makeStatesChecker(QAudio::StoppedState),
+                       toRawState(active ? QAudio::ActiveState : QAudio::IdleState));
 }
 
-void QAudioStateMachine::waitForDrained(std::chrono::milliseconds timeout)
+bool QAudioStateMachine::isActiveOrIdle() const
 {
-    if (m_sychronizer)
-        m_sychronizer->waitForDrained(m_state, timeout);
+    const auto state = this->state();
+    return state == QAudio::ActiveState || state == QAudio::IdleState;
 }
 
-void QAudioStateMachine::onDrained()
+bool QAudioStateMachine::onDrained()
 {
-    if (m_sychronizer)
-        m_sychronizer->changeState([this]() { m_state &= ~DrainingFlag; });
+    return changeState(isDrainingState, removeDrainingFlag);
 }
 
 bool QAudioStateMachine::isDraining() const
 {
-    return isDrainingState(m_state);
-}
-
-bool QAudioStateMachine::isActiveOrIdle() const {
-    const auto state = this->state();
-    return state == QAudio::ActiveState || state == QAudio::IdleState;
+    return isDrainingState(m_state.load(std::memory_order_acquire));
 }
 
 std::pair<bool, bool> QAudioStateMachine::getDrainedAndStopped() const
 {
-    const RawState state = m_state;
+    const auto state = m_state.load(std::memory_order_acquire);
     return { !isDrainingState(state), toAudioState(state) == QAudio::StoppedState };
 }
 
@@ -149,11 +91,11 @@ Notifier QAudioStateMachine::suspend()
     // Due to the current documentation, we set QAudio::NoError.
     // TBD: leave the previous error should be more reasonable (IgnoreError)
     const auto error = QAudio::NoError;
-    auto result = changeState(makeStatesSet(QAudio::ActiveState, QAudio::IdleState),
-                              QAudio::SuspendedState, error);
+    auto result = changeState(makeStatesChecker(QAudio::ActiveState, QAudio::IdleState),
+                              toRawState(QAudio::SuspendedState, error));
 
     if (result)
-        m_suspendedInState = result.prevState();
+        m_suspendedInState = result.prevAudioState();
 
     return result;
 }
@@ -163,53 +105,46 @@ Notifier QAudioStateMachine::resume()
     // Due to the current documentation, we set QAudio::NoError.
     // TBD: leave the previous error should be more reasonable (IgnoreError)
     const auto error = QAudio::NoError;
-    return changeState(makeStatesSet(QAudio::SuspendedState), m_suspendedInState, error);
+    return changeState(makeStatesChecker(QAudio::SuspendedState),
+                       toRawState(m_suspendedInState, error));
 }
 
 Notifier QAudioStateMachine::activateFromIdle()
 {
-    return changeState(makeStatesSet(QAudio::IdleState), QAudio::ActiveState);
+    return changeState(makeStatesChecker(QAudio::IdleState), toRawState(QAudio::ActiveState));
 }
 
 Notifier QAudioStateMachine::updateActiveOrIdle(bool isActive, QAudio::Error error)
 {
     const auto state = isActive ? QAudio::ActiveState : QAudio::IdleState;
-    return changeState(makeStatesSet(QAudio::ActiveState, QAudio::IdleState), state, error);
+    return changeState(makeStatesChecker(QAudio::ActiveState, QAudio::IdleState),
+                       toRawState(state, error));
 }
 
-void QAudioStateMachine::setError(QAudio::Error error)
+Notifier QAudioStateMachine::setError(QAudio::Error error)
 {
-    if (m_error.exchange(error) != error && m_notifier)
-        emit m_notifier->errorChanged(error);
+    auto fixState = [error](RawState prevState) { return setStateError(prevState, error); };
+    return changeState([](RawState) { return true; }, fixState);
 }
 
 Notifier QAudioStateMachine::forceSetState(QAudio::State state, QAudio::Error error)
 {
-    return changeState(makeStatesSet(QAudio::ActiveState, QAudio::IdleState, QAudio::SuspendedState,
-                                     QAudio::StoppedState),
-                       state, error);
+    return changeState([](RawState) { return true; }, toRawState(state, error));
 }
 
-void QAudioStateMachine::reset(RawState state, RawState prevState, QAudio::Error error)
+void QAudioStateMachine::reset(RawState state, RawState prevState)
 {
-    Q_ASSERT(!isWaitingState(state));
-
-    if (!m_sychronizer && m_state != state)
-        return;
-
-    const auto isErrorChanged = m_error.exchange(error) != error;
-
-    if (m_sychronizer)
-        m_sychronizer->changeState([&](){ m_state = state; });
-
     auto notifier = m_notifier;
 
-    if (prevState != state && notifier)
-        emit notifier->stateChanged(toAudioState(state));
+    const auto audioState = toAudioState(state);
+    const auto audioError = toAudioError(state);
+
+    if (toAudioState(prevState) != audioState && notifier)
+        emit notifier->stateChanged(audioState);
 
     // check the notifier in case the object was deleted in
-    if (isErrorChanged && notifier)
-        emit notifier->errorChanged(error);
+    if (toAudioError(prevState) != audioError && notifier)
+        emit notifier->errorChanged(audioError);
 }
 
 QT_END_NAMESPACE
diff --git a/src/multimedia/audio/qaudiostatemachine_p.h b/src/multimedia/audio/qaudiostatemachine_p.h
index b3207230b..385453020 100644
--- a/src/multimedia/audio/qaudiostatemachine_p.h
+++ b/src/multimedia/audio/qaudiostatemachine_p.h
@@ -14,12 +14,11 @@
 //
 // We mean it.
 //
+
 #include "qaudiostatemachineutils_p.h"
-#include <qmutex.h>
-#include <qwaitcondition.h>
+
 #include <qpointer.h>
 #include <atomic>
-#include <chrono>
 
 QT_BEGIN_NAMESPACE
 
@@ -45,7 +44,7 @@ public:
         void reset()
         {
             if (auto stateMachine = std::exchange(m_stateMachine, nullptr))
-                stateMachine->reset(m_state, m_prevState, m_error);
+                stateMachine->reset(m_state, m_prevState);
         }
 
         ~Notifier() { reset(); }
@@ -54,29 +53,24 @@ public:
         Notifier(Notifier &&other) noexcept
             : m_stateMachine(std::exchange(other.m_stateMachine, nullptr)),
               m_state(other.m_state),
-              m_prevState(other.m_prevState),
-              m_error(other.m_error)
+              m_prevState(other.m_prevState)
         {
         }
 
         operator bool() const { return m_stateMachine != nullptr; }
 
-        void setError(QAudio::Error error) { m_error = error; }
+        QAudio::State prevAudioState() const { return AudioStateMachineUtils::toAudioState(m_prevState); }
 
-        // Can be added make state changing more flexible
-        // but needs some investigation to ensure state change consistency
-        // The method is supposed to be used for sync read/write
-        // under "notifier = updateActiveOrIdle(isActive)"
-        // void setState(QAudio::State state) { ... }
+        QAudio::State audioState() const { return AudioStateMachineUtils::toAudioState(m_state); }
 
-        bool isStateChanged() const { return m_state != m_prevState; }
+        bool isDraining() const { return AudioStateMachineUtils::isDrainingState(m_state); }
 
-        QAudio::State prevState() const { return QAudio::State(m_prevState); }
+        bool isStateChanged() const { return prevAudioState() != audioState(); }
 
     private:
         Notifier(QAudioStateMachine *stateMachine = nullptr, RawState state = QAudio::StoppedState,
-                 RawState prevState = QAudio::StoppedState, QAudio::Error error = QAudio::NoError)
-            : m_stateMachine(stateMachine), m_state(state), m_prevState(prevState), m_error(error)
+                 RawState prevState = QAudio::StoppedState)
+            : m_stateMachine(stateMachine), m_state(state), m_prevState(prevState)
         {
         }
 
@@ -84,12 +78,11 @@ public:
         QAudioStateMachine *m_stateMachine;
         RawState m_state;
         const RawState m_prevState;
-        QAudio::Error m_error;
 
         friend class QAudioStateMachine;
     };
 
-    QAudioStateMachine(QAudioStateChangeNotifier &notifier, bool synchronize = true);
+    QAudioStateMachine(QAudioStateChangeNotifier &notifier);
 
     ~QAudioStateMachine();
 
@@ -97,20 +90,18 @@ public:
 
     QAudio::Error error() const;
 
-    bool isDraining() const;
-
     bool isActiveOrIdle() const;
 
+    bool isDraining() const;
+
     // atomicaly checks if the state is stopped and marked as drained
     std::pair<bool, bool> getDrainedAndStopped() const;
 
-    // waits if the method stop(error, true) has bee called
-    void waitForDrained(std::chrono::milliseconds timeout);
-
-    // mark as drained and wake up the method waitForDrained
-    void onDrained();
+    // Stopped[draining] -> Stopped
+    bool onDrained();
 
     // Active/Idle/Suspended -> Stopped
+    // or Active -> Stopped[draining] for shouldDrain = true
     Notifier stop(QAudio::Error error = QAudio::NoError, bool shouldDrain = false,
                   bool forceUpdateError = false);
 
@@ -139,22 +130,18 @@ public:
     Notifier forceSetState(QAudio::State state, QAudio::Error error = QAudio::NoError);
 
     // force set the error
-    void setError(QAudio::Error error);
+    Notifier setError(QAudio::Error error);
 
 private:
-    Notifier changeState(std::pair<RawState, uint32_t> prevStatesSet, RawState state,
-                         QAudio::Error error = QAudio::NoError, bool shouldDrain = false);
+    template <typename StatesChecker, typename NewState>
+    Notifier changeState(const StatesChecker &statesChecker, const NewState &newState);
 
-    void reset(RawState state, RawState prevState, QAudio::Error error);
+    void reset(RawState state, RawState prevState);
 
 private:
     QPointer<QAudioStateChangeNotifier> m_notifier;
     std::atomic<RawState> m_state = QAudio::StoppedState;
-    std::atomic<QAudio::Error> m_error = QAudio::NoError;
-    RawState m_suspendedInState = QAudio::SuspendedState;
-
-    struct Synchronizer;
-    std::unique_ptr<Synchronizer> m_sychronizer;
+    QAudio::State m_suspendedInState = QAudio::SuspendedState;
 };
 
 QT_END_NAMESPACE
diff --git a/src/multimedia/audio/qaudiostatemachineutils_p.h b/src/multimedia/audio/qaudiostatemachineutils_p.h
index a1c5ef579..f80517f77 100644
--- a/src/multimedia/audio/qaudiostatemachineutils_p.h
+++ b/src/multimedia/audio/qaudiostatemachineutils_p.h
@@ -20,47 +20,72 @@
 QT_BEGIN_NAMESPACE
 
 namespace AudioStateMachineUtils {
+
 using RawState = int;
-constexpr RawState DrainingFlag = 1 << 16;
-constexpr RawState InProgressFlag = 1 << 17;
-constexpr RawState WaitingFlags = DrainingFlag | InProgressFlag;
 
-constexpr bool isWaitingState(RawState state)
-{
-    return (state & WaitingFlags) != 0;
-}
+constexpr uint32_t AudioStateBitsCount = 8;
+constexpr RawState AudioStateMask = 0xFF;
+constexpr RawState AudioErrorMask = 0xFF00;
+constexpr RawState DrainingFlag = 0x10000;
+
+static_assert(!(AudioStateMask & DrainingFlag) && !(AudioStateMask & AudioErrorMask)
+                      && !(AudioErrorMask & DrainingFlag),
+              "Invalid masks");
 
 constexpr bool isDrainingState(RawState state)
 {
     return (state & DrainingFlag) != 0;
 }
 
-constexpr RawState fromWaitingState(RawState state)
+constexpr RawState addDrainingFlag(RawState state)
 {
-    return state & ~WaitingFlags;
+    return state | DrainingFlag;
+}
+
+constexpr RawState removeDrainingFlag(RawState state)
+{
+    return state & ~DrainingFlag;
 }
 
 constexpr QAudio::State toAudioState(RawState state)
 {
-    return QAudio::State(fromWaitingState(state));
+    return QAudio::State(state & AudioStateMask);
+}
+
+constexpr QAudio::Error toAudioError(RawState state)
+{
+    return QAudio::Error((state & AudioErrorMask) >> AudioStateBitsCount);
+}
+
+constexpr RawState toRawState(QAudio::State state, QAudio::Error error = QAudio::NoError)
+{
+    return state | (error << AudioStateBitsCount);
+}
+
+constexpr RawState setStateError(RawState state, QAudio::Error error)
+{
+    return (error << AudioStateBitsCount) | (state & ~AudioErrorMask);
 }
 
 template <typename... States>
-constexpr std::pair<RawState, uint32_t> makeStatesSet(QAudio::State first, States... others)
+constexpr auto makeStatesChecker(States... states)
 {
-    return { first, ((1 << first) | ... | (1 << others)) };
+    return [=](RawState state) {
+        state &= (AudioStateMask | DrainingFlag);
+        return (... || (state == states));
+    };
 }
 
 // ensures compareExchange (testAndSet) operation with opportunity
 // to check several states, can be considered as atomic
-template <typename T, typename Predicate>
-bool multipleCompareExchange(std::atomic<T> &target, T &prevValue, T newValue, Predicate predicate)
+template <typename T, typename Predicate, typename NewValueGetter>
+bool multipleCompareExchange(std::atomic<T> &target, T &prevValue, Predicate predicate,
+                             NewValueGetter newValueGetter)
 {
-    Q_ASSERT(predicate(prevValue));
-    do {
-        if (target.compare_exchange_strong(prevValue, newValue))
+    while (predicate(prevValue))
+        if (target.compare_exchange_strong(prevValue, newValueGetter(prevValue),
+                                           std::memory_order_acq_rel))
             return true;
-    } while (predicate(prevValue));
 
     return false;
 }