diff --git a/src/library/coverart.cpp b/src/library/coverart.cpp index 491dbbe1396e..85620cdd87fb 100644 --- a/src/library/coverart.cpp +++ b/src/library/coverart.cpp @@ -1,5 +1,7 @@ #include "library/coverart.h" +#include + #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(art) + << ',' + << art.loadedImage + << ',' + << art.resizedToWidth + << '}'; } diff --git a/src/library/coverart.h b/src/library/coverart.h index d9179c3d0874..74fa85722a4b 100644 --- a/src/library/coverart.h +++ b/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; }; diff --git a/src/library/coverartcache.cpp b/src/library/coverartcache.cpp index 019afdb0dd2e..4e523dfd6c5d 100644 --- a/src/library/coverartcache.cpp +++ b/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); } } diff --git a/src/library/coverartcache.h b/src/library/coverartcache.h index 170c8b82a381..22ab59a00581 100644 --- a/src/library/coverartcache.h +++ b/src/library/coverartcache.h @@ -65,7 +65,7 @@ class CoverArtCache : public QObject, public Singleton { 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 { int desiredWidth, Loading loading); - QSet > m_runningRequests; + QSet> m_runningRequests; }; inline diff --git a/src/test/coverartcache_test.cpp b/src/test/coverartcache_test.cpp index 5ea4f0917765..8b13f0a8da09 100644 --- a/src/test/coverartcache_test.cpp +++ b/src/test/coverartcache_test.cpp @@ -20,8 +20,8 @@ class CoverArtCacheTest : public LibraryTest, public CoverArtCache { CoverArtCache::FutureResult res; res = CoverArtCache::loadCover(nullptr, TrackPointer(), info, 0, false); - EXPECT_QSTRING_EQ(QString(), res.cover.coverLocation); - EXPECT_TRUE(CoverImageUtils::isValidHash(res.cover.hash)); + EXPECT_QSTRING_EQ(QString(), res.coverArt.coverLocation); + EXPECT_TRUE(CoverImageUtils::isValidHash(res.coverArt.hash)); EXPECT_TRUE(res.coverInfoUpdated); SecurityTokenPointer securityToken = @@ -29,25 +29,25 @@ class CoverArtCacheTest : public LibraryTest, public CoverArtCache { QImage img = SoundSourceProxy::importTemporaryCoverImage( trackLocation, securityToken); EXPECT_FALSE(img.isNull()); - EXPECT_EQ(img, res.cover.image); + EXPECT_EQ(img, res.coverArt.loadedImage.image); } void loadCoverFromFile(QString trackLocation, QString coverLocation, QString absoluteCoverLocation) { - QImage img = QImage(absoluteCoverLocation); + const QImage img = QImage(absoluteCoverLocation); + ASSERT_FALSE(img.isNull()); CoverInfo info; info.type = CoverInfo::FILE; info.source = CoverInfo::GUESSED; info.coverLocation = coverLocation; info.trackLocation = trackLocation; - info.hash = 39287; // actual cover image hash! CoverArtCache::FutureResult res; 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()); - EXPECT_EQ(img, res.cover.image); + EXPECT_TRUE(res.coverInfoUpdated); // hash updated + EXPECT_EQ(img, res.coverArt.loadedImage.image); + EXPECT_EQ(CoverImageUtils::calculateHash(img), res.coverArt.hash); + EXPECT_QSTRING_EQ(info.coverLocation, res.coverArt.coverLocation); } }; diff --git a/src/test/coverartutils_test.cpp b/src/test/coverartutils_test.cpp index 52c1a71a12ee..0d0a30c72823 100644 --- a/src/test/coverartutils_test.cpp +++ b/src/test/coverartutils_test.cpp @@ -142,7 +142,7 @@ TEST_F(CoverArtUtilTest, searchImage) { // stuff below. result.trackLocation = kTrackLocationTest; - const QImage img = result.loadImage(); + const QImage img = result.loadImage().image; EXPECT_EQ(img.isNull(), false); QString trackBaseName = "cover-test";