From 2a0f01e3027b3e69aaf69109cb48a973e5dc521a Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 20 Feb 2020 10:09:33 +0100 Subject: [PATCH 1/3] Add utility class for handling cover image hashes consistently --- src/library/coverart.cpp | 20 +++++++++++++++++--- src/library/coverart.h | 26 ++++++++++++++++++-------- src/library/coverartutils.cpp | 6 +++--- src/library/coverartutils.h | 9 --------- src/test/coverartutils_test.cpp | 10 +++++----- src/widget/wcoverartmenu.cpp | 2 +- 6 files changed, 44 insertions(+), 29 deletions(-) diff --git a/src/library/coverart.cpp b/src/library/coverart.cpp index f009b481aa4..da33aaa2791 100644 --- a/src/library/coverart.cpp +++ b/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(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) { diff --git a/src/library/coverart.h b/src/library/coverart.h index 6202cc824c8..ac5fb262ee2 100644 --- a/src/library/coverart.h +++ b/src/library/coverart.h @@ -1,11 +1,23 @@ -#ifndef COVERART_H -#define COVERART_H +#pragma once #include #include -#include #include -#include + +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: @@ -89,12 +101,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 */ diff --git a/src/library/coverartutils.cpp b/src/library/coverartutils.cpp index 6aa0a247559..5527f511576 100644 --- a/src/library/coverartutils.cpp +++ b/src/library/coverartutils.cpp @@ -117,7 +117,7 @@ CoverInfoRelative CoverArtUtils::selectCoverArtForTrack( const QList& 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 +181,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 +197,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; } diff --git a/src/library/coverartutils.h b/src/library/coverartutils.h index b53bd7635f2..eb407a8a4aa 100644 --- a/src/library/coverartutils.h +++ b/src/library/coverartutils.h @@ -27,15 +27,6 @@ class CoverArtUtils { SecurityTokenPointer pToken); static QImage loadCover(const CoverInfo& info); - static quint16 calculateHash(const QImage& image) { - return qChecksum(reinterpret_cast(image.constBits()), -#if QT_VERSION >= QT_VERSION_CHECK(5, 10, 0) - image.sizeInBytes() -#else - image.byteCount() -#endif - ); - } static QStringList supportedCoverArtExtensions(); static QString supportedCoverArtExtensionsRegex(); diff --git a/src/test/coverartutils_test.cpp b/src/test/coverartutils_test.cpp index 2f93c469c36..e5b17d46e9a 100644 --- a/src/test/coverartutils_test.cpp +++ b/src/test/coverartutils_test.cpp @@ -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); diff --git a/src/widget/wcoverartmenu.cpp b/src/widget/wcoverartmenu.cpp index 57e23d27192..c77e93731ce 100644 --- a/src/widget/wcoverartmenu.cpp +++ b/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); } From 6ae12120234fcc0c0fc0f37f7ba0eb9ad54ba2ba Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 20 Feb 2020 10:15:19 +0100 Subject: [PATCH 2/3] Make loading of cover images a member function --- src/library/coverart.cpp | 42 +++++++++++++++++++++++++++++++++ src/library/coverart.h | 5 ++++ src/library/coverartcache.cpp | 2 +- src/library/coverartutils.cpp | 37 ----------------------------- src/library/coverartutils.h | 2 -- src/test/coverartutils_test.cpp | 2 +- 6 files changed, 49 insertions(+), 41 deletions(-) diff --git a/src/library/coverart.cpp b/src/library/coverart.cpp index da33aaa2791..a7e44ea3523 100644 --- a/src/library/coverart.cpp +++ b/src/library/coverart.cpp @@ -81,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(a) == static_cast(b) && diff --git a/src/library/coverart.h b/src/library/coverart.h index ac5fb262ee2..9a79ecc3446 100644 --- a/src/library/coverart.h +++ b/src/library/coverart.h @@ -4,6 +4,8 @@ #include #include +#include "util/sandbox.h" + class CoverImageUtils { public: static quint16 calculateHash( @@ -76,6 +78,9 @@ class CoverInfo : public CoverInfoRelative { CoverInfo(CoverInfo&&) = default; CoverInfo& operator=(CoverInfo&&) = default; + QImage loadImage( + const SecurityTokenPointer& pTrackLocationToken = SecurityTokenPointer()) const; + QString trackLocation; }; diff --git a/src/library/coverartcache.cpp b/src/library/coverartcache.cpp index 20713464bea..0baac7358da 100644 --- a/src/library/coverartcache.cpp +++ b/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 diff --git a/src/library/coverartutils.cpp b/src/library/coverartutils.cpp index 5527f511576..3b7c0b39ca8 100644 --- a/src/library/coverartutils.cpp +++ b/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 CoverArtUtils::findPossibleCoversInFolder(const QString& folder) { // Search for image files in the track directory. diff --git a/src/library/coverartutils.h b/src/library/coverartutils.h index eb407a8a4aa..a7abc84dfe6 100644 --- a/src/library/coverartutils.h +++ b/src/library/coverartutils.h @@ -26,8 +26,6 @@ class CoverArtUtils { TrackFile trackFile, SecurityTokenPointer pToken); - static QImage loadCover(const CoverInfo& info); - static QStringList supportedCoverArtExtensions(); static QString supportedCoverArtExtensionsRegex(); diff --git a/src/test/coverartutils_test.cpp b/src/test/coverartutils_test.cpp index e5b17d46e9a..52c1a71a12e 100644 --- a/src/test/coverartutils_test.cpp +++ b/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"; From 4f5b06424946514501a3512969d5cde35480be27 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 20 Feb 2020 23:25:17 +0100 Subject: [PATCH 3/3] Add debug assertions and update log messages --- src/library/coverart.cpp | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/library/coverart.cpp b/src/library/coverart.cpp index a7e44ea3523..b499cdc3bf7 100644 --- a/src/library/coverart.cpp +++ b/src/library/coverart.cpp @@ -3,9 +3,12 @@ #include "library/coverart.h" #include "library/coverartutils.h" #include "util/debug.h" +#include "util/logger.h" namespace { +mixxx::Logger kLogger("CoverArt"); + QString sourceToString(CoverInfo::Source source) { switch (source) { case CoverInfo::UNKNOWN: @@ -84,17 +87,23 @@ QDebug operator<<(QDebug dbg, const CoverInfoRelative& infoRelative) { QImage CoverInfo::loadImage( const SecurityTokenPointer& pTrackLocationToken) const { if (type == CoverInfo::METADATA) { - if (trackLocation.isEmpty()) { - qDebug() << "CoverArtUtils::loadCover METADATA cover with empty trackLocation."; + VERIFY_OR_DEBUG_ASSERT(!trackLocation.isEmpty()) { + kLogger.warning() + << "loadImage" + << type + << "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."; + VERIFY_OR_DEBUG_ASSERT(!trackLocation.isEmpty()) { + kLogger.warning() + << "loadImage" + << type + << "cover with empty trackLocation." + << "Relative paths will not work."; SecurityTokenPointer pToken = Sandbox::openSecurityToken( QFileInfo(coverLocation), @@ -107,8 +116,11 @@ QImage CoverInfo::loadImage( coverLocation); const QString coverFilePath = coverFile.filePath(); if (!coverFile.exists()) { - qDebug() << "CoverArtUtils::loadCover FILE cover does not exist:" - << coverFilePath; + kLogger.warning() + << "loadImage" + << type + << "cover does not exist:" + << coverFilePath; return QImage(); } SecurityTokenPointer pToken = @@ -117,8 +129,7 @@ QImage CoverInfo::loadImage( } else if (type == CoverInfo::NONE) { return QImage(); } else { - qDebug() << "CoverArtUtils::loadCover unhandled type"; - DEBUG_ASSERT(false); + DEBUG_ASSERT(!"unhandled CoverInfo::Type"); return QImage(); } }