From e25e9efffc794c973949ed0f45597e198456423f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Bara?= Date: Sat, 12 Mar 2011 13:24:30 +0000 Subject: [PATCH] Clementine now skips broken streams (radios for example). At least I hope it always does. ;) Fixes issue #1562. --- src/core/player.cpp | 2 -- src/engines/gstengine.cpp | 10 ++++++++-- src/engines/gstenginepipeline.cpp | 31 +++++++++++++++++-------------- src/engines/gstenginepipeline.h | 14 ++++++-------- 4 files changed, 31 insertions(+), 26 deletions(-) diff --git a/src/core/player.cpp b/src/core/player.cpp index 39562a006..2af0f0259 100644 --- a/src/core/player.cpp +++ b/src/core/player.cpp @@ -54,8 +54,6 @@ Player::Player(PlaylistManagerInterface* playlists, LastFMService* lastfm, connect(engine_.get(), SIGNAL(ValidSongRequested(QUrl)), SLOT(ValidSongRequested(QUrl))); connect(engine_.get(), SIGNAL(InvalidSongRequested(QUrl)), SLOT(InvalidSongRequested(QUrl))); - - connect(playlists, SIGNAL(CurrentSongChanged(Song)), SLOT(CurrentMetadataChanged(Song))); } Player::~Player() { diff --git a/src/engines/gstengine.cpp b/src/engines/gstengine.cpp index f1208f0e3..55a0d157b 100644 --- a/src/engines/gstengine.cpp +++ b/src/engines/gstengine.cpp @@ -702,8 +702,14 @@ void GstEngine::HandlePipelineError(const QString& message, int domain, int erro // unable to play media stream with this url emit InvalidSongRequested(url_); - // no user error message when the error is 'no such URI' - if(!(domain == GST_RESOURCE_ERROR && error_code == GST_RESOURCE_ERROR_NOT_FOUND)) { + // TODO: the types of errors listed below won't be shown to user - they will + // get logged and the current song will be skipped; instead of maintaining + // the list we should probably: + // - don't report any engine's errors to user (always just log and skip) + // - come up with a less intrusive error box (not a dialog but a notification + // popup of some kind) and then report all errors + if(!(domain == GST_RESOURCE_ERROR && error_code == GST_RESOURCE_ERROR_NOT_FOUND) && + !(domain == GST_STREAM_ERROR && error_code == GST_STREAM_ERROR_TYPE_NOT_FOUND)) { emit Error(message); } } diff --git a/src/engines/gstenginepipeline.cpp b/src/engines/gstenginepipeline.cpp index ab160f10b..47f91cf14 100644 --- a/src/engines/gstenginepipeline.cpp +++ b/src/engines/gstenginepipeline.cpp @@ -56,7 +56,6 @@ GstEnginePipeline::GstEnginePipeline(GstEngine* engine) ignore_tags_(false), pipeline_is_initialised_(false), pipeline_is_connected_(false), - pipeline_error_(PipelineError()), pending_seek_nanosec_(-1), volume_percent_(100), volume_modifier_(1.0), @@ -269,11 +268,6 @@ GstEnginePipeline::~GstEnginePipeline() { gst_element_set_state(pipeline_, GST_STATE_NULL); gst_object_unref(GST_OBJECT(pipeline_)); } - - if(!pipeline_error_.message.isEmpty()) { - emit Error(pipeline_error_.message, pipeline_error_.domain, - pipeline_error_.error_code); - } } @@ -314,7 +308,7 @@ GstBusSyncReply GstEnginePipeline::BusCallbackSync(GstBus*, GstMessage* msg, gpo break; case GST_MESSAGE_ERROR: - instance->ErrorMessageReceived(msg); + QtConcurrent::run(instance, &GstEnginePipeline::ErrorMessageReceived, msg); break; case GST_MESSAGE_ELEMENT: @@ -365,13 +359,18 @@ void GstEnginePipeline::ErrorMessageReceived(GstMessage* msg) { return; } - // we'll send the error later, when pipeline is done with it's state changes - pipeline_error_ = PipelineError(); - pipeline_error_.message = message; - pipeline_error_.domain = domain; - pipeline_error_.error_code = code; - qDebug() << debugstr; + + // wait until the change of state is complete and if something went + // wrong signal the error; this makes skipping songs work even for + // ASYNC changes of state + // we're using a higher than usual timeout here to avoid race + // conditions; those are visible when we try to play another song + // due to skipping a broken one while the broken song's state is + // still being processed in GStreamer + if(state(kGstStateTimeoutNanosecs * 5) == GST_STATE_NULL) { + emit Error(message, domain, code); + } } void GstEnginePipeline::TagMessageReceived(GstMessage* msg) { @@ -563,8 +562,12 @@ qint64 GstEnginePipeline::length() const { } GstState GstEnginePipeline::state() const { + return state(kGstStateTimeoutNanosecs); +} + +GstState GstEnginePipeline::state(int delay) const { GstState s, sp; - if (gst_element_get_state(pipeline_, &s, &sp, kGstStateTimeoutNanosecs) == + if (gst_element_get_state(pipeline_, &s, &sp, delay) == GST_STATE_CHANGE_FAILURE) return GST_STATE_NULL; diff --git a/src/engines/gstenginepipeline.h b/src/engines/gstenginepipeline.h index 5999794b4..97597ce07 100644 --- a/src/engines/gstenginepipeline.h +++ b/src/engines/gstenginepipeline.h @@ -40,12 +40,6 @@ class GstEnginePipeline : public QObject { Q_OBJECT public: - struct PipelineError { - QString message; - int domain; - int error_code; - }; - GstEnginePipeline(GstEngine* engine); ~GstEnginePipeline(); @@ -87,6 +81,8 @@ class GstEnginePipeline : public QObject { // Please note that this method (unlike GstEngine's.length()) is // multiple-section media unaware. qint64 length() const; + // Returns this pipeline's state. May return GST_STATE_NULL if the state check + // timed out. The timeout value is a reasonable default. GstState state() const; qint64 segment_start() const { return segment_start_; } @@ -133,6 +129,10 @@ class GstEnginePipeline : public QObject { void TransitionToNext(); + // Returns this pipeline's state. May return GST_STATE_NULL if the state check + // timed out. The timeout value used is the given one. + GstState state(int timeout) const; + private slots: void FaderTimelineFinished(); @@ -201,8 +201,6 @@ class GstEnginePipeline : public QObject { // Also we have to wait for the decodebin to be connected. bool pipeline_is_initialised_; bool pipeline_is_connected_; - // Cached error thrown from GStreamer during pipeline's initialization. - PipelineError pipeline_error_; qint64 pending_seek_nanosec_; int volume_percent_;