Cover Image Hash: Refactoring

src/library/coverart.cpp

@@ -43,12 +43,26 @@ QString coverInfoToString(const CoverInfo& info) {
 }
 } // anonymous namespace
 
+//static
+quint16 CoverImageUtils::calculateHash(
+        const QImage& image) {
+    const auto hash = qChecksum(
+            reinterpret_cast<const char*>(image.constBits()),
+#if QT_VERSION >= QT_VERSION_CHECK(5, 10, 0)
+            image.sizeInBytes()
+#else
+            image.byteCount()
+#endif
+            );
+    DEBUG_ASSERT(image.isNull() || isValidHash(hash));
+    DEBUG_ASSERT(!image.isNull() || hash == defaultHash());
+    return hash;
+}
+
 CoverInfoRelative::CoverInfoRelative()
         : source(UNKNOWN),
           type(NONE),
-          hash(0) {
-    // The default hash value should match the calculated hash for a null image
-    DEBUG_ASSERT(CoverArtUtils::calculateHash(QImage()) == hash);
+          hash(CoverImageUtils::defaultHash()) {
 }
 
 bool operator==(const CoverInfoRelative& a, const CoverInfoRelative& b) {
@@ -67,6 +81,48 @@ QDebug operator<<(QDebug dbg, const CoverInfoRelative& infoRelative) {
             .arg(coverInfoRelativeToString(infoRelative));
 }
 
+QImage CoverInfo::loadImage(
+        const SecurityTokenPointer& pTrackLocationToken) const {
+    if (type == CoverInfo::METADATA) {
+        if (trackLocation.isEmpty()) {
+            qDebug() << "CoverArtUtils::loadCover METADATA cover with empty trackLocation.";
+            return QImage();
+        }
+        return CoverArtUtils::extractEmbeddedCover(
+                TrackFile(trackLocation),
+                pTrackLocationToken);
+    } else if (type == CoverInfo::FILE) {
+        if (trackLocation.isEmpty()) {
+            qDebug() << "CoverArtUtils::loadCover FILE cover with empty trackLocation."
+                     << "Relative paths will not work.";
+            SecurityTokenPointer pToken =
+                    Sandbox::openSecurityToken(
+                            QFileInfo(coverLocation),
+                            true);
+            return QImage(coverLocation);
+        }
+
+        const QFileInfo coverFile(
+                TrackFile(trackLocation).directory(),
+                coverLocation);
+        const QString coverFilePath = coverFile.filePath();
+        if (!coverFile.exists()) {
+            qDebug() << "CoverArtUtils::loadCover FILE cover does not exist:"
+                     << coverFilePath;
+            return QImage();
+        }
+        SecurityTokenPointer pToken =
+                Sandbox::openSecurityToken(coverFile, true);
+        return QImage(coverFilePath);
+    } else if (type == CoverInfo::NONE) {
+        return QImage();
+    } else {
+        qDebug() << "CoverArtUtils::loadCover unhandled type";
+        DEBUG_ASSERT(false);
+        return QImage();
+    }
+}
+
 bool operator==(const CoverInfo& a, const CoverInfo& b) {
     return static_cast<const CoverInfoRelative&>(a) ==
                     static_cast<const CoverInfoRelative&>(b) &&

src/library/coverart.h

@@ -1,11 +1,25 @@
-#ifndef COVERART_H
-#define COVERART_H
+#pragma once
 
 #include <QImage>
 #include <QString>
-#include <QObject>
 #include <QtDebug>
-#include <QtGlobal>
+
+#include "util/sandbox.h"
+
+class CoverImageUtils {
+  public:
+    static quint16 calculateHash(
+            const QImage& image);
+
+    static constexpr quint16 defaultHash() {
+        return 0;
+    }
+
+    static constexpr bool isValidHash(
+            quint16 hash) {
+        return hash != defaultHash();
+    }
+};
 
 class CoverInfoRelative {
   public:
@@ -64,6 +78,9 @@ class CoverInfo : public CoverInfoRelative {
     CoverInfo(CoverInfo&&) = default;
     CoverInfo& operator=(CoverInfo&&) = default;
 
+    QImage loadImage(
+            const SecurityTokenPointer& pTrackLocationToken = SecurityTokenPointer()) const;
+
     QString trackLocation;
 };
 
@@ -89,12 +106,10 @@ class CoverArt : public CoverInfo {
     CoverArt(CoverArt&&) = default;
     CoverArt& operator=(CoverArt&&) = default;
 
-    // it is not a QPixmap, because it is not safe to use pixmaps 
+    // it is not a QPixmap, because it is not safe to use pixmaps
     // outside the GUI thread
-    QImage image; 
+    QImage image;
     int resizedToWidth;
 };
 
 QDebug operator<<(QDebug dbg, const CoverArt& art);
-
-#endif /* COVERART_H */

src/library/coverartcache.cpp

@@ -133,7 +133,7 @@ CoverArtCache::FutureResult CoverArtCache::loadCover(
                  << info << desiredWidth << signalWhenDone;
     }
 
-    QImage image = CoverArtUtils::loadCover(info);
+    QImage image = info.loadImage();
 
     // TODO(XXX) Should we re-hash here? If the cover file (or track metadata)
     // has changed then info.hash may be incorrect. The fix

src/library/coverartutils.cpp

@@ -42,43 +42,6 @@ QImage CoverArtUtils::extractEmbeddedCover(
             std::move(trackFile), std::move(pToken));
 }
 
-//static
-QImage CoverArtUtils::loadCover(const CoverInfo& info) {
-    if (info.type == CoverInfo::METADATA) {
-        if (info.trackLocation.isEmpty()) {
-            qDebug() << "CoverArtUtils::loadCover METADATA cover with empty trackLocation.";
-            return QImage();
-        }
-        const TrackFile trackFile(info.trackLocation);
-        return extractEmbeddedCover(trackFile);
-    } else if (info.type == CoverInfo::FILE) {
-        if (info.trackLocation.isEmpty()) {
-            qDebug() << "CoverArtUtils::loadCover FILE cover with empty trackLocation."
-                     << "Relative paths will not work.";
-            SecurityTokenPointer pToken = Sandbox::openSecurityToken(
-                QFileInfo(info.coverLocation), true);
-            return QImage(info.coverLocation);
-        }
-
-        QFileInfo coverFile(TrackFile(info.trackLocation).directory(), info.coverLocation);
-        QString coverFilePath = coverFile.filePath();
-        if (!coverFile.exists()) {
-            qDebug() << "CoverArtUtils::loadCover FILE cover does not exist:"
-                     << coverFilePath;
-            return QImage();
-        }
-        SecurityTokenPointer pToken = Sandbox::openSecurityToken(
-            coverFile, true);
-        return QImage(coverFilePath);
-    } else if (info.type == CoverInfo::NONE) {
-        return QImage();
-    } else {
-        qDebug() << "CoverArtUtils::loadCover unhandled type";
-        DEBUG_ASSERT(false);
-        return QImage();
-    }
-}
-
 //static
 QList<QFileInfo> CoverArtUtils::findPossibleCoversInFolder(const QString& folder) {
     // Search for image files in the track directory.
@@ -117,7 +80,7 @@ CoverInfoRelative CoverArtUtils::selectCoverArtForTrack(
         const QList<QFileInfo>& covers) {
     CoverInfoRelative coverInfoRelative;
     DEBUG_ASSERT(coverInfoRelative.type == CoverInfo::NONE);
-    DEBUG_ASSERT(coverInfoRelative.hash == 0);
+    DEBUG_ASSERT(!CoverImageUtils::isValidHash(coverInfoRelative.hash));
     DEBUG_ASSERT(coverInfoRelative.coverLocation.isNull());
     coverInfoRelative.source = CoverInfo::GUESSED;
     if (covers.isEmpty()) {
@@ -181,7 +144,7 @@ CoverInfoRelative CoverArtUtils::selectCoverArtForTrack(
         if (!image.isNull()) {
             coverInfoRelative.type = CoverInfo::FILE;
             // TODO() here we may introduce a duplicate hash code
-            coverInfoRelative.hash = calculateHash(image);
+            coverInfoRelative.hash = CoverImageUtils::calculateHash(image);
             coverInfoRelative.coverLocation = bestInfo->fileName();
         }
     }
@@ -197,7 +160,7 @@ CoverInfoRelative CoverInfoGuesser::guessCoverInfo(
         CoverInfoRelative coverInfo;
         coverInfo.source = CoverInfo::GUESSED;
         coverInfo.type = CoverInfo::METADATA;
-        coverInfo.hash = CoverArtUtils::calculateHash(embeddedCover);
+        coverInfo.hash = CoverImageUtils::calculateHash(embeddedCover);
         DEBUG_ASSERT(coverInfo.coverLocation.isNull());
         return coverInfo;
     }

src/library/coverartutils.h

@@ -26,17 +26,6 @@ class CoverArtUtils {
             TrackFile trackFile,
             SecurityTokenPointer pToken);
 
-    static QImage loadCover(const CoverInfo& info);
-    static quint16 calculateHash(const QImage& image) {
-        return qChecksum(reinterpret_cast<const char*>(image.constBits()),
-#if QT_VERSION >= QT_VERSION_CHECK(5, 10, 0)
-                         image.sizeInBytes()
-#else
-                         image.byteCount()
-#endif
-                );
-    }
-
     static QStringList supportedCoverArtExtensions();
     static QString supportedCoverArtExtensionsRegex();
 

src/test/coverartutils_test.cpp

@@ -142,7 +142,7 @@ TEST_F(CoverArtUtilTest, searchImage) {
     // stuff below.
 
     result.trackLocation = kTrackLocationTest;
-    const QImage img = CoverArtUtils::loadCover(result);
+    const QImage img = result.loadImage();
     EXPECT_EQ(img.isNull(), false);
 
     QString trackBaseName = "cover-test";
@@ -172,7 +172,7 @@ TEST_F(CoverArtUtilTest, searchImage) {
 
     // looking for cover in an directory with one image will select that one.
     expected2.coverLocation = "foo.jpg";
-    expected2.hash = CoverArtUtils::calculateHash(QImage(cLoc_foo));
+    expected2.hash = CoverImageUtils::calculateHash(QImage(cLoc_foo));
     covers << QFileInfo(cLoc_foo);
     CoverInfoRelative res2 = CoverArtUtils::selectCoverArtForTrack(
             trackBaseName, trackAlbum, covers);
@@ -244,7 +244,7 @@ TEST_F(CoverArtUtilTest, searchImage) {
         } else {
             expected2.type = CoverInfo::FILE;
             expected2.coverLocation = cover.fileName();
-            expected2.hash = CoverArtUtils::calculateHash(QImage(cover.filePath()));
+            expected2.hash = CoverImageUtils::calculateHash(QImage(cover.filePath()));
         }
         res2 = CoverArtUtils::selectCoverArtForTrack(trackBaseName, trackAlbum,
                                                     prefCovers);
@@ -272,7 +272,7 @@ TEST_F(CoverArtUtilTest, searchImage) {
 
     res2 = CoverArtUtils::selectCoverArtForTrack(trackBaseName, trackAlbum,
                                                 prefCovers);
-    expected2.hash = CoverArtUtils::calculateHash(QImage(cLoc_coverJPG));
+    expected2.hash = CoverImageUtils::calculateHash(QImage(cLoc_coverJPG));
     expected2.coverLocation = "cover.JPG";
     EXPECT_EQ(expected2, res2);
 
@@ -289,7 +289,7 @@ TEST_F(CoverArtUtilTest, searchImage) {
     prefCovers.append(QFileInfo(cLoc_albumName));
     res2 = CoverArtUtils::selectCoverArtForTrack(trackBaseName, trackAlbum,
                                                 prefCovers);
-    expected2.hash = CoverArtUtils::calculateHash(QImage(cLoc_albumName));
+    expected2.hash = CoverImageUtils::calculateHash(QImage(cLoc_albumName));
     expected2.coverLocation = trackAlbum % ".jpg";
     EXPECT_EQ(expected2, res2);
 
@@ -300,7 +300,7 @@ TEST_F(CoverArtUtilTest, searchImage) {
     res2 = CoverArtUtils::selectCoverArtForTrack(trackBaseName, trackAlbum,
                                                 prefCovers);
 
-    expected2.hash = CoverArtUtils::calculateHash(QImage(cLoc_filename));
+    expected2.hash = CoverImageUtils::calculateHash(QImage(cLoc_filename));
     expected2.coverLocation = trackBaseName % ".jpg";
     EXPECT_EQ(expected2, res2);
 

src/widget/wcoverartmenu.cpp

@@ -83,7 +83,7 @@ void WCoverArtMenu::slotChange() {
     coverInfo.source = CoverInfo::USER_SELECTED;
     coverInfo.coverLocation = selectedCoverPath;
     // TODO() here we may introduce a duplicate hash code
-    coverInfo.hash = CoverArtUtils::calculateHash(image);
+    coverInfo.hash = CoverImageUtils::calculateHash(image);
     qDebug() << "WCoverArtMenu::slotChange emit" << coverInfo;
     emit coverInfoSelected(coverInfo);
 }