CddaSongLoader now reads CD-Text for metadata

currently this gets overwritten by musicbrainz response almost immediately, though

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,
+  connect(&cdda_song_loader_, SIGNAL(SongsUpdated(SongList)), this,
-          SLOT(SongsLoaded(SongList)));
-  connect(&cdda_song_loader_, SIGNAL(SongsDurationLoaded(SongList)), this,
-          SLOT(SongsLoaded(SongList)));
-  connect(&cdda_song_loader_, SIGNAL(SongsMetadataLoaded(SongList)), this,
           SLOT(SongsLoaded(SongList)));
   connect(this, SIGNAL(SongsDiscovered(SongList)), model_,
           SLOT(SongsDiscovered(SongList)));

src/devices/cddasongloader.cpp

@@ -1,5 +1,6 @@
 /* This file is part of Clementine.
    Copyright 2014, David Sansome <me@davidsansome.com>
+   Copyright 2021, Lukas Prediger <lumip@lumip.de>
 
    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) {
+    : QObject(parent), url_(url), cdda_(nullptr), may_load_(true), disc_() {
-  connect(this, SIGNAL(MusicBrainzDiscIdLoaded(const QString&)),
+  connect(this, &CddaSongLoader::MusicBrainzDiscIdLoaded, this,
-          SLOT(LoadAudioCDTags(const QString&)));
+          &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<int>(track_number) <= songs.size());
+  Song& song = songs[static_cast<int>(track_number - 1)];
+  *track_no = static_cast<gint>(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 track_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) ||
+  if (!gst_element_query_duration(cdda_, track_fmt, &num_tracks)) {
-      out_fmt != fmt) {
+    qLog(Error) << "Error while querying cdda GstElement for track count";
-    qLog(Error) << "Error while querying cdda GstElement";
     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<GstMessageType>(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<GstTocEntry*>(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<GstMessageType>(
               static_cast<int>(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<GstSeekFlags>(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<GstSeekFlags>(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<GstMessageType>(static_cast<int>(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&,
+          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<MusicBrainzClient*>(sender());
   musicbrainz_client->deleteLater();
-  SongList songs;
   if (results.empty()) return;
-  int track_number = 1;
+
-  for (const MusicBrainzClient::Result& ret : results) {
+  if (disc_.tracks.length() != results.length()) {
-    Song song;
+    qLog(Warning) << "Number of tracks in metadata does not match number of "
-    song.set_artist(artist);
+                     "songs on disc!";
-    song.set_album(album);
+    return;  // no idea how to recover; just do nothing
-    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;
   }
-  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);
 }

src/devices/cddasongloader.h

@@ -1,5 +1,6 @@
 /* This file is part of Clementine.
    Copyright 2014, David Sansome <me@davidsansome.com>
+   Copyright 2021, Lukas Prediger <lumip@lumip.de>
 
    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,
+  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<void> loading_future_;
   std::atomic<bool> may_load_;
+  Disc disc_;
 };
 
 #endif  // CDDASONGLOADER_H

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)),
+  connect(loader_, SIGNAL(SongsUpdated(SongList)), SLOT(SongsLoaded(SongList)));
-          SLOT(SongsLoaded(SongList)));
-  connect(loader_, SIGNAL(SongsMetadataLoaded(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());
+  if (!song.artist().isEmpty())
     ui_->artistLineEdit->setText(song.artist());
-  ui_->yearLineEdit->setText(QString::number(song.year()));
+  else
+    ui_->artistLineEdit->setText(song.albumartist());
+  ui_->yearLineEdit->setText(song.PrettyYear());
+  ui_->genreLineEdit->setText(song.genre());
 }
 
 void RipCDDialog::DiscChanged() { ResetDialog(); }