From ae13fe7f52362fe0a1527640c75301838885581d Mon Sep 17 00:00:00 2001 From: Jonas Kvinge Date: Mon, 9 Jun 2025 04:16:07 +0200 Subject: [PATCH] Fix loading CD tracks in devices Fixes #1676 --- src/device/cddadevice.cpp | 78 ++++++++++++++++++++--- src/device/cddadevice.h | 21 +++++- src/device/cddasongloader.cpp | 117 ++++++++++++++++++++-------------- src/device/cddasongloader.h | 21 +++--- src/device/connecteddevice.h | 3 - 5 files changed, 166 insertions(+), 74 deletions(-) diff --git a/src/device/cddadevice.cpp b/src/device/cddadevice.cpp index ed0479c78..1a3d2824c 100644 --- a/src/device/cddadevice.cpp +++ b/src/device/cddadevice.cpp @@ -2,7 +2,7 @@ * Strawberry Music Player * This file was part of Clementine. * Copyright 2010, David Sansome - * Copyright 2018-2021, Jonas Kvinge + * Copyright 2018-2025, Jonas Kvinge * * 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 + +#include +#include + +#include + #include #include +#include #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 - cdda_song_loader_.LoadSongs(); + if (!cdio_) { + 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()) { - return; + if (watch && !timer_disc_changed_->isActive()) { + 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); + +} + diff --git a/src/device/cddadevice.h b/src/device/cddadevice.h index c4040a4c2..52012cd49 100644 --- a/src/device/cddadevice.h +++ b/src/device/cddadevice.h @@ -2,7 +2,7 @@ * Strawberry Music Player * This file was part of Clementine. * Copyright 2010, David Sansome - * Copyright 2018-2021, Jonas Kvinge + * Copyright 2018-2025, Jonas Kvinge * * 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 + +#include +#include + #include #include #include @@ -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 diff --git a/src/device/cddasongloader.cpp b/src/device/cddasongloader.cpp index ec8c829cf..28b107087 100644 --- a/src/device/cddasongloader.cpp +++ b/src/device/cddasongloader.cpp @@ -2,7 +2,7 @@ * Strawberry Music Player * This file was part of Clementine. * Copyright 2010, David Sansome - * Copyright 2018-2021, Jonas Kvinge + * Copyright 2018-2025, Jonas Kvinge * * 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 +#include -#include +#include #include #include #include -#include - #include #include -#include #include #include #include #include #include +#include #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()), - cdda_(nullptr), - cdio_(nullptr) {} + cdda_(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_); - cdio_ = cdio_open(url_.path().toLocal8Bit().constData(), DRIVER_DEVICE); - if (cdio_ == nullptr) { - Error(u"Unable to open CDIO device."_s); + if (IsActive()) { 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(songs.size()) <= g_list_length(entries)) { - int i = 0; - for (GList *node = entries; node != nullptr; node = node->next) { - GstTocEntry *entry = static_cast(node->data); - qint64 duration = 0; - gint64 start = 0, stop = 0; - if (gst_toc_entry_get_start_stop_times(entry, &start, &stop)) duration = stop - start; - songs[i++].set_length_nanosec(duration); + if (entries) { + if (static_cast(songs.size()) <= g_list_length(entries)) { + int i = 0; + for (GList *node = entries; node != nullptr; node = node->next) { + GstTocEntry *entry = static_cast(node->data); + qint64 duration = 0; + gint64 start = 0, stop = 0; + 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); - 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); + musicbrainz_discid = QString::fromUtf8(string_mb); 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(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() { - - 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; - -} +#endif // HAVE_MUSICBRAINZ void CddaSongLoader::Error(const QString &error) { diff --git a/src/device/cddasongloader.h b/src/device/cddasongloader.h index 4e8f38fdd..c98ebef74 100644 --- a/src/device/cddasongloader.h +++ b/src/device/cddasongloader.h @@ -2,7 +2,7 @@ * Strawberry Music Player * This file was part of Clementine. * Copyright 2010, David Sansome - * Copyright 2018-2021, Jonas Kvinge + * Copyright 2018-2025, Jonas Kvinge * * 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 - -#include -#include - #include #include @@ -36,6 +31,7 @@ #include #include #include +#include #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 network_; GstElement *cdda_; - CdIo_t *cdio_; QMutex mutex_load_; + QFuture loading_future_; }; #endif // CDDASONGLOADER_H diff --git a/src/device/connecteddevice.h b/src/device/connecteddevice.h index 1bcc3d190..341501169 100644 --- a/src/device/connecteddevice.h +++ b/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;