diff --git a/src/library/basesqltablemodel.cpp b/src/library/basesqltablemodel.cpp index 177c0452034..bea29d4dd32 100644 --- a/src/library/basesqltablemodel.cpp +++ b/src/library/basesqltablemodel.cpp @@ -943,16 +943,20 @@ TrackPointer BaseSqlTableModel::getTrack(const QModelIndex& index) const { return m_pTrackCollectionManager->internalCollection()->getTrackById(getTrackId(index)); } +TrackPointer BaseSqlTableModel::getTrackByRef( + const TrackRef& trackRef) const { + return m_pTrackCollectionManager->internalCollection()->getTrackByRef(trackRef); +} + QString BaseSqlTableModel::getTrackLocation(const QModelIndex& index) const { if (!index.isValid()) { - return ""; + return QString(); } QString nativeLocation = index.sibling(index.row(), fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_NATIVELOCATION)) .data().toString(); - QString location = QDir::fromNativeSeparators(nativeLocation); - return location; + return QDir::fromNativeSeparators(nativeLocation); } void BaseSqlTableModel::trackLoaded(QString group, TrackPointer pTrack) { diff --git a/src/library/basesqltablemodel.h b/src/library/basesqltablemodel.h index 017c6df0958..55e3e0d38d7 100644 --- a/src/library/basesqltablemodel.h +++ b/src/library/basesqltablemodel.h @@ -66,6 +66,7 @@ class BaseSqlTableModel : public QAbstractTableModel, public TrackModel { /////////////////////////////////////////////////////////////////////////// bool isColumnHiddenByDefault(int column) override; TrackPointer getTrack(const QModelIndex& index) const override; + TrackPointer getTrackByRef(const TrackRef& trackRef) const override; TrackId getTrackId(const QModelIndex& index) const override; const QLinkedList getTrackRows(TrackId trackId) const override { return m_trackIdToRows.value(trackId); diff --git a/src/library/browse/browsetablemodel.cpp b/src/library/browse/browsetablemodel.cpp index 085bc618101..13ea7d4d2d3 100644 --- a/src/library/browse/browsetablemodel.cpp +++ b/src/library/browse/browsetablemodel.cpp @@ -154,13 +154,16 @@ void BrowseTableModel::setPath(const MDir& path) { } TrackPointer BrowseTableModel::getTrack(const QModelIndex& index) const { - QString trackLocation = getTrackLocation(index); - if (m_pRecordingManager->getRecordingLocation() == trackLocation) { + return getTrackByRef(TrackRef::fromFileInfo(getTrackLocation(index))); +} + +TrackPointer BrowseTableModel::getTrackByRef(const TrackRef& trackRef) const { + if (m_pRecordingManager->getRecordingLocation() == trackRef.getLocation()) { QMessageBox::critical( 0, tr("Mixxx Library"), tr("Could not load the following file because" " it is in use by Mixxx or another application.") - + "\n" + trackLocation); + + "\n" + trackRef.getLocation()); return TrackPointer(); } // NOTE(uklotzde, 2015-12-08): Accessing tracks from the browse view @@ -171,8 +174,7 @@ TrackPointer BrowseTableModel::getTrack(const QModelIndex& index) const { // them edit the tracks in a way that persists across sessions // and we didn't want to edit the files on disk by default // unless the user opts in to that. - return m_pTrackCollectionManager->getOrAddTrack( - TrackRef::fromFileInfo(trackLocation)); + return m_pTrackCollectionManager->getOrAddTrack(trackRef); } QString BrowseTableModel::getTrackLocation(const QModelIndex& index) const { diff --git a/src/library/browse/browsetablemodel.h b/src/library/browse/browsetablemodel.h index 9a2256e746e..0b61810f5d1 100644 --- a/src/library/browse/browsetablemodel.h +++ b/src/library/browse/browsetablemodel.h @@ -51,6 +51,7 @@ class BrowseTableModel final : public QStandardItemModel, public virtual TrackMo void setPath(const MDir& path); TrackPointer getTrack(const QModelIndex& index) const override; + TrackPointer getTrackByRef(const TrackRef& trackRef) const override; TrackModel::CapabilitiesFlags getCapabilities() const override; QString getTrackLocation(const QModelIndex& index) const override; diff --git a/src/library/coverart.cpp b/src/library/coverart.cpp index b499cdc3bf7..4e2e1ded37e 100644 --- a/src/library/coverart.cpp +++ b/src/library/coverart.cpp @@ -1,6 +1,5 @@ -#include - #include "library/coverart.h" + #include "library/coverartutils.h" #include "util/debug.h" #include "util/logger.h" @@ -98,34 +97,42 @@ QImage CoverInfo::loadImage( TrackFile(trackLocation), pTrackLocationToken); } else if (type == CoverInfo::FILE) { - 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), - true); - return QImage(coverLocation); + auto coverFile = QFileInfo(coverLocation); + if (coverFile.isRelative()) { + VERIFY_OR_DEBUG_ASSERT(!trackLocation.isEmpty()) { + // This is not expected to happen, because every track + // must have a valid location, i.e. a file path. Most + // likely a programming error, but might also be caused + // by yet unknown circumstances. + kLogger.warning() + << "loadImage" + << type + << "cover with empty track location" + << "and relative file path:" + << coverFile.filePath(); + return QImage(); + } + // Compose track directory with relative path + const auto trackFile = TrackFile(trackLocation); + DEBUG_ASSERT(trackFile.asFileInfo().isAbsolute()); + coverFile = QFileInfo( + trackFile.directory(), + coverLocation); } - - const QFileInfo coverFile( - TrackFile(trackLocation).directory(), - coverLocation); - const QString coverFilePath = coverFile.filePath(); + DEBUG_ASSERT(coverFile.isAbsolute()); if (!coverFile.exists()) { kLogger.warning() << "loadImage" << type << "cover does not exist:" - << coverFilePath; + << coverFile.filePath(); return QImage(); } SecurityTokenPointer pToken = - Sandbox::openSecurityToken(coverFile, true); - return QImage(coverFilePath); + Sandbox::openSecurityToken( + coverFile, + true); + return QImage(coverFile.filePath()); } else if (type == CoverInfo::NONE) { return QImage(); } else { @@ -134,6 +141,32 @@ QImage CoverInfo::loadImage( } } +bool CoverInfo::refreshImageHash( + 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. + return false; + } + QImage image = loadedImage; + if (loadedImage.isNull()) { + image = loadImage(pTrackLocationToken); + } + if (image.isNull() && type != CoverInfo::NONE) { + kLogger.warning() + << "Resetting cover info" + << *this; + reset(); + return true; + } + hash = CoverImageUtils::calculateHash(image); + DEBUG_ASSERT(image.isNull() || CoverImageUtils::isValidHash(hash)); + DEBUG_ASSERT(!image.isNull() || hash == CoverImageUtils::defaultHash()); + return true; +} + 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 9a79ecc3446..d9179c3d087 100644 --- a/src/library/coverart.h +++ b/src/library/coverart.h @@ -52,6 +52,13 @@ class CoverInfoRelative { CoverInfoRelative(CoverInfoRelative&&) = default; CoverInfoRelative& operator=(CoverInfoRelative&&) = default; + /*non-virtual*/ void reset() { + // Slicing when invoked from a subclass is intended! + // Only the contents of this base class are supposed + // to be reset, i.e. trackLocation should be preserved. + *this = CoverInfoRelative(); + } + Source source; Type type; QString coverLocation; // relative path, from track location @@ -81,6 +88,14 @@ class CoverInfo : public CoverInfoRelative { QImage loadImage( const SecurityTokenPointer& pTrackLocationToken = SecurityTokenPointer()) const; + // Verify the image hash and update it if necessary. + // If the corresponding image has already been loaded it + // could be provided as a parameter to avoid reloading + // if actually needed. + bool refreshImageHash( + const QImage& loadedImage = QImage(), + const SecurityTokenPointer& pTrackLocationToken = SecurityTokenPointer()); + QString trackLocation; }; diff --git a/src/library/coverartcache.cpp b/src/library/coverartcache.cpp index 0baac7358da..019afdb0dd2 100644 --- a/src/library/coverartcache.cpp +++ b/src/library/coverartcache.cpp @@ -13,6 +13,15 @@ namespace { mixxx::Logger kLogger("CoverArtCache"); +// The initial QPixmapCache limit is 10MB. +// But it is not used just by the coverArt stuff, +// it is also used by Qt to handle other things behind the scenes. +// Consequently coverArt cache will always have less than those +// 10MB available to store the pixmaps. +// So, we must increase this size a bit more, +// in order to allow CoverCache handle more covers (performance gain). +constexpr int kPixmapCacheLimit = 20480; + QString pixmapCacheKey(quint16 hash, int width) { return QString("CoverArtCache_%1_%2") .arg(QString::number(hash), QString::number(width)); @@ -26,47 +35,69 @@ inline QImage resizeImageWidth(const QImage& image, int width) { return image.scaledToWidth(width, kTransformationMode); } -const bool sDebug = false; - } // anonymous namespace CoverArtCache::CoverArtCache() { - // The initial QPixmapCache limit is 10MB. - // But it is not used just by the coverArt stuff, - // it is also used by Qt to handle other things behind the scenes. - // Consequently coverArt cache will always have less than those - // 10MB available to store the pixmaps. - // So, we must increase this size a bit more, - // in order to allow CoverCache handle more covers (performance gain). - QPixmapCache::setCacheLimit(20480); + QPixmapCache::setCacheLimit(kPixmapCacheLimit); } -CoverArtCache::~CoverArtCache() { - qDebug() << "~CoverArtCache()"; +//static +void CoverArtCache::requestCover( + const QObject* pRequestor, + const CoverInfo& coverInfo, + const TrackPointer& pTrack) { + CoverArtCache* pCache = CoverArtCache::instance(); + VERIFY_OR_DEBUG_ASSERT(pCache) { + return; + } + pCache->tryLoadCover( + pRequestor, + pTrack, + coverInfo, + 0, // original size + Loading::Default); } -QPixmap CoverArtCache::requestCover(const CoverInfo& requestInfo, - const QObject* pRequestor, - const int desiredWidth, - const bool onlyCached, - const bool signalWhenDone) { - if (sDebug) { - kLogger.debug() << "requestCover" - << requestInfo << pRequestor << - desiredWidth << onlyCached << signalWhenDone; +//static +void CoverArtCache::requestTrackCover( + const QObject* pRequestor, + const TrackPointer& pTrack) { + VERIFY_OR_DEBUG_ASSERT(pTrack) { + return; } + requestCover( + pRequestor, + pTrack->getCoverInfoWithLocation(), + pTrack); +} - if (requestInfo.type == CoverInfo::NONE) { - if (signalWhenDone) { - emit coverFound(pRequestor, requestInfo, - QPixmap(), true); +QPixmap CoverArtCache::tryLoadCover( + const QObject* pRequestor, + const TrackPointer& pTrack, + const CoverInfo& coverInfo, + int desiredWidth, + Loading loading) { + if (kLogger.traceEnabled()) { + kLogger.trace() + << "requestCover" + << pRequestor + << coverInfo + << desiredWidth + << loading; + } + DEBUG_ASSERT(!pTrack || + pTrack->getLocation() == coverInfo.trackLocation); + + if (coverInfo.type == CoverInfo::NONE) { + if (loading == Loading::Default) { + emit coverFound(pRequestor, coverInfo, QPixmap(), coverInfo.hash, false); } return QPixmap(); } // keep a list of trackIds for which a future is currently running // to avoid loading the same picture again while we are loading it - QPair requestId = qMakePair(pRequestor, requestInfo.hash); + QPair requestId = qMakePair(pRequestor, coverInfo.hash); if (m_runningRequests.contains(requestId)) { return QPixmap(); } @@ -76,35 +107,44 @@ QPixmap CoverArtCache::requestCover(const CoverInfo& requestInfo, // column). It's very important to keep the cropped covers in cache because // it avoids having to rescale+crop it ALWAYS (which brings a lot of // performance issues). - QString cacheKey = pixmapCacheKey(requestInfo.hash, desiredWidth); + QString cacheKey = pixmapCacheKey(coverInfo.hash, desiredWidth); QPixmap pixmap; if (QPixmapCache::find(cacheKey, &pixmap)) { - if (sDebug) { - kLogger.debug() << "CoverArtCache::requestCover cover found in cache" << requestInfo << signalWhenDone; + if (kLogger.traceEnabled()) { + kLogger.trace() + << "requestCover cache hit" + << coverInfo + << loading; } - if (signalWhenDone) { - emit coverFound(pRequestor, requestInfo, pixmap, true); + if (loading == Loading::Default) { + emit coverFound(pRequestor, coverInfo, pixmap, coverInfo.hash, false); } return pixmap; } - if (onlyCached) { - if (sDebug) { - kLogger.debug() << "requestCover cache miss"; + if (loading == Loading::CachedOnly) { + if (kLogger.traceEnabled()) { + kLogger.trace() << "requestCover cache miss"; } return QPixmap(); } - if (sDebug) { - kLogger.debug() << "CoverArtCache::requestCover starting future for" << requestInfo; + if (kLogger.traceEnabled()) { + kLogger.trace() + << "requestCover starting future for" + << coverInfo; } m_runningRequests.insert(requestId); // The watcher will be deleted in coverLoaded() QFutureWatcher* watcher = new QFutureWatcher(this); QFuture future = QtConcurrent::run( - this, &CoverArtCache::loadCover, requestInfo, pRequestor, - desiredWidth, signalWhenDone); + &CoverArtCache::loadCover, + pRequestor, + pTrack, + coverInfo, + desiredWidth, + loading == Loading::Default); connect(watcher, &QFutureWatcher::finished, this, @@ -114,43 +154,48 @@ QPixmap CoverArtCache::requestCover(const CoverInfo& requestInfo, } //static -void CoverArtCache::requestCover(const Track& track, - const QObject* pRequestor) { - CoverArtCache* pCache = CoverArtCache::instance(); - if (pCache == nullptr) return; - - CoverInfo info = track.getCoverInfoWithLocation(); - pCache->requestCover(info, pRequestor, 0, false, true); -} - CoverArtCache::FutureResult CoverArtCache::loadCover( - const CoverInfo& info, const QObject* pRequestor, - const int desiredWidth, - const bool signalWhenDone) { - if (sDebug) { - kLogger.debug() << "loadCover" - << info << desiredWidth << signalWhenDone; - } + TrackPointer pTrack, + CoverInfo coverInfo, + int desiredWidth, + bool signalWhenDone) { + if (kLogger.traceEnabled()) { + kLogger.trace() + << "loadCover" + << coverInfo + << desiredWidth + << signalWhenDone; + } + DEBUG_ASSERT(!pTrack || + pTrack->getLocation() == coverInfo.trackLocation); + + FutureResult res; + res.pRequestor = pRequestor; + res.requestedHash = coverInfo.hash; + res.signalWhenDone = signalWhenDone; + DEBUG_ASSERT(!res.coverInfoUpdated); - QImage image = info.loadImage(); + QImage image = coverInfo.loadImage( + pTrack ? pTrack->getSecurityToken() : SecurityTokenPointer()); - // TODO(XXX) Should we re-hash here? If the cover file (or track metadata) - // has changed then info.hash may be incorrect. The fix - // will also require noticing a hash mis-match at higher levels and - // recording the hash change in the database. + // Refresh hash before resizing the original image! + res.coverInfoUpdated = coverInfo.refreshImageHash(image); + if (pTrack && res.coverInfoUpdated) { + kLogger.info() + << "Updating cover info of track" + << coverInfo.trackLocation; + pTrack->setCoverInfo(coverInfo); + } - // Adjust the cover size according to the request or downsize the image for - // efficiency. + // Resize image to requested size if (!image.isNull() && desiredWidth > 0) { + // Adjust the cover size according to the request + // or downsize the image for efficiency. image = resizeImageWidth(image, desiredWidth); } - FutureResult res; - res.pRequestor = pRequestor; - res.cover = CoverArt(info, image, desiredWidth); - res.signalWhenDone = signalWhenDone; - + res.cover = CoverArt(coverInfo, image, desiredWidth); return res; } @@ -158,14 +203,17 @@ CoverArtCache::FutureResult CoverArtCache::loadCover( void CoverArtCache::coverLoaded() { FutureResult res; { - QFutureWatcher* watcher = + QFutureWatcher* pFutureWatcher = static_cast*>(sender()); - res = watcher->result(); - watcher->deleteLater(); + VERIFY_OR_DEBUG_ASSERT(pFutureWatcher) { + return; + } + res = pFutureWatcher->result(); + pFutureWatcher->deleteLater(); } - if (sDebug) { - kLogger.debug() << "coverLoaded" << res.cover; + if (kLogger.traceEnabled()) { + kLogger.trace() << "coverLoaded" << res.cover; } // Don't cache full size covers (resizedToWidth = 0) @@ -186,42 +234,9 @@ void CoverArtCache::coverLoaded() { QPixmapCache::insert(cacheKey, pixmap); } - m_runningRequests.remove(qMakePair(res.pRequestor, res.cover.hash)); + m_runningRequests.remove(qMakePair(res.pRequestor, res.requestedHash)); if (res.signalWhenDone) { - emit coverFound(res.pRequestor, res.cover, pixmap, false); - } -} - -void CoverArtCache::requestGuessCovers(QList tracks) { - QtConcurrent::run(this, &CoverArtCache::guessCovers, tracks); -} - -void CoverArtCache::requestGuessCover(TrackPointer pTrack) { - QtConcurrent::run(this, &CoverArtCache::guessCover, pTrack); -} - -void CoverArtCache::guessCover(TrackPointer pTrack) { - 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 tracks) { - if (kLogger.debugEnabled()) { - kLogger.debug() - << "Guessing cover art for" - << tracks.size() - << "track(s)"; - } - for (TrackPointer pTrack : qAsConst(tracks)) { - guessCover(pTrack); + emit coverFound(res.pRequestor, res.cover, pixmap, res.requestedHash, res.coverInfoUpdated); } } diff --git a/src/library/coverartcache.h b/src/library/coverartcache.h index 89e9efd5a10..170c8b82a38 100644 --- a/src/library/coverartcache.h +++ b/src/library/coverartcache.h @@ -1,8 +1,10 @@ -#ifndef COVERARTCACHE_H -#define COVERARTCACHE_H +#pragma once #include +#include #include +#include +#include #include "library/coverart.h" #include "util/singleton.h" @@ -11,6 +13,15 @@ class CoverArtCache : public QObject, public Singleton { Q_OBJECT public: + static void requestCover( + const QObject* pRequestor, + const CoverInfo& coverInfo) { + requestCover(pRequestor, coverInfo, TrackPointer()); + } + static void requestTrackCover( + const QObject* pRequestor, + const TrackPointer& pTrack); + /* This method is used to request a cover art pixmap. * * @param pRequestor : an arbitrary pointer (can be any number you'd like, @@ -23,58 +34,83 @@ class CoverArtCache : public QObject, public Singleton { * In this way, the method will just look into CoverCache and return * a Pixmap if it is already loaded in the QPixmapCache. */ - QPixmap requestCover(const CoverInfo& info, - const QObject* pRequestor, - const int desiredWidth, - const bool onlyCached, - const bool signalWhenDone); - - static void requestCover(const Track& track, - const QObject* pRequestor); - - // Guesses the cover art for the provided tracks by searching the tracks' - // metadata and folders for image files. All I/O is done in a separate - // thread. - void requestGuessCovers(QList tracks); - void requestGuessCover(TrackPointer pTrack); + enum class Loading { + CachedOnly, + NoSignal, + Default, // signal when done + }; + QPixmap tryLoadCover( + const QObject* pRequestor, + const CoverInfo& info, + int desiredWidth = 0, // <= 0: original size + Loading loading = Loading::Default) { + return tryLoadCover( + pRequestor, + TrackPointer(), + info, + desiredWidth, + loading); + } + // Only public for testing struct FutureResult { FutureResult() - : pRequestor(NULL), - signalWhenDone(false) { + : pRequestor(nullptr), + requestedHash(CoverImageUtils::defaultHash()), + signalWhenDone(false), + coverInfoUpdated(false) { } - CoverArt cover; const QObject* pRequestor; + quint16 requestedHash; bool signalWhenDone; + + CoverArt cover; + bool coverInfoUpdated; }; + // Load cover from path indicated in coverInfo. WARNING: This is run in a + // worker thread. + static FutureResult loadCover( + const QObject* pRequestor, + TrackPointer pTrack, + CoverInfo coverInfo, + int desiredWidth, + bool emitSignals); - public slots: + private slots: // Called when loadCover is complete in the main thread. void coverLoaded(); signals: - void coverFound(const QObject* requestor, - const CoverInfoRelative& info, QPixmap pixmap, bool fromCache); + void coverFound( + const QObject* requestor, + const CoverInfo& coverInfo, + const QPixmap& pixmap, + quint16 requestedHash, + bool coverInfoUpdated); protected: CoverArtCache(); - virtual ~CoverArtCache(); + ~CoverArtCache() override = default; friend class Singleton; - // Load cover from path indicated in coverInfo. WARNING: This is run in a - // worker thread. - FutureResult loadCover(const CoverInfo& coverInfo, - const QObject* pRequestor, - const int desiredWidth, - const bool emitSignals); + private: + static void requestCover( + const QObject* pRequestor, + const CoverInfo& coverInfo, + const TrackPointer& /*optional*/ pTrack); - // Guesses the cover art for each track. - void guessCovers(QList tracks); - void guessCover(TrackPointer pTrack); + QPixmap tryLoadCover( + const QObject* pRequestor, + const TrackPointer& pTrack, + const CoverInfo& info, + int desiredWidth, + Loading loading); - private: QSet > m_runningRequests; }; -#endif // COVERARTCACHE_H +inline +QDebug operator<<(QDebug dbg, CoverArtCache::Loading loading) { + return dbg << static_cast(loading); +} diff --git a/src/library/coverartdelegate.cpp b/src/library/coverartdelegate.cpp index 3e31c6cf8b6..6dd45b44062 100644 --- a/src/library/coverartdelegate.cpp +++ b/src/library/coverartdelegate.cpp @@ -6,12 +6,19 @@ #include "library/trackmodel.h" #include "widget/wlibrarytableview.h" #include "util/compatibility.h" +#include "util/logger.h" #include "util/math.h" +namespace { + +const mixxx::Logger kLogger("CoverArtDelegate"); + +} // anonymous namespace CoverArtDelegate::CoverArtDelegate(WLibraryTableView* parent) : TableItemDelegate(parent), m_pTableView(parent), + m_pTrackModel(nullptr), m_bOnlyCachedCover(false), m_iCoverColumn(-1), m_iCoverSourceColumn(-1), @@ -34,26 +41,25 @@ CoverArtDelegate::CoverArtDelegate(WLibraryTableView* parent) &CoverArtDelegate::slotCoverFound); } - TrackModel* pTrackModel = nullptr; QTableView* pTableView = qobject_cast(parent); if (pTableView) { - pTrackModel = dynamic_cast(pTableView->model()); + m_pTrackModel = dynamic_cast(pTableView->model()); } - if (pTrackModel) { - m_iCoverColumn = pTrackModel->fieldIndex( + if (m_pTrackModel) { + m_iCoverColumn = m_pTrackModel->fieldIndex( LIBRARYTABLE_COVERART); - m_iCoverSourceColumn = pTrackModel->fieldIndex( + m_iCoverSourceColumn = m_pTrackModel->fieldIndex( LIBRARYTABLE_COVERART_SOURCE); - m_iCoverTypeColumn = pTrackModel->fieldIndex( + m_iCoverTypeColumn = m_pTrackModel->fieldIndex( LIBRARYTABLE_COVERART_TYPE); - m_iCoverHashColumn = pTrackModel->fieldIndex( + m_iCoverHashColumn = m_pTrackModel->fieldIndex( LIBRARYTABLE_COVERART_HASH); - m_iCoverLocationColumn = pTrackModel->fieldIndex( + m_iCoverLocationColumn = m_pTrackModel->fieldIndex( LIBRARYTABLE_COVERART_LOCATION); - m_iTrackLocationColumn = pTrackModel->fieldIndex( + m_iTrackLocationColumn = m_pTrackModel->fieldIndex( TRACKLOCATIONSTABLE_LOCATION); - m_iIdColumn = pTrackModel->fieldIndex( + m_iIdColumn = m_pTrackModel->fieldIndex( LIBRARYTABLE_ID); } } @@ -71,15 +77,30 @@ void CoverArtDelegate::slotOnlyCachedCoverArt(bool b) { } } -void CoverArtDelegate::slotCoverFound(const QObject* pRequestor, - const CoverInfoRelative& info, - QPixmap pixmap, bool fromCache) { - if (pRequestor == this && !pixmap.isNull() && !fromCache) { - // qDebug() << "CoverArtDelegate::slotCoverFound" << pRequestor << info - // << pixmap.size(); - QLinkedList rows = m_hashToRow.take(info.hash); - foreach(int row, rows) { - emit coverReadyForCell(row, m_iCoverColumn); +void CoverArtDelegate::slotCoverFound( + const QObject* pRequestor, + const CoverInfo& coverInfo, + const QPixmap& pixmap, + quint16 requestedHash, + bool coverInfoUpdated) { + Q_UNUSED(pixmap); + if (pRequestor != this) { + return; + } + const QLinkedList rows = + m_hashToRow.take(requestedHash); + foreach(int row, rows) { + emit coverReadyForCell(row, m_iCoverColumn); + } + if (m_pTrackModel && coverInfoUpdated) { + const auto pTrack = + m_pTrackModel->getTrackByRef( + TrackRef::fromFileInfo(coverInfo.trackLocation)); + if (pTrack) { + kLogger.info() + << "Updating cover info of track" + << coverInfo.trackLocation; + pTrack->setCoverInfo(coverInfo); } } } @@ -87,9 +108,10 @@ void CoverArtDelegate::slotCoverFound(const QObject* pRequestor, void CoverArtDelegate::paintItem(QPainter *painter, const QStyleOptionViewItem &option, const QModelIndex &index) const { - CoverArtCache* pCache = CoverArtCache::instance(); - if (pCache == NULL || m_iIdColumn == -1 || m_iCoverSourceColumn == -1 || - m_iCoverTypeColumn == -1 || m_iCoverLocationColumn == -1 || + if (m_iIdColumn < 0 || + m_iCoverSourceColumn == -1 || + m_iCoverTypeColumn == -1 || + m_iCoverLocationColumn == -1 || m_iCoverHashColumn == -1) { return; } @@ -112,19 +134,31 @@ void CoverArtDelegate::paintItem(QPainter *painter, double scaleFactor = getDevicePixelRatioF(static_cast(parent())); // We listen for updates via slotCoverFound above and signal to // BaseSqlTableModel when a row's cover is ready. - QPixmap pixmap = pCache->requestCover(info, this, option.rect.width() * scaleFactor, - m_bOnlyCachedCover, true); + CoverArtCache* const pCache = CoverArtCache::instance(); + VERIFY_OR_DEBUG_ASSERT(pCache) { + return; + } + QPixmap pixmap = pCache->tryLoadCover( + this, + info, + option.rect.width() * scaleFactor, + m_bOnlyCachedCover ? CoverArtCache::Loading::CachedOnly : CoverArtCache::Loading::Default); if (!pixmap.isNull()) { + // Cache hit pixmap.setDevicePixelRatio(scaleFactor); painter->drawPixmap(option.rect.topLeft(), pixmap); - } else if (!m_bOnlyCachedCover) { + return; + } + + if (m_bOnlyCachedCover) { + // We are requesting cache-only covers and got a cache + // miss. Record this row so that when we switch to requesting + // non-cache we can request an update. + m_cacheMissRows.append(index.row()); + } else { // If we asked for a non-cache image and got a null pixmap, then our - // request was queued. + // request was queued. We cannot use the cover image hash, because this + // might be refreshed while loading the image! m_hashToRow[info.hash].append(index.row()); - } else { - // Otherwise, we are requesting cache-only covers and got a cache - // miss. Record this row so that when we switch to requesting non-cache - // we can request an update. - m_cacheMissRows.append(index.row()); } } diff --git a/src/library/coverartdelegate.h b/src/library/coverartdelegate.h index 026d67bca5d..1d3e00ac8f3 100644 --- a/src/library/coverartdelegate.h +++ b/src/library/coverartdelegate.h @@ -7,7 +7,7 @@ #include "library/tableitemdelegate.h" -class CoverInfoRelative; +class CoverInfo; class TrackModel; class WLibraryTableView; @@ -37,12 +37,16 @@ class CoverArtDelegate : public TableItemDelegate { // which could bring performance issues. void slotOnlyCachedCoverArt(bool b); - void slotCoverFound(const QObject* pRequestor, - const CoverInfoRelative& info, - QPixmap pixmap, bool fromCache); + void slotCoverFound( + const QObject* pRequestor, + const CoverInfo& coverInfo, + const QPixmap& pixmap, + quint16 requestedHash, + bool coverInfoUpdated); private: QTableView* m_pTableView; + TrackModel* m_pTrackModel; bool m_bOnlyCachedCover; int m_iCoverColumn; int m_iCoverSourceColumn; diff --git a/src/library/coverartutils.cpp b/src/library/coverartutils.cpp index 3b7c0b39ca8..db266a751ed 100644 --- a/src/library/coverartutils.cpp +++ b/src/library/coverartutils.cpp @@ -1,12 +1,25 @@ +#include "library/coverartutils.h" + #include #include - -#include "library/coverartutils.h" +#include #include "sources/soundsourceproxy.h" +#include "util/compatibility.h" +#include "util/logger.h" #include "util/regex.h" +namespace { + +mixxx::Logger kLogger("CoverArtUtils"); + +// The concurrent guessing of cover art in background tasks +// is enabled, unless it is explicitly disabled during tests! +volatile bool s_enableConcurrentGuessingOfTrackCoverInfo = true; + +} // anonymous namespace + //static QString CoverArtUtils::defaultCoverLocation() { return QString(":/images/library/cover_default.svg"); @@ -25,15 +38,6 @@ QString CoverArtUtils::supportedCoverArtExtensionsRegex() { return RegexUtils::fileExtensionsRegex(extensions); } -//static -QImage CoverArtUtils::extractEmbeddedCover( - TrackFile trackFile) { - SecurityTokenPointer pToken = Sandbox::openSecurityToken( - trackFile.asFileInfo(), true); - return extractEmbeddedCover( - std::move(trackFile), std::move(pToken)); -} - //static QImage CoverArtUtils::extractEmbeddedCover( TrackFile trackFile, @@ -181,6 +185,11 @@ CoverInfoRelative CoverInfoGuesser::guessCoverInfo( CoverInfoRelative CoverInfoGuesser::guessCoverInfoForTrack( const Track& track) { const auto trackFile = track.getFileInfo(); + if (kLogger.debugEnabled()) { + kLogger.debug() + << "Guessing cover art for track" + << trackFile; + } return guessCoverInfo( trackFile, track.getAlbum(), @@ -188,3 +197,46 @@ CoverInfoRelative CoverInfoGuesser::guessCoverInfoForTrack( trackFile, track.getSecurityToken())); } + +void CoverInfoGuesser::guessAndSetCoverInfoForTracks( + const QList& tracks) { + for (const auto& pTrack : tracks) { + VERIFY_OR_DEBUG_ASSERT(pTrack) { + continue; + } + guessAndSetCoverInfoForTrack(*pTrack); + } +} + +void guessTrackCoverInfoConcurrently( + TrackPointer pTrack) { + VERIFY_OR_DEBUG_ASSERT(pTrack) { + return; + } + if (s_enableConcurrentGuessingOfTrackCoverInfo) { + QtConcurrent::run([pTrack] { + CoverInfoGuesser().guessAndSetCoverInfoForTrack(*pTrack); + }); + } else { + // Disabled only during tests + CoverInfoGuesser().guessAndSetCoverInfoForTrack(*pTrack); + } +} + +void guessTrackCoverInfoConcurrently( + QList tracks) { + if (tracks.isEmpty()) { + return; + } + if (s_enableConcurrentGuessingOfTrackCoverInfo) { + QtConcurrent::run([tracks] { + CoverInfoGuesser().guessAndSetCoverInfoForTracks(tracks); + }); + } else { + CoverInfoGuesser().guessAndSetCoverInfoForTracks(tracks); + } +} + +void disableConcurrentGuessingOfTrackCoverInfoDuringTests() { + s_enableConcurrentGuessingOfTrackCoverInfo = false; +} diff --git a/src/library/coverartutils.h b/src/library/coverartutils.h index a7abc84dfe6..af67e881c78 100644 --- a/src/library/coverartutils.h +++ b/src/library/coverartutils.h @@ -18,10 +18,7 @@ class CoverArtUtils { public: static QString defaultCoverLocation(); - // Extracts the first cover art image embedded within the file at - // fileInfo. If no security token is provided a new one is created. - static QImage extractEmbeddedCover( - TrackFile trackFile); + // Extracts the first cover art image embedded within the file. static QImage extractEmbeddedCover( TrackFile trackFile, SecurityTokenPointer pToken); @@ -80,7 +77,24 @@ class CoverInfoGuesser { CoverInfoRelative guessCoverInfoForTrack( const Track& track); + void guessAndSetCoverInfoForTrack( + Track& track) { + track.setCoverInfo(guessCoverInfoForTrack(track)); + } + void guessAndSetCoverInfoForTracks( + const QList& tracks); + private: QString m_cachedFolder; QList m_cachedPossibleCoversInFolder; }; + +// Guesses the cover art for the provided tracks by searching the tracks' +// metadata and folders for image files. All I/O is done in a separate +// thread. +void guessTrackCoverInfoConcurrently(TrackPointer pTrack); +void guessTrackCoverInfoConcurrently(QList tracks); + +// Concurrent guessing of track covers during short running +// tests may cause spurious test failures due to timing issues. +void disableConcurrentGuessingOfTrackCoverInfoDuringTests(); diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 50d10d312b6..985e7609bd0 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -24,7 +24,6 @@ #include "library/dao/playlistdao.h" #include "library/dao/analysisdao.h" #include "library/dao/libraryhashdao.h" -#include "library/coverartcache.h" #include "track/beatfactory.h" #include "track/beats.h" #include "track/keyfactory.h" @@ -1292,6 +1291,9 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const { SoundSourceProxy(pTrack).updateTrackFromSource(); } + // Validate and refresh cover image hash values if needed. + pTrack->refreshCoverImageHash(); + // Listen to signals from Track objects and forward them to // receivers. TrackDAO works as a relay for selected track signals // that allows receivers to use permament connections with @@ -1892,9 +1894,12 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel, continue; } + SecurityTokenPointer pToken = Sandbox::openSecurityToken( + trackFile.asFileInfo(), true); const auto embeddedCover = CoverArtUtils::extractEmbeddedCover( - trackFile); + trackFile, + pToken); const auto coverInfo = coverInfoGuesser.guessCoverInfo( trackFile, @@ -1959,11 +1964,7 @@ TrackPointer TrackDAO::getOrAddTrack( // If the track wasn't in the library already then it has not yet // been checked for cover art. - CoverArtCache* pCache = CoverArtCache::instance(); - if (pCache) { - // Process cover art asynchronously - pCache->requestGuessCover(pTrack); - } + guessTrackCoverInfoConcurrently(pTrack); return pTrack; } diff --git a/src/library/dlgcoverartfullsize.cpp b/src/library/dlgcoverartfullsize.cpp index 73a3c31b7c7..1393cf9f446 100644 --- a/src/library/dlgcoverartfullsize.cpp +++ b/src/library/dlgcoverartfullsize.cpp @@ -14,11 +14,11 @@ DlgCoverArtFullSize::DlgCoverArtFullSize(QWidget* parent, BaseTrackPlayer* pPlay m_pPlayer(pPlayer), m_pCoverMenu(make_parented(this)) { CoverArtCache* pCache = CoverArtCache::instance(); - if (pCache != nullptr) { - connect(pCache, SIGNAL(coverFound(const QObject*, - const CoverInfoRelative&, QPixmap, bool)), - this, SLOT(slotCoverFound(const QObject*, - const CoverInfoRelative&, QPixmap, bool))); + if (pCache) { + connect(pCache, + &CoverArtCache::coverFound, + this, + &DlgCoverArtFullSize::slotCoverFound); } setContextMenuPolicy(Qt::CustomContextMenu); @@ -112,21 +112,22 @@ void DlgCoverArtFullSize::slotLoadTrack(TrackPointer pTrack) { } void DlgCoverArtFullSize::slotTrackCoverArtUpdated() { - if (m_pLoadedTrack != nullptr) { - CoverArtCache::requestCover(*m_pLoadedTrack, this); + if (m_pLoadedTrack) { + CoverArtCache::requestTrackCover(this, m_pLoadedTrack); } } -void DlgCoverArtFullSize::slotCoverFound(const QObject* pRequestor, - const CoverInfoRelative& info, QPixmap pixmap, - bool fromCache) { - Q_UNUSED(info); - Q_UNUSED(fromCache); - - if (pRequestor == this && m_pLoadedTrack != nullptr && - m_pLoadedTrack->getCoverHash() == info.hash) { - // qDebug() << "DlgCoverArtFullSize::slotCoverFound" << pRequestor << info - // << pixmap.size(); +void DlgCoverArtFullSize::slotCoverFound( + const QObject* pRequestor, + const CoverInfo& coverInfo, + const QPixmap& pixmap, + quint16 requestedHash, + bool coverInfoUpdated) { + Q_UNUSED(requestedHash); + Q_UNUSED(coverInfoUpdated); + if (pRequestor == this && + m_pLoadedTrack && + m_pLoadedTrack->getLocation() == coverInfo.trackLocation) { m_pixmap = pixmap; // Scale down dialog if the pixmap is larger than the screen. // Use 90% of screen size instead of 100% to prevent an issue with diff --git a/src/library/dlgcoverartfullsize.h b/src/library/dlgcoverartfullsize.h index 7be9d571913..7ae2a7ba714 100644 --- a/src/library/dlgcoverartfullsize.h +++ b/src/library/dlgcoverartfullsize.h @@ -28,8 +28,12 @@ class DlgCoverArtFullSize public slots: void slotLoadTrack(TrackPointer); - void slotCoverFound(const QObject* pRequestor, - const CoverInfoRelative& info, QPixmap pixmap, bool fromCache); + void slotCoverFound( + const QObject* pRequestor, + const CoverInfo& coverInfo, + const QPixmap& pixmap, + quint16 requestedHash, + bool coverInfoUpdated); void slotTrackCoverArtUpdated(); // slots that handle signals from WCoverArtMenu diff --git a/src/library/dlgtrackinfo.cpp b/src/library/dlgtrackinfo.cpp index a0c40049ed7..fdffce1de51 100644 --- a/src/library/dlgtrackinfo.cpp +++ b/src/library/dlgtrackinfo.cpp @@ -225,10 +225,7 @@ void DlgTrackInfo::populateFields(const Track& track) { m_loadedCoverInfo = track.getCoverInfoWithLocation(); m_pWCoverArtLabel->setCoverArt(m_loadedCoverInfo, QPixmap()); - CoverArtCache* pCache = CoverArtCache::instance(); - if (pCache != NULL) { - pCache->requestCover(m_loadedCoverInfo, this, 0, false, true); - } + CoverArtCache::requestCover(this, m_loadedCoverInfo); } void DlgTrackInfo::reloadTrackBeats(const Track& track) { @@ -274,15 +271,19 @@ void DlgTrackInfo::loadTrack(TrackPointer pTrack) { &DlgTrackInfo::slotTrackChanged); } -void DlgTrackInfo::slotCoverFound(const QObject* pRequestor, - const CoverInfoRelative& info, - QPixmap pixmap, bool fromCache) { - Q_UNUSED(fromCache); - if (pRequestor == this && m_pLoadedTrack && - m_loadedCoverInfo.hash == info.hash) { - qDebug() << "DlgTrackInfo::slotPixmapFound" << pRequestor << info - << pixmap.size(); - m_pWCoverArtLabel->setCoverArt(m_loadedCoverInfo, pixmap); +void DlgTrackInfo::slotCoverFound( + const QObject* pRequestor, + const CoverInfo& coverInfo, + const QPixmap& pixmap, + quint16 requestedHash, + bool coverInfoUpdated) { + Q_UNUSED(requestedHash); + Q_UNUSED(coverInfoUpdated); + if (pRequestor == this && + m_pLoadedTrack && + m_loadedCoverInfo.trackLocation == coverInfo.trackLocation) { + m_loadedCoverInfo = coverInfo; + m_pWCoverArtLabel->setCoverArt(coverInfo, pixmap); } } @@ -301,10 +302,7 @@ void DlgTrackInfo::slotCoverInfoSelected(const CoverInfoRelative& coverInfo) { return; } m_loadedCoverInfo = CoverInfo(coverInfo, m_pLoadedTrack->getLocation()); - CoverArtCache* pCache = CoverArtCache::instance(); - if (pCache) { - pCache->requestCover(m_loadedCoverInfo, this, 0, false, true); - } + CoverArtCache::requestCover(this, m_loadedCoverInfo); } void DlgTrackInfo::slotOpenInFileBrowser() { diff --git a/src/library/dlgtrackinfo.h b/src/library/dlgtrackinfo.h index 677483ead30..eb721c96c79 100644 --- a/src/library/dlgtrackinfo.h +++ b/src/library/dlgtrackinfo.h @@ -61,8 +61,12 @@ class DlgTrackInfo : public QDialog, public Ui::DlgTrackInfo { void slotTrackChanged(TrackId trackId); void slotOpenInFileBrowser(); - void slotCoverFound(const QObject* pRequestor, - const CoverInfoRelative& info, QPixmap pixmap, bool fromCache); + void slotCoverFound( + const QObject* pRequestor, + const CoverInfo& info, + const QPixmap& pixmap, + quint16 requestedHash, + bool coverInfoUpdated); void slotCoverInfoSelected(const CoverInfoRelative& coverInfo); void slotReloadCoverArt(); diff --git a/src/library/proxytrackmodel.cpp b/src/library/proxytrackmodel.cpp index 969b721bf85..65b3e4b73a5 100644 --- a/src/library/proxytrackmodel.cpp +++ b/src/library/proxytrackmodel.cpp @@ -47,6 +47,10 @@ TrackPointer ProxyTrackModel::getTrack(const QModelIndex& index) const { return m_pTrackModel ? m_pTrackModel->getTrack(indexSource) : TrackPointer(); } +TrackPointer ProxyTrackModel::getTrackByRef(const TrackRef& trackRef) const { + return m_pTrackModel ? m_pTrackModel->getTrackByRef(trackRef) : TrackPointer(); +} + QString ProxyTrackModel::getTrackLocation(const QModelIndex& index) const { QModelIndex indexSource = mapToSource(index); return m_pTrackModel ? m_pTrackModel->getTrackLocation(indexSource) : QString(); diff --git a/src/library/proxytrackmodel.h b/src/library/proxytrackmodel.h index 8b4b097be86..7ed49b25fd8 100644 --- a/src/library/proxytrackmodel.h +++ b/src/library/proxytrackmodel.h @@ -25,6 +25,7 @@ class ProxyTrackModel : public QSortFilterProxyModel, public TrackModel { // Inherited from TrackModel CapabilitiesFlags getCapabilities() const final; TrackPointer getTrack(const QModelIndex& index) const final; + TrackPointer getTrackByRef(const TrackRef& trackRef) const final; QString getTrackLocation(const QModelIndex& index) const final; TrackId getTrackId(const QModelIndex& index) const final; const QLinkedList getTrackRows(TrackId trackId) const final; diff --git a/src/library/trackmodel.h b/src/library/trackmodel.h index c4614a2385e..4516630c28d 100644 --- a/src/library/trackmodel.h +++ b/src/library/trackmodel.h @@ -7,6 +7,7 @@ #include #include "track/track.h" +#include "track/trackref.h" #include "library/dao/settingsdao.h" /** Pure virtual (abstract) class that provides an interface for data models which @@ -86,9 +87,10 @@ class TrackModel { NUM_SORTCOLUMNIDS }; - // Deserialize and return the track at the given QModelIndex in this result - // set. + // Deserialize and return the track at the given QModelIndex + // or TrackRef in this result set. virtual TrackPointer getTrack(const QModelIndex& index) const = 0; + virtual TrackPointer getTrackByRef(const TrackRef& trackRef) const = 0; // Gets the on-disk location of the track at the given location // with Qt separator "/". diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index ba98e8e2c4f..dcbf2075336 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -464,7 +464,7 @@ QImage SoundSourceProxy::importCoverImage() const { return coverImg; } } - // Failed ore unavailable + // Failed or unavailable return QImage(); } diff --git a/src/test/autodjprocessor_test.cpp b/src/test/autodjprocessor_test.cpp index 5e7b9058b4f..d1253761c77 100644 --- a/src/test/autodjprocessor_test.cpp +++ b/src/test/autodjprocessor_test.cpp @@ -211,8 +211,8 @@ class AutoDJProcessorTest : public LibraryTest { } TrackId addTrackToCollection(const QString& trackLocation) { - TrackPointer pTrack = internalCollection()->getOrAddTrack( - TrackRef::fromFileInfo(trackLocation)); + TrackPointer pTrack = + getOrAddTrackByLocation(trackLocation); return pTrack ? pTrack->getId() : TrackId(); } diff --git a/src/test/coverartcache_test.cpp b/src/test/coverartcache_test.cpp index 2c86f03ced2..5ea4f091776 100644 --- a/src/test/coverartcache_test.cpp +++ b/src/test/coverartcache_test.cpp @@ -19,9 +19,10 @@ class CoverArtCacheTest : public LibraryTest, public CoverArtCache { info.trackLocation = trackLocation; CoverArtCache::FutureResult res; - res = CoverArtCache::loadCover(info, NULL, 0, false); + res = CoverArtCache::loadCover(nullptr, TrackPointer(), info, 0, false); EXPECT_QSTRING_EQ(QString(), res.cover.coverLocation); - EXPECT_EQ(info.hash, res.cover.hash); + EXPECT_TRUE(CoverImageUtils::isValidHash(res.cover.hash)); + EXPECT_TRUE(res.coverInfoUpdated); SecurityTokenPointer securityToken = Sandbox::openSecurityToken(QDir(trackLocation), true); @@ -39,10 +40,10 @@ class CoverArtCacheTest : public LibraryTest, public CoverArtCache { info.source = CoverInfo::GUESSED; info.coverLocation = coverLocation; info.trackLocation = trackLocation; - info.hash = 4321; // fake cover hash + info.hash = 39287; // actual cover image hash! CoverArtCache::FutureResult res; - res = CoverArtCache::loadCover(info, NULL, 0, false); + res = CoverArtCache::loadCover(nullptr, TrackPointer(), info, 0, false); EXPECT_QSTRING_EQ(info.coverLocation, res.cover.coverLocation); EXPECT_EQ(info.hash, res.cover.hash); EXPECT_FALSE(img.isNull()); diff --git a/src/test/librarytest.cpp b/src/test/librarytest.cpp index f5760b05faa..3b4d0ba5720 100644 --- a/src/test/librarytest.cpp +++ b/src/test/librarytest.cpp @@ -31,3 +31,9 @@ LibraryTest::LibraryTest() m_dbConnectionPooler(m_mixxxDb.connectionPool()), m_pTrackCollectionManager(newTrackCollectionManager(config(), m_dbConnectionPooler)) { } + +TrackPointer LibraryTest::getOrAddTrackByLocation( + const QString& trackLocation) { + return m_pTrackCollectionManager->getOrAddTrack( + TrackRef::fromFileInfo(trackLocation)); +} diff --git a/src/test/librarytest.h b/src/test/librarytest.h index 00cfa317851..0dc7927c3de 100644 --- a/src/test/librarytest.h +++ b/src/test/librarytest.h @@ -31,6 +31,9 @@ class LibraryTest : public MixxxTest { return trackCollections()->internalCollection(); } + TrackPointer getOrAddTrackByLocation( + const QString& trackLocation); + private: const MixxxDb m_mixxxDb; const mixxx::DbConnectionPooler m_dbConnectionPooler; diff --git a/src/test/mixxxtest.cpp b/src/test/mixxxtest.cpp index b91cf32ad50..f70495e2255 100644 --- a/src/test/mixxxtest.cpp +++ b/src/test/mixxxtest.cpp @@ -1,5 +1,6 @@ #include "test/mixxxtest.h" +#include "library/coverartutils.h" #include "sources/soundsourceproxy.h" namespace { @@ -22,11 +23,15 @@ MixxxTest::ApplicationScope::ApplicationScope(int& argc, char** argv) { s_pApplication.reset(new MixxxApplication(argc, argv)); SoundSourceProxy::registerSoundSourceProviders(); + + // All guessing of cover art should be done synchronously + // in the same thread during tests to prevent test failures + // due to timing issues. + disableConcurrentGuessingOfTrackCoverInfoDuringTests(); } MixxxTest::ApplicationScope::~ApplicationScope() { DEBUG_ASSERT(s_pApplication); - s_pApplication.reset(); } diff --git a/src/test/searchqueryparsertest.cpp b/src/test/searchqueryparsertest.cpp index 0e4ce9dbc84..72f78805e05 100644 --- a/src/test/searchqueryparsertest.cpp +++ b/src/test/searchqueryparsertest.cpp @@ -17,8 +17,8 @@ class SearchQueryParserTest : public LibraryTest { } TrackId addTrackToCollection(const QString& trackLocation) { - TrackPointer pTrack = internalCollection()->getOrAddTrack( - TrackRef::fromFileInfo(trackLocation)); + TrackPointer pTrack = + getOrAddTrackByLocation(trackLocation); return pTrack ? pTrack->getId() : TrackId(); } diff --git a/src/track/track.cpp b/src/track/track.cpp index 60f93f51ea6..27e04c8282f 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -962,6 +962,30 @@ void Track::setCoverInfo(const CoverInfoRelative& coverInfo) { } } +bool Track::refreshCoverImageHash( + const QImage& loadedImage) { + QMutexLocker lock(&m_qMutex); + auto coverInfo = CoverInfo( + m_record.getCoverInfo(), + m_fileInfo.location()); + if (!coverInfo.refreshImageHash( + loadedImage, + m_pSecurityToken)) { + return false; + } + if (!compareAndSet( + &m_record.refCoverInfo(), + static_cast(coverInfo))) { + return false; + } + kLogger.info() + << "Refreshed cover image hash" + << m_fileInfo.location(); + markDirtyAndUnlock(&lock); + emit coverArtUpdated(); + return true; +} + CoverInfoRelative Track::getCoverInfo() const { QMutexLocker lock(&m_qMutex); return m_record.getCoverInfo(); diff --git a/src/track/track.h b/src/track/track.h index 35aa9735144..78965cf65df 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -279,6 +279,12 @@ class Track : public QObject { void setCoverInfo(const CoverInfoRelative& coverInfo); CoverInfoRelative getCoverInfo() const; CoverInfo getCoverInfoWithLocation() const; + // Verify the cover image hash and update it if necessary. + // If the corresponding image has already been loaded it + // could be provided as a parameter to avoid reloading + // if actually needed. + bool refreshCoverImageHash( + const QImage& loadedImage = QImage()); quint16 getCoverHash() const; diff --git a/src/widget/wcoverart.cpp b/src/widget/wcoverart.cpp index e3a78b0503c..add4f52c4a2 100644 --- a/src/widget/wcoverart.cpp +++ b/src/widget/wcoverart.cpp @@ -32,11 +32,11 @@ WCoverArt::WCoverArt(QWidget* parent, setAcceptDrops(!m_group.isEmpty()); CoverArtCache* pCache = CoverArtCache::instance(); - if (pCache != nullptr) { - connect(pCache, SIGNAL(coverFound(const QObject*, - const CoverInfoRelative&, QPixmap, bool)), - this, SLOT(slotCoverFound(const QObject*, - const CoverInfoRelative&, QPixmap, bool))); + if (pCache) { + connect(pCache, + &CoverArtCache::coverFound, + this, + &WCoverArt::slotCoverFound); } connect(m_pMenu, SIGNAL(coverInfoSelected(const CoverInfoRelative&)), this, SLOT(slotCoverInfoSelected(const CoverInfoRelative&))); @@ -98,12 +98,10 @@ void WCoverArt::setup(const QDomNode& node, const SkinContext& context) { } void WCoverArt::slotReloadCoverArt() { - if (m_loadedTrack) { - CoverArtCache* pCache = CoverArtCache::instance(); - if (pCache) { - pCache->requestGuessCover(m_loadedTrack); - } + if (!m_loadedTrack) { + return; } + guessTrackCoverInfoConcurrently(m_loadedTrack); } void WCoverArt::slotCoverInfoSelected(const CoverInfoRelative& coverInfo) { @@ -131,8 +129,11 @@ void WCoverArt::slotLoadingTrack(TrackPointer pNewTrack, TrackPointer pOldTrack) void WCoverArt::slotReset() { if (m_loadedTrack) { - disconnect(m_loadedTrack.get(), SIGNAL(coverArtUpdated()), - this, SLOT(slotTrackCoverArtUpdated())); + disconnect( + m_loadedTrack.get(), + &Track::coverArtUpdated, + this, + &WCoverArt::slotTrackCoverArtUpdated); } m_loadedTrack.reset(); m_lastRequestedCover = CoverInfo(); @@ -143,23 +144,26 @@ void WCoverArt::slotReset() { void WCoverArt::slotTrackCoverArtUpdated() { if (m_loadedTrack) { - CoverArtCache::requestCover(*m_loadedTrack, this); + CoverArtCache::requestTrackCover(this, m_loadedTrack); } } -void WCoverArt::slotCoverFound(const QObject* pRequestor, - const CoverInfoRelative& info, QPixmap pixmap, - bool fromCache) { - Q_UNUSED(info); - Q_UNUSED(fromCache); +void WCoverArt::slotCoverFound( + const QObject* pRequestor, + const CoverInfo& coverInfo, + const QPixmap& pixmap, + quint16 requestedHash, + bool coverInfoUpdated) { + Q_UNUSED(requestedHash); + Q_UNUSED(coverInfoUpdated); if (!m_bEnable) { return; } - if (pRequestor == this && m_loadedTrack && - m_loadedTrack->getCoverHash() == info.hash) { - qDebug() << "WCoverArt::slotCoverFound" << pRequestor << info - << pixmap.size(); + if (pRequestor == this && + m_loadedTrack && + m_loadedTrack->getLocation() == coverInfo.trackLocation) { + m_lastRequestedCover = coverInfo; m_loadedCover = pixmap; m_loadedCoverScaled = scaledCoverArt(pixmap); update(); @@ -176,8 +180,10 @@ void WCoverArt::slotLoadTrack(TrackPointer pTrack) { m_loadedCoverScaled = QPixmap(); m_loadedTrack = pTrack; if (m_loadedTrack) { - connect(m_loadedTrack.get(), SIGNAL(coverArtUpdated()), - this, SLOT(slotTrackCoverArtUpdated())); + connect(m_loadedTrack.get(), + &Track::coverArtUpdated, + this, + &WCoverArt::slotTrackCoverArtUpdated); } if (!m_bEnable) { diff --git a/src/widget/wcoverart.h b/src/widget/wcoverart.h index f03ecbfbd6b..98b2ab32e9b 100644 --- a/src/widget/wcoverart.h +++ b/src/widget/wcoverart.h @@ -38,8 +38,12 @@ class WCoverArt : public QWidget, public WBaseWidget, public TrackDropTarget { void cloneDeck(QString source_group, QString target_group) override; private slots: - void slotCoverFound(const QObject* pRequestor, - const CoverInfoRelative& info, QPixmap pixmap, bool fromCache); + void slotCoverFound( + const QObject* pRequestor, + const CoverInfo& coverInfo, + const QPixmap& pixmap, + quint16 requestedHash, + bool coverInfoUpdated); void slotCoverInfoSelected(const CoverInfoRelative& coverInfo); void slotReloadCoverArt(); void slotTrackCoverArtUpdated(); diff --git a/src/widget/wspinny.cpp b/src/widget/wspinny.cpp index 7e1c9dfc73c..2dc4f2eecfc 100644 --- a/src/widget/wspinny.cpp +++ b/src/widget/wspinny.cpp @@ -8,6 +8,7 @@ #include "control/controlobject.h" #include "control/controlproxy.h" #include "library/coverartcache.h" +#include "library/coverartutils.h" #include "util/compatibility.h" #include "util/dnd.h" #include "waveform/sharedglcontext.h" @@ -76,11 +77,11 @@ WSpinny::WSpinny( } CoverArtCache* pCache = CoverArtCache::instance(); - if (pCache != nullptr) { - connect(pCache, SIGNAL(coverFound(const QObject*, - const CoverInfoRelative&, QPixmap, bool)), - this, SLOT(slotCoverFound(const QObject*, - const CoverInfoRelative&, QPixmap, bool))); + if (pCache) { + connect(pCache, + &CoverArtCache::coverFound, + this, + &WSpinny::slotCoverFound); } if (m_pPlayer != nullptr) { @@ -264,20 +265,21 @@ void WSpinny::slotLoadingTrack(TrackPointer pNewTrack, TrackPointer pOldTrack) { void WSpinny::slotTrackCoverArtUpdated() { if (m_loadedTrack) { - CoverArtCache::requestCover(*m_loadedTrack, this); + CoverArtCache::requestTrackCover(this, m_loadedTrack); } } -void WSpinny::slotCoverFound(const QObject* pRequestor, - const CoverInfoRelative& info, QPixmap pixmap, - bool fromCache) { - Q_UNUSED(info); - Q_UNUSED(fromCache); - - if (pRequestor == this && m_loadedTrack && - m_loadedTrack->getCoverHash() == info.hash) { - qDebug() << "WSpinny::slotCoverFound" << pRequestor << info - << pixmap.size(); +void WSpinny::slotCoverFound( + const QObject* pRequestor, + const CoverInfo& coverInfo, + const QPixmap& pixmap, + quint16 requestedHash, + bool coverInfoUpdated) { + Q_UNUSED(requestedHash); + Q_UNUSED(coverInfoUpdated); + if (pRequestor == this && + m_loadedTrack && + m_loadedTrack->getLocation() == coverInfo.trackLocation) { m_loadedCover = pixmap; m_loadedCoverScaled = scaledCoverArt(pixmap); update(); @@ -292,12 +294,10 @@ void WSpinny::slotCoverInfoSelected(const CoverInfoRelative& coverInfo) { } void WSpinny::slotReloadCoverArt() { - if (m_loadedTrack != nullptr) { - CoverArtCache* pCache = CoverArtCache::instance(); - if (pCache) { - pCache->requestGuessCover(m_loadedTrack); - } + if (!m_loadedTrack) { + return; } + guessTrackCoverInfoConcurrently(m_loadedTrack); } void WSpinny::paintEvent(QPaintEvent *e) { diff --git a/src/widget/wspinny.h b/src/widget/wspinny.h index 9bc96a43f4b..5ae7a28c895 100644 --- a/src/widget/wspinny.h +++ b/src/widget/wspinny.h @@ -50,8 +50,12 @@ class WSpinny : public QGLWidget, public WBaseWidget, public VinylSignalQualityL void swap(); protected slots: - void slotCoverFound(const QObject* pRequestor, - const CoverInfoRelative& info, QPixmap pixmap, bool fromCache); + void slotCoverFound( + const QObject* pRequestor, + const CoverInfo& coverInfo, + const QPixmap& pixmap, + quint16 requestedHash, + bool coverInfoUpdated); void slotCoverInfoSelected(const CoverInfoRelative& coverInfo); void slotReloadCoverArt(); void slotTrackCoverArtUpdated(); diff --git a/src/widget/wtracktableview.cpp b/src/widget/wtracktableview.cpp index eb9cafca48c..35b426e7628 100644 --- a/src/widget/wtracktableview.cpp +++ b/src/widget/wtracktableview.cpp @@ -16,7 +16,7 @@ #include "widget/wskincolor.h" #include "widget/wtracktableviewheader.h" #include "widget/wwidget.h" -#include "library/coverartcache.h" +#include "library/coverartutils.h" #include "library/dlgtagfetcher.h" #include "library/dlgtrackinfo.h" #include "library/librarytablemodel.h" @@ -2170,21 +2170,22 @@ void WTrackTableView::slotCoverInfoSelected(const CoverInfoRelative& coverInfo) void WTrackTableView::slotReloadCoverArt() { TrackModel* trackModel = getTrackModel(); - if (trackModel == nullptr) { + if (!trackModel) { return; } - QList selectedTracks; const QModelIndexList selection = selectionModel()->selectedRows(); + if (selection.isEmpty()) { + return; + } + QList selectedTracks; + selectedTracks.reserve(selection.size()); for (const QModelIndex& index : selection) { TrackPointer pTrack = trackModel->getTrack(index); if (pTrack) { selectedTracks.append(pTrack); } } - CoverArtCache* pCache = CoverArtCache::instance(); - if (pCache) { - pCache->requestGuessCovers(selectedTracks); - } + guessTrackCoverInfoConcurrently(selectedTracks); } void WTrackTableView::slotSortingChanged(int headerSection, Qt::SortOrder order) {