lp1879160: Log warning about cover load failure only once

src/library/coverart.cpp

@@ -1,5 +1,7 @@
 #include "library/coverart.h"
 
+#include <QDebugStateSaver>
+
 #include "library/coverartutils.h"
 #include "util/debug.h"
 #include "util/logger.h"
@@ -83,19 +85,27 @@ QDebug operator<<(QDebug dbg, const CoverInfoRelative& infoRelative) {
             .arg(coverInfoRelativeToString(infoRelative));
 }
 
-QImage CoverInfo::loadImage(
+CoverInfo::LoadedImage CoverInfo::loadImage(
         const SecurityTokenPointer& pTrackLocationToken) const {
+    LoadedImage loadedImage(LoadedImage::Result::ErrorUnknown);
     if (type == CoverInfo::METADATA) {
         VERIFY_OR_DEBUG_ASSERT(!trackLocation.isEmpty()) {
-            kLogger.warning()
-                    << "loadImage"
-                    << type
-                    << "cover with empty trackLocation";
-            return QImage();
+            loadedImage.result = LoadedImage::Result::ErrorMetadataWithEmptyTrackLocation;
+            return loadedImage;
         }
-        return CoverArtUtils::extractEmbeddedCover(
+        loadedImage.filePath = trackLocation;
+        loadedImage.image = CoverArtUtils::extractEmbeddedCover(
                 TrackFile(trackLocation),
                 pTrackLocationToken);
+        if (loadedImage.image.isNull()) {
+            // TODO: extractEmbeddedCover() should indicate if no image
+            // is available or if loading the embedded image failed.
+            // Until then we assume optimistically that no image is
+            // available instead of presuming that an error occurred.
+            loadedImage.result = LoadedImage::Result::NoImage;
+        } else {
+            loadedImage.result = LoadedImage::Result::Ok;
+        }
     } else if (type == CoverInfo::FILE) {
         auto coverFile = QFileInfo(coverLocation);
         if (coverFile.isRelative()) {
@@ -104,13 +114,9 @@ QImage CoverInfo::loadImage(
                 // 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();
+                loadedImage.result = LoadedImage::Result::
+                        ErrorRelativeFilePathWithEmptyTrackLocation;
+                return loadedImage;
             }
             // Compose track directory with relative path
             const auto trackFile = TrackFile(trackLocation);
@@ -120,28 +126,28 @@ QImage CoverInfo::loadImage(
                     coverLocation);
         }
         DEBUG_ASSERT(coverFile.isAbsolute());
+        loadedImage.filePath = coverFile.filePath();
         if (!coverFile.exists()) {
-            // Disabled because this code can cause high CPU and thus possibly
-            // xruns as it might print the warning repeatedly.
-            // ToDo: Print warning about missing cover image only once.
-            //            kLogger.warning()
-            //                    << "loadImage"
-            //                    << type
-            //                    << "cover does not exist:"
-            //                    << coverFile.filePath();
-            return QImage();
+            loadedImage.result = LoadedImage::Result::ErrorFilePathDoesNotExist;
+            return loadedImage;
         }
         SecurityTokenPointer pToken =
                 Sandbox::openSecurityToken(
                         coverFile,
                         true);
-        return QImage(coverFile.filePath());
+        if (loadedImage.image.load(loadedImage.filePath)) {
+            DEBUG_ASSERT(!loadedImage.image.isNull());
+            loadedImage.result = LoadedImage::Result::Ok;
+        } else {
+            DEBUG_ASSERT(loadedImage.image.isNull());
+            loadedImage.result = LoadedImage::Result::ErrorLoadingFailed;
+        }
     } else if (type == CoverInfo::NONE) {
-        return QImage();
+        loadedImage.result = LoadedImage::Result::NoImage;
     } else {
         DEBUG_ASSERT(!"unhandled CoverInfo::Type");
-        return QImage();
     }
+    return loadedImage;
 }
 
 bool CoverInfo::refreshImageHash(
@@ -154,8 +160,8 @@ bool CoverInfo::refreshImageHash(
         return false;
     }
     QImage image = loadedImage;
-    if (loadedImage.isNull()) {
-        image = loadImage(pTrackLocationToken);
+    if (image.isNull()) {
+        image = loadImage(pTrackLocationToken).image;
     }
     if (image.isNull() && type != CoverInfo::NONE) {
         kLogger.warning()
@@ -185,9 +191,49 @@ QDebug operator<<(QDebug dbg, const CoverInfo& info) {
             .arg(coverInfoToString(info));
 }
 
+QDebug operator<<(QDebug dbg, const CoverInfo::LoadedImage::Result& result) {
+    switch (result) {
+    case CoverInfo::LoadedImage::Result::Ok:
+        return dbg << "Ok";
+    case CoverInfo::LoadedImage::Result::NoImage:
+        return dbg << "NoImage";
+    case CoverInfo::LoadedImage::Result::ErrorMetadataWithEmptyTrackLocation:
+        return dbg << "ErrorMetadataWithEmptyTrackLocation";
+    case CoverInfo::LoadedImage::Result::ErrorRelativeFilePathWithEmptyTrackLocation:
+        return dbg << "ErrorRelativeFilePathWithEmptyTrackLocation";
+    case CoverInfo::LoadedImage::Result::ErrorFilePathDoesNotExist:
+        return dbg << "ErrorFilePathDoesNotExist";
+    case CoverInfo::LoadedImage::Result::ErrorLoadingFailed:
+        return dbg << "ErrorLoadingFailed";
+    case CoverInfo::LoadedImage::Result::ErrorUnknown:
+        return dbg << "ErrorUnknown";
+    }
+    DEBUG_ASSERT(!"unreachable");
+    return dbg;
+}
+
+QDebug operator<<(QDebug dbg, const CoverInfo::LoadedImage& loadedImage) {
+    const QDebugStateSaver saver(dbg);
+    dbg = dbg.maybeSpace() << "CoverInfo::LoadedImage";
+    return dbg.nospace()
+            << '{'
+            << loadedImage.filePath
+            << ','
+            << loadedImage.image.size()
+            << ','
+            << loadedImage.result
+            << '}';
+}
+
 QDebug operator<<(QDebug dbg, const CoverArt& art) {
-    return dbg.maybeSpace() << QString("CoverArt(%1,%2,%3)")
-            .arg(coverInfoToString(art),
-                 toDebugString(art.image.size()),
-                 QString::number(art.resizedToWidth));
+    const QDebugStateSaver saver(dbg);
+    dbg = dbg.maybeSpace() << "CoverArt";
+    return dbg.nospace()
+            << '{'
+            << static_cast<const CoverInfo&>(art)
+            << ','
+            << art.loadedImage
+            << ','
+            << art.resizedToWidth
+            << '}';
 }

src/library/coverart.h

@@ -85,7 +85,40 @@ class CoverInfo : public CoverInfoRelative {
     CoverInfo(CoverInfo&&) = default;
     CoverInfo& operator=(CoverInfo&&) = default;
 
-    QImage loadImage(
+    struct LoadedImage final {
+        enum class Result {
+            Ok,
+            NoImage,
+            ErrorMetadataWithEmptyTrackLocation,
+            ErrorRelativeFilePathWithEmptyTrackLocation,
+            ErrorFilePathDoesNotExist,
+            ErrorLoadingFailed, // if the image file is not readable or the format is not supported
+            ErrorUnknown,       // should never happen
+        };
+
+        LoadedImage(const LoadedImage&) = default;
+        LoadedImage(LoadedImage&&) = default;
+        LoadedImage& operator=(const LoadedImage&) = default;
+        LoadedImage& operator=(LoadedImage&&) = default;
+
+        /// The loaded image if available.
+        QImage image;
+
+        /// Either the track location if the image was embedded in
+        /// the metadata or the (absolute) path of the image file.
+        QString filePath;
+
+        /// The result of the operation.
+        Result result;
+
+      private:
+        friend class CoverArt;
+        friend class CoverInfo;
+        LoadedImage(Result result)
+                : result(result) {
+        }
+    };
+    LoadedImage loadImage(
             const SecurityTokenPointer& pTrackLocationToken = SecurityTokenPointer()) const;
 
     // Verify the image hash and update it if necessary.
@@ -103,16 +136,22 @@ bool operator==(const CoverInfo& a, const CoverInfo& b);
 bool operator!=(const CoverInfo& a, const CoverInfo& b);
 QDebug operator<<(QDebug dbg, const CoverInfo& info);
 
+QDebug operator<<(QDebug dbg, const CoverInfo::LoadedImage::Result& result);
+QDebug operator<<(QDebug dbg, const CoverInfo::LoadedImage& loadedImage);
+
 class CoverArt : public CoverInfo {
   public:
     CoverArt()
-        : resizedToWidth(0) {
+            : loadedImage(LoadedImage::Result::NoImage), // not loaded = no image
+              resizedToWidth(0) {
     }
-
-    CoverArt(const CoverInfo& base, const QImage& img, int rtw)
-        : CoverInfo(base),
-          image(img),
-          resizedToWidth(rtw) {
+    CoverArt(
+            const CoverInfo&& base,
+            CoverInfo::LoadedImage&& loadedImage,
+            int resizedToWidth)
+            : CoverInfo(std::move(base)),
+              loadedImage(std::move(loadedImage)),
+              resizedToWidth(resizedToWidth) {
     }
 
     // all-default memory management
@@ -123,7 +162,7 @@ class CoverArt : public CoverInfo {
 
     // it is not a QPixmap, because it is not safe to use pixmaps
     // outside the GUI thread
-    QImage image;
+    CoverInfo::LoadedImage loadedImage;
     int resizedToWidth;
 };
 

src/library/coverartcache.cpp

@@ -176,26 +176,30 @@ CoverArtCache::FutureResult CoverArtCache::loadCover(
     res.signalWhenDone = signalWhenDone;
     DEBUG_ASSERT(!res.coverInfoUpdated);
 
-    QImage image = coverInfo.loadImage(
+    auto loadedImage = coverInfo.loadImage(
             pTrack ? pTrack->getSecurityToken() : SecurityTokenPointer());
+    if (!loadedImage.image.isNull()) {
+        // Refresh hash before resizing the original image!
+        res.coverInfoUpdated = coverInfo.refreshImageHash(loadedImage.image);
+        if (pTrack && res.coverInfoUpdated) {
+            kLogger.info()
+                    << "Updating cover info of track"
+                    << coverInfo.trackLocation;
+            pTrack->setCoverInfo(coverInfo);
+        }
 
-    // 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);
-    }
-
-    // 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);
+        // Resize image to requested size
+        if (desiredWidth > 0) {
+            // Adjust the cover size according to the request
+            // or downsize the image for efficiency.
+            loadedImage.image = resizeImageWidth(loadedImage.image, desiredWidth);
+        }
     }
 
-    res.cover = CoverArt(coverInfo, image, desiredWidth);
+    res.coverArt = CoverArt(
+            std::move(coverInfo),
+            std::move(loadedImage),
+            desiredWidth);
     return res;
 }
 
@@ -213,30 +217,61 @@ void CoverArtCache::coverLoaded() {
     }
 
     if (kLogger.traceEnabled()) {
-        kLogger.trace() << "coverLoaded" << res.cover;
+        kLogger.trace() << "coverLoaded" << res.coverArt;
     }
 
-    // Don't cache full size covers (resizedToWidth = 0)
-    // Large cover art wastes space in our cache and will likely
-    // uncache a lot of the small covers we need in the library
-    // table.
-    // Full size covers are used in the Skin Widgets, which are
-    // loaded with an artificial delay anyway and an additional
-    // re-load delay can be accepted.
-
-    // Create pixmap, GUI thread only
-    QPixmap pixmap = QPixmap::fromImage(res.cover.image);
-    if (!pixmap.isNull() && res.cover.resizedToWidth != 0) {
-        // we have to be sure that res.cover.hash is unique
-        // because insert replaces the images with the same key
-        QString cacheKey = pixmapCacheKey(
-                res.cover.hash, res.cover.resizedToWidth);
-        QPixmapCache::insert(cacheKey, pixmap);
+    QPixmap pixmap;
+    if (res.coverArt.loadedImage.result != CoverInfo::LoadedImage::Result::NoImage) {
+        if (res.coverArt.loadedImage.result == CoverInfo::LoadedImage::Result::Ok) {
+            DEBUG_ASSERT(!res.coverArt.loadedImage.filePath.isEmpty());
+        } else {
+            DEBUG_ASSERT(res.coverArt.loadedImage.image.isNull());
+            kLogger.warning()
+                    << "Failed to load cover art image"
+                    << res.coverArt.loadedImage
+                    << "for track"
+                    << res.coverArt.trackLocation;
+            // Substitute missing cover art with a placeholder image to avoid high CPU load
+            // See also: https://bugs.launchpad.net/mixxx/+bug/1879160
+            const int imageSize = math_max(1, res.coverArt.resizedToWidth);
+            QImage placeholderImage(imageSize, imageSize, QImage::Format_RGB32);
+            // TODO(uklotzde): Use optional cover art background color (if available)
+            // instead of Qt::darkGray
+            placeholderImage.fill(
+                    mixxx::RgbColor::toQColor(std::nullopt /*res.coverArt.color*/, Qt::darkGray));
+            res.coverArt.loadedImage.image = placeholderImage;
+        }
+        // Create pixmap, GUI thread only!
+        DEBUG_ASSERT(QThread::currentThread() ==
+                QCoreApplication::instance()->thread());
+        DEBUG_ASSERT(!res.coverArt.loadedImage.image.isNull());
+        pixmap = QPixmap::fromImage(res.coverArt.loadedImage.image);
+        // Don't cache full size covers (resizedToWidth = 0)
+        // Large cover art wastes space in our cache and will likely
+        // uncache a lot of the small covers we need in the library
+        // table.
+        // Full size covers are used in the Skin Widgets, which are
+        // loaded with an artificial delay anyway and an additional
+        // re-load delay can be accepted.
+        if (res.coverArt.resizedToWidth > 0) {
+            DEBUG_ASSERT(!pixmap.isNull());
+            // 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.hash, res.coverArt.resizedToWidth);
+            QPixmapCache::insert(cacheKey, pixmap);
+        }
     }
 
     m_runningRequests.remove(qMakePair(res.pRequestor, res.requestedHash));
 
     if (res.signalWhenDone) {
-        emit coverFound(res.pRequestor, res.cover, pixmap, res.requestedHash, res.coverInfoUpdated);
+        emit coverFound(
+                res.pRequestor,
+                std::move(res.coverArt),
+                pixmap,
+                res.requestedHash,
+                res.coverInfoUpdated);
     }
 }

src/library/coverartcache.h

@@ -65,7 +65,7 @@ class CoverArtCache : public QObject, public Singleton<CoverArtCache> {
         quint16 requestedHash;
         bool signalWhenDone;
 
-        CoverArt cover;
+        CoverArt coverArt;
         bool coverInfoUpdated;
     };
     // Load cover from path indicated in coverInfo. WARNING: This is run in a
@@ -107,7 +107,7 @@ class CoverArtCache : public QObject, public Singleton<CoverArtCache> {
             int desiredWidth,
             Loading loading);
 
-    QSet<QPair<const QObject*, quint16> > m_runningRequests;
+    QSet<QPair<const QObject*, quint16>> m_runningRequests;
 };
 
 inline