[PictureLoader] Fix freezes from local image search (#5994)

* [PictureLoader] Fix freezes/crashes from parallel folder search

* fix build failure
This commit is contained in:
RickyRister 2025-06-21 18:10:29 -07:00 committed by GitHub
parent d42bfa88e1
commit 6f3a07b756
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
11 changed files with 280 additions and 154 deletions

View file

@ -67,6 +67,7 @@ set(cockatrice_SOURCES
src/client/ui/line_edit_completer.cpp
src/client/ui/phases_toolbar.cpp
src/client/ui/picture_loader/picture_loader.cpp
src/client/ui/picture_loader/picture_loader_local.cpp
src/client/ui/picture_loader/picture_loader_request_status_display_widget.cpp
src/client/ui/picture_loader/picture_loader_status_bar.cpp
src/client/ui/picture_loader/picture_loader_worker.cpp

View file

@ -148,6 +148,7 @@ void PictureLoader::getPixmap(QPixmap &pixmap, CardInfoPtr card, QSize size)
void PictureLoader::imageLoaded(CardInfoPtr card, const QImage &image)
{
if (image.isNull()) {
qCDebug(PictureLoaderLog) << "Caching NULL pixmap for" << card->getName();
QPixmapCache::insert(card->getPixmapCacheKey(), QPixmap());
} else {
if (card->getUpsideDownArt()) {

View file

@ -0,0 +1,146 @@
#include "picture_loader_local.h"
#include "../../../game/cards/card_database_manager.h"
#include "../../../settings/cache_settings.h"
#include "picture_to_load.h"
#include <QDirIterator>
#include <QMovie>
PictureLoaderLocal::PictureLoaderLocal(QObject *parent)
: QObject(parent), picsPath(SettingsCache::instance().getPicsPath()),
customPicsPath(SettingsCache::instance().getCustomPicsPath()),
overrideAllCardArtWithPersonalPreference(SettingsCache::instance().getOverrideAllCardArtWithPersonalPreference())
{
// Hook up signals to settings
connect(&SettingsCache::instance(), &SettingsCache::picsPathChanged, this, &PictureLoaderLocal::picsPathChanged);
connect(&SettingsCache::instance(), &SettingsCache::overrideAllCardArtWithPersonalPreferenceChanged, this,
&PictureLoaderLocal::setOverrideAllCardArtWithPersonalPreference);
createIndex();
}
void PictureLoaderLocal::createIndex()
{
QDirIterator it(customPicsPath, QDirIterator::Subdirectories | QDirIterator::FollowSymlinks);
// Recursively check all subdirectories of the CUSTOM folder
while (it.hasNext()) {
QString thisPath(it.next());
QFileInfo thisFileInfo(thisPath);
if (thisFileInfo.isFile()) {
// We don't know which name is the correctedName because there might be '.'s in the cardName.
// Just add all possibilities to be sure.
customFolderIndex.insert(thisFileInfo.baseName(), thisFileInfo.absoluteFilePath());
customFolderIndex.insert(thisFileInfo.completeBaseName(), thisFileInfo.absoluteFilePath());
}
}
qCInfo(PictureLoaderLocalLog) << "Finished indexing local image folder CUSTOM; map now has"
<< customFolderIndex.size() << "entries.";
}
/**
* Tries to load the card image from the local images.
*
* @param toLoad The card to load
* @return The loaded image, or an empty QImage if loading failed.
*/
QImage PictureLoaderLocal::tryLoad(const CardInfoPtr &toLoad) const
{
CardSetPtr set = PictureToLoad::extractSetsSorted(toLoad).first();
QString setName = set ? set->getCorrectedShortName() : QString();
QString cardName = toLoad->getName();
QString correctedCardName = toLoad->getCorrectedName();
// FIXME: This is a hack so that to keep old Cockatrice behavior
// (ignoring provider ID) when the "override all card art with personal
// preference" is set.
//
// Figure out a proper way to integrate the two systems at some point.
//
// Note: need to go through a member for
// overrideAllCardArtWithPersonalPreference as reading from the
// SettingsCache instance from the PictureLoaderWorker thread could
// cause race conditions.
//
// XXX: Reading from the CardDatabaseManager instance from the
// PictureLoaderWorker thread might not be safe either
bool searchCustomPics =
overrideAllCardArtWithPersonalPreference ||
CardDatabaseManager::getInstance()->isProviderIdForPreferredPrinting(cardName, toLoad->getPixmapCacheKey());
if (searchCustomPics) {
qCDebug(PictureLoaderLocalLog).nospace()
<< "[card: " << cardName << " set: " << setName << "]: Attempting to load picture from local";
return tryLoadCardImageFromDisk(setName, correctedCardName, searchCustomPics);
}
qCDebug(PictureLoaderLocalLog).nospace()
<< "[card: " << cardName << " set: " << setName << "]: Skipping load picture from local";
return QImage();
}
QImage PictureLoaderLocal::tryLoadCardImageFromDisk(const QString &setName,
const QString &correctedCardName,
const bool searchCustomPics) const
{
QImage image;
QImageReader imgReader;
imgReader.setDecideFormatFromContent(true);
QList<QString> picsPaths = QList<QString>();
if (searchCustomPics) {
picsPaths << customFolderIndex.values(correctedCardName);
}
if (!setName.isEmpty()) {
picsPaths << picsPath + "/" + setName + "/" + correctedCardName
// We no longer store downloaded images there, but don't just ignore
// stuff that old versions have put there.
<< picsPath + "/downloadedPics/" + setName + "/" + correctedCardName;
}
// Iterates through the list of paths, searching for images with the desired
// name with any QImageReader-supported extension
for (const auto &_picsPath : picsPaths) {
if (!QFileInfo(_picsPath).isFile()) {
continue;
}
imgReader.setFileName(_picsPath);
if (imgReader.read(&image)) {
qCDebug(PictureLoaderLocalLog).nospace()
<< "[card: " << correctedCardName << " set: " << setName << "]: Picture found on disk.";
return image;
}
imgReader.setFileName(_picsPath + ".full");
if (imgReader.read(&image)) {
qCDebug(PictureLoaderLocalLog).nospace()
<< "[card: " << correctedCardName << " set: " << setName << "]: Picture.full found on disk.";
return image;
}
imgReader.setFileName(_picsPath + ".xlhq");
if (imgReader.read(&image)) {
qCDebug(PictureLoaderLocalLog).nospace()
<< "[card: " << correctedCardName << " set: " << setName << "]: Picture.xlhq found on disk.";
return image;
}
}
qCDebug(PictureLoaderLocalLog).nospace()
<< "[card: " << correctedCardName << " set: " << setName << "]: Picture NOT found on disk.";
return QImage();
}
void PictureLoaderLocal::picsPathChanged()
{
picsPath = SettingsCache::instance().getPicsPath();
customPicsPath = SettingsCache::instance().getCustomPicsPath();
}
void PictureLoaderLocal::setOverrideAllCardArtWithPersonalPreference(bool _overrideAllCardArtWithPersonalPreference)
{
overrideAllCardArtWithPersonalPreference = _overrideAllCardArtWithPersonalPreference;
}

View file

@ -0,0 +1,40 @@
#ifndef PICTURE_LOADER_LOCAL_H
#define PICTURE_LOADER_LOCAL_H
#include "../../../game/cards/card_info.h"
#include <QLoggingCategory>
#include <QObject>
inline Q_LOGGING_CATEGORY(PictureLoaderLocalLog, "picture_loader.local");
/**
* Handles searching for and loading card images from the local pics and custom image folders.
* This class maintains an index of the CUSTOM folder, to avoid repeatedly searching the entire directory.
*/
class PictureLoaderLocal : public QObject
{
Q_OBJECT
public:
explicit PictureLoaderLocal(QObject *parent);
QImage tryLoad(const CardInfoPtr &toLoad) const;
private:
QString picsPath, customPicsPath;
bool overrideAllCardArtWithPersonalPreference;
QMultiHash<QString, QString> customFolderIndex; // multimap of cardName to picPaths
void createIndex();
QImage
tryLoadCardImageFromDisk(const QString &setName, const QString &correctedCardName, bool searchCustomPics) const;
private slots:
void picsPathChanged();
void setOverrideAllCardArtWithPersonalPreference(bool _overrideAllCardArtWithPersonalPreference);
};
#endif // PICTURE_LOADER_LOCAL_H

View file

@ -2,10 +2,10 @@
#include "../../../game/cards/card_database_manager.h"
#include "../../../settings/cache_settings.h"
#include "picture_loader_local.h"
#include "picture_loader_worker_work.h"
#include <QDirIterator>
#include <QJsonDocument>
#include <QMovie>
#include <QNetworkDiskCache>
#include <QNetworkReply>
@ -43,10 +43,14 @@ PictureLoaderWorker::PictureLoaderWorker() : QObject(nullptr), picDownload(Setti
connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this,
&PictureLoaderWorker::saveRedirectCache);
localLoader = new PictureLoaderLocal(this);
pictureLoaderThread = new QThread;
pictureLoaderThread->start(QThread::LowPriority);
moveToThread(pictureLoaderThread);
connect(this, &PictureLoaderWorker::imageLoadEnqueued, this, &PictureLoaderWorker::handleImageLoadEnqueued);
connect(&requestTimer, &QTimer::timeout, this, &PictureLoaderWorker::processQueuedRequests);
requestTimer.setInterval(1000);
requestTimer.start();
@ -133,13 +137,38 @@ void PictureLoaderWorker::processQueuedRequests()
void PictureLoaderWorker::enqueueImageLoad(const CardInfoPtr &card)
{
auto worker = new PictureLoaderWorkerWork(this, card);
Q_UNUSED(worker);
// Send call through a connection to ensure the handling is run on the pictureLoader thread
emit imageLoadEnqueued(card);
}
void PictureLoaderWorker::imageLoadedSuccessfully(CardInfoPtr card, const QImage &image)
void PictureLoaderWorker::handleImageLoadEnqueued(const CardInfoPtr &card)
{
emit imageLoaded(std::move(card), image);
// deduplicate loads for the same card
if (currentlyLoading.contains(card)) {
qCDebug(PictureLoaderWorkerLog())
<< "Skipping enqueued" << card->getName() << "because it's already being loaded";
return;
}
currentlyLoading.insert(card);
// try to load image from local first
QImage image = localLoader->tryLoad(card);
if (!image.isNull()) {
imageLoadedSuccessfully(card, image);
} else {
// queue up to load image from remote only after local loading failed
new PictureLoaderWorkerWork(this, card);
}
}
/**
* Called when image loading is done
* Contrary to the name, this is called on both success and failure. Failures are indicated by an empty QImage.
*/
void PictureLoaderWorker::imageLoadedSuccessfully(const CardInfoPtr &card, const QImage &image)
{
currentlyLoading.remove(card);
emit imageLoaded(card, image);
}
void PictureLoaderWorker::cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl)

View file

@ -3,6 +3,7 @@
#include "../../../game/cards/card_database.h"
#include "../../../game/cards/card_info.h"
#include "picture_loader_local.h"
#include "picture_loader_worker_work.h"
#include "picture_to_load.h"
@ -37,7 +38,7 @@ public:
public slots:
QNetworkReply *makeRequest(const QUrl &url, PictureLoaderWorkerWork *workThread);
void processQueuedRequests();
void imageLoadedSuccessfully(CardInfoPtr card, const QImage &image);
void imageLoadedSuccessfully(const CardInfoPtr &card, const QImage &image);
private:
static QStringList md5Blacklist;
@ -48,18 +49,24 @@ private:
QHash<QUrl, QPair<QUrl, QDateTime>> redirectCache; // Stores redirect and timestamp
QString cacheFilePath; // Path to persistent storage
static constexpr int CacheTTLInDays = 30; // TODO: Make user configurable
PictureToLoad cardBeingDownloaded;
bool picDownload;
QQueue<QPair<QUrl, PictureLoaderWorkerWork *>> requestLoadQueue;
QTimer requestTimer; // Timer for processing delayed requests
PictureLoaderLocal *localLoader;
QSet<CardInfoPtr> currentlyLoading; // for deduplication purposes
void cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl);
QUrl getCachedRedirect(const QUrl &originalUrl) const;
void loadRedirectCache();
void saveRedirectCache() const;
void cleanStaleEntries();
private slots:
void handleImageLoadEnqueued(const CardInfoPtr &card);
signals:
void imageLoadEnqueued(const CardInfoPtr &card);
void imageLoaded(CardInfoPtr card, const QImage &image);
void imageLoadQueued(const QUrl &url, PictureLoaderWorkerWork *worker);
void imageLoadSuccessful(const QUrl &url, PictureLoaderWorkerWork *worker);

View file

@ -15,11 +15,8 @@
// Card back returned by gatherer when card is not found
QStringList PictureLoaderWorkerWork::md5Blacklist = QStringList() << "db0c48db407a907c16ade38de048a441";
PictureLoaderWorkerWork::PictureLoaderWorkerWork(PictureLoaderWorker *_worker, const CardInfoPtr &toLoad)
: QThread(nullptr), worker(_worker), cardToDownload(toLoad), picsPath(SettingsCache::instance().getPicsPath()),
customPicsPath(SettingsCache::instance().getCustomPicsPath()),
overrideAllCardArtWithPersonalPreference(SettingsCache::instance().getOverrideAllCardArtWithPersonalPreference())
PictureLoaderWorkerWork::PictureLoaderWorkerWork(const PictureLoaderWorker *worker, const CardInfoPtr &toLoad)
: QThread(nullptr), cardToDownload(toLoad)
{
// Hook up signals to the orchestrator
connect(this, &PictureLoaderWorkerWork::requestImageDownload, worker, &PictureLoaderWorker::queueRequest,
@ -28,15 +25,14 @@ PictureLoaderWorkerWork::PictureLoaderWorkerWork(PictureLoaderWorker *_worker, c
Qt::QueuedConnection);
// Hook up signals to settings
connect(&SettingsCache::instance(), SIGNAL(picsPathChanged()), this, SLOT(picsPathChanged()));
connect(&SettingsCache::instance(), SIGNAL(picDownloadChanged()), this, SLOT(picDownloadChanged()));
connect(&SettingsCache::instance(), &SettingsCache::overrideAllCardArtWithPersonalPreferenceChanged, this,
&PictureLoaderWorkerWork::setOverrideAllCardArtWithPersonalPreference);
pictureLoaderThread = new QThread;
pictureLoaderThread->start(QThread::LowPriority);
moveToThread(pictureLoaderThread);
startWork();
connect(pictureLoaderThread, &QThread::started, this, &PictureLoaderWorkerWork::startNextPicDownload);
pictureLoaderThread->start(QThread::LowPriority);
}
PictureLoaderWorkerWork::~PictureLoaderWorkerWork()
@ -44,99 +40,6 @@ PictureLoaderWorkerWork::~PictureLoaderWorkerWork()
pictureLoaderThread->deleteLater();
}
void PictureLoaderWorkerWork::startWork()
{
QString setName = cardToDownload.getSetName();
QString cardName = cardToDownload.getCard()->getName();
QString correctedCardName = cardToDownload.getCard()->getCorrectedName();
qCDebug(PictureLoaderWorkerLog).nospace()
<< "[card: " << cardName << " set: " << setName << "]: Trying to load picture";
// FIXME: This is a hack so that to keep old Cockatrice behavior
// (ignoring provider ID) when the "override all card art with personal
// preference" is set.
//
// Figure out a proper way to integrate the two systems at some point.
//
// Note: need to go through a member for
// overrideAllCardArtWithPersonalPreference as reading from the
// SettingsCache instance from the PictureLoaderWorker thread could
// cause race conditions.
//
// XXX: Reading from the CardDatabaseManager instance from the
// PictureLoaderWorker thread might not be safe either
bool searchCustomPics = overrideAllCardArtWithPersonalPreference ||
CardDatabaseManager::getInstance()->isProviderIdForPreferredPrinting(
cardName, cardToDownload.getCard()->getPixmapCacheKey());
if (!(searchCustomPics && cardImageExistsOnDisk(setName, correctedCardName, searchCustomPics))) {
startNextPicDownload();
}
}
bool PictureLoaderWorkerWork::cardImageExistsOnDisk(const QString &setName,
const QString &correctedCardName,
const bool searchCustomPics)
{
QImage image;
QImageReader imgReader;
imgReader.setDecideFormatFromContent(true);
QList<QString> picsPaths = QList<QString>();
if (searchCustomPics) {
QDirIterator it(customPicsPath, QDirIterator::Subdirectories | QDirIterator::FollowSymlinks);
// Recursively check all subdirectories of the CUSTOM folder
while (it.hasNext()) {
QString thisPath(it.next());
QFileInfo thisFileInfo(thisPath);
if (thisFileInfo.isFile() &&
(thisFileInfo.fileName() == correctedCardName || thisFileInfo.completeBaseName() == correctedCardName ||
thisFileInfo.baseName() == correctedCardName)) {
picsPaths << thisPath; // Card found in the CUSTOM directory, somewhere
}
}
}
if (!setName.isEmpty()) {
picsPaths << picsPath + "/" + setName + "/" + correctedCardName
// We no longer store downloaded images there, but don't just ignore
// stuff that old versions have put there.
<< picsPath + "/downloadedPics/" + setName + "/" + correctedCardName;
}
// Iterates through the list of paths, searching for images with the desired
// name with any QImageReader-supported
// extension
for (const auto &_picsPath : picsPaths) {
imgReader.setFileName(_picsPath);
if (imgReader.read(&image)) {
qCDebug(PictureLoaderWorkerLog).nospace()
<< "[card: " << correctedCardName << " set: " << setName << "]: Picture found on disk.";
imageLoaded(cardToDownload.getCard(), image);
return true;
}
imgReader.setFileName(_picsPath + ".full");
if (imgReader.read(&image)) {
qCDebug(PictureLoaderWorkerLog).nospace()
<< "[card: " << correctedCardName << " set: " << setName << "]: Picture.full found on disk.";
imageLoaded(cardToDownload.getCard(), image);
return true;
}
imgReader.setFileName(_picsPath + ".xlhq");
if (imgReader.read(&image)) {
qCDebug(PictureLoaderWorkerLog).nospace()
<< "[card: " << correctedCardName << " set: " << setName << "]: Picture.xlhq found on disk.";
imageLoaded(cardToDownload.getCard(), image);
return true;
}
}
qCDebug(PictureLoaderWorkerLog).nospace()
<< "[card: " << correctedCardName << " set: " << setName << "]: Picture NOT found on disk.";
return false;
}
void PictureLoaderWorkerWork::startNextPicDownload()
{
QString picUrl = cardToDownload.getCurrentUrl();
@ -280,18 +183,6 @@ void PictureLoaderWorkerWork::picDownloadChanged()
picDownload = SettingsCache::instance().getPicDownload();
}
void PictureLoaderWorkerWork::picsPathChanged()
{
picsPath = SettingsCache::instance().getPicsPath();
customPicsPath = SettingsCache::instance().getCustomPicsPath();
}
void PictureLoaderWorkerWork::setOverrideAllCardArtWithPersonalPreference(
bool _overrideAllCardArtWithPersonalPreference)
{
overrideAllCardArtWithPersonalPreference = _overrideAllCardArtWithPersonalPreference;
}
bool PictureLoaderWorkerWork::imageIsBlackListed(const QByteArray &picData)
{
QString md5sum = QCryptographicHash::hash(picData, QCryptographicHash::Md5).toHex();

View file

@ -24,10 +24,9 @@ class PictureLoaderWorkerWork : public QThread
{
Q_OBJECT
public:
explicit PictureLoaderWorkerWork(PictureLoaderWorker *worker, const CardInfoPtr &toLoad);
explicit PictureLoaderWorkerWork(const PictureLoaderWorker *worker, const CardInfoPtr &toLoad);
~PictureLoaderWorkerWork() override;
void startWork();
PictureLoaderWorker *worker;
PictureToLoad cardToDownload;
public slots:
@ -35,21 +34,16 @@ public slots:
void picDownloadFailed();
private:
QString picsPath, customPicsPath;
bool overrideAllCardArtWithPersonalPreference;
static QStringList md5Blacklist;
QThread *pictureLoaderThread;
QNetworkAccessManager *networkManager;
bool picDownload, downloadRunning, loadQueueRunning;
void startNextPicDownload();
bool cardImageExistsOnDisk(const QString &setName, const QString &correctedCardName, bool searchCustomPics);
bool imageIsBlackListed(const QByteArray &);
private slots:
void picDownloadChanged();
void picsPathChanged();
void setOverrideAllCardArtWithPersonalPreference(bool _overrideAllCardArtWithPersonalPreference);
signals:
void startLoadQueue();

View file

@ -13,35 +13,49 @@ PictureToLoad::PictureToLoad(CardInfoPtr _card)
: card(std::move(_card)), urlTemplates(SettingsCache::instance().downloads().getAllURLs())
{
if (card) {
for (const auto &cardInfoPerSetList : card->getSets()) {
for (const auto &set : cardInfoPerSetList) {
sortedSets << set.getPtr();
}
}
if (sortedSets.empty()) {
sortedSets << CardSet::newInstance("", "", "", QDate());
}
std::sort(sortedSets.begin(), sortedSets.end(), SetPriorityComparator());
// If the user hasn't disabled arts other than their personal preference...
if (!SettingsCache::instance().getOverrideAllCardArtWithPersonalPreference()) {
// If the pixmapCacheKey corresponds to a specific set, we have to try to load it first.
for (const auto &cardInfoPerSetList : card->getSets()) {
for (const auto &set : cardInfoPerSetList) {
if (QLatin1String("card_") + card->getName() + QString("_") + QString(set.getProperty("uuid")) ==
card->getPixmapCacheKey()) {
long long setIndex = sortedSets.indexOf(set.getPtr());
CardSetPtr setForCardProviderID = sortedSets.takeAt(setIndex);
sortedSets.prepend(setForCardProviderID);
}
}
}
}
sortedSets = extractSetsSorted(card);
// The first time called, nextSet will also populate the Urls for the first set.
nextSet();
}
}
/**
* Extracts a list of all the sets from the card, sorted in priority order.
* If the card does not contain any sets, then a dummy set will be inserted into the list.
*
* @return A list of sets. Will not be empty.
*/
QList<CardSetPtr> PictureToLoad::extractSetsSorted(const CardInfoPtr &card)
{
QList<CardSetPtr> sortedSets;
for (const auto &cardInfoPerSetList : card->getSets()) {
for (const auto &set : cardInfoPerSetList) {
sortedSets << set.getPtr();
}
}
if (sortedSets.empty()) {
sortedSets << CardSet::newInstance("", "", "", QDate());
}
std::sort(sortedSets.begin(), sortedSets.end(), SetPriorityComparator());
// If the user hasn't disabled arts other than their personal preference...
if (!SettingsCache::instance().getOverrideAllCardArtWithPersonalPreference()) {
// If the pixmapCacheKey corresponds to a specific set, we have to try to load it first.
for (const auto &cardInfoPerSetList : card->getSets()) {
for (const auto &set : cardInfoPerSetList) {
if (QLatin1String("card_") + card->getName() + QString("_") + QString(set.getProperty("uuid")) ==
card->getPixmapCacheKey()) {
long long setIndex = sortedSets.indexOf(set.getPtr());
CardSetPtr setForCardProviderID = sortedSets.takeAt(setIndex);
sortedSets.prepend(setForCardProviderID);
}
}
}
}
return sortedSets;
}
void PictureToLoad::populateSetUrls()
{
/* currentSetUrls is a list, populated each time a new set is requested for a particular card

View file

@ -41,6 +41,8 @@ public:
bool nextSet();
bool nextUrl();
void populateSetUrls();
static QList<CardSetPtr> extractSetsSorted(const CardInfoPtr &card);
};
#endif // PICTURE_TO_LOAD_H

View file

@ -21,6 +21,7 @@ set(oracle_SOURCES
../cockatrice/src/game/cards/card_database_manager.cpp
../cockatrice/src/game/cards/card_info.cpp
../cockatrice/src/client/ui/picture_loader/picture_loader.cpp
../cockatrice/src/client/ui/picture_loader/picture_loader_local.cpp
../cockatrice/src/client/ui/picture_loader/picture_loader_request_status_display_widget.cpp
../cockatrice/src/client/ui/picture_loader/picture_loader_status_bar.cpp
../cockatrice/src/client/ui/picture_loader/picture_loader_worker.cpp