diff options
author | Scott Murray <scott.murray@konsulko.com> | 2023-02-19 17:39:28 -0500 |
---|---|---|
committer | Scott Murray <scott.murray@konsulko.com> | 2023-02-21 17:41:22 +0000 |
commit | eb2f0cdd984307cec1df7ab3821f8af2248211df (patch) | |
tree | 0eaed6fb57fa9548a6652cbbef39dc42db888740 /mediaplayer | |
parent | 22ad8cbaad00d89361cbd92023dd20a807bcf5cb (diff) |
mediaplayer: Improve MPD failure handling
Changes:
- Avoid crashing in the MPD event handler code when our connection
times out because it has hung. Additionally, attempt to update
the front end UI to a sane state with respect to metadata when
this happens.
- Lower the MPD connection timeout and keepalive timeouts to try to
catch MPD failures faster. There are still problems in this area
with respect to the front end UI getting stuck if it does an
operation that would attempt to use the MPD API while it is hung.
Fixing that completely would require a significant refactoring to
move all the MPD API code to a separate thread so that the QML
rendering thread does not get stuck. It's unclear at present if
such a refactoring is justified, as the MPD hang in this case is
fixed by changes in MPD 0.23.8.
Bug-AGL: SPEC-4661
Signed-off-by: Scott Murray <scott.murray@konsulko.com>
Change-Id: Ia5e74d00acdc6b3b28cdf87b26486dbddfa02a98
(cherry picked from commit 2cbbf914242d0766ff4e97ee3c0d02505d13e422)
Diffstat (limited to 'mediaplayer')
-rw-r--r-- | mediaplayer/MediaplayerMpdBackend.cpp | 82 | ||||
-rw-r--r-- | mediaplayer/MediaplayerMpdBackend.h | 1 | ||||
-rw-r--r-- | mediaplayer/MpdEventHandler.cpp | 13 |
3 files changed, 66 insertions, 30 deletions
diff --git a/mediaplayer/MediaplayerMpdBackend.cpp b/mediaplayer/MediaplayerMpdBackend.cpp index 5cc9dce..8ffb346 100644 --- a/mediaplayer/MediaplayerMpdBackend.cpp +++ b/mediaplayer/MediaplayerMpdBackend.cpp @@ -19,14 +19,15 @@ #include "MpdEventHandler.h" #include "mediaplayer.h" -// Use a 60s timeout on our MPD connection +// Use a 30s timeout on our MPD connection // NOTE: The connection is actively poked at a higher frequency than // this to ensure we don't hit the timeout. The alternatives // are to either open and close a connection for every command, // or try to keep the connection in idle mode when not using it. // The latter is deemed too complicated for our purposes for now, // due to it likely requiring another thread. -#define MPD_CONNECTION_TIMEOUT 60000 +#define MPD_CONNECTION_TIMEOUT 30000 +#define MPD_KEEPALIVE_TIMEOUT 5000 MediaplayerMpdBackend::MediaplayerMpdBackend(Mediaplayer *player, QQmlContext *context, QObject *parent) : MediaplayerBackend(player, parent) @@ -43,7 +44,7 @@ MediaplayerMpdBackend::MediaplayerMpdBackend(Mediaplayer *player, QQmlContext *c // Set up connection keepalive timer m_mpd_conn_timer = new QTimer(this); connect(m_mpd_conn_timer, &QTimer::timeout, this, &MediaplayerMpdBackend::connectionKeepaliveTimeout); - m_mpd_conn_timer->start(MPD_CONNECTION_TIMEOUT / 2); + m_mpd_conn_timer->start(MPD_KEEPALIVE_TIMEOUT); m_song_pos_timer = new QTimer(this); connect(m_song_pos_timer, &QTimer::timeout, this, &MediaplayerMpdBackend::songPositionTimeout); @@ -57,6 +58,7 @@ MediaplayerMpdBackend::MediaplayerMpdBackend(Mediaplayer *player, QQmlContext *c // Create thread to handle polling for MPD events MpdEventHandler *handler = new MpdEventHandler(); handler->moveToThread(&m_handlerThread); + connect(&m_handlerThread, &QThread::finished, this, &MediaplayerMpdBackend::handleHandlerFinish); connect(&m_handlerThread, &QThread::finished, handler, &QObject::deleteLater); connect(this, &MediaplayerMpdBackend::startHandler, @@ -169,25 +171,37 @@ void MediaplayerMpdBackend::connectionKeepaliveTimeout(void) m_mpd_conn_mutex.lock(); // Clear any lingering non-fatal errors - if (!mpd_connection_clear_error(m_mpd_conn)) { + if (m_mpd_conn && !mpd_connection_clear_error(m_mpd_conn)) { // NOTE: There should likely be an attempt to reconnect here, // but it definitely would complicate things for all the // other users. qWarning() << "MPD connection in error state!"; - m_mpd_conn_mutex.unlock(); - return; + mpd_connection_free(m_mpd_conn); + m_mpd_conn = NULL; + m_mpd_conn_timer->stop(); } + m_mpd_conn_mutex.unlock(); +} - struct mpd_status *status = mpd_run_status(m_mpd_conn); - if (!status) { - qWarning() << "MPD connection status check failed"; - } else { - mpd_status_free(status); - } +void MediaplayerMpdBackend::handleHandlerFinish(void) +{ + m_mpd_conn_mutex.lock(); + + // If the event thread finished, our connection is likely dead. + // Given the behavior seen when mpd is hung is most client API + // calls hang, trying to determine mpd liveliness here seems + // problematic. Attempting to reconnect would probably be the + // only robust solution. + mpd_connection_free(m_mpd_conn); + m_mpd_conn = NULL; m_mpd_conn_mutex.unlock(); + + m_mpd_conn_timer->stop(); + delete m_mpd_conn_timer; } + void MediaplayerMpdBackend::songPositionTimeout(void) { m_state_mutex.lock(); @@ -271,7 +285,7 @@ void MediaplayerMpdBackend::play() m_mpd_conn_mutex.lock(); m_state_mutex.lock(); - if (!m_playing) { + if (m_mpd_conn && !m_playing) { mpd_run_play(m_mpd_conn); } m_state_mutex.unlock(); @@ -284,7 +298,7 @@ void MediaplayerMpdBackend::pause() m_mpd_conn_mutex.lock(); m_state_mutex.lock(); - if (m_playing) { + if (m_mpd_conn && m_playing) { mpd_run_pause(m_mpd_conn, true); } m_state_mutex.unlock(); @@ -298,7 +312,7 @@ void MediaplayerMpdBackend::previous() // MPD only allows next/previous if playing m_state_mutex.lock(); - if (m_playing) { + if (m_mpd_conn && m_playing) { mpd_run_previous(m_mpd_conn); } m_state_mutex.unlock(); @@ -312,7 +326,7 @@ void MediaplayerMpdBackend::next() // MPD only allows next/previous if playing m_state_mutex.lock(); - if (m_playing) { + if (m_mpd_conn && m_playing) { mpd_run_next(m_mpd_conn); } m_state_mutex.unlock(); @@ -324,9 +338,11 @@ void MediaplayerMpdBackend::seek(int milliseconds) { m_mpd_conn_mutex.lock(); - float t = milliseconds; - t /= 1000.0; - mpd_run_seek_current(m_mpd_conn, t, false); + if (m_mpd_conn) { + float t = milliseconds; + t /= 1000.0; + mpd_run_seek_current(m_mpd_conn, t, false); + } m_mpd_conn_mutex.unlock(); } @@ -336,9 +352,11 @@ void MediaplayerMpdBackend::fastforward(int milliseconds) { m_mpd_conn_mutex.lock(); - float t = milliseconds; - t /= 1000.0; - mpd_run_seek_current(m_mpd_conn, t, true); + if (m_mpd_conn) { + float t = milliseconds; + t /= 1000.0; + mpd_run_seek_current(m_mpd_conn, t, true); + } m_mpd_conn_mutex.unlock(); } @@ -348,9 +366,11 @@ void MediaplayerMpdBackend::rewind(int milliseconds) { m_mpd_conn_mutex.lock(); - float t = -milliseconds; - t /= 1000.0; - mpd_run_seek_current(m_mpd_conn, t, true); + if (m_mpd_conn) { + float t = -milliseconds; + t /= 1000.0; + mpd_run_seek_current(m_mpd_conn, t, true); + } m_mpd_conn_mutex.unlock(); } @@ -359,7 +379,7 @@ void MediaplayerMpdBackend::picktrack(int track) { m_mpd_conn_mutex.lock(); - if (track >= 0) { + if (m_mpd_conn && track >= 0) { mpd_run_play_pos(m_mpd_conn, track); } @@ -383,10 +403,12 @@ void MediaplayerMpdBackend::loop(QString state) // mpd_run_single_state(m_mpd_conn, MPD_SINGLE_OFF) (default) // mpd_run_repeat(m_mpd_conn, true) to loop - if (state == "playlist") { - mpd_run_repeat(m_mpd_conn, true); - } else { - mpd_run_repeat(m_mpd_conn, false); + if (m_mpd_conn) { + if (state == "playlist") { + mpd_run_repeat(m_mpd_conn, true); + } else { + mpd_run_repeat(m_mpd_conn, false); + } } m_mpd_conn_mutex.unlock(); diff --git a/mediaplayer/MediaplayerMpdBackend.h b/mediaplayer/MediaplayerMpdBackend.h index 4b8d74c..2a44655 100644 --- a/mediaplayer/MediaplayerMpdBackend.h +++ b/mediaplayer/MediaplayerMpdBackend.h @@ -55,6 +55,7 @@ signals: private slots: void connectionKeepaliveTimeout(void); + void handleHandlerFinish(void); void songPositionTimeout(void); void updatePlaybackState(int queue_pos, int song_pos_ms, bool state); void updateMetadata(QVariantMap metadata); diff --git a/mediaplayer/MpdEventHandler.cpp b/mediaplayer/MpdEventHandler.cpp index 56a4612..977f96c 100644 --- a/mediaplayer/MpdEventHandler.cpp +++ b/mediaplayer/MpdEventHandler.cpp @@ -16,6 +16,7 @@ #include <QDebug> #include <QFileInfo> +#include <QThread> #include "MpdEventHandler.h" MpdEventHandler::MpdEventHandler(QObject *parent) : @@ -62,9 +63,13 @@ void MpdEventHandler::handleEvents(void) else if (events & MPD_IDLE_PLAYER) { handlePlayerEvent(); } + if (mpd_connection_get_error(m_mpd_conn) != MPD_ERROR_SUCCESS) { + done = true; + } } qDebug() << "MpdEventHandler::handleEvents: exit"; + QThread::currentThread()->quit(); } void MpdEventHandler::handleDatabaseEvent(void) @@ -176,6 +181,14 @@ void MpdEventHandler::handlePlayerEvent(void) // corking in WirePlumber... struct mpd_status *status = mpd_run_status(m_mpd_conn); + if (!status) { + // mpd has gone away, attempt to get the UI into a good state + metadata["position"] = 0; + metadata["status"] = QString("stopped"); + emit metadataUpdate(metadata); + emit playbackStateUpdate(pos, 0, false); + return; + } int elapsed_ms = mpd_status_get_elapsed_ms(status); metadata["position"] = elapsed_ms; |