From 8f4c1dbbf60b6d637f10a711a5f8ff26e25238ba Mon Sep 17 00:00:00 2001 From: David Sansome Date: Sun, 17 Apr 2011 14:11:37 +0000 Subject: [PATCH] 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 | 6 +++-- src/engines/gstenginepipeline.cpp | 14 ++++++++--- src/engines/gstenginepipeline.h | 1 + src/playlist/playlist.cpp | 2 ++ src/playlist/playlist.h | 18 ++++++-------- src/ui/mainwindow.cpp | 41 +++++++++++++++++++------------ 6 files changed, 51 insertions(+), 31 deletions(-) diff --git a/src/core/player.cpp b/src/core/player.cpp index 946dada57..9829a5835 100644 --- a/src/core/player.cpp +++ b/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); diff --git a/src/engines/gstenginepipeline.cpp b/src/engines/gstenginepipeline.cpp index 922f8c084..3d9dfbd08 100644 --- a/src/engines/gstenginepipeline.cpp +++ b/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(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. diff --git a/src/engines/gstenginepipeline.h b/src/engines/gstenginepipeline.h index ed71b51f3..e6eaf8666 100644 --- a/src/engines/gstenginepipeline.h +++ b/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_; diff --git a/src/playlist/playlist.cpp b/src/playlist/playlist.cpp index a5596fc86..a2c572934 100644 --- a/src/playlist/playlist.cpp +++ b/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() { diff --git a/src/playlist/playlist.h b/src/playlist/playlist.h index 0b604677b..70604afd0 100644 --- a/src/playlist/playlist.h +++ b/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_; diff --git a/src/ui/mainwindow.cpp b/src/ui/mainwindow.cpp index b50dce7f7..7250a1071 100644 --- a/src/ui/mainwindow.cpp +++ b/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()->IsScrobblingEnabled() && RadioModel::Service()->IsAuthenticated(); + LastFMService* lastfm_service = RadioModel::Service(); + 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()->IsScrobblingEnabled()) { - qDebug() << "Scrobbling at" << scrobble_point; - radio_model_->RadioModel::Service()->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