When doing gapless playback, fix a bug where the TrackEnded signal would be emitted too early, before the track had actually ended. This caused the song after the current song to get scrobbled and its playcount increased. Fixes issue 1771

src/core/player.cpp

@@ -155,7 +155,9 @@ void Player::TrackEnded() {
   }
 
   if (current_item_ && current_item_->IsLocalLibraryItem() &&
-      current_item_->Metadata().id() != -1 && playlists_->active()->get_lastfm_status() != Playlist::LastFM_Scrobbled) {
+      current_item_->Metadata().id() != -1 &&
+      !playlists_->active()->have_incremented_playcount() &&
+      playlists_->active()->get_lastfm_status() != Playlist::LastFM_Seeked) {
     // The track finished before its scrobble point (30 seconds), so increment
     // the play count now.
     playlists_->library_backend()->IncrementPlayCountAsync(
@@ -299,7 +301,7 @@ void Player::SeekTo(int seconds) {
   // If we seek the track we don't want to submit it to last.fm
   qDebug() << "Track seeked to" << nanosec << "ns - not scrobbling";
   if (playlists_->active()->get_lastfm_status() == Playlist::LastFM_New) {
-    playlists_->active()->set_lastfm_status(Playlist::LastFM_Skipped);
+    playlists_->active()->set_lastfm_status(Playlist::LastFM_Seeked);
   }
 
   emit Seeked(nanosec / 1000);

src/engines/gstenginepipeline.cpp

@@ -44,6 +44,7 @@ GstEnginePipeline::GstEnginePipeline(GstEngine* engine)
     sink_(GstEngine::kAutoSink),
     segment_start_(0),
     segment_start_received_(false),
+    emit_track_ended_on_segment_start_(false),
     eq_enabled_(false),
     eq_preamp_(0),
     rg_enabled_(false),
@@ -286,7 +287,7 @@ GstEnginePipeline::~GstEnginePipeline() {
 gboolean GstEnginePipeline::BusCallback(GstBus*, GstMessage* msg, gpointer self) {
   GstEnginePipeline* instance = reinterpret_cast<GstEnginePipeline*>(self);
 
-  switch ( GST_MESSAGE_TYPE(msg)) {
+  switch (GST_MESSAGE_TYPE(msg)) {
     case GST_MESSAGE_ERROR:
       instance->ErrorMessageReceived(msg);
       break;
@@ -509,6 +510,11 @@ bool GstEnginePipeline::EventHandoffCallback(GstPad*, GstEvent* e, gpointer self
     gst_event_parse_new_segment(e, NULL, NULL, NULL, &start, NULL, NULL);
     instance->segment_start_ = start;
     instance->segment_start_received_ = true;
+
+    if (instance->emit_track_ended_on_segment_start_) {
+      instance->emit_track_ended_on_segment_start_ = false;
+      emit instance->EndOfStreamReached(instance->id(), true);
+    }
   }
 
   return true;
@@ -536,8 +542,10 @@ void GstEnginePipeline::TransitionToNext() {
   next_beginning_offset_nanosec_ = 0;
   next_end_offset_nanosec_ = 0;
 
-  // This just tells the UI that we've moved on to the next song
-  emit EndOfStreamReached(id(), true);
+  // This function gets called when the source has been drained, even if the
+  // song hasn't finished playing yet.  We'll get a new segment when it really
+  // does finish, so emit TrackEnded then.
+  emit_track_ended_on_segment_start_ = true;
 
   // This has to happen *after* the gst_element_set_state on the new bin to
   // fix an occasional race condition deadlock.

src/engines/gstenginepipeline.h

@@ -163,6 +163,7 @@ class GstEnginePipeline : public QObject {
   QMutex buffer_consumers_mutex_;
   qint64 segment_start_;
   bool segment_start_received_;
+  bool emit_track_ended_on_segment_start_;
 
   // Equalizer
   bool eq_enabled_;

src/playlist/playlist.cpp

@@ -93,6 +93,7 @@ Playlist::Playlist(PlaylistBackend* backend,
     is_shuffled_(false),
     scrobble_point_(-1),
     lastfm_status_(LastFM_New),
+    have_incremented_playcount_(false),
     playlist_sequence_(NULL),
     ignore_sorting_(false),
     undo_stack_(new QUndoStack(this))
@@ -1391,6 +1392,7 @@ void Playlist::UpdateScrobblePoint() {
   }
 
   set_lastfm_status(LastFM_New);
+  have_incremented_playcount_ = false;
 }
 
 void Playlist::Clear() {

src/playlist/playlist.h

@@ -119,16 +119,11 @@ class Playlist : public QAbstractListModel {
   };
 
   enum LastFMStatus {
-    //new song to scrobble
-    LastFM_New = 0,
-    //song already scrobbled
-    LastFM_Scrobbled,
-    //song we don't want to scrobble, e.g. if we seeked through it
-    LastFM_Skipped,
-    //error submitting
-    LastFM_Error,
-    //invalid Song, e.g. tags not available
-    LastFM_Invalid
+    LastFM_New = 0,   // Haven't scrobbled yet, but we want to later
+    LastFM_Scrobbled, // Scrobbled ok
+    LastFM_Seeked,    // The user seeked so don't scrobble
+    LastFM_Error,     // Tried to scrobble but got an error
+    LastFM_Invalid,   // The song isn't suitable for scrobbling
   };
 
   static const char* kRowsMimetype;
@@ -200,7 +195,9 @@ class Playlist : public QAbstractListModel {
   // Scrobbling
   qint64 scrobble_point_nanosec() const { return scrobble_point_; }
   LastFMStatus get_lastfm_status() const { return lastfm_status_; }
+  bool have_incremented_playcount() const { return have_incremented_playcount_; }
   void set_lastfm_status(LastFMStatus status) {lastfm_status_ = status; }
+  void set_have_incremented_playcount() { have_incremented_playcount_ = true; }
 
   // Changing the playlist
   void InsertItems              (const PlaylistItemList& items,     int pos = -1, bool play_now = false, bool enqueue = false);
@@ -352,6 +349,7 @@ class Playlist : public QAbstractListModel {
 
   qint64 scrobble_point_;
   LastFMStatus lastfm_status_;
+  bool have_incremented_playcount_;
 
   PlaylistSequence* playlist_sequence_;
 

src/ui/mainwindow.cpp

@@ -893,7 +893,7 @@ void MainWindow::ScrobblingEnabledChanged(bool value) {
   else {
     //invalidate current song, we will scrobble the next one
     if (playlists_->active()->get_lastfm_status() == Playlist::LastFM_New)
-      playlists_->active()->set_lastfm_status(Playlist::LastFM_Skipped);
+      playlists_->active()->set_lastfm_status(Playlist::LastFM_Seeked);
   }
 
   bool is_lastfm = (player_->GetCurrentItem()->options() & PlaylistItem::LastFMControls);
@@ -1028,11 +1028,13 @@ void MainWindow::Seeked(qlonglong microseconds) {
 
 void MainWindow::UpdateTrackPosition() {
   // Track position in seconds
+  Playlist* playlist = playlists_->active();
+
   PlaylistItemPtr item(player_->GetCurrentItem());
   const int position = std::floor(
       float(player_->engine()->position_nanosec()) / kNsecPerSec + 0.5);
   const int length = item->Metadata().length_nanosec() / kNsecPerSec;
-  const int scrobble_point = playlists_->active()->scrobble_point_nanosec() / kNsecPerSec;
+  const int scrobble_point = playlist->scrobble_point_nanosec() / kNsecPerSec;
 
   if (length <= 0) {
     // Probably a stream that we don't know the length of
@@ -1041,24 +1043,29 @@ void MainWindow::UpdateTrackPosition() {
     return;
   }
 #ifdef HAVE_LIBLASTFM
-  bool last_fm_enabled = ui_->action_toggle_scrobbling->isVisible() && RadioModel::Service<LastFMService>()->IsScrobblingEnabled() && RadioModel::Service<LastFMService>()->IsAuthenticated();
+  LastFMService* lastfm_service = RadioModel::Service<LastFMService>();
+  const bool last_fm_enabled = ui_->action_toggle_scrobbling->isVisible() &&
+                               lastfm_service->IsScrobblingEnabled() &&
+                               lastfm_service->IsAuthenticated();
 #endif
 
   // Time to scrobble?
-  if (playlists_->active()->get_lastfm_status() == Playlist::LastFM_New && position >= scrobble_point) {
-#ifdef HAVE_LIBLASTFM
-    if (RadioModel::Service<LastFMService>()->IsScrobblingEnabled()) {
-      qDebug() << "Scrobbling at" << scrobble_point;
-      radio_model_->RadioModel::Service<LastFMService>()->Scrobble();
-    } else
-#endif
-      // If we're not scrobbling or last.fm is compiled out, mark the song
-      // as "won't scrobble", so we only update the play count below once.
-      playlists_->active()->set_lastfm_status(Playlist::LastFM_Invalid);
+  if (position >= scrobble_point) {
+    if (playlist->get_lastfm_status() == Playlist::LastFM_New) {
+      #ifdef HAVE_LIBLASTFM
+        if (lastfm_service->IsScrobblingEnabled()) {
+          qDebug() << "Scrobbling at" << scrobble_point;
+          lastfm_service->Scrobble();
+        }
+      #endif
+    }
 
     // Update the play count for the song if it's from the library
-    if (item->IsLocalLibraryItem() && item->Metadata().id() != -1) {
+    if (!playlist->have_incremented_playcount() &&
+        item->IsLocalLibraryItem() && item->Metadata().id() != -1 &&
+        playlist->get_lastfm_status() != Playlist::LastFM_Seeked) {
       library_->backend()->IncrementPlayCountAsync(item->Metadata().id());
+      playlist->set_have_incremented_playcount();
     }
   }
 
@@ -1068,12 +1075,14 @@ void MainWindow::UpdateTrackPosition() {
   // Update the tray icon every 10 seconds
   if (position % 10 == 0) {
     qDebug() << "position" << position << "scrobble point" << scrobble_point
-        << "status" << playlists_->active()->get_lastfm_status();
+        << "status" << playlist->get_lastfm_status();
     tray_icon_->SetProgress(double(position) / length * 100);
 
     //if we're waiting for the scrobble point, update the icon
 #ifdef HAVE_LIBLASTFM
-    if (position < scrobble_point && playlists_->active()->get_lastfm_status() == Playlist::LastFM_New && last_fm_enabled) {
+    if (position < scrobble_point &&
+        playlist->get_lastfm_status() == Playlist::LastFM_New &&
+        last_fm_enabled) {
       ui_->action_toggle_scrobbling->setIcon(CreateOverlayedIcon(position, scrobble_point));
     }
 #endif