From 90ec6f6a24f58a338a7fffa23a4c1291380ac280 Mon Sep 17 00:00:00 2001 From: Lukas Prediger Date: Fri, 21 May 2021 20:52:29 +0300 Subject: [PATCH] CddaSongLoader now reads CD-Text for metadata currently this gets overwritten by musicbrainz response almost immediately, though --- src/devices/cddadevice.cpp | 6 +- src/devices/cddasongloader.cpp | 233 ++++++++++++++++++++++++++------- src/devices/cddasongloader.h | 42 +++++- src/ripper/ripcddialog.cpp | 13 +- 4 files changed, 236 insertions(+), 58 deletions(-) diff --git a/src/devices/cddadevice.cpp b/src/devices/cddadevice.cpp index d48750358..a790a2b04 100644 --- a/src/devices/cddadevice.cpp +++ b/src/devices/cddadevice.cpp @@ -32,11 +32,7 @@ CddaDevice::CddaDevice(const QUrl& url, DeviceLister* lister, cdio_(nullptr), disc_changed_timer_(), cdda_song_loader_(url) { - connect(&cdda_song_loader_, SIGNAL(SongsLoaded(SongList)), this, - SLOT(SongsLoaded(SongList))); - connect(&cdda_song_loader_, SIGNAL(SongsDurationLoaded(SongList)), this, - SLOT(SongsLoaded(SongList))); - connect(&cdda_song_loader_, SIGNAL(SongsMetadataLoaded(SongList)), this, + connect(&cdda_song_loader_, SIGNAL(SongsUpdated(SongList)), this, SLOT(SongsLoaded(SongList))); connect(this, SIGNAL(SongsDiscovered(SongList)), model_, SLOT(SongsDiscovered(SongList))); diff --git a/src/devices/cddasongloader.cpp b/src/devices/cddasongloader.cpp index edbb30305..36946fc0f 100644 --- a/src/devices/cddasongloader.cpp +++ b/src/devices/cddasongloader.cpp @@ -1,5 +1,6 @@ /* This file is part of Clementine. Copyright 2014, David Sansome + Copyright 2021, Lukas Prediger Clementine is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -27,9 +28,21 @@ #include "core/timeconstants.h" CddaSongLoader::CddaSongLoader(const QUrl& url, QObject* parent) - : QObject(parent), url_(url), cdda_(nullptr), may_load_(true) { - connect(this, SIGNAL(MusicBrainzDiscIdLoaded(const QString&)), - SLOT(LoadAudioCDTags(const QString&))); + : QObject(parent), url_(url), cdda_(nullptr), may_load_(true), disc_() { + connect(this, &CddaSongLoader::MusicBrainzDiscIdLoaded, this, + &CddaSongLoader::LoadAudioCDTags); + connect(this, &CddaSongLoader::SongsLoaded, + [this](const SongList& song_list) { + SetDiscTracks(song_list, /*has_titles=*/false); + }); + connect(this, &CddaSongLoader::SongsDurationLoaded, + [this](const SongList& song_list) { + SetDiscTracks(song_list, /*has_titles=*/false); + }); + connect(this, &CddaSongLoader::SongsMetadataLoaded, + [this](const SongList& song_list) { + SetDiscTracks(song_list, /*has_titles=*/true); + }); } CddaSongLoader::~CddaSongLoader() { @@ -55,17 +68,90 @@ 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()) { + disc_ = Disc(); loading_future_ = QtConcurrent::run(this, &CddaSongLoader::LoadSongsFromCdda); } } +bool CddaSongLoader::ParseSongTags(SongList& songs, GstTagList* tags, + gint* track_no) { + //// cdiocddasrc reads cd-text with following mapping from cdio + /// + /// DISC LEVEL : + /// CDTEXT_FIELD_PERFORMER -> GST_TAG_ALBUM_ARTIST + /// CDTEXT_FIELD_TITLE -> GST_TAG_ALBUM + /// CDTEXT_FIELD_GENRE -> GST_TAG_GENRE + /// + /// TRACK LEVEL : + /// CDTEXT_FIELD_PERFORMER -> GST_TAG_ARTIST + /// CDTEXT_FIELD_TITLE -> GST_TAG_TITLE + + guint track_number; + if (!gst_tag_list_get_uint(tags, GST_TAG_TRACK_NUMBER, &track_number)) { + qLog(Error) << "Track tags do not contain track number!"; + return false; + } + + Q_ASSERT(track_number != 0u); + Q_ASSERT(static_cast(track_number) <= songs.size()); + Song& song = songs[static_cast(track_number - 1)]; + *track_no = static_cast(track_number) - 1; + + // qLog(Debug) << gst_tag_list_to_string(tags); + + bool has_loaded_tags = false; + + gchar* buffer = nullptr; + if (gst_tag_list_get_string(tags, GST_TAG_ALBUM, &buffer)) { + has_loaded_tags = true; + song.set_album(QString::fromUtf8(buffer)); + g_free(buffer); + } + + if (gst_tag_list_get_string(tags, GST_TAG_ALBUM_ARTIST, &buffer)) { + has_loaded_tags = true; + song.set_albumartist(QString::fromUtf8(buffer)); + g_free(buffer); + } + + if (gst_tag_list_get_string(tags, GST_TAG_GENRE, &buffer)) { + has_loaded_tags = true; + song.set_genre(QString::fromUtf8(buffer)); + g_free(buffer); + } + + if (gst_tag_list_get_string(tags, GST_TAG_ARTIST, &buffer)) { + has_loaded_tags = true; + song.set_artist(QString::fromUtf8(buffer)); + g_free(buffer); + } + + if (gst_tag_list_get_string(tags, GST_TAG_TITLE, &buffer)) { + has_loaded_tags = true; + song.set_title(QString::fromUtf8(buffer)); + g_free(buffer); + } + + guint64 duration; + if (gst_tag_list_get_uint64(tags, GST_TAG_DURATION, &duration)) { + has_loaded_tags = true; + song.set_length_nanosec(duration); + } + + song.set_track(track_number); + song.set_id(track_number); + song.set_filetype(Song::Type_Cdda); + song.set_valid(true); + song.set_url(GetUrlFromTrack(track_number)); + return has_loaded_tags; +} + void CddaSongLoader::LoadSongsFromCdda() { if (!may_load_) return; - // Create gstreamer cdda element GError* error = nullptr; - cdda_ = gst_element_make_from_uri(GST_URI_SRC, "cdda://", nullptr, &error); + GstElement* cdda_ = gst_element_factory_make("cdiocddasrc", nullptr); if (error) { qLog(Error) << error->code << QString::fromLocal8Bit(error->message); } @@ -93,17 +179,15 @@ void CddaSongLoader::LoadSongsFromCdda() { } // Get number of tracks - GstFormat fmt = gst_format_get_by_nick("track"); - GstFormat out_fmt = fmt; + GstFormat track_fmt = gst_format_get_by_nick("track"); 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_, track_fmt, &num_tracks)) { + qLog(Error) << "Error while querying cdda GstElement for track count"; gst_object_unref(GST_OBJECT(cdda_)); return; } - SongList songs; + SongList initial_song_list; for (int track_number = 1; track_number <= num_tracks; track_number++) { // Init song Song song; @@ -113,9 +197,11 @@ void CddaSongLoader::LoadSongsFromCdda() { song.set_url(GetUrlFromTrack(track_number)); song.set_title(QString("Track %1").arg(track_number)); song.set_track(track_number); - songs << song; + initial_song_list << song; } - emit SongsLoaded(songs); + emit SongsLoaded(initial_song_list); + + SongList tagged_song_list(initial_song_list); gst_tag_register_musicbrainz_tags(); @@ -131,6 +217,7 @@ void CddaSongLoader::LoadSongsFromCdda() { GstMessageType msg_filter = static_cast(GST_MESSAGE_TOC | GST_MESSAGE_TAG); QString musicbrainz_discid; + bool loaded_cd_tags = false; while (may_load_ && msg_filter && (msg = gst_bus_timed_pop_filtered(GST_ELEMENT_BUS(pipeline), 10 * GST_SECOND, msg_filter))) { @@ -140,7 +227,7 @@ void CddaSongLoader::LoadSongsFromCdda() { gst_message_parse_toc(msg, &toc, nullptr); if (toc) { GList* entries = gst_toc_get_entries(toc); - if (entries && songs.size() <= g_list_length(entries)) { + if (entries && initial_song_list.size() <= g_list_length(entries)) { int i = 0; for (GList* node = entries; node != nullptr; node = node->next) { GstTocEntry* entry = static_cast(node->data); @@ -148,35 +235,83 @@ void CddaSongLoader::LoadSongsFromCdda() { gint64 start, stop; if (gst_toc_entry_get_start_stop_times(entry, &start, &stop)) duration = stop - start; - songs[i++].set_length_nanosec(duration); + initial_song_list[i++].set_length_nanosec(duration); } - emit SongsDurationLoaded(songs); + emit SongsDurationLoaded(initial_song_list); msg_filter = static_cast( static_cast(msg_filter) ^ GST_MESSAGE_TOC); } gst_toc_unref(toc); } } else if (GST_MESSAGE_TYPE(msg) == GST_MESSAGE_TAG) { - // Handle TAG message: generate MusicBrainz DiscId + // Handle TAG message: generate MusicBrainz DiscId and read CD-TEXT if + // present + + gint64 + track_number_from_query; // track number gstreamer thinks we are at + gst_element_query_position(cdda_, track_fmt, &track_number_from_query); GstTagList* tags = nullptr; gst_message_parse_tag(msg, &tags); char* string_mb = nullptr; - if (gst_tag_list_get_string(tags, GST_TAG_CDDA_MUSICBRAINZ_DISCID, + if (musicbrainz_discid.isEmpty() && + 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); g_free(string_mb); - qLog(Info) << "MusicBrainz discid: " << musicbrainz_discid; - emit MusicBrainzDiscIdLoaded(musicbrainz_discid); + // emit MusicBrainzDiscIdLoaded(musicbrainz_discid); + // for now, we'll invoke musicbrainz only after having read all CD-TEXT + // tags and emitted a message for it + } + + gint track_number_from_tags; // track number contained in the tag message + loaded_cd_tags |= + ParseSongTags(tagged_song_list, tags, &track_number_from_tags); + gst_tag_list_free(tags); + + // We may receive a tag message for a track we have already seen, not for + // the track we seeked to previously, i.e., track_number_from_tags and + // track_number_from_query do not agree. If we would just wait now, + // nothing else would happen: It seems, gstreamer will for some reason not + // pass the tag message for the song we seeked to in this case or it gets + // lost somewhere. We can't seek again to the track we want to see, + // because gstreamer thinks we are already there and will do nothing. We + // therefore seek to the previous track and resume from there. + // note(lumip): There's a slight risk of an infinite loop here where if + // the above behavior repeats consistently, but in my tests this does not + // happen. + if (track_number_from_tags < track_number_from_query) { + qLog(Debug) << "message query mismatch! : " << track_number_from_tags + << " vs " << track_number_from_query; + gst_element_seek_simple( + pipeline, track_fmt, + static_cast(GST_SEEK_FLAG_FLUSH | + GST_SEEK_FLAG_TRICKMODE), + track_number_from_tags); + continue; + } + gint next_track_number = track_number_from_tags + 1; + + if (next_track_number < num_tracks) { + // more to go, seek to next track to get a tag message for it + gst_element_seek_simple( + pipeline, track_fmt, + static_cast(GST_SEEK_FLAG_FLUSH | + GST_SEEK_FLAG_TRICKMODE), + next_track_number); + } else // we are done with reading track tags: do no longer filter msg_filter = static_cast(static_cast(msg_filter) ^ GST_MESSAGE_TAG); - } - gst_tag_list_free(tags); } gst_message_unref(msg); } + if (loaded_cd_tags) emit SongsMetadataLoaded(tagged_song_list); + if (!musicbrainz_discid.isEmpty()) + emit MusicBrainzDiscIdLoaded(musicbrainz_discid); + + // cleanup gst_element_set_state(pipeline, GST_STATE_NULL); // This will also cause cdda_ to be unref'd. gst_object_unref(pipeline); @@ -187,36 +322,46 @@ void CddaSongLoader::LoadAudioCDTags(const QString& musicbrainz_discid) const { connect(musicbrainz_client, SIGNAL(Finished(const QString&, const QString&, MusicBrainzClient::ResultList)), - SLOT(AudioCDTagsLoaded(const QString&, const QString&, - MusicBrainzClient::ResultList))); + SLOT(ProcessMusicBrainzResponse(const QString&, const QString&, + MusicBrainzClient::ResultList))); musicbrainz_client->StartDiscIdRequest(musicbrainz_discid); } -void CddaSongLoader::AudioCDTagsLoaded( +void CddaSongLoader::ProcessMusicBrainzResponse( const QString& artist, const QString& album, const MusicBrainzClient::ResultList& results) { MusicBrainzClient* musicbrainz_client = qobject_cast(sender()); musicbrainz_client->deleteLater(); - SongList songs; if (results.empty()) return; - int track_number = 1; - for (const MusicBrainzClient::Result& ret : results) { - Song song; - song.set_artist(artist); - song.set_album(album); - song.set_title(ret.title_); - song.set_length_nanosec(ret.duration_msec_ * kNsecPerMsec); - song.set_track(track_number); - song.set_year(ret.year_); - song.set_id(track_number); - song.set_filetype(Song::Type_Cdda); - song.set_valid(true); - // We need to set url: that's how playlist will find the correct item to - // update - song.set_url(GetUrlFromTrack(track_number++)); - songs << song; + + if (disc_.tracks.length() != results.length()) { + qLog(Warning) << "Number of tracks in metadata does not match number of " + "songs on disc!"; + return; // no idea how to recover; just do nothing } - emit SongsMetadataLoaded(songs); + + 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 SongsUpdated(disc_.tracks); +} + +void CddaSongLoader::SetDiscTracks(const SongList& songs, bool has_titles) { + disc_.tracks = songs; + disc_.has_titles = has_titles; + emit SongsUpdated(disc_.tracks); } diff --git a/src/devices/cddasongloader.h b/src/devices/cddasongloader.h index b2e5d3919..9716de414 100644 --- a/src/devices/cddasongloader.h +++ b/src/devices/cddasongloader.h @@ -1,5 +1,6 @@ /* This file is part of Clementine. Copyright 2014, David Sansome + Copyright 2021, Lukas Prediger Clementine is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -42,30 +43,65 @@ class CddaSongLoader : public QObject { ~CddaSongLoader(); // Load songs. - // Signals declared below will be emitted anytime new information will be + // Signals declared below will be emitted anytime new information becomes // available. void LoadSongs(); bool IsActive() const; signals: + // Emitted whenever information about tracks were updated. + // Guarantees consistency with previous updates, i.e., consumers can rely + // entirely on the updated list and do not have to merge metadata. May be + // emitted multiple times during reading the disc. + void SongsUpdated(const SongList& songs); + + // The following signals are mostly for internal processing; other classes + // can get all relevant updates by just connecting to SongsUpdated. However, + // the more specialised remain available. + + // Emitted when the number of tracks has been initially loaded from the disc. + // This is a specialised signal; subscribe to SongsUpdated for general updates + // information about tracks is updated. void SongsLoaded(const SongList& songs); + // Emitted when track durations have been loaded from the disc. + // This is a specialised signal; subscribe to SongsUpdated for general updates + // information about tracks is updated. void SongsDurationLoaded(const SongList& songs); + // Emitted when metadata has been loaded from the disc. + // This is a specialised signal; subscribe to SongsUpdated for general updates + // information about tracks is updated. void SongsMetadataLoaded(const SongList& songs); + // Emitted when the MusicBrainz disc id has been determined. + // This is a specialised signal; subscribe to SongsUpdated for general updates + // information about tracks is updated. void MusicBrainzDiscIdLoaded(const QString& musicbrainz_discid); private slots: void LoadAudioCDTags(const QString& musicbrainz_discid) const; - void AudioCDTagsLoaded(const QString& artist, const QString& album, - const MusicBrainzClient::ResultList& results); + void ProcessMusicBrainzResponse(const QString& artist, const QString& album, + const MusicBrainzClient::ResultList& results); + void SetDiscTracks(const SongList& songs, bool has_titles); private: QUrl GetUrlFromTrack(int track_number) const; void LoadSongsFromCdda(); + // Parse gstreamer taglist for a song + // Returns true if any tags were read, updates the Song object in songs + // accordingly, and returns the zero-based track index via the track_no + // argument. + bool ParseSongTags(SongList& songs, GstTagList* tags, gint* track_no); + + struct Disc { + SongList tracks; + bool has_titles; // indicates that titles have been read and are not + // defaulted + }; QUrl url_; GstElement* cdda_; QFuture loading_future_; std::atomic may_load_; + Disc disc_; }; #endif // CDDASONGLOADER_H diff --git a/src/ripper/ripcddialog.cpp b/src/ripper/ripcddialog.cpp index 0e59e8873..e60aea27c 100644 --- a/src/ripper/ripcddialog.cpp +++ b/src/ripper/ripcddialog.cpp @@ -343,10 +343,7 @@ void RipCDDialog::DeviceSelected(int device_index) { loader_ = cdda_device_->loader(); Q_ASSERT(loader_); - connect(loader_, SIGNAL(SongsDurationLoaded(SongList)), - SLOT(SongsLoaded(SongList))); - connect(loader_, SIGNAL(SongsMetadataLoaded(SongList)), - SLOT(SongsLoaded(SongList))); + connect(loader_, SIGNAL(SongsUpdated(SongList)), SLOT(SongsLoaded(SongList))); // load songs from new SongLoader loader_->LoadSongs(); @@ -434,8 +431,12 @@ void RipCDDialog::UpdateMetadataEdits() { const Song& song = songs_.first(); ui_->albumLineEdit->setText(song.album()); - ui_->artistLineEdit->setText(song.artist()); - ui_->yearLineEdit->setText(QString::number(song.year())); + if (!song.artist().isEmpty()) + ui_->artistLineEdit->setText(song.artist()); + else + ui_->artistLineEdit->setText(song.albumartist()); + ui_->yearLineEdit->setText(song.PrettyYear()); + ui_->genreLineEdit->setText(song.genre()); } void RipCDDialog::DiscChanged() { ResetDialog(); }