From 3efc496c4143f2e0fff30cb17de98977e18c9b28 Mon Sep 17 00:00:00 2001 From: Jonas Kvinge Date: Tue, 7 Apr 2020 16:48:12 +0200 Subject: [PATCH] Add better error handling for CDDA loader --- src/core/songloader.cpp | 12 ++++++--- src/core/songloader.h | 8 +++--- src/device/cddasongloader.cpp | 38 +++++++++++++++++++++++++---- src/device/cddasongloader.h | 15 ++++++------ src/playlist/songloaderinserter.cpp | 21 ++++++++++++---- src/playlist/songloaderinserter.h | 7 +++--- 6 files changed, 73 insertions(+), 28 deletions(-) diff --git a/src/core/songloader.cpp b/src/core/songloader.cpp index ffe91d229..6924caa71 100644 --- a/src/core/songloader.cpp +++ b/src/core/songloader.cpp @@ -185,7 +185,7 @@ SongLoader::Result SongLoader::LoadAudioCD() { #if defined(HAVE_AUDIOCD) && defined(HAVE_GSTREAMER) if (player_->engine()->type() == Engine::GStreamer) { CddaSongLoader *cdda_song_loader = new CddaSongLoader(QUrl(), this); - connect(cdda_song_loader, SIGNAL(SongsDurationLoaded(SongList)), this, SLOT(AudioCDTracksLoadedSlot(SongList))); + connect(cdda_song_loader, SIGNAL(SongsDurationLoaded(SongList, QString)), this, SLOT(AudioCDTracksLoadFinishedSlot(SongList, QString))); connect(cdda_song_loader, SIGNAL(SongsMetadataLoaded(SongList)), this, SLOT(AudioCDTracksTagsLoaded(SongList))); cdda_song_loader->LoadSongs(); return Success; @@ -201,16 +201,22 @@ SongLoader::Result SongLoader::LoadAudioCD() { } #if defined(HAVE_AUDIOCD) && defined(HAVE_GSTREAMER) -void SongLoader::AudioCDTracksLoadedSlot(const SongList &songs) { + +void SongLoader::AudioCDTracksLoadFinishedSlot(const SongList &songs, const QString &error) { + songs_ = songs; - emit AudioCDTracksLoaded(); + errors_ << error; + emit AudioCDTracksLoadFinished(); + } void SongLoader::AudioCDTracksTagsLoaded(const SongList &songs) { + CddaSongLoader *cdda_song_loader = qobject_cast(sender()); cdda_song_loader->deleteLater(); songs_ = songs; emit LoadAudioCDFinished(true); + } #endif diff --git a/src/core/songloader.h b/src/core/songloader.h index 4ac5bb3a9..0268bc136 100644 --- a/src/core/songloader.h +++ b/src/core/songloader.h @@ -57,7 +57,7 @@ class CddaSongLoader; class SongLoader : public QObject { Q_OBJECT public: - SongLoader(CollectionBackendInterface *collection, const Player *player, QObject *parent = nullptr); + explicit SongLoader(CollectionBackendInterface *collection, const Player *player, QObject *parent = nullptr); ~SongLoader(); enum Result { @@ -87,15 +87,15 @@ class SongLoader : public QObject { QStringList errors() { return errors_; } signals: - void AudioCDTracksLoaded(); - void LoadAudioCDFinished(bool success); + void AudioCDTracksLoadFinished(); + void LoadAudioCDFinished(const bool success); void LoadRemoteFinished(); private slots: void Timeout(); void StopTypefind(); #if defined(HAVE_AUDIOCD) && defined(HAVE_GSTREAMER) - void AudioCDTracksLoadedSlot(const SongList &songs); + void AudioCDTracksLoadFinishedSlot(const SongList &songs, const QString &error); void AudioCDTracksTagsLoaded(const SongList &songs); #endif // HAVE_AUDIOCD && HAVE_GSTREAMER diff --git a/src/device/cddasongloader.cpp b/src/device/cddasongloader.cpp index ab7f7fbd1..abdfa0ac6 100644 --- a/src/device/cddasongloader.cpp +++ b/src/device/cddasongloader.cpp @@ -67,6 +67,7 @@ void CddaSongLoader::LoadSongs() { QMutexLocker locker(&mutex_load_); cdio_ = cdio_open(url_.path().toLocal8Bit().constData(), DRIVER_DEVICE); if (cdio_ == nullptr) { + Error("Unable to open CDIO device."); return; } @@ -74,7 +75,7 @@ void CddaSongLoader::LoadSongs() { GError *error = nullptr; cdda_ = gst_element_make_from_uri(GST_URI_SRC, "cdda://", nullptr, &error); if (error) { - qLog(Error) << error->code << error->message; + Error(QString("%1: %2").arg(error->code).arg(error->message)); } if (cdda_ == nullptr) { return; @@ -84,13 +85,23 @@ void CddaSongLoader::LoadSongs() { g_object_set(cdda_, "device", g_strdup(url_.path().toLocal8Bit().constData()), nullptr); } if (g_object_class_find_property (G_OBJECT_GET_CLASS (cdda_), "paranoia-mode")) { - g_object_set (cdda_, "paranoia-mode", 0, nullptr); + g_object_set(cdda_, "paranoia-mode", 0, nullptr); } // Change the element's state to ready and paused, to be able to query it - if (gst_element_set_state(cdda_, GST_STATE_READY) == GST_STATE_CHANGE_FAILURE || gst_element_set_state(cdda_, GST_STATE_PAUSED) == GST_STATE_CHANGE_FAILURE) { + if (gst_element_set_state(cdda_, GST_STATE_READY) == GST_STATE_CHANGE_FAILURE) { gst_element_set_state(cdda_, GST_STATE_NULL); gst_object_unref(GST_OBJECT(cdda_)); + cdda_ = nullptr; + Error(tr("Error while setting CDDA device to ready state.")); + return; + } + + if (gst_element_set_state(cdda_, GST_STATE_PAUSED) == GST_STATE_CHANGE_FAILURE) { + gst_element_set_state(cdda_, GST_STATE_NULL); + gst_object_unref(GST_OBJECT(cdda_)); + cdda_ = nullptr; + Error(tr("Error while setting CDDA device to pause state.")); return; } @@ -98,9 +109,20 @@ void CddaSongLoader::LoadSongs() { GstFormat fmt = gst_format_get_by_nick("track"); GstFormat out_fmt = fmt; gint64 num_tracks = 0; - if (!gst_element_query_duration(cdda_, out_fmt, &num_tracks) || out_fmt != fmt) { - qLog(Error) << "Error while querying cdda GstElement"; + if (!gst_element_query_duration(cdda_, out_fmt, &num_tracks)) { + gst_element_set_state(cdda_, GST_STATE_NULL); gst_object_unref(GST_OBJECT(cdda_)); + cdda_ = nullptr; + Error(tr("Error while querying CDDA tracks.")); + return; + } + + if (out_fmt != fmt) { + qLog(Error) << "Error while querying cdda GstElement (2)."; + gst_element_set_state(cdda_, GST_STATE_NULL); + gst_object_unref(GST_OBJECT(cdda_)); + cdda_ = nullptr; + Error(tr("Error while querying CDDA tracks.")); return; } @@ -232,3 +254,9 @@ bool CddaSongLoader::HasChanged() { } +void CddaSongLoader::Error(const QString &error) { + + qLog(Error) << error; + emit SongsDurationLoaded(SongList(), error); + +} diff --git a/src/device/cddasongloader.h b/src/device/cddasongloader.h index 48fed1c7d..05eb0264b 100644 --- a/src/device/cddasongloader.h +++ b/src/device/cddasongloader.h @@ -44,19 +44,21 @@ class CddaSongLoader : public QObject { Q_OBJECT public: - CddaSongLoader( - // Url of the CD device. Will use the default device if empty - const QUrl &url = QUrl(), - QObject *parent = nullptr); + explicit CddaSongLoader(const QUrl &url = QUrl(), QObject *parent = nullptr); ~CddaSongLoader(); // Load songs. Signals declared below will be emitted anytime new information will be available. void LoadSongs(); bool HasChanged(); + private: + void Error(const QString &error); + QUrl GetUrlFromTrack(const int track_number) const; + signals: + void SongsLoadError(const QString &error); void SongsLoaded(const SongList &songs); - void SongsDurationLoaded(const SongList &songs); + void SongsDurationLoaded(const SongList &songs, const QString &error = QString()); void SongsMetadataLoaded(const SongList &songs); private slots: @@ -65,8 +67,6 @@ class CddaSongLoader : public QObject { #endif private: - QUrl GetUrlFromTrack(int track_number) const; - QUrl url_; GstElement *cdda_; CdIo_t *cdio_; @@ -74,4 +74,3 @@ class CddaSongLoader : public QObject { }; #endif // CDDASONGLOADER_H - diff --git a/src/playlist/songloaderinserter.cpp b/src/playlist/songloaderinserter.cpp index 04de98a90..e6ba57753 100644 --- a/src/playlist/songloaderinserter.cpp +++ b/src/playlist/songloaderinserter.cpp @@ -100,7 +100,7 @@ void SongLoaderInserter::LoadAudioCD(Playlist *destination, int row, bool play_n enqueue_next_ = enqueue_next; SongLoader *loader = new SongLoader(collection_, player_, this); - NewClosure(loader, SIGNAL(AudioCDTracksLoaded()), this, SLOT(AudioCDTracksLoaded(SongLoader*)), loader); + NewClosure(loader, SIGNAL(AudioCDTracksLoadFinished()), this, SLOT(AudioCDTracksLoadFinished(SongLoader*)), loader); connect(loader, SIGNAL(LoadAudioCDFinished(bool)), SLOT(AudioCDTagsLoaded(bool))); qLog(Info) << "Loading audio CD..."; SongLoader::Result ret = loader->LoadAudioCD(); @@ -114,18 +114,27 @@ void SongLoaderInserter::LoadAudioCD(Playlist *destination, int row, bool play_n } delete loader; } - // Songs will be loaded later: see AudioCDTracksLoaded and AudioCDTagsLoaded slots + // Songs will be loaded later: see AudioCDTracksLoadFinished and AudioCDTagsLoaded slots } void SongLoaderInserter::DestinationDestroyed() { destination_ = nullptr; } -void SongLoaderInserter::AudioCDTracksLoaded(SongLoader *loader) { +void SongLoaderInserter::AudioCDTracksLoadFinished(SongLoader *loader) { + songs_ = loader->songs(); - InsertSongs(); + if (songs_.isEmpty()) { + for (const QString &error : loader->errors()) { + emit Error(error); + } + } + else { + InsertSongs(); + } + } -void SongLoaderInserter::AudioCDTagsLoaded(bool success) { +void SongLoaderInserter::AudioCDTagsLoaded(const bool success) { SongLoader *loader = qobject_cast(sender()); if (!loader || !destination_) return; @@ -140,10 +149,12 @@ void SongLoaderInserter::AudioCDTagsLoaded(bool success) { } void SongLoaderInserter::InsertSongs() { + // Insert songs (that haven't been completely loaded) to allow user to see and play them while not loaded completely if (destination_) { destination_->InsertSongsOrCollectionItems(songs_, row_, play_now_, enqueue_, enqueue_next_); } + } void SongLoaderInserter::AsyncLoad() { diff --git a/src/playlist/songloaderinserter.h b/src/playlist/songloaderinserter.h index 68bfda683..41f44d6cc 100644 --- a/src/playlist/songloaderinserter.h +++ b/src/playlist/songloaderinserter.h @@ -38,8 +38,9 @@ class Playlist; class SongLoaderInserter : public QObject { Q_OBJECT + public: - SongLoaderInserter(TaskManager *task_manager, CollectionBackendInterface *collection, const Player *player); + explicit SongLoaderInserter(TaskManager *task_manager, CollectionBackendInterface *collection, const Player *player); ~SongLoaderInserter(); void Load(Playlist *destination, int row, bool play_now, bool enqueue, bool enqueue_next, const QList &urls); @@ -52,8 +53,8 @@ class SongLoaderInserter : public QObject { private slots: void DestinationDestroyed(); - void AudioCDTracksLoaded(SongLoader *loader); - void AudioCDTagsLoaded(bool success); + void AudioCDTracksLoadFinished(SongLoader *loader); + void AudioCDTagsLoaded(const bool success); void InsertSongs(); private: