diff options
author | Uwe Klotz <uklotz@mixxx.org> | 2020-02-28 14:10:42 +0100 |
---|---|---|
committer | Uwe Klotz <uklotz@mixxx.org> | 2020-02-28 15:16:58 +0100 |
commit | 3af36447bc990423bf1366799b944baff17b4918 (patch) | |
tree | 50063087d1cb62bd2c18250bc9aeb14470d8bb6e | |
parent | 60b03bcb97c1c10af6871dccad29d1e6e37a00ff (diff) |
Prepare CoverArtCache for migration to new hash function
-rw-r--r-- | src/library/coverart.cpp | 26 | ||||
-rw-r--r-- | src/library/coverart.h | 15 | ||||
-rw-r--r-- | src/library/coverartcache.cpp | 194 | ||||
-rw-r--r-- | src/library/coverartcache.h | 90 | ||||
-rw-r--r-- | src/library/coverartdelegate.cpp | 62 | ||||
-rw-r--r-- | src/library/coverartdelegate.h | 11 | ||||
-rw-r--r-- | src/library/dlgcoverartfullsize.cpp | 25 | ||||
-rw-r--r-- | src/library/dlgcoverartfullsize.h | 8 | ||||
-rw-r--r-- | src/library/dlgtrackinfo.cpp | 32 | ||||
-rw-r--r-- | src/library/dlgtrackinfo.h | 8 | ||||
-rw-r--r-- | src/test/coverartcache_test.cpp | 9 | ||||
-rw-r--r-- | src/widget/wcoverart.cpp | 23 | ||||
-rw-r--r-- | src/widget/wcoverart.h | 8 | ||||
-rw-r--r-- | src/widget/wspinny.cpp | 23 | ||||
-rw-r--r-- | src/widget/wspinny.h | 8 |
15 files changed, 359 insertions, 183 deletions
diff --git a/src/library/coverart.cpp b/src/library/coverart.cpp index 27f317d6ba..3b43890ceb 100644 --- a/src/library/coverart.cpp +++ b/src/library/coverart.cpp @@ -137,6 +137,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<const CoverInfoRelative&>(a) == static_cast<const CoverInfoRelative&>(b) && diff --git a/src/library/coverart.h b/src/library/coverart.h index 9a79ecc344..d9179c3d08 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 d518d111f8..019afdb0dd 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); +} + +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 (requestInfo.type == CoverInfo::NONE) { - if (signalWhenDone) { - emit coverFound(pRequestor, requestInfo, - QPixmap(), true); + 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<const QObject*, quint16> requestId = qMakePair(pRequestor, requestInfo.hash); + QPair<const QObject*, quint16> 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<FutureResult>* watcher = new QFutureWatcher<FutureResult>(this); QFuture<FutureResult> future = QtConcurrent::run( - this, &CoverArtCache::loadCover, requestInfo, pRequestor, - desiredWidth, signalWhenDone); + &CoverArtCache::loadCover, + pRequestor, + pTrack, + coverInfo, + desiredWidth, + loading == Loading::Default); connect(watcher, &QFutureWatcher<FutureResult>::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); - 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 - // will also require noticing a hash mis-match at higher levels and - // recording the hash change in the database. + FutureResult res; + res.pRequestor = pRequestor; + res.requestedHash = coverInfo.hash; + res.signalWhenDone = signalWhenDone; + DEBUG_ASSERT(!res.coverInfoUpdated); + + QImage image = coverInfo.loadImage( + pTrack ? pTrack->getSecurityToken() : SecurityTokenPointer()); + + // 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<FutureResult>* watcher = + QFutureWatcher<FutureResult>* pFutureWatcher = static_cast<QFutureWatcher<FutureResult>*>(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,9 +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); + emit coverFound(res.pRequestor, res.cover, pixmap, res.requestedHash, res.coverInfoUpdated); } } diff --git a/src/library/coverartcache.h b/src/library/coverartcache.h index b1541b27c4..c89b5c8a63 100644 --- a/src/library/coverartcache.h +++ b/src/library/coverartcache.h @@ -1,8 +1,10 @@ -#ifndef COVERARTCACHE_H -#define COVERARTCACHE_H +#pragma once #include <QObject> +#include <QPair> #include <QPixmap> +#include <QSet> +#include <QtDebug> #include "library/coverart.h" #include "util/singleton.h" @@ -11,6 +13,14 @@ class CoverArtCache : public QObject, public Singleton<CoverArtCache> { Q_OBJECT public: + static void requestCover( + const QObject* pRequestor, + const CoverInfo& coverInfo, + const TrackPointer& pTrack = 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,48 +33,78 @@ class CoverArtCache : public QObject, public Singleton<CoverArtCache> { * 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); + 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<CoverArtCache>; - // 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: + QPixmap tryLoadCover( + const QObject* pRequestor, + const TrackPointer& pTrack, + const CoverInfo& info, + int desiredWidth, + Loading loading); + QSet<QPair<const QObject*, quint16> > m_runningRequests; }; -#endif // COVERARTCACHE_H +inline +QDebug operator<<(QDebug dbg, CoverArtCache::Loading loading) { + return dbg << static_cast<int>(loading); +} diff --git a/src/library/coverartdelegate.cpp b/src/library/coverartdelegate.cpp index 3e31c6cf8b..f9d7c5175e 100644 --- a/src/library/coverartdelegate.cpp +++ b/src/library/coverartdelegate.cpp @@ -6,8 +6,14 @@ #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), @@ -71,13 +77,18 @@ 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<int> rows = m_hashToRow.take(info.hash); +void CoverArtDelegate::slotCoverFound( + const QObject* pRequestor, + const CoverInfo& coverInfo, + const QPixmap& pixmap, + quint16 requestedHash, + bool coverInfoUpdated) { + Q_UNUSED(coverInfo); + Q_UNUSED(pixmap); + Q_UNUSED(coverInfoUpdated); + if (pRequestor == this) { + const QLinkedList<int> rows = + m_hashToRow.take(requestedHash); foreach(int row, rows) { emit coverReadyForCell(row, m_iCoverColumn); } @@ -87,9 +98,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 +124,31 @@ void CoverArtDelegate::paintItem(QPainter *painter, double scaleFactor = getDevicePixelRatioF(static_cast<QWidget*>(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 026d67bca5..e21da8143f 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,9 +37,12 @@ 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; diff --git a/src/library/dlgcoverartfullsize.cpp b/src/library/dlgcoverartfullsize.cpp index 3a120ad432..1393cf9f44 100644 --- a/src/library/dlgcoverartfullsize.cpp +++ b/src/library/dlgcoverartfullsize.cpp @@ -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 7be9d57191..7ae2a7ba71 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 64ae4c8b29..45ee553d06 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 677483ead3..eb721c96c7 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/test/coverartcache_test.cpp b/src/test/coverartcache_test.cpp index 2c86f03ced..5ea4f09177 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/widget/wcoverart.cpp b/src/widget/wcoverart.cpp index 19d611b375..a105d9385d 100644 --- a/src/widget/wcoverart.cpp +++ b/src/widget/wcoverart.cpp @@ -144,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(); diff --git a/src/widget/wcoverart.h b/src/widget/wcoverart.h index f03ecbfbd6..98b2ab32e9 100644 --- a/src/widget/wcoverart.h +++ b/src/widget/wcoverart.h @@ -38,8 +38,12 @@ class WCoverArt : public QWidget, public WBaseWidg |