CoverArt: SHA-256 digest and (background) color

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog
 
+## [2.4.0](https://launchpad.net/mixxx/+milestone/2.4.0) (Unreleased)
+
+* Cover art: Prevent wrong cover art display due to hash conflicts
+* Cover art: Add background color for quick cover art preview
+
 ## [2.3.0](https://launchpad.net/mixxx/+milestone/2.3.0) (Unreleased)
 ### Hotcues ###
 * Add hotcue colors and custom labels by right clicking hotcue buttons or right clicking hotcues on overview waveforms [#2016](https://github.com/mixxxdj/mixxx/pull/2016) [#2520](https://github.com/mixxxdj/mixxx/pull/2520) [#2238](https://github.com/mixxxdj/mixxx/pull/2238) [#2560](https://github.com/mixxxdj/mixxx/pull/2560) [#2557](https://github.com/mixxxdj/mixxx/pull/2557) [#2362](https://github.com/mixxxdj/mixxx/pull/2362)

CMakeLists.txt

@@ -757,6 +757,7 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL
   src/util/duration.cpp
   src/util/experiment.cpp
   src/util/file.cpp
+  src/util/imageutils.cpp
   src/util/indexrange.cpp
   src/util/logger.cpp
   src/util/logging.cpp
@@ -1229,6 +1230,7 @@ add_executable(mixxx-test
   src/test/enginemicrophonetest.cpp
   src/test/enginesynctest.cpp
   src/test/globaltrackcache_test.cpp
+  src/test/imageutils_test.cpp
   src/test/indexrange_test.cpp
   src/test/keyutilstest.cpp
   src/test/lcstest.cpp

build/depends.py

@@ -1321,6 +1321,7 @@ def sources(self, build):
                    "src/util/db/sqlqueryfinisher.cpp",
                    "src/util/db/sqlstringformatter.cpp",
                    "src/util/db/sqltransaction.cpp",
+                   "src/util/imageutils.cpp",
                    "src/util/sample.cpp",
                    "src/util/samplebuffer.cpp",
                    "src/util/readaheadsamplebuffer.cpp",

res/schema.xml

@@ -495,4 +495,13 @@ METADATA
       UPDATE cues SET color = (color & 0xFFFFFF) WHERE color > 0xFFFFFF;
     </sql>
   </revision>
+  <revision version="33" min_compatible="3">
+    <description>
+      Add cover art image digest and (background) color
+    </description>
+    <sql>
+      ALTER TABLE library ADD COLUMN coverart_color INTEGER;
+      ALTER TABLE library ADD COLUMN coverart_digest BLOB;
+    </sql>
+  </revision>
 </schema>

src/database/mixxxdb.cpp

@@ -11,7 +11,7 @@
 const QString MixxxDb::kDefaultSchemaFile(":/schema.xml");
 
 //static
-const int MixxxDb::kRequiredSchemaVersion = 32;
+const int MixxxDb::kRequiredSchemaVersion = 33;
 
 namespace {
 

src/library/basecoverartdelegate.cpp

@@ -73,7 +73,7 @@ void BaseCoverArtDelegate::slotCoverFound(
         const QObject* pRequestor,
         const CoverInfo& coverInfo,
         const QPixmap& pixmap,
-        mixxx::cache_key_t requestedImageHash,
+        mixxx::cache_key_t requestedCacheKey,
         bool coverInfoUpdated) {
     Q_UNUSED(pixmap);
     if (pRequestor != this) {
@@ -89,8 +89,8 @@ void BaseCoverArtDelegate::slotCoverFound(
             pTrack->setCoverInfo(coverInfo);
         }
     }
-    QList<int> refreshRows = m_pendingCacheRows.values(requestedImageHash);
-    m_pendingCacheRows.remove(requestedImageHash);
+    QList<int> refreshRows = m_pendingCacheRows.values(requestedCacheKey);
+    m_pendingCacheRows.remove(requestedCacheKey);
     emitRowsChanged(std::move(refreshRows));
 }
 
@@ -110,7 +110,7 @@ void BaseCoverArtDelegate::paintItem(
     paintItemBackground(painter, option, index);
 
     CoverInfo coverInfo = coverInfoForIndex(index);
-    if (CoverImageUtils::isValidHash(coverInfo.hash)) {
+    if (coverInfo.hasImage()) {
         VERIFY_OR_DEBUG_ASSERT(m_pCache) {
             return;
         }
@@ -131,7 +131,7 @@ void BaseCoverArtDelegate::paintItem(
             } else {
                 // If we asked for a non-cache image and got a null pixmap,
                 // then our request was queued.
-                m_pendingCacheRows.insert(coverInfo.hash, index.row());
+                m_pendingCacheRows.insert(coverInfo.cacheKey(), index.row());
             }
         } else {
             // Cache hit
@@ -140,4 +140,11 @@ void BaseCoverArtDelegate::paintItem(
             return;
         }
     }
+    // Fallback: Use the (background) color as a placeholder
+    //  - while the cover art is loaded asynchronously in the background
+    //  - if the audio file with embedded cover art is missing
+    //  - if the external image file with custom cover art is missing
+    if (coverInfo.color) {
+        painter->fillRect(option.rect, mixxx::RgbColor::toQColor(coverInfo.color));
+    }
 }

src/library/basecoverartdelegate.h

@@ -49,7 +49,7 @@ class BaseCoverArtDelegate : public TableItemDelegate {
             const QObject* pRequestor,
             const CoverInfo& coverInfo,
             const QPixmap& pixmap,
-            mixxx::cache_key_t requestedImageHash,
+            mixxx::cache_key_t requestedCacheKey,
             bool coverInfoUpdated);
 
   protected:

src/library/basetrackcache.cpp

@@ -407,11 +407,16 @@ void BaseTrackCache::getTrackValueForColumn(TrackPointer pTrack,
         trackValue.setValue(mixxx::RgbColor::toQVariant(pTrack->getColor()));
     } else if (fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COVERART_LOCATION) == column) {
         trackValue.setValue(pTrack->getCoverInfo().coverLocation);
-    } else if (fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COVERART_HASH) == column ||
-               fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COVERART) == column) {
+    } else if (
+            fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COVERART) == column ||
+            fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COVERART_HASH) == column) {
         // For sorting, we give COLUMN_LIBRARYTABLE_COVERART the same value as
-        // the cover hash.
-        trackValue.setValue(pTrack->getCoverHash());
+        // the cover digest.
+        trackValue.setValue(pTrack->getCoverInfo().imageDigest());
+    } else if (fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COVERART_COLOR) == column) {
+        trackValue.setValue(mixxx::RgbColor::toQVariant(pTrack->getCoverInfo().color));
+    } else if (fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COVERART_DIGEST) == column) {
+        trackValue.setValue(pTrack->getCoverInfo().imageDigest());
     } else if (fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COVERART_SOURCE) == column) {
         trackValue.setValue(static_cast<int>(pTrack->getCoverInfo().source));
     } else if (fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COVERART_TYPE) == column) {

src/library/columncache.cpp

@@ -66,6 +66,10 @@ void ColumnCache::setColumns(const QStringList& columns) {
     m_columnIndexByEnum[COLUMN_LIBRARYTABLE_COVERART_SOURCE] = fieldIndex(LIBRARYTABLE_COVERART_SOURCE);
     m_columnIndexByEnum[COLUMN_LIBRARYTABLE_COVERART_TYPE] = fieldIndex(LIBRARYTABLE_COVERART_TYPE);
     m_columnIndexByEnum[COLUMN_LIBRARYTABLE_COVERART_LOCATION] = fieldIndex(LIBRARYTABLE_COVERART_LOCATION);
+    m_columnIndexByEnum[COLUMN_LIBRARYTABLE_COVERART_COLOR] =
+            fieldIndex(LIBRARYTABLE_COVERART_COLOR);
+    m_columnIndexByEnum[COLUMN_LIBRARYTABLE_COVERART_DIGEST] =
+            fieldIndex(LIBRARYTABLE_COVERART_DIGEST);
     m_columnIndexByEnum[COLUMN_LIBRARYTABLE_COVERART_HASH] = fieldIndex(LIBRARYTABLE_COVERART_HASH);
 
     m_columnIndexByEnum[COLUMN_TRACKLOCATIONSTABLE_FSDELETED] = fieldIndex(TRACKLOCATIONSTABLE_FSDELETED);

src/library/columncache.h

@@ -14,7 +14,6 @@
 class ColumnCache : public QObject {
   Q_OBJECT
   public:
-
     enum Column {
         COLUMN_LIBRARYTABLE_INVALID = -1,
         COLUMN_LIBRARYTABLE_ID = 0,
@@ -54,6 +53,8 @@ class ColumnCache : public QObject {
         COLUMN_LIBRARYTABLE_COVERART_SOURCE,
         COLUMN_LIBRARYTABLE_COVERART_TYPE,
         COLUMN_LIBRARYTABLE_COVERART_LOCATION,
+        COLUMN_LIBRARYTABLE_COVERART_COLOR,
+        COLUMN_LIBRARYTABLE_COVERART_DIGEST,
         COLUMN_LIBRARYTABLE_COVERART_HASH,
 
         COLUMN_TRACKLOCATIONSTABLE_FSDELETED,

src/library/coverart.cpp

@@ -34,55 +34,69 @@ QString typeToString(CoverInfo::Type type) {
     return "INVALID TYPE VALUE";
 }
 
-QString coverInfoRelativeToString(const CoverInfoRelative& infoRelative) {
-    return typeToString(infoRelative.type) % "," %
-           sourceToString(infoRelative.source) % "," %
-           infoRelative.coverLocation % "," %
-           "0x" % QString::number(infoRelative.hash, 16);
-}
-
-QString coverInfoToString(const CoverInfo& info) {
-    return coverInfoRelativeToString(info) % "," %
-           info.trackLocation % ",";
-}
-} // anonymous namespace
-
-//static
-quint16 CoverImageUtils::calculateHash(
+quint16 calculateLegacyHash(
         const QImage& image) {
-    const auto hash = qChecksum(
+    const auto legacyHash = 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;
+    );
+    DEBUG_ASSERT(image.isNull() || legacyHash != CoverInfo::defaultLegacyHash());
+    DEBUG_ASSERT(!image.isNull() || legacyHash == CoverInfo::defaultLegacyHash());
+    return legacyHash;
 }
 
+} // anonymous namespace
+
 CoverInfoRelative::CoverInfoRelative()
         : source(UNKNOWN),
           type(NONE),
-          hash(CoverImageUtils::defaultHash()) {
+          m_legacyHash(defaultLegacyHash()) {
+}
+
+void CoverInfoRelative::setImage(
+        const QImage& image) {
+    color = CoverImageUtils::extractBackgroundColor(image);
+    m_imageDigest = CoverImageUtils::calculateDigest(image);
+    DEBUG_ASSERT(image.isNull() == m_imageDigest.isEmpty());
+    m_legacyHash = calculateLegacyHash(image);
+    DEBUG_ASSERT(image.isNull() == (m_legacyHash == defaultLegacyHash()));
+    DEBUG_ASSERT(image.isNull() != hasImage());
 }
 
-bool operator==(const CoverInfoRelative& a, const CoverInfoRelative& b) {
-    return a.source == b.source &&
-            a.type == b.type &&
-            a.hash == b.hash &&
-            a.coverLocation == b.coverLocation;
+bool operator==(const CoverInfoRelative& lhs, const CoverInfoRelative& rhs) {
+    return lhs.source == rhs.source &&
+            lhs.type == rhs.type &&
+            lhs.color == rhs.color &&
+            lhs.legacyHash() == rhs.legacyHash() &&
+            lhs.imageDigest() == rhs.imageDigest() &&
+            lhs.coverLocation == rhs.coverLocation;
 }
 
-bool operator!=(const CoverInfoRelative& a, const CoverInfoRelative& b) {
-    return !(a == b);
+bool operator!=(const CoverInfoRelative& lhs, const CoverInfoRelative& rhs) {
+    return !(lhs == rhs);
 }
 
-QDebug operator<<(QDebug dbg, const CoverInfoRelative& infoRelative) {
-    return dbg.maybeSpace() << QString("CoverInfoRelative(%1)")
-            .arg(coverInfoRelativeToString(infoRelative));
+QDebug operator<<(QDebug dbg, const CoverInfoRelative& info) {
+    const QDebugStateSaver saver(dbg);
+    dbg = dbg.maybeSpace() << "CoverInfoRelative";
+    return dbg.nospace()
+            << '{'
+            << typeToString(info.type)
+            << ','
+            << sourceToString(info.source)
+            << ','
+            << info.color
+            << ','
+            << info.coverLocation
+            << ','
+            << info.imageDigest()
+            << ','
+            << info.legacyHash()
+            << '}';
 }
 
 CoverInfo::LoadedImage CoverInfo::loadImage(
@@ -150,13 +164,15 @@ CoverInfo::LoadedImage CoverInfo::loadImage(
     return loadedImage;
 }
 
-bool CoverInfo::refreshImageHash(
+bool CoverInfo::refreshImageDigest(
         const QImage& loadedImage,
         const SecurityTokenPointer& pTrackLocationToken) {
-    if (CoverImageUtils::isValidHash(hash)) {
-        // Trust that a valid hash has been calculated from the
-        // corresponding image. Otherwise we would refresh all
-        // hashes over and over again.
+    if (!imageDigest().isEmpty()) {
+        // Assume that a non-empty digest has been calculated from
+        // the corresponding image. Otherwise we would refresh all
+        // digests over and over again.
+        // Invalid legacy hash values are ignored. These will only
+        // be refreshed when opened with a previous version.
         return false;
     }
     QImage image = loadedImage;
@@ -170,25 +186,29 @@ bool CoverInfo::refreshImageHash(
         reset();
         return true;
     }
-    hash = CoverImageUtils::calculateHash(image);
-    DEBUG_ASSERT(image.isNull() || CoverImageUtils::isValidHash(hash));
-    DEBUG_ASSERT(!image.isNull() || hash == CoverImageUtils::defaultHash());
+    setImage(image);
     return true;
 }
 
-bool operator==(const CoverInfo& a, const CoverInfo& b) {
-    return static_cast<const CoverInfoRelative&>(a) ==
-                    static_cast<const CoverInfoRelative&>(b) &&
-            a.trackLocation == b.trackLocation;
+bool operator==(const CoverInfo& lhs, const CoverInfo& rhs) {
+    return static_cast<const CoverInfoRelative&>(lhs) ==
+            static_cast<const CoverInfoRelative&>(rhs) &&
+            lhs.trackLocation == rhs.trackLocation;
 }
 
-bool operator!=(const CoverInfo& a, const CoverInfo& b) {
-    return !(a == b);
+bool operator!=(const CoverInfo& lhs, const CoverInfo& rhs) {
+    return !(lhs == rhs);
 }
 
 QDebug operator<<(QDebug dbg, const CoverInfo& info) {
-    return dbg.maybeSpace() << QString("CoverInfo(%1)")
-            .arg(coverInfoToString(info));
+    const QDebugStateSaver saver(dbg);
+    dbg = dbg.maybeSpace() << "CoverInfo";
+    return dbg.nospace()
+            << '{'
+            << static_cast<const CoverInfoRelative&>(info)
+            << ','
+            << info.trackLocation
+            << '}';
 }
 
 QDebug operator<<(QDebug dbg, const CoverInfo::LoadedImage::Result& result) {