From fd585e8aa4f53e1264f6017b592064d7867049ee Mon Sep 17 00:00:00 2001 From: Lukas Prediger Date: Mon, 23 Aug 2021 19:15:23 +0300 Subject: [PATCH] RipCDDialog: no longer forces Cdda* to emit signals - CddaDevice: Removed LoadSongs() method (then renamed ForceLoadSongs to LoadSongs) - CddaDevice: added songs() method to get currently song list - CddaSongLoader: cached_tracks is now thread-safe --- src/devices/cddadevice.cpp | 13 ++++----- src/devices/cddadevice.h | 4 +-- src/devices/cddasongloader.cpp | 53 ++++++++++++++++++++-------------- src/devices/cddasongloader.h | 5 ++-- src/ripper/ripcddialog.cpp | 4 ++- 5 files changed, 44 insertions(+), 35 deletions(-) diff --git a/src/devices/cddadevice.cpp b/src/devices/cddadevice.cpp index f51da3be1..ced593888 100644 --- a/src/devices/cddadevice.cpp +++ b/src/devices/cddadevice.cpp @@ -52,7 +52,7 @@ bool CddaDevice::Init() { if (!cdio_) { cdio_ = cdio_open(url_.path().toLocal8Bit().constData(), DRIVER_DEVICE); if (!cdio_) return false; - ForceLoadSongs(); + LoadSongs(); WatchForDiscChanges(true); } return true; @@ -71,16 +71,11 @@ void CddaDevice::WatchForDiscChanges(bool watch) { disc_changed_timer_.stop(); } -void CddaDevice::ForceLoadSongs() { +void CddaDevice::LoadSongs() { cdda_song_loader_.LoadSongs(); disc_changed_timer_.stop(); } -void CddaDevice::LoadSongs() { - SongList songs = cdda_song_loader_.cached_tracks(); - SongsLoaded(songs); -} - void CddaDevice::SongsLoaded(const SongList& songs) { model_->Reset(); song_count_ = songs.size(); @@ -111,6 +106,8 @@ void CddaDevice::CheckDiscChanged() { song_count_ = 0; SongList no_songs; SongsLoaded(no_songs); - ForceLoadSongs(); + LoadSongs(); } } + +SongList CddaDevice::songs() const { return cdda_song_loader_.cached_tracks(); } diff --git a/src/devices/cddadevice.h b/src/devices/cddadevice.h index 6859a8183..4a326cbd8 100644 --- a/src/devices/cddadevice.h +++ b/src/devices/cddadevice.h @@ -54,7 +54,7 @@ class CddaDevice : public ConnectedDevice { // Check whether a valid device handle was opened. bool IsValid() const; void WatchForDiscChanges(bool watch); - void LoadSongs(); + SongList songs() const; static QStringList url_schemes() { return QStringList() << "cdda"; } @@ -79,7 +79,7 @@ class CddaDevice : public ConnectedDevice { void CheckDiscChanged(); private: - void ForceLoadSongs(); + void LoadSongs(); CdIo_t* cdio_; QTimer disc_changed_timer_; diff --git a/src/devices/cddasongloader.cpp b/src/devices/cddasongloader.cpp index dd92b8fc6..a84d7656d 100644 --- a/src/devices/cddasongloader.cpp +++ b/src/devices/cddasongloader.cpp @@ -68,6 +68,7 @@ bool CddaSongLoader::IsActive() const { return loading_future_.isRunning(); } void CddaSongLoader::LoadSongs() { // only dispatch a new thread for loading tracks if not already running. if (!IsActive()) { + QMutexLocker lock(&disc_mutex_); disc_ = Disc(); loading_future_ = QtConcurrent::run(this, &CddaSongLoader::LoadSongsFromCdda); @@ -350,37 +351,45 @@ void CddaSongLoader::ProcessMusicBrainzResponse( return; } - if (disc_.tracks.length() != results.length()) { - qLog(Warning) << "Number of tracks in metadata does not match number of " - "songs on disc!"; - // no idea how to recover; signal that no further updates will follow now - emit Finished(); - return; + { + QMutexLocker lock(&disc_mutex_); + + if (disc_.tracks.length() != results.length()) { + qLog(Warning) << "Number of tracks in metadata does not match number of " + "songs on disc!"; + // no idea how to recover; signal that no further updates will follow now + emit Finished(); + return; + } + + for (int i = 0; i < results.length(); ++i) { + const MusicBrainzClient::Result& new_song_info = results[i]; + Song& song = disc_.tracks[i]; + + if (!disc_.has_titles) song.set_title(new_song_info.title_); + if (song.album().isEmpty()) song.set_album(album); + if (song.artist().isEmpty()) song.set_artist(artist); + + if (song.length_nanosec() == -1) + song.set_length_nanosec(new_song_info.duration_msec_ * kNsecPerMsec); + if (song.track() < 1) song.set_track(new_song_info.track_); + if (song.year() == -1) song.set_year(new_song_info.year_); + } + disc_.has_titles = true; } - for (int i = 0; i < results.length(); ++i) { - const MusicBrainzClient::Result& new_song_info = results[i]; - Song& song = disc_.tracks[i]; - - if (!disc_.has_titles) song.set_title(new_song_info.title_); - if (song.album().isEmpty()) song.set_album(album); - if (song.artist().isEmpty()) song.set_artist(artist); - - if (song.length_nanosec() == -1) - song.set_length_nanosec(new_song_info.duration_msec_ * kNsecPerMsec); - if (song.track() < 1) song.set_track(new_song_info.track_); - if (song.year() == -1) song.set_year(new_song_info.year_); - } - disc_.has_titles = true; - emit SongsMetadataLoaded(disc_.tracks); emit Finished(); // no further updates will follow } void CddaSongLoader::SetDiscTracks(const SongList& songs, bool has_titles) { + QMutexLocker lock(&disc_mutex_); disc_.tracks = songs; disc_.has_titles = has_titles; emit SongsUpdated(disc_.tracks); } -SongList CddaSongLoader::cached_tracks() const { return disc_.tracks; } +SongList CddaSongLoader::cached_tracks() const { + QMutexLocker lock(&disc_mutex_); + return disc_.tracks; +} diff --git a/src/devices/cddasongloader.h b/src/devices/cddasongloader.h index 989bfffab..842a82e28 100644 --- a/src/devices/cddasongloader.h +++ b/src/devices/cddasongloader.h @@ -48,8 +48,8 @@ class CddaSongLoader : public QObject { void LoadSongs(); bool IsActive() const; - // The list of currently cached tracks. This gets updated during calls - // LoadSongs() is called. Not thread-safe. + // The list of currently cached tracks. This gets updated when + // LoadSongs() is called. SongList cached_tracks() const; signals: @@ -110,6 +110,7 @@ class CddaSongLoader : public QObject { QFuture loading_future_; std::atomic may_load_; Disc disc_; + mutable QMutex disc_mutex_; }; #endif // CDDASONGLOADER_H diff --git a/src/ripper/ripcddialog.cpp b/src/ripper/ripcddialog.cpp index f5e44e351..d6ba661c2 100644 --- a/src/ripper/ripcddialog.cpp +++ b/src/ripper/ripcddialog.cpp @@ -335,10 +335,12 @@ void RipCDDialog::DeviceSelected(int device_index) { return; } + SongList songs = cdda_device_->songs(); + SongsLoaded(songs); + connect(cdda_device_.get(), SIGNAL(DiscChanged()), SLOT(DiscChanged())); connect(cdda_device_.get(), SIGNAL(SongsDiscovered(SongList)), SLOT(SongsLoaded(SongList))); - cdda_device_->LoadSongs(); } void RipCDDialog::Finished(Ripper* ripper) {