Clementine now skips broken streams (radios for example). At least I hope it always does. ;) Fixes issue #1562.

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() {

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);
   }
 }

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;
 

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_;