Fix loading CD tracks in devices

Fixes #1676

src/device/cddadevice.cpp

@@ -2,7 +2,7 @@
  * Strawberry Music Player
  * This file was part of Clementine.
  * Copyright 2010, David Sansome <me@davidsansome.com>
- * Copyright 2018-2021, Jonas Kvinge <jonas@jkvinge.net>
+ * Copyright 2018-2025, Jonas Kvinge <jonas@jkvinge.net>
  *
  * Strawberry is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -21,10 +21,19 @@
 
 #include "config.h"
 
+#include <cstddef>
+
+#include <cdio/types.h>
+#include <cdio/cdio.h>
+
+#include <chrono>
+
 #include <QString>
 #include <QUrl>
+#include <QTimer>
 
 #include "includes/shared_ptr.h"
+#include "core/logging.h"
 #include "collection/collectionmodel.h"
 #include "cddasongloader.h"
 #include "connecteddevice.h"
@@ -33,6 +42,8 @@
 class DeviceLister;
 class DeviceManager;
 
+using namespace std::chrono_literals;
+
 CddaDevice::CddaDevice(const QUrl &url,
                        DeviceLister *lister,
                        const QString &unique_id,
@@ -45,29 +56,72 @@ CddaDevice::CddaDevice(const QUrl &url,
                        const bool first_time,
                        QObject *parent)
     : ConnectedDevice(url, lister, unique_id, device_manager, task_manager, database, tagreader_client, albumcover_loader, database_id, first_time, parent),
-      cdda_song_loader_(url) {
+      cdda_song_loader_(url),
+      cdio_(nullptr),
+      timer_disc_changed_(new QTimer(this)) {
+
+  timer_disc_changed_->setInterval(1s);
 
   QObject::connect(&cdda_song_loader_, &CddaSongLoader::SongsLoaded, this, &CddaDevice::SongsLoaded);
   QObject::connect(&cdda_song_loader_, &CddaSongLoader::SongsDurationLoaded, this, &CddaDevice::SongsLoaded);
   QObject::connect(&cdda_song_loader_, &CddaSongLoader::SongsMetadataLoaded, this, &CddaDevice::SongsLoaded);
+  QObject::connect(&cdda_song_loader_, &CddaSongLoader::SongLoadingFinished, this, &CddaDevice::SongLoadingFinished);
   QObject::connect(this, &CddaDevice::SongsDiscovered, collection_model_, &CollectionModel::AddReAddOrUpdate);
+  QObject::connect(timer_disc_changed_, &QTimer::timeout, this, &CddaDevice::CheckDiscChanged);
+
+}
+
+CddaDevice::~CddaDevice() {
+
+  if (cdio_) {
+    cdio_destroy(cdio_);
+    cdio_ = nullptr;
+  }
 
 }
 
 bool CddaDevice::Init() {
 
-  song_count_ = 0;  // Reset song count, in case it was already set
+  if (!cdio_) {
-  cdda_song_loader_.LoadSongs();
+    cdio_ = cdio_open(url_.path().toLocal8Bit().constData(), DRIVER_DEVICE);
+    if (!cdio_) return false;
+  }
+
+  LoadSongs();
+
+  WatchForDiscChanges(true);
+
   return true;
 
 }
 
-void CddaDevice::Refresh() {
+void CddaDevice::WatchForDiscChanges(const bool watch) {
 
-  if (!cdda_song_loader_.HasChanged()) {
+  if (watch && !timer_disc_changed_->isActive()) {
-    return;
+    timer_disc_changed_->start();
   }
-  Init();
+  else if (!watch && timer_disc_changed_->isActive()) {
+    timer_disc_changed_->stop();
+  }
+
+}
+
+void CddaDevice::CheckDiscChanged() {
+
+  if (!cdio_ || cdda_song_loader_.IsActive()) return;
+
+  if (cdio_get_media_changed(cdio_) == 1) {
+    qLog(Debug) << "CD changed, reloading songs";
+    SongsLoaded();
+    LoadSongs();
+  }
+
+}
+
+void CddaDevice::LoadSongs() {
+
+  cdda_song_loader_.LoadSongs();
+  WatchForDiscChanges(false);
 
 }
 
@@ -76,5 +130,13 @@ void CddaDevice::SongsLoaded(const SongList &songs) {
   collection_model_->Reset();
   Q_EMIT SongsDiscovered(songs);
   song_count_ = songs.size();
+  (void)cdio_get_media_changed(cdio_);
 
 }
+
+void CddaDevice::SongLoadingFinished() {
+
+  WatchForDiscChanges(true);
+
+}
+

src/device/cddadevice.h

@@ -2,7 +2,7 @@
  * Strawberry Music Player
  * This file was part of Clementine.
  * Copyright 2010, David Sansome <me@davidsansome.com>
- * Copyright 2018-2021, Jonas Kvinge <jonas@jkvinge.net>
+ * Copyright 2018-2025, Jonas Kvinge <jonas@jkvinge.net>
  *
  * Strawberry is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -24,6 +24,11 @@
 
 #include "config.h"
 
+#include <cstddef>
+
+#include <cdio/types.h>
+#include <cdio/cdio.h>
+
 #include <QObject>
 #include <QString>
 #include <QStringList>
@@ -35,6 +40,8 @@
 #include "cddasongloader.h"
 #include "connecteddevice.h"
 
+class QTimer;
+
 class DeviceLister;
 class DeviceManager;
 class TaskManager;
@@ -58,21 +65,29 @@ class CddaDevice : public ConnectedDevice {
                                   const bool first_time,
                                   QObject *parent = nullptr);
 
+  ~CddaDevice();
+
   bool Init() override;
-  void Refresh() override;
   bool CopyToStorage(const CopyJob&, QString&) override { return false; }
   bool DeleteFromStorage(const MusicStorage::DeleteJob&) override { return false; }
 
   static QStringList url_schemes() { return QStringList() << QStringLiteral("cdda"); }
 
+  void LoadSongs();
+  void WatchForDiscChanges(const bool watch);
+
  Q_SIGNALS:
   void SongsDiscovered(const SongList &songs);
 
  private Q_SLOTS:
-  void SongsLoaded(const SongList &songs);
+  void CheckDiscChanged();
+  void SongsLoaded(const SongList &songs = SongList());
+  void SongLoadingFinished();
 
  private:
   CddaSongLoader cdda_song_loader_;
+  CdIo_t *cdio_;
+  QTimer *timer_disc_changed_;
 };
 
 #endif  // CDDADEVICE_H

src/device/cddasongloader.cpp

@@ -2,7 +2,7 @@
  * Strawberry Music Player
  * This file was part of Clementine.
  * Copyright 2010, David Sansome <me@davidsansome.com>
- * Copyright 2018-2021, Jonas Kvinge <jonas@jkvinge.net>
+ * Copyright 2018-2025, Jonas Kvinge <jonas@jkvinge.net>
  *
  * Strawberry is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -21,25 +21,23 @@
 
 #include "config.h"
 
-#include <memory>
+#include <QtGlobal>
 
-#include <cstddef>
+#include <memory>
 
 #include <glib.h>
 #include <glib/gtypes.h>
 #include <glib-object.h>
 
-#include <cdio/cdio.h>
-
 #include <gst/gst.h>
 #include <gst/tag/tag.h>
 
-#include <QtGlobal>
 #include <QObject>
 #include <QMutex>
 #include <QByteArray>
 #include <QString>
 #include <QUrl>
+#include <QtConcurrentRun>
 
 #include "cddasongloader.h"
 #include "includes/shared_ptr.h"
@@ -55,11 +53,14 @@ CddaSongLoader::CddaSongLoader(const QUrl &url, QObject *parent)
     : QObject(parent),
       url_(url),
       network_(make_shared<NetworkAccessManager>()),
-      cdda_(nullptr),
+      cdda_(nullptr) {
-      cdio_(nullptr) {}
+
+  QObject::connect(this, &CddaSongLoader::MusicBrainzDiscIdLoaded, this, &CddaSongLoader::LoadMusicBrainzCDTags);
+
+}
 
 CddaSongLoader::~CddaSongLoader() {
-  if (cdio_) cdio_destroy(cdio_);
+  loading_future_.waitForFinished();
 }
 
 QUrl CddaSongLoader::GetUrlFromTrack(int track_number) const {
@@ -74,20 +75,27 @@ QUrl CddaSongLoader::GetUrlFromTrack(int track_number) const {
 
 void CddaSongLoader::LoadSongs() {
 
-  QMutexLocker locker(&mutex_load_);
+  if (IsActive()) {
-  cdio_ = cdio_open(url_.path().toLocal8Bit().constData(), DRIVER_DEVICE);
-  if (cdio_ == nullptr) {
-    Error(u"Unable to open CDIO device."_s);
     return;
   }
 
-  // Create gstreamer cdda element
+  loading_future_ = QtConcurrent::run(&CddaSongLoader::LoadSongsFromCDDA, this);
+
+}
+
+void CddaSongLoader::LoadSongsFromCDDA() {
+
+  QMutexLocker l(&mutex_load_);
+
   GError *error = nullptr;
   cdda_ = gst_element_make_from_uri(GST_URI_SRC, "cdda://", nullptr, &error);
   if (error) {
     Error(QStringLiteral("%1: %2").arg(error->code).arg(QString::fromUtf8(error->message)));
   }
-  if (!cdda_) return;
+  if (!cdda_) {
+    Q_EMIT SongLoadingFinished();
+    return;
+  }
 
   if (!url_.isEmpty()) {
     g_object_set(cdda_, "device", g_strdup(url_.path().toLocal8Bit().constData()), nullptr);
@@ -102,6 +110,7 @@ void CddaSongLoader::LoadSongs() {
     gst_object_unref(GST_OBJECT(cdda_));
     cdda_ = nullptr;
     Error(tr("Error while setting CDDA device to ready state."));
+    Q_EMIT SongLoadingFinished();
     return;
   }
 
@@ -110,6 +119,7 @@ void CddaSongLoader::LoadSongs() {
     gst_object_unref(GST_OBJECT(cdda_));
     cdda_ = nullptr;
     Error(tr("Error while setting CDDA device to pause state."));
+    Q_EMIT SongLoadingFinished();
     return;
   }
 
@@ -122,6 +132,7 @@ void CddaSongLoader::LoadSongs() {
     gst_object_unref(GST_OBJECT(cdda_));
     cdda_ = nullptr;
     Error(tr("Error while querying CDDA tracks."));
+    Q_EMIT SongLoadingFinished();
     return;
   }
 
@@ -131,6 +142,7 @@ void CddaSongLoader::LoadSongs() {
     gst_object_unref(GST_OBJECT(cdda_));
     cdda_ = nullptr;
     Error(tr("Error while querying CDDA tracks."));
+    Q_EMIT SongLoadingFinished();
     return;
   }
 
@@ -149,7 +161,6 @@ void CddaSongLoader::LoadSongs() {
   }
   Q_EMIT SongsLoaded(songs);
 
-
   gst_tag_register_musicbrainz_tags();
 
   GstElement *pipeline = gst_pipeline_new("pipeline");
@@ -180,14 +191,16 @@ void CddaSongLoader::LoadSongs() {
     gst_message_parse_toc(msg_toc, &toc, nullptr);
     if (toc) {
       GList *entries = gst_toc_get_entries(toc);
-      if (entries && static_cast<guint>(songs.size()) <= g_list_length(entries)) {
+      if (entries) {
-        int i = 0;
+        if (static_cast<guint>(songs.size()) <= g_list_length(entries)) {
-        for (GList *node = entries; node != nullptr; node = node->next) {
+          int i = 0;
-          GstTocEntry *entry = static_cast<GstTocEntry*>(node->data);
+          for (GList *node = entries; node != nullptr; node = node->next) {
-          qint64 duration = 0;
+            GstTocEntry *entry = static_cast<GstTocEntry*>(node->data);
-          gint64 start = 0, stop = 0;
+            qint64 duration = 0;
-          if (gst_toc_entry_get_start_stop_times(entry, &start, &stop)) duration = stop - start;
+            gint64 start = 0, stop = 0;
-          songs[i++].set_length_nanosec(duration);
+            if (gst_toc_entry_get_start_stop_times(entry, &start, &stop)) duration = stop - start;
+            songs[i++].set_length_nanosec(duration);
+          }
         }
       }
       gst_toc_unref(toc);
@@ -196,6 +209,7 @@ void CddaSongLoader::LoadSongs() {
   }
   Q_EMIT SongsDurationLoaded(songs);
 
+  QString musicbrainz_discid;
 #ifdef HAVE_MUSICBRAINZ
   // Handle TAG message: generate MusicBrainz DiscId
   if (msg_tag) {
@@ -204,15 +218,10 @@ void CddaSongLoader::LoadSongs() {
     if (tags) {
       char *string_mb = nullptr;
       if (gst_tag_list_get_string(tags, GST_TAG_CDDA_MUSICBRAINZ_DISCID, &string_mb)) {
-        QString musicbrainz_discid = QString::fromUtf8(string_mb);
+        musicbrainz_discid = QString::fromUtf8(string_mb);
-        qLog(Info) << "MusicBrainz Disc ID: " << musicbrainz_discid;
-
-        MusicBrainzClient *musicbrainz_client = new MusicBrainzClient(network_);
-        QObject::connect(musicbrainz_client, &MusicBrainzClient::DiscIdFinished, this, &CddaSongLoader::AudioCDTagsLoaded);
-        musicbrainz_client->StartDiscIdRequest(musicbrainz_discid);
         g_free(string_mb);
       }
-      gst_tag_list_unref(tags);
+      gst_tag_list_free(tags);
     }
     gst_message_unref(msg_tag);
   }
@@ -222,14 +231,36 @@ void CddaSongLoader::LoadSongs() {
   // This will also cause cdda_ to be unref'd.
   gst_object_unref(pipeline);
 
+  if (musicbrainz_discid.isEmpty()) {
+    Q_EMIT SongLoadingFinished();
+  }
+  else {
+    qLog(Info) << "MusicBrainz Disc ID:" << musicbrainz_discid;
+    Q_EMIT MusicBrainzDiscIdLoaded(musicbrainz_discid);
+  }
+
 }
 
 #ifdef HAVE_MUSICBRAINZ
-void CddaSongLoader::AudioCDTagsLoaded(const QString &artist, const QString &album, const MusicBrainzClient::ResultList &results) {
+
+void CddaSongLoader::LoadMusicBrainzCDTags(const QString &musicbrainz_discid) const {
+
+  MusicBrainzClient *musicbrainz_client = new MusicBrainzClient(network_);
+  QObject::connect(musicbrainz_client, &MusicBrainzClient::DiscIdFinished, this, &CddaSongLoader::MusicBrainzCDTagsLoaded);
+  musicbrainz_client->StartDiscIdRequest(musicbrainz_discid);
+
+}
+
+void CddaSongLoader::MusicBrainzCDTagsLoaded(const QString &artist, const QString &album, const MusicBrainzClient::ResultList &results) {
 
   MusicBrainzClient *musicbrainz_client = qobject_cast<MusicBrainzClient*>(sender());
   musicbrainz_client->deleteLater();
-  if (results.empty()) return;
+
+  if (results.empty()) {
+    Q_EMIT SongLoadingFinished();
+    return;
+  }
+
   SongList songs;
   songs.reserve(results.count());
   int track_number = 1;
@@ -244,29 +275,17 @@ void CddaSongLoader::AudioCDTagsLoaded(const QString &artist, const QString &alb
     song.set_id(track_number);
     song.set_filetype(Song::FileType::CDDA);
     song.set_valid(true);
-    // We need to set url: that's how playlist will find the correct item to update
+    // We need to set URL, that's how playlist will find the correct item to update
     song.set_url(GetUrlFromTrack(track_number++));
     songs << song;
   }
+
   Q_EMIT SongsMetadataLoaded(songs);
+  Q_EMIT SongLoadingFinished();
 
 }
-#endif
 
-bool CddaSongLoader::HasChanged() {
+#endif  // HAVE_MUSICBRAINZ
-
-  if (cdio_ && cdio_get_media_changed(cdio_) != 1) {
-    return false;
-  }
-  // Check if mutex is already token (i.e. init is already taking place)
-  if (!mutex_load_.tryLock()) {
-    return false;
-  }
-  mutex_load_.unlock();
-
-  return true;
-
-}
 
 void CddaSongLoader::Error(const QString &error) {
 

src/device/cddasongloader.h

@@ -2,7 +2,7 @@
  * Strawberry Music Player
  * This file was part of Clementine.
  * Copyright 2010, David Sansome <me@davidsansome.com>
- * Copyright 2018-2021, Jonas Kvinge <jonas@jkvinge.net>
+ * Copyright 2018-2025, Jonas Kvinge <jonas@jkvinge.net>
  *
  * Strawberry is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -24,11 +24,6 @@
 
 #include "config.h"
 
-#include <cstddef>
-
-#include <cdio/types.h>
-#include <cdio/cdio.h>
-
 #include <gst/gstelement.h>
 #include <gst/audio/gstaudiocdsrc.h>
 
@@ -36,6 +31,7 @@
 #include <QMutex>
 #include <QString>
 #include <QUrl>
+#include <QFuture>
 
 #include "includes/shared_ptr.h"
 #include "core/song.h"
@@ -45,7 +41,6 @@
 
 class NetworkAccessManager;
 
-// This class provides a (hopefully) nice, high level interface to get CD information and load tracks
 class CddaSongLoader : public QObject {
   Q_OBJECT
 
@@ -53,31 +48,35 @@ class CddaSongLoader : public QObject {
   explicit CddaSongLoader(const QUrl &url, QObject *parent = nullptr);
   ~CddaSongLoader() override;
 
-  // Load songs. Signals declared below will be emitted anytime new information will be available.
   void LoadSongs();
-  bool HasChanged();
+
+  bool IsActive() const { return loading_future_.isRunning(); }
 
  private:
+  void LoadSongsFromCDDA();
   void Error(const QString &error);
   QUrl GetUrlFromTrack(const int track_number) const;
 
  Q_SIGNALS:
   void SongsLoadError(const QString &error);
   void SongsLoaded(const SongList &songs);
+  void SongLoadingFinished();
   void SongsDurationLoaded(const SongList &songs, const QString &error = QString());
   void SongsMetadataLoaded(const SongList &songs);
+  void MusicBrainzDiscIdLoaded(const QString &musicbrainz_discid);
 
  private Q_SLOTS:
 #ifdef HAVE_MUSICBRAINZ
-  void AudioCDTagsLoaded(const QString &artist, const QString &album, const MusicBrainzClient::ResultList &results);
+  void LoadMusicBrainzCDTags(const QString &musicbrainz_discid) const;
+  void MusicBrainzCDTagsLoaded(const QString &artist, const QString &album, const MusicBrainzClient::ResultList &results);
 #endif
 
  private:
   const QUrl url_;
   SharedPtr<NetworkAccessManager> network_;
   GstElement *cdda_;
-  CdIo_t *cdio_;
   QMutex mutex_load_;
+  QFuture<void> loading_future_;
 };
 
 #endif  // CDDASONGLOADER_H

src/device/connecteddevice.h

@@ -67,9 +67,6 @@ class ConnectedDevice : public QObject, public virtual MusicStorage, public enab
   virtual bool IsLoading() { return false; }
   virtual void NewConnection() {}
   virtual void ConnectAsync();
-  // For some devices (e.g. CD devices) we don't have callbacks to be notified when something change:
-  // we can call this method to refresh device's state
-  virtual void Refresh() {}
 
   TranscodeMode GetTranscodeMode() const override;
   Song::FileType GetTranscodeFormat() const override;