From 834e008f4772462c536726b80cb3e4e19b851b53 Mon Sep 17 00:00:00 2001 From: Artem Dyomin Date: Mon, 11 Sep 2023 15:44:52 +0200 Subject: Improve and prettify logic and usage of QAudioStateMachine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 Reviewed-by: Artem Dyomin --- src/multimedia/audio/qaudiostatemachine.cpp | 163 +++++++---------------- src/multimedia/audio/qaudiostatemachine_p.h | 55 +++----- src/multimedia/audio/qaudiostatemachineutils_p.h | 61 ++++++--- 3 files changed, 113 insertions(+), 166 deletions(-) (limited to 'src/multimedia/audio') 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 - void changeState(Changer&& changer) { - { - QMutexLocker locker(&m_mutex); - changer(); - } - - m_condition.notify_all(); - } - - void waitForOperationFinished(std::atomic& state) - { - QMutexLocker locker(&m_mutex); - while (isWaitingState(state)) - m_condition.wait(&m_mutex); - } - - void waitForDrained(std::atomic& state, std::chrono::milliseconds timeout) { - QMutexLocker locker(&m_mutex); - if (isDrainingState(state)) - m_condition.wait(&m_mutex, timeout.count()); - } -}; - -QAudioStateMachine::QAudioStateMachine(QAudioStateChangeNotifier ¬ifier, bool synchronize) : - m_notifier(¬ifier), - m_sychronizer(synchronize ? std::make_unique() : nullptr) +QAudioStateMachine::QAudioStateMachine(QAudioStateChangeNotifier ¬ifier) : m_notifier(¬ifier) { } @@ -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 prevStatesSet, - RawState newState, QAudio::Error error, bool shouldDrain) +template +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) + 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 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 -#include + #include #include -#include 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 ¬ifier, bool synchronize = true); + QAudioStateMachine(QAudioStateChangeNotifier ¬ifier); ~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 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 prevStatesSet, RawState state, - QAudio::Error error = QAudio::NoError, bool shouldDrain = false); + template + 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 m_notifier; std::atomic m_state = QAudio::StoppedState; - std::atomic m_error = QAudio::NoError; - RawState m_suspendedInState = QAudio::SuspendedState; - - struct Synchronizer; - std::unique_ptr 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 -constexpr std::pair 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 -bool multipleCompareExchange(std::atomic &target, T &prevValue, T newValue, Predicate predicate) +template +bool multipleCompareExchange(std::atomic &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; } -- cgit v1.2.3