Merge pull request #2406 from uklotzde/dev_library_metadata

Improve synchronization of track metadata and file tags

src/library/coverartcache.cpp

@@ -5,6 +5,7 @@
 
 #include "library/coverartcache.h"
 #include "library/coverartutils.h"
+#include "util/compatibility.h"
 #include "util/logger.h"
 
 
@@ -201,25 +202,26 @@ void CoverArtCache::requestGuessCover(TrackPointer pTrack) {
 }
 
 void CoverArtCache::guessCover(TrackPointer pTrack) {
-    if (pTrack) {
-        if (kLogger.debugEnabled()) {
-            kLogger.debug()
-                    << "Guessing cover art for"
-                    << pTrack->getFileInfo();
-        }
-        CoverInfo cover = CoverArtUtils::guessCoverInfo(*pTrack);
-        pTrack->setCoverInfo(cover);
+    VERIFY_OR_DEBUG_ASSERT(pTrack) {
+        return;
+    }
+    if (kLogger.debugEnabled()) {
+        kLogger.debug()
+                << "Guessing cover art for"
+                << pTrack->getFileInfo();
     }
+    pTrack->setCoverInfo(
+            CoverInfoGuesser().guessCoverInfoForTrack(*pTrack));
 }
 
 void CoverArtCache::guessCovers(QList<TrackPointer> tracks) {
     if (kLogger.debugEnabled()) {
         kLogger.debug()
                 << "Guessing cover art for"
                 << tracks.size()
-                << "tracks";
+                << "track(s)";
     }
-    foreach (TrackPointer pTrack, tracks) {
+    for (TrackPointer pTrack : qAsConst(tracks)) {
         guessCover(pTrack);
     }
 }

src/library/coverartutils.cpp

@@ -80,44 +80,15 @@ QImage CoverArtUtils::loadCover(const CoverInfo& info) {
 }
 
 //static
-CoverInfo CoverArtUtils::guessCoverInfo(const Track& track) {
-    CoverInfo coverInfo;
-
-    coverInfo.trackLocation = track.getLocation();
-    coverInfo.source = CoverInfo::GUESSED;
-
-    const auto trackFile = track.getFileInfo();
-    QImage image = extractEmbeddedCover(trackFile, track.getSecurityToken());
-    if (!image.isNull()) {
-        // TODO() here we my introduce a duplicate hash code
-        coverInfo.hash = calculateHash(image);
-        coverInfo.coverLocation = QString();
-        coverInfo.type = CoverInfo::METADATA;
-        qDebug() << "CoverArtUtils::guessCover found metadata art" << coverInfo;
-        return coverInfo;
-    }
-
-    QLinkedList<QFileInfo> possibleCovers = findPossibleCoversInFolder(
-            trackFile.directory());
-    coverInfo = selectCoverArtForTrack(track, possibleCovers);
-    if (coverInfo.type == CoverInfo::FILE) {
-        qDebug() << "CoverArtUtils::guessCover found file art" << coverInfo;
-    } else {
-        qDebug() << "CoverArtUtils::guessCover didn't find art" << coverInfo;
-    }
-    return coverInfo;
-}
-
-//static
-QLinkedList<QFileInfo> CoverArtUtils::findPossibleCoversInFolder(const QString& folder) {
+QList<QFileInfo> CoverArtUtils::findPossibleCoversInFolder(const QString& folder) {
     // Search for image files in the track directory.
     QRegExp coverArtFilenames(supportedCoverArtExtensionsRegex(),
                               Qt::CaseInsensitive);
     QDirIterator it(folder,
                     QDir::Dirs | QDir::Files | QDir::NoDotAndDotDot);
     QFile currentFile;
     QFileInfo currentFileInfo;
-    QLinkedList<QFileInfo> possibleCovers;
+    QList<QFileInfo> possibleCovers;
     while (it.hasNext()) {
         it.next();
         currentFileInfo = it.fileInfo();
@@ -130,21 +101,24 @@ QLinkedList<QFileInfo> CoverArtUtils::findPossibleCoversInFolder(const QString&
 }
 
 //static
-CoverInfo CoverArtUtils::selectCoverArtForTrack(
+CoverInfoRelative CoverArtUtils::selectCoverArtForTrack(
         const Track& track,
-        const QLinkedList<QFileInfo>& covers) {
-
-    CoverInfoRelative coverInfoRelative =
-            selectCoverArtForTrack(track.getFileInfo(), track.getAlbum(), covers);
-    return CoverInfo(coverInfoRelative, track.getLocation());
+        const QList<QFileInfo>& covers) {
+    return selectCoverArtForTrack(
+            track.getFileInfo(),
+            track.getAlbum(),
+            covers);
 }
 
 //static
 CoverInfoRelative CoverArtUtils::selectCoverArtForTrack(
         const TrackFile& trackFile,
         const QString& albumName,
-        const QLinkedList<QFileInfo>& covers) {
+        const QList<QFileInfo>& covers) {
     CoverInfoRelative coverInfoRelative;
+    DEBUG_ASSERT(coverInfoRelative.type == CoverInfo::NONE);
+    DEBUG_ASSERT(coverInfoRelative.hash == 0);
+    DEBUG_ASSERT(coverInfoRelative.coverLocation.isNull());
     coverInfoRelative.source = CoverInfo::GUESSED;
     if (covers.isEmpty()) {
         return coverInfoRelative;
@@ -205,14 +179,49 @@ CoverInfoRelative CoverArtUtils::selectCoverArtForTrack(
     if (bestInfo != NULL) {
         QImage image(bestInfo->filePath());
         if (!image.isNull()) {
-            coverInfoRelative.source = CoverInfo::GUESSED;
             coverInfoRelative.type = CoverInfo::FILE;
             // TODO() here we may introduce a duplicate hash code
             coverInfoRelative.hash = calculateHash(image);
             coverInfoRelative.coverLocation = bestInfo->fileName();
-            return coverInfoRelative;
         }
     }
 
     return coverInfoRelative;
 }
+
+CoverInfoRelative CoverInfoGuesser::guessCoverInfo(
+        const TrackFile& trackFile,
+        const QString& albumName,
+        const QImage& embeddedCover) {
+    if (!embeddedCover.isNull()) {
+        CoverInfoRelative coverInfo;
+        coverInfo.source = CoverInfo::GUESSED;
+        coverInfo.type = CoverInfo::METADATA;
+        coverInfo.hash = CoverArtUtils::calculateHash(embeddedCover);
+        DEBUG_ASSERT(coverInfo.coverLocation.isNull());
+        return coverInfo;
+    }
+
+    const auto trackFolder = trackFile.directory();
+    if (trackFolder != m_cachedFolder) {
+        m_cachedFolder = trackFile.directory();
+        m_cachedPossibleCoversInFolder =
+                CoverArtUtils::findPossibleCoversInFolder(
+                        m_cachedFolder);
+    }
+    return CoverArtUtils::selectCoverArtForTrack(
+            trackFile,
+            albumName,
+            m_cachedPossibleCoversInFolder);
+}
+
+CoverInfoRelative CoverInfoGuesser::guessCoverInfoForTrack(
+        const Track& track) {
+    const auto trackFile = track.getFileInfo();
+    return guessCoverInfo(
+            trackFile,
+            track.getAlbum(),
+            CoverArtUtils::extractEmbeddedCover(
+                    trackFile,
+                    track.getSecurityToken()));
+}

src/library/coverartutils.h

@@ -1,19 +1,19 @@
-#ifndef COVERARTUTILS_H
-#define COVERARTUTILS_H
+#pragma once
 
 #include <QImage>
 #include <QString>
 #include <QStringList>
 #include <QSize>
 #include <QFileInfo>
-#include <QLinkedList>
+#include <QList>
 
 #include "track/track.h"
 #include "util/sandbox.h"
 
 class CoverInfo;
 class CoverInfoRelative;
 
+
 class CoverArtUtils {
   public:
     static QString defaultCoverLocation();
@@ -51,27 +51,47 @@ class CoverArtUtils {
     };
 
     // Guesses the cover art for the provided track.
-    static CoverInfo guessCoverInfo(const Track& track);
+    static CoverInfoRelative guessCoverInfo(
+            const Track& track);
 
-    static QLinkedList<QFileInfo> findPossibleCoversInFolder(
+    static QList<QFileInfo> findPossibleCoversInFolder(
             const QString& folder);
 
     // Selects an appropriate cover file from provided list of image files.
-    static CoverInfo selectCoverArtForTrack(
+    static CoverInfoRelative selectCoverArtForTrack(
             const Track& track,
-            const QLinkedList<QFileInfo>& covers);
+            const QList<QFileInfo>& covers);
 
     // Selects an appropriate cover file from provided list of image
     // files. Assumes a SecurityTokenPointer is held by the caller for all files
     // in 'covers'.
     static CoverInfoRelative selectCoverArtForTrack(
             const TrackFile& trackFile,
             const QString& albumName,
-            const QLinkedList<QFileInfo>& covers);
+            const QList<QFileInfo>& covers);
 
 
   private:
     CoverArtUtils() {}
 };
 
-#endif /* COVERARTUTILS_H */
+// Stateful guessing of cover art by caching the possible
+// covers from the last visited folder.
+class CoverInfoGuesser {
+  public:
+    // Guesses the cover art for the provided track.
+    // An embedded cover must be extracted beforehand and provided.
+    CoverInfoRelative guessCoverInfo(
+            const TrackFile& trackFile,
+            const QString& albumName,
+            const QImage& embeddedCover);
+
+    // Extracts an embedded cover image if available and guesses
+    // the cover art for the provided track.
+    CoverInfoRelative guessCoverInfoForTrack(
+            const Track& track);
+
+  private:
+    QString m_cachedFolder;
+    QList<QFileInfo> m_cachedPossibleCoversInFolder;
+};

src/library/dao/trackdao.cpp

@@ -1372,33 +1372,6 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const {
         SoundSourceProxy(pTrack).updateTrackFromSource();
     }
 
-    // Data migration: Reload track total from file tags if not initialized
-    // yet. The added column "tracktotal" has been initialized with the
-    // default value "//".
-    // See also: Schema revision 26 in schema.xml
-    if (pTrack->getTrackTotal() == "//") {
-        // Reload track total from file tags if the special track
-        // total migration value "//" indicates that the track total
-        // is missing and needs to be reloaded.
-        qDebug() << "Reloading value for 'tracktotal' once-only from file"
-                " to replace the default value introduced with a previous"
-                " schema upgrade";
-        mixxx::TrackMetadata trackMetadata;
-        if (SoundSourceProxy(pTrack).importTrackMetadata(&trackMetadata) == mixxx::MetadataSource::ImportResult::Succeeded) {
-            // Copy the track total from the temporary track object
-            pTrack->setTrackTotal(trackMetadata.getTrackInfo().getTrackTotal());
-            // Also set the track number if it is still empty due
-            // to insufficient parsing capabilities of Mixxx in
-            // previous versions.
-            if (!trackMetadata.getTrackInfo().getTrackNumber().isEmpty() && pTrack->getTrackNumber().isEmpty()) {
-                pTrack->setTrackNumber(trackMetadata.getTrackInfo().getTrackNumber());
-            }
-        } else {
-            qWarning() << "Failed to reload value for 'tracktotal' from file tags:"
-                    << trackLocation;
-        }
-    }
-
     // Listen to dirty and changed signals
     connect(pTrack.get(),
             &Track::dirty,
@@ -1969,10 +1942,7 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel,
         "WHERE id=:track_id");
 
 
-    QString currentDirectoryPath;
-    MDir currentDirectory;
-    QLinkedList<QFileInfo> possibleCovers;
-
+    CoverInfoGuesser coverInfoGuesser;
     for (const auto& track: tracksWithoutCover) {
         if (*pCancel) {
             return;
@@ -1987,35 +1957,15 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel,
             continue;
         }
 
-        QImage image(CoverArtUtils::extractEmbeddedCover(trackFile));
-        if (!image.isNull()) {
-            updateQuery.bindValue(":coverart_type",
-                                  static_cast<int>(CoverInfo::METADATA));
-            updateQuery.bindValue(":coverart_source",
-                                  static_cast<int>(CoverInfo::GUESSED));
-            // TODO() here we may introduce a duplicate hash code
-            updateQuery.bindValue(":coverart_hash",
-                                  CoverArtUtils::calculateHash(image));
-            updateQuery.bindValue(":coverart_location", "");
-            updateQuery.bindValue(":track_id", track.trackId.toVariant());
-            if (!updateQuery.exec()) {
-                LOG_FAILED_QUERY(updateQuery) << "failed to write metadata cover";
-            } else {
-                pTracksChanged->insert(track.trackId);
-            }
-            continue;
-        }
-
-        if (track.directoryPath != currentDirectoryPath) {
-            possibleCovers.clear();
-            currentDirectoryPath = track.directoryPath;
-            currentDirectory = MDir(currentDirectoryPath);
-            possibleCovers = CoverArtUtils::findPossibleCoversInFolder(
-                currentDirectoryPath);
-        }
-
-        CoverInfoRelative coverInfo = CoverArtUtils::selectCoverArtForTrack(
-            trackFile, track.trackAlbum, possibleCovers);
+        const auto embeddedCover =
+                CoverArtUtils::extractEmbeddedCover(
+                        trackFile);
+        const auto coverInfo =
+                coverInfoGuesser.guessCoverInfo(
+                        trackFile,
+                        track.trackAlbum,
+                        embeddedCover);
+        DEBUG_ASSERT(coverInfo.source != CoverInfo::UNKNOWN);
 
         updateQuery.bindValue(":coverart_type",
                               static_cast<int>(coverInfo.type));

src/library/dlgcoverartfullsize.cpp

@@ -166,18 +166,20 @@ void DlgCoverArtFullSize::slotCoverFound(const QObject* pRequestor,
 
 // slots to handle signals from the context menu
 void DlgCoverArtFullSize::slotReloadCoverArt() {
-    if (m_pLoadedTrack != nullptr) {
-        auto coverInfo =
-                CoverArtUtils::guessCoverInfo(*m_pLoadedTrack);
-        slotCoverInfoSelected(coverInfo);
+    if (!m_pLoadedTrack) {
+        return;
     }
+    slotCoverInfoSelected(
+            CoverInfoGuesser().guessCoverInfoForTrack(
+                    *m_pLoadedTrack));
 }
 
-void DlgCoverArtFullSize::slotCoverInfoSelected(const CoverInfoRelative& coverInfo) {
-    // qDebug() << "DlgCoverArtFullSize::slotCoverInfoSelected" << coverInfo;
-    if (m_pLoadedTrack != nullptr) {
-        m_pLoadedTrack->setCoverInfo(coverInfo);
+void DlgCoverArtFullSize::slotCoverInfoSelected(
+        const CoverInfoRelative& coverInfo) {
+    if (!m_pLoadedTrack) {
+        return;
     }
+    m_pLoadedTrack->setCoverInfo(coverInfo);
 }
 
 void DlgCoverArtFullSize::mousePressEvent(QMouseEvent* event) {

src/library/dlgtrackinfo.cpp

@@ -290,9 +290,9 @@ void DlgTrackInfo::slotReloadCoverArt() {
     VERIFY_OR_DEBUG_ASSERT(m_pLoadedTrack) {
         return;
     }
-    CoverInfo coverInfo =
-            CoverArtUtils::guessCoverInfo(*m_pLoadedTrack);
-    slotCoverInfoSelected(coverInfo);
+    slotCoverInfoSelected(
+            CoverInfoGuesser().guessCoverInfoForTrack(
+                    *m_pLoadedTrack));
 }
 
 void DlgTrackInfo::slotCoverInfoSelected(const CoverInfoRelative& coverInfo) {