Skip to content

Commit

Permalink
Completely revert the Spotify seeking stuff - it's hacky and I don't …
Browse files Browse the repository at this point in the history
…like

waiting 4 seconds to have my songs start.  I'll reimplement it properly later.

Reverts commits 9638780 and 160b151.
  • Loading branch information
davidsansome committed Oct 14, 2014
1 parent c428e53 commit 88a6300
Show file tree
Hide file tree
Showing 10 changed files with 10 additions and 79 deletions.
5 changes: 2 additions & 3 deletions ext/clementine-spotifyblob/mediapipeline.cpp
Expand Up @@ -81,9 +81,8 @@ bool MediaPipeline::Init(int sample_rate, int channels) {

// Try to send 5 seconds of audio in advance to initially fill Clementine's
// buffer.
// Commented for now as otherwise the seek will take too long.
//g_object_set(G_OBJECT(tcpsink_), "ts-offset", qint64(-5 * kNsecPerSec),
// nullptr);
g_object_set(G_OBJECT(tcpsink_), "ts-offset", qint64(-5 * kNsecPerSec),
nullptr);

// We know the time of each buffer
g_object_set(G_OBJECT(appsrc_), "format", GST_FORMAT_TIME, nullptr);
Expand Down
16 changes: 2 additions & 14 deletions ext/clementine-spotifyblob/spotifyclient.cpp
Expand Up @@ -758,9 +758,6 @@ int SpotifyClient::MusicDeliveryCallback(sp_session* session,
}

if (num_frames == 0) {
// According to libspotify documentation, this occurs when a discontinuity
// has occurred (such as after a seek). Maybe should clear buffers here as
// well? (in addition of clearing buffers in gstenginepipeline.cpp)
return 0;
}

Expand Down Expand Up @@ -919,17 +916,8 @@ void SpotifyClient::StartPlayback(const pb::spotify::PlaybackRequest& req) {
}

void SpotifyClient::Seek(qint64 offset_nsec) {
if (sp_session_player_seek(session_, offset_nsec / kNsecPerMsec)
!= SP_ERROR_OK) {
qLog(Error) << "Seek error";
return;
}

pb::spotify::Message message;

pb::spotify::SeekCompleted* response = message.mutable_seek_completed();
Q_UNUSED(response);
SendMessage(message);
// TODO
qLog(Error) << "TODO seeking";
}

void SpotifyClient::TryPlaybackAgain(const PendingPlaybackRequest& req) {
Expand Down
1 change: 0 additions & 1 deletion ext/clementine-spotifyblob/spotifyclient.h
Expand Up @@ -59,7 +59,6 @@ class SpotifyClient : public AbstractMessageHandler<pb::spotify::Message> {
pb::spotify::LoginResponse_Error error_code);
void SendPlaybackError(const QString& error);
void SendSearchResponse(sp_search* result);
void SendSeekCompleted();

// Spotify session callbacks.
static void SP_CALLCONV LoggedInCallback(sp_session* session, sp_error error);
Expand Down
5 changes: 1 addition & 4 deletions ext/libclementine-spotifyblob/spotifymessages.proto
Expand Up @@ -176,9 +176,6 @@ message SeekRequest {
optional int64 offset_nsec = 1;
}

message SeekCompleted {
}

enum Bitrate {
Bitrate96k = 1;
Bitrate160k = 2;
Expand Down Expand Up @@ -229,7 +226,7 @@ message Message {
optional BrowseToplistRequest browse_toplist_request = 19;
optional BrowseToplistResponse browse_toplist_response = 20;
optional PauseRequest pause_request = 21;
optional SeekCompleted seek_completed = 22;
// ID 22 unused.
optional AddTracksToPlaylistRequest add_tracks_to_playlist = 23;
optional RemoveTracksFromPlaylistRequest remove_tracks_from_playlist = 24;
}
40 changes: 0 additions & 40 deletions src/engines/gstenginepipeline.cpp
Expand Up @@ -68,7 +68,6 @@ GstEnginePipeline::GstEnginePipeline(GstEngine* engine)
buffering_(false),
mono_playback_(false),
end_offset_nanosec_(-1),
spotify_offset_(0),
next_beginning_offset_nanosec_(-1),
next_end_offset_nanosec_(-1),
ignore_next_seek_(false),
Expand Down Expand Up @@ -96,19 +95,6 @@ GstEnginePipeline::GstEnginePipeline(GstEngine* engine)
}

for (int i = 0; i < kEqBandCount; ++i) eq_band_gains_ << 0;

// FIXME Currently useless Spotify hack: we currently don't know what to do
// when the seek is completed. We should flush the current buffers, but see
// comments in SpotifySeekCompleted for why that doesn't work.
// If we fix and reactivate this code, we will have another problem though:
// calling server() try to login the user. If the user doesn't use Spotify, it
// will receive an error message. We should have a lightweight version of
// server() that just return it (or NULL) without trying to create it IMO to
// avoid this issue.
//if (InternetModel::Service<SpotifyService>()->IsBlobInstalled()) {
// connect(InternetModel::Service<SpotifyService>()->server(), SIGNAL(SeekCompleted()),
// SLOT(SpotifySeekCompleted()));
//}
}

void GstEnginePipeline::set_output_device(const QString& sink,
Expand Down Expand Up @@ -184,7 +170,6 @@ bool GstEnginePipeline::ReplaceDecodeBin(const QUrl& url) {
// Tell spotify to start sending data to us.
InternetModel::Service<SpotifyService>()->server()->StartPlaybackLater(
url.toString(), port);
spotify_offset_ = 0;
} else {
new_bin = engine_->CreateElement("uridecodebin");
g_object_set(G_OBJECT(new_bin), "uri", url.toEncoded().constData(),
Expand Down Expand Up @@ -979,10 +964,6 @@ qint64 GstEnginePipeline::position() const {
gint64 value = 0;
gst_element_query_position(pipeline_, GST_FORMAT_TIME, &value);

if (url_.scheme() == "spotify") {
value += spotify_offset_;
}

return value;
}

Expand Down Expand Up @@ -1030,17 +1011,6 @@ bool GstEnginePipeline::Seek(qint64 nanosec) {
return true;
}

if (url_.scheme() == "spotify" && !buffering_) {
SpotifyService* spotify = InternetModel::Service<SpotifyService>();
// Need to schedule this in the spotify service's thread
QMetaObject::invokeMethod(spotify, "Seek", Qt::QueuedConnection,
Q_ARG(qint64, nanosec));
// Need to reset spotify_offset_ to get the real pipeline position, as it is
// used in position()
spotify_offset_ = nanosec - position();
return true;
}

if (!pipeline_is_connected_ || !pipeline_is_initialised_) {
pending_seek_nanosec_ = nanosec;
return true;
Expand All @@ -1051,16 +1021,6 @@ bool GstEnginePipeline::Seek(qint64 nanosec) {
GST_SEEK_FLAG_FLUSH, nanosec);
}

void GstEnginePipeline::SpotifySeekCompleted() {
qLog(Debug) << "Spotify Seek completed";
// FIXME: we should clear buffers to start playing data from seek point right
// now (currently there is small delay) but I didn't managed to tell gstreamer
// to do this without breaking the streaming completely...
// Funny thing to notice: for me the delay varies when changing buffer size,
// but a larger buffer doesn't necessary increase the delay.
// FIXME: also, this method is never called currently (see constructor)
}

void GstEnginePipeline::SetEqualizerEnabled(bool enabled) {
eq_enabled_ = enabled;
UpdateEqualizer();
Expand Down
8 changes: 0 additions & 8 deletions src/engines/gstenginepipeline.h
Expand Up @@ -164,7 +164,6 @@ class GstEnginePipeline : public QObject {

private slots:
void FaderTimelineFinished();
void SpotifySeekCompleted();

private:
static const int kGstStateTimeoutNanosecs;
Expand Down Expand Up @@ -230,13 +229,6 @@ class GstEnginePipeline : public QObject {
// past this position.
qint64 end_offset_nanosec_;

// Another Spotify hack...
// Used in position(). We need this because when seeking Spotify tracks, we
// don't actually seek the pipeline, but ask libspotify to send us data with
// a seek offset instead. So querying the pipeline to get track's position
// wouldn't make sense.
qint64 spotify_offset_;

// We store the beginning and end for the preloading song too, so we can just
// carry on without reloading the file if the sections carry on from each
// other.
Expand Down
2 changes: 0 additions & 2 deletions src/internet/spotifyserver.cpp
Expand Up @@ -155,8 +155,6 @@ void SpotifyServer::MessageArrived(const pb::spotify::Message& message) {
emit AlbumBrowseResults(message.browse_album_response());
} else if (message.has_browse_toplist_response()) {
emit ToplistBrowseResults(message.browse_toplist_response());
} else if (message.has_seek_completed()) {
emit SeekCompleted();
}
}

Expand Down
1 change: 0 additions & 1 deletion src/internet/spotifyserver.h
Expand Up @@ -75,7 +75,6 @@ class SpotifyServer : public AbstractMessageHandler<pb::spotify::Message> {
void SyncPlaylistProgress(const pb::spotify::SyncPlaylistProgress& progress);
void AlbumBrowseResults(const pb::spotify::BrowseAlbumResponse& response);
void ToplistBrowseResults(const pb::spotify::BrowseToplistResponse& response);
void SeekCompleted();

protected:
void MessageArrived(const pb::spotify::Message& message);
Expand Down
9 changes: 4 additions & 5 deletions src/internet/spotifyservice.cpp
Expand Up @@ -585,6 +585,10 @@ QList<QAction*> SpotifyService::playlistitem_actions(const Song& song) {
return playlistitem_actions_;
}

PlaylistItem::Options SpotifyService::playlistitem_options() const {
return PlaylistItem::SeekDisabled;
}

QWidget* SpotifyService::HeaderWidget() const {
if (IsLoggedIn()) return search_box_;
return nullptr;
Expand Down Expand Up @@ -767,11 +771,6 @@ void SpotifyService::SetPaused(bool paused) {
server_->SetPaused(paused);
}

void SpotifyService::Seek(qint64 offset_nsec) {
EnsureServerCreated();
server_->Seek(offset_nsec);
}

void SpotifyService::SyncPlaylistProgress(
const pb::spotify::SyncPlaylistProgress& progress) {
qLog(Debug) << "Sync progress:" << progress.sync_progress();
Expand Down
2 changes: 1 addition & 1 deletion src/internet/spotifyservice.h
Expand Up @@ -56,13 +56,13 @@ class SpotifyService : public InternetService {
void ItemDoubleClicked(QStandardItem* item);
void DropMimeData(const QMimeData* data, const QModelIndex& index);
QList<QAction*> playlistitem_actions(const Song& song) override;
PlaylistItem::Options playlistitem_options() const;
QWidget* HeaderWidget() const;

void Logout();
void Login(const QString& username, const QString& password);
Q_INVOKABLE void LoadImage(const QString& id);
Q_INVOKABLE void SetPaused(bool paused);
Q_INVOKABLE void Seek(qint64 offset_nsec);

SpotifyServer* server() const;

Expand Down

1 comment on commit 88a6300

@ArnaudBienner
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh! :(
Yes indeed, it was hacky. But I'm surprised about the "I don't like waiting 4 seconds to have my songs start": this change wasn't supposed to add any extra delay (quite the opposite actually, as we didn't tried to fill the buffer as much as before).
The delay I was mentioning was the delay to have the "seek" taken into account, which depends on how "old" data we have in our buffers before receiving the new/sought ones.
But well, if this broke something for you, and reverting fixed the issue, then fine :)

Please sign in to comment.