Merge pull request #12103 from daschuer/gh11131

Fix reading the Spinny cover on Windows
mixxxdj · Nov 23, 2023 · 6e9ea54 · 6e9ea54
2 parents 174c6d9 + 94538c2
commit 6e9ea54
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 30 deletions.
diff --git a/src/library/coverartcache.cpp b/src/library/coverartcache.cpp
@@ -161,11 +161,14 @@ void CoverArtCache::tryLoadCover(
         return;
     }
 
-    const auto requestedCacheKey = coverInfo.cacheKey();
-    // 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*, mixxx::cache_key_t> requestId = qMakePair(pRequester, requestedCacheKey);
-    if (m_runningRequests.contains(requestId)) {
+    const mixxx::cache_key_t requestedCacheKey = coverInfo.cacheKey();
+    // keep a list of cache keys for which a future is currently running
+    // to avoid loading the same picture again while we are loading it.
+    // This fixes also https://github.com/mixxxdj/mixxx/issues/11131 on
+    // Windows where simultaneous open the same file from two threads fails.
+    bool requestPending = m_runningRequests.contains(requestedCacheKey);
+    m_runningRequests.insert(requestedCacheKey, {pRequester, desiredWidth});
+    if (requestPending) {
         return;
     }
 
@@ -174,12 +177,11 @@ void CoverArtCache::tryLoadCover(
                 << "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(
             &CoverArtCache::loadCover,
-            pRequester,
             pTrack,
             coverInfo,
             desiredWidth);
@@ -193,7 +195,6 @@ void CoverArtCache::tryLoadCover(
 
 //static
 CoverArtCache::FutureResult CoverArtCache::loadCover(
-        const QObject* pRequester,
         TrackPointer pTrack,
         CoverInfo coverInfo,
         int desiredWidth) {
@@ -207,7 +208,6 @@ CoverArtCache::FutureResult CoverArtCache::loadCover(
             pTrack->getLocation() == coverInfo.trackLocation);
 
     auto res = FutureResult(
-            pRequester,
             coverInfo.cacheKey());
 
     CoverInfo::LoadedImage loadedImage = coverInfo.loadImage(pTrack);
@@ -258,6 +258,8 @@ void CoverArtCache::coverLoaded() {
         kLogger.trace() << "coverLoaded" << res.coverArt;
     }
 
+    QString cacheKey = pixmapCacheKey(
+            res.coverArt.cacheKey(), res.coverArt.resizedToWidth);
     QPixmap pixmap;
     if (res.coverArt.loadedImage.result != CoverInfo::LoadedImage::Result::NoImage) {
         if (res.coverArt.loadedImage.result == CoverInfo::LoadedImage::Result::Ok) {
@@ -293,18 +295,29 @@ void CoverArtCache::coverLoaded() {
             // It is very unlikely that res.coverArt.hash generates the
             // same hash for different images. Otherwise the wrong image would
             // be displayed when loaded from the cache.
-            QString cacheKey = pixmapCacheKey(
-                    res.coverArt.cacheKey(), res.coverArt.resizedToWidth);
             QPixmapCache::insert(cacheKey, pixmap);
         }
     }
 
-    m_runningRequests.remove(qMakePair(res.pRequester, res.requestedCacheKey));
-
-    if (res.pRequester) {
-        emit coverFound(
-                res.pRequester,
-                std::move(res.coverArt),
-                pixmap);
+    auto runningRequests = m_runningRequests;
+    // First remove all requests for this cover that way we can
+    // re-add cover with different sizes via tryLoadCover() as usual
+    m_runningRequests.remove(res.coverArt.cacheKey());
+
+    auto i = runningRequests.find(res.coverArt.cacheKey());
+    while (i != runningRequests.end() && i.key() == res.coverArt.cacheKey()) {
+        if (i.value().desiredWidth == res.coverArt.resizedToWidth) {
+            emit coverFound(
+                    i.value().pRequester,
+                    res.coverArt,
+                    pixmap);
+        } else {
+            tryLoadCover(
+                    i.value().pRequester,
+                    nullptr,
+                    res.coverArt,
+                    i.value().desiredWidth);
+        }
+        ++i;
     }
 }
diff --git a/src/library/coverartcache.h b/src/library/coverartcache.h
@@ -40,25 +40,18 @@ class CoverArtCache : public QObject, public Singleton<CoverArtCache> {
     // Only public for testing
     struct FutureResult {
         FutureResult()
-                : pRequester(nullptr),
-                  requestedCacheKey(CoverImageUtils::defaultCacheKey()) {
+                : requestedCacheKey(CoverImageUtils::defaultCacheKey()) {
         }
         FutureResult(
-                const QObject* pRequestorArg,
                 mixxx::cache_key_t requestedCacheKeyArg)
-                : pRequester(pRequestorArg),
-                  requestedCacheKey(requestedCacheKeyArg) {
+                : requestedCacheKey(requestedCacheKeyArg) {
         }
-
-        const QObject* pRequester;
         mixxx::cache_key_t requestedCacheKey;
-
         CoverArt coverArt;
     };
     // Load cover from path indicated in coverInfo. WARNING: This is run in a
     // worker thread.
     static FutureResult loadCover(
-            const QObject* pRequester,
             TrackPointer pTrack,
             CoverInfo coverInfo,
             int desiredWidth);
@@ -91,5 +84,9 @@ class CoverArtCache : public QObject, public Singleton<CoverArtCache> {
             const CoverInfo& info,
             int desiredWidth);
 
-    QSet<QPair<const QObject*, mixxx::cache_key_t>> m_runningRequests;
+    struct RequestData {
+        const QObject* pRequester;
+        int desiredWidth;
+    };
+    QMultiHash<mixxx::cache_key_t, RequestData> m_runningRequests;
 };
diff --git a/src/test/coverartcache_test.cpp b/src/test/coverartcache_test.cpp
@@ -29,7 +29,7 @@ class CoverArtCacheTest : public LibraryTest, public CoverArtCache {
         info.trackLocation = trackLocation;
 
         CoverArtCache::FutureResult res;
-        res = CoverArtCache::loadCover(nullptr, TrackPointer(), info, 0);
+        res = CoverArtCache::loadCover(TrackPointer(), info, 0);
         EXPECT_EQ(img, res.coverArt.loadedImage.image);
         EXPECT_TRUE(res.coverArt.coverLocation.isNull());
     }
@@ -47,7 +47,7 @@ class CoverArtCacheTest : public LibraryTest, public CoverArtCache {
         info.trackLocation = trackLocation;
 
         CoverArtCache::FutureResult res;
-        res = CoverArtCache::loadCover(nullptr, TrackPointer(), info, 0);
+        res = CoverArtCache::loadCover(TrackPointer(), info, 0);
         EXPECT_EQ(img, res.coverArt.loadedImage.image);
         EXPECT_QSTRING_EQ(info.coverLocation, res.coverArt.coverLocation);
     }