From feffdf7cfef2d2ca69906aca9a11fe46acee7fa4 Mon Sep 17 00:00:00 2001 From: Artem Dyomin Date: Mon, 17 Apr 2023 13:33:52 +0200 Subject: Refactor DarwinAudioSink and PulseAudioSink and run CI test - the main change: implemented QAudioStateMachine allowing atomic state changing. The main reasons for creating the state machine: * get rid of state changing race conditions without mutex lockings and contentions. * it's good to have a common approach for audio states changings and notifications. The implementation of the state machine supports synchronization of actions after states toggling (turned off for pulse slink sinse it has own synchronization approach). Other audio sinks and sources might be refactored in the future. - remove a hack in PulseAudioSink with a callback based on QObject::destroyed connection, the hack appeared to cause a memory leak time-to-time. - the main business logic is not changed Testing: - tested manually on linux and macOS - auto tests * Linux QAudioSink tests became working locally and on CI. * macOS tests work locally, on CI only one test is blacklisted due to the issue not related to race conditions. Task-number: QTBUG-113194 Pick-to: 6.5 6.6 Change-Id: I68002c243dea874c4309b14b1fbd7b618392ab1f Reviewed-by: Qt CI Bot Reviewed-by: Artem Dyomin --- src/multimedia/audio/qaudiostatemachine.cpp | 264 ++++++++++++++++++++++++++++ src/multimedia/audio/qaudiostatemachine_p.h | 167 ++++++++++++++++++ src/multimedia/audio/qaudiosystem.cpp | 9 +- src/multimedia/audio/qaudiosystem_p.h | 23 +-- 4 files changed, 448 insertions(+), 15 deletions(-) create mode 100644 src/multimedia/audio/qaudiostatemachine.cpp create mode 100644 src/multimedia/audio/qaudiostatemachine_p.h (limited to 'src/multimedia/audio') diff --git a/src/multimedia/audio/qaudiostatemachine.cpp b/src/multimedia/audio/qaudiostatemachine.cpp new file mode 100644 index 000000000..4073abd48 --- /dev/null +++ b/src/multimedia/audio/qaudiostatemachine.cpp @@ -0,0 +1,264 @@ +// Copyright (C) 2023 The Qt Company Ltd. +// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR LGPL-3.0-only OR GPL-2.0-only OR GPL-3.0-only + +#include "qaudiostatemachine_p.h" +#include "qaudiosystem_p.h" +#include +#include + +QT_BEGIN_NAMESPACE + +using Guard = QAudioStateMachine::StateChangeGuard; +using RawState = QAudioStateMachine::RawState; + +namespace { +constexpr RawState DrainingFlag = 1 << 16; +constexpr RawState InProgressFlag = 1 << 17; +constexpr RawState WaitingFlags = DrainingFlag | InProgressFlag; + +const auto IgnoreError = static_cast(-1); + +inline bool isWaitingState(RawState state) +{ + return (state & WaitingFlags) != 0; +} + +inline bool isDrainingState(RawState state) +{ + return (state & DrainingFlag) != 0; +} + +inline RawState fromWaitingState(RawState state) +{ + return state & ~WaitingFlags; +} + +inline QAudio::State toAudioState(RawState state) +{ + return QAudio::State(fromWaitingState(state)); +} + +template +constexpr std::pair makeStatesSet(QAudio::State first, States... others) +{ + return { first, ((1 << first) | ... | (1 << others)) }; +} + +// 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) +{ + Q_ASSERT(predicate(prevValue)); + do { + if (target.compare_exchange_strong(prevValue, newValue)) + return true; + } while (predicate(prevValue)); + + return false; +} + +} // namespace + +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() = default; + +QAudio::State QAudioStateMachine::state() const +{ + return toAudioState(m_state); +} + +QAudio::Error QAudioStateMachine::error() const +{ + return m_error; +} + +Guard QAudioStateMachine::changeState(std::pair prevStatesSet, + RawState newState, QAudio::Error error, bool shouldDrain) +{ + 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 (Q_LIKELY(exchanged)) + return { this, newState, prevState, error }; + + 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); + } +} + +Guard QAudioStateMachine::stop(QAudio::Error error, bool shouldDrain, bool forceUpdateError) +{ + auto result = changeState( + makeStatesSet(QAudio::ActiveState, QAudio::IdleState, QAudio::SuspendedState), + QAudio::StoppedState, error, shouldDrain); + + if (!result && forceUpdateError) + setError(error); + + return result; +} + +Guard QAudioStateMachine::start(bool active) +{ + return changeState(makeStatesSet(QAudio::StoppedState), + active ? QAudio::ActiveState : QAudio::IdleState); +} + +void QAudioStateMachine::waitForDrained(std::chrono::milliseconds timeout) +{ + if (m_sychronizer) + m_sychronizer->waitForDrained(m_state, timeout); +} + +void QAudioStateMachine::onDrained() +{ + if (m_sychronizer) + m_sychronizer->changeState([this]() { m_state &= ~DrainingFlag; }); +} + +bool QAudioStateMachine::isDraining() const +{ + return isDrainingState(m_state); +} + +bool QAudioStateMachine::isActiveOrIdle() const { + const auto state = this->state(); + return state == QAudio::ActiveState || state == QAudio::IdleState; +} + +std::pair QAudioStateMachine::getDrainedAndStopped() const +{ + const RawState state = m_state; + return { !isDrainingState(state), toAudioState(state) == QAudio::StoppedState }; +} + +Guard 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); + + if (result) + m_suspendedInState = result.prevState(); + + return result; +} + +Guard 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); +} + +Guard QAudioStateMachine::activateFromIdle() +{ + return changeState(makeStatesSet(QAudio::IdleState), QAudio::ActiveState); +} + +Guard QAudioStateMachine::updateActiveOrIdle(bool isActive, QAudio::Error error) +{ + const auto state = isActive ? QAudio::ActiveState : QAudio::IdleState; + return changeState(makeStatesSet(QAudio::ActiveState, QAudio::IdleState), state, error); +} + +void QAudioStateMachine::setError(QAudio::Error error) +{ + if (m_error.exchange(error) != error && m_notifier) + emit m_notifier->errorChanged(error); +} + +Guard QAudioStateMachine::forceSetState(QAudio::State state, QAudio::Error error) +{ + return changeState(makeStatesSet(QAudio::ActiveState, QAudio::IdleState, QAudio::SuspendedState, + QAudio::StoppedState), + state, error); +} + +void QAudioStateMachine::reset(RawState state, RawState prevState, QAudio::Error error) +{ + Q_ASSERT(!isWaitingState(state)); + + if (!m_sychronizer && m_state != state) + return; + + const auto isErrorChanged = error != IgnoreError && 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)); + + // check the notifier in case the object was deleted in + if (isErrorChanged && notifier) + emit notifier->errorChanged(error); +} + +QT_END_NAMESPACE diff --git a/src/multimedia/audio/qaudiostatemachine_p.h b/src/multimedia/audio/qaudiostatemachine_p.h new file mode 100644 index 000000000..44bef3ab0 --- /dev/null +++ b/src/multimedia/audio/qaudiostatemachine_p.h @@ -0,0 +1,167 @@ +// Copyright (C) 2023 The Qt Company Ltd. +// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR LGPL-3.0-only OR GPL-2.0-only OR GPL-3.0-only + +#ifndef QAUDIOSTATEMACHINE_P_H +#define QAUDIOSTATEMACHINE_P_H + +// +// W A R N I N G +// ------------- +// +// This file is not part of the Qt API. It exists purely as an +// implementation detail. This header file may change from version to +// version without notice, or even be removed. +// +// We mean it. +// + +#include + +#include +#include +#include +#include +#include + +QT_BEGIN_NAMESPACE + +class QAudioStateChangeNotifier; + +/* QAudioStateMachine provides an opportunity to + * toggle QAudio::State with QAudio::Error in + * a thread-safe manner. + * The toggling functions return a guard, + * which scope guaranties thread safety (if synchronization enabled) + * and notifies about the change via + * QAudioStateChangeNotifier::stateChanged and errorChanged. + * + * The state machine is supposed to be used mostly in + * QAudioSink and QAudioSource implementations. + */ +class Q_MULTIMEDIA_EXPORT QAudioStateMachine +{ +public: + using RawState = int; + class StateChangeGuard + { + public: + void reset() + { + if (auto stateMachine = std::exchange(m_stateMachine, nullptr)) + stateMachine->reset(m_state, m_prevState, m_error); + } + + ~StateChangeGuard() { reset(); } + + StateChangeGuard(const StateChangeGuard &) = delete; + StateChangeGuard(StateChangeGuard &&other) + : m_stateMachine(std::exchange(other.m_stateMachine, nullptr)), + m_state(other.m_state), + m_prevState(other.m_prevState), + m_error(other.m_error) + { + } + + operator bool() const { return m_stateMachine != nullptr; } + + void setError(QAudio::Error error) { m_error = error; } + + // 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 "guard = updateActiveOrIdle(isActive)" + // void setState(QAudio::State state) { ... } + + bool isStateChanged() const { return m_state != m_prevState; } + + QAudio::State prevState() const { return QAudio::State(m_prevState); } + + private: + StateChangeGuard(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) + { + } + + private: + QAudioStateMachine *m_stateMachine; + RawState m_state; + const RawState m_prevState; + QAudio::Error m_error; + + friend class QAudioStateMachine; + }; + + QAudioStateMachine(QAudioStateChangeNotifier ¬ifier, bool synchronize = true); + + ~QAudioStateMachine(); + + QAudio::State state() const; + + QAudio::Error error() const; + + bool isDraining() const; + + bool isActiveOrIdle() 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(); + + // Active/Idle/Suspended -> Stopped + StateChangeGuard stop(QAudio::Error error = QAudio::NoError, bool shouldDrain = false, + bool forceUpdateError = false); + + // Active/Idle/Suspended -> Stopped + StateChangeGuard stopOrUpdateError(QAudio::Error error = QAudio::NoError) + { + return stop(error, false, true); + } + + // Stopped -> Active/Idle + StateChangeGuard start(bool isActive = true); + + // Active/Idle -> Suspended + saves the exchanged state + StateChangeGuard suspend(); + + // Suspended -> saved state (Active/Idle) + StateChangeGuard resume(); + + // Idle -> Active + StateChangeGuard activateFromIdle(); + + // Active/Idle -> Active/Idle + updateError + StateChangeGuard updateActiveOrIdle(bool isActive, QAudio::Error error = QAudio::NoError); + + // Any -> Any; better use more strict methods + StateChangeGuard forceSetState(QAudio::State state, QAudio::Error error = QAudio::NoError); + + // force set the error + void setError(QAudio::Error error); + +private: + StateChangeGuard changeState(std::pair prevStatesSet, RawState state, + QAudio::Error error = QAudio::NoError, bool shouldDrain = false); + + void reset(RawState state, RawState prevState, QAudio::Error error); + +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; +}; + +QT_END_NAMESPACE + +#endif // QAUDIOSTATEMACHINE_P_H diff --git a/src/multimedia/audio/qaudiosystem.cpp b/src/multimedia/audio/qaudiosystem.cpp index b052b78a6..355771f6b 100644 --- a/src/multimedia/audio/qaudiosystem.cpp +++ b/src/multimedia/audio/qaudiosystem.cpp @@ -7,17 +7,16 @@ QT_BEGIN_NAMESPACE -QPlatformAudioSink::QPlatformAudioSink(QObject *parent) : QObject(parent) -{ - QPlatformMediaDevices::instance()->prepareAudio(); -} +QAudioStateChangeNotifier::QAudioStateChangeNotifier(QObject *parent) : QObject(parent) { } + +QPlatformAudioSink::QPlatformAudioSink(QObject *parent) : QAudioStateChangeNotifier(parent) { } qreal QPlatformAudioSink::volume() const { return 1.0; } -QPlatformAudioSource::QPlatformAudioSource(QObject *parent) : QObject(parent) { } +QPlatformAudioSource::QPlatformAudioSource(QObject *parent) : QAudioStateChangeNotifier(parent) { } QT_END_NAMESPACE diff --git a/src/multimedia/audio/qaudiosystem_p.h b/src/multimedia/audio/qaudiosystem_p.h index e85968b86..4a0650e80 100644 --- a/src/multimedia/audio/qaudiosystem_p.h +++ b/src/multimedia/audio/qaudiosystem_p.h @@ -28,7 +28,18 @@ QT_BEGIN_NAMESPACE class QIODevice; -class Q_MULTIMEDIA_EXPORT QPlatformAudioSink : public QObject +class Q_MULTIMEDIA_EXPORT QAudioStateChangeNotifier : public QObject +{ + Q_OBJECT +public: + QAudioStateChangeNotifier(QObject *parent = nullptr); + +signals: + void errorChanged(QAudio::Error error); + void stateChanged(QAudio::State state); +}; + +class Q_MULTIMEDIA_EXPORT QPlatformAudioSink : public QAudioStateChangeNotifier { Q_OBJECT @@ -52,13 +63,9 @@ public: virtual qreal volume() const; QElapsedTimer elapsedTime; - -Q_SIGNALS: - void errorChanged(QAudio::Error error); - void stateChanged(QAudio::State state); }; -class Q_MULTIMEDIA_EXPORT QPlatformAudioSource : public QObject +class Q_MULTIMEDIA_EXPORT QPlatformAudioSource : public QAudioStateChangeNotifier { Q_OBJECT @@ -82,10 +89,6 @@ public: virtual qreal volume() const = 0; QElapsedTimer elapsedTime; - -Q_SIGNALS: - void errorChanged(QAudio::Error error); - void stateChanged(QAudio::State state); }; QT_END_NAMESPACE -- cgit v1.2.3