From 0b9b39fef7a8ee307896a86bac47480e1c5863cf Mon Sep 17 00:00:00 2001 From: RickyRister <42636155+RickyRister@users.noreply.github.com> Date: Sat, 5 Jul 2025 19:42:54 -0700 Subject: [PATCH] [PictureLoader] Reduce downtime between load attempts (#6020) * [PictureLoader] Reduce downtime between load attempts * rename some stuff * better comments * Fix segfault from status bar Pass just the relevant data through the signals to the status bar, instead of passing the entire Work object. That way the data is detached from the Work object and we won't segfault when Work self-deletes before status bar tries to use that data. * Rename method --- ...e_loader_request_status_display_widget.cpp | 23 ++++++----- ...ure_loader_request_status_display_widget.h | 6 ++- .../picture_loader_status_bar.cpp | 7 ++-- .../picture_loader_status_bar.h | 4 +- .../picture_loader/picture_loader_worker.cpp | 39 +++++++++++++------ .../ui/picture_loader/picture_loader_worker.h | 13 ++++--- .../picture_loader_worker_work.cpp | 2 +- 7 files changed, 56 insertions(+), 38 deletions(-) diff --git a/cockatrice/src/client/ui/picture_loader/picture_loader_request_status_display_widget.cpp b/cockatrice/src/client/ui/picture_loader/picture_loader_request_status_display_widget.cpp index 3647eb71e..a792087d5 100644 --- a/cockatrice/src/client/ui/picture_loader/picture_loader_request_status_display_widget.cpp +++ b/cockatrice/src/client/ui/picture_loader/picture_loader_request_status_display_widget.cpp @@ -2,23 +2,22 @@ PictureLoaderRequestStatusDisplayWidget::PictureLoaderRequestStatusDisplayWidget(QWidget *parent, const QUrl &_url, - PictureLoaderWorkerWork *worker) + const CardInfoPtr &card, + const QString &setName) : QWidget(parent) { layout = new QHBoxLayout(this); - if (worker->cardToDownload.getCard()) { - name = new QLabel(this); - name->setText(worker->cardToDownload.getCard()->getName()); - setShortname = new QLabel(this); - setShortname->setText(worker->cardToDownload.getSetName()); - providerId = new QLabel(this); - providerId->setText(worker->cardToDownload.getCard()->getProperty("uuid")); + name = new QLabel(this); + name->setText(card->getName()); + setShortname = new QLabel(this); + setShortname->setText(setName); + providerId = new QLabel(this); + providerId->setText(card->getProperty("uuid")); - layout->addWidget(name); - layout->addWidget(setShortname); - layout->addWidget(providerId); - } + layout->addWidget(name); + layout->addWidget(setShortname); + layout->addWidget(providerId); startTime = new QLabel(QDateTime::currentDateTime().toString(), this); elapsedTime = new QLabel("0", this); diff --git a/cockatrice/src/client/ui/picture_loader/picture_loader_request_status_display_widget.h b/cockatrice/src/client/ui/picture_loader/picture_loader_request_status_display_widget.h index 01eee1b48..46a809e47 100644 --- a/cockatrice/src/client/ui/picture_loader/picture_loader_request_status_display_widget.h +++ b/cockatrice/src/client/ui/picture_loader/picture_loader_request_status_display_widget.h @@ -10,8 +10,10 @@ class PictureLoaderRequestStatusDisplayWidget : public QWidget { Q_OBJECT public: - PictureLoaderRequestStatusDisplayWidget(QWidget *parent, const QUrl &url, PictureLoaderWorkerWork *worker); - PictureLoaderWorkerWork *worker; + PictureLoaderRequestStatusDisplayWidget(QWidget *parent, + const QUrl &url, + const CardInfoPtr &card, + const QString &setName); void setFinished() { diff --git a/cockatrice/src/client/ui/picture_loader/picture_loader_status_bar.cpp b/cockatrice/src/client/ui/picture_loader/picture_loader_status_bar.cpp index 67b50a1f2..a2beeac1f 100644 --- a/cockatrice/src/client/ui/picture_loader/picture_loader_status_bar.cpp +++ b/cockatrice/src/client/ui/picture_loader/picture_loader_status_bar.cpp @@ -38,15 +38,14 @@ void PictureLoaderStatusBar::cleanOldEntries() } } -void PictureLoaderStatusBar::addQueuedImageLoad(const QUrl &url, PictureLoaderWorkerWork *worker) +void PictureLoaderStatusBar::addQueuedImageLoad(const QUrl &url, const CardInfoPtr &card, const QString &setName) { - loadLog->addSettingsWidget(new PictureLoaderRequestStatusDisplayWidget(loadLog, url, worker)); + loadLog->addSettingsWidget(new PictureLoaderRequestStatusDisplayWidget(loadLog, url, card, setName)); progressBar->setMaximum(progressBar->maximum() + 1); } -void PictureLoaderStatusBar::addSuccessfulImageLoad(const QUrl &url, PictureLoaderWorkerWork *worker) +void PictureLoaderStatusBar::addSuccessfulImageLoad(const QUrl &url) { - Q_UNUSED(worker) progressBar->setValue(progressBar->value() + 1); for (PictureLoaderRequestStatusDisplayWidget *statusDisplayWidget : loadLog->popup->findChildren()) { diff --git a/cockatrice/src/client/ui/picture_loader/picture_loader_status_bar.h b/cockatrice/src/client/ui/picture_loader/picture_loader_status_bar.h index 6a507cbb0..6f5111a34 100644 --- a/cockatrice/src/client/ui/picture_loader/picture_loader_status_bar.h +++ b/cockatrice/src/client/ui/picture_loader/picture_loader_status_bar.h @@ -15,8 +15,8 @@ public: explicit PictureLoaderStatusBar(QWidget *parent); public slots: - void addQueuedImageLoad(const QUrl &url, PictureLoaderWorkerWork *worker); - void addSuccessfulImageLoad(const QUrl &url, PictureLoaderWorkerWork *worker); + void addQueuedImageLoad(const QUrl &url, const CardInfoPtr &card, const QString &setName); + void addSuccessfulImageLoad(const QUrl &url); void cleanOldEntries(); private: diff --git a/cockatrice/src/client/ui/picture_loader/picture_loader_worker.cpp b/cockatrice/src/client/ui/picture_loader/picture_loader_worker.cpp index b0ab0b4c0..c7378fb27 100644 --- a/cockatrice/src/client/ui/picture_loader/picture_loader_worker.cpp +++ b/cockatrice/src/client/ui/picture_loader/picture_loader_worker.cpp @@ -12,7 +12,10 @@ #include #include -PictureLoaderWorker::PictureLoaderWorker() : QObject(nullptr), picDownload(SettingsCache::instance().getPicDownload()) +static constexpr int MAX_REQUESTS_PER_SEC = 10; + +PictureLoaderWorker::PictureLoaderWorker() + : QObject(nullptr), picDownload(SettingsCache::instance().getPicDownload()), requestQuota(MAX_REQUESTS_PER_SEC) { networkManager = new QNetworkAccessManager(this); // We need a timeout to ensure requests don't hang indefinitely in case of @@ -48,7 +51,7 @@ PictureLoaderWorker::PictureLoaderWorker() : QObject(nullptr), picDownload(Setti connect(this, &PictureLoaderWorker::imageLoadEnqueued, this, &PictureLoaderWorker::handleImageLoadEnqueued); - connect(&requestTimer, &QTimer::timeout, this, &PictureLoaderWorker::processQueuedRequests); + connect(&requestTimer, &QTimer::timeout, this, &PictureLoaderWorker::resetRequestQuota); requestTimer.setInterval(1000); requestTimer.start(); } @@ -69,7 +72,8 @@ void PictureLoaderWorker::queueRequest(const QUrl &url, PictureLoaderWorkerWork makeRequest(url, worker); } else { requestLoadQueue.append(qMakePair(url, worker)); - emit imageLoadQueued(url, worker); + emit imageLoadQueued(url, worker->cardToDownload.getCard(), worker->cardToDownload.getSetName()); + processQueuedRequests(); } } @@ -78,7 +82,7 @@ QNetworkReply *PictureLoaderWorker::makeRequest(const QUrl &url, PictureLoaderWo // Check for cached redirects QUrl cachedRedirect = getCachedRedirect(url); if (!cachedRedirect.isEmpty()) { - emit imageLoadSuccessful(url, worker); + emit imageLoadSuccessful(url); return makeRequest(cachedRedirect, worker); } @@ -95,22 +99,34 @@ QNetworkReply *PictureLoaderWorker::makeRequest(const QUrl &url, PictureLoaderWo return reply; } +void PictureLoaderWorker::resetRequestQuota() +{ + requestQuota = MAX_REQUESTS_PER_SEC; + processQueuedRequests(); +} + +/** + * Keeps processing requests from the queue until it is empty or until the quota runs out. + */ void PictureLoaderWorker::processQueuedRequests() { - for (int i = 0; i < 10; i++) { - processSingleRequest(); + while (requestQuota > 0 && processSingleRequest()) { + --requestQuota; } } /** - * Immediately processes a single queued request + * Immediately processes a single queued request. No-ops if the load queue is empty + * @return If a request was processed */ -void PictureLoaderWorker::processSingleRequest() +bool PictureLoaderWorker::processSingleRequest() { if (!requestLoadQueue.isEmpty()) { auto request = requestLoadQueue.takeFirst(); makeRequest(request.first, request.second); + return true; } + return false; } void PictureLoaderWorker::enqueueImageLoad(const CardInfoPtr &card) @@ -132,7 +148,7 @@ void PictureLoaderWorker::handleImageLoadEnqueued(const CardInfoPtr &card) // try to load image from local first QImage image = localLoader->tryLoad(card); if (!image.isNull()) { - imageLoadedSuccessfully(card, image); + handleImageLoaded(card, image); } else { // queue up to load image from remote only after local loading failed new PictureLoaderWorkerWork(this, card); @@ -140,10 +156,9 @@ void PictureLoaderWorker::handleImageLoadEnqueued(const CardInfoPtr &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. + * Called when image loading is done. Failures are indicated by an empty QImage. */ -void PictureLoaderWorker::imageLoadedSuccessfully(const CardInfoPtr &card, const QImage &image) +void PictureLoaderWorker::handleImageLoaded(const CardInfoPtr &card, const QImage &image) { currentlyLoading.remove(card); emit imageLoaded(card, image); diff --git a/cockatrice/src/client/ui/picture_loader/picture_loader_worker.h b/cockatrice/src/client/ui/picture_loader/picture_loader_worker.h index 2f0a737ad..f5b0a3507 100644 --- a/cockatrice/src/client/ui/picture_loader/picture_loader_worker.h +++ b/cockatrice/src/client/ui/picture_loader/picture_loader_worker.h @@ -38,8 +38,8 @@ public: public slots: QNetworkReply *makeRequest(const QUrl &url, PictureLoaderWorkerWork *workThread); void processQueuedRequests(); - void processSingleRequest(); - void imageLoadedSuccessfully(const CardInfoPtr &card, const QImage &image); + bool processSingleRequest(); + void handleImageLoaded(const CardInfoPtr &card, const QImage &image); void cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl); void removedCachedUrl(const QUrl &url); @@ -52,7 +52,9 @@ private: static constexpr int CacheTTLInDays = 30; // TODO: Make user configurable bool picDownload; QQueue> requestLoadQueue; - QTimer requestTimer; // Timer for processing delayed requests + + int requestQuota; + QTimer requestTimer; // Timer for refreshing request quota PictureLoaderLocal *localLoader; QSet currentlyLoading; // for deduplication purposes @@ -63,13 +65,14 @@ private: void cleanStaleEntries(); private slots: + void resetRequestQuota(); 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); + void imageLoadQueued(const QUrl &url, const CardInfoPtr &card, const QString &setName); + void imageLoadSuccessful(const QUrl &url); }; #endif // PICTURE_LOADER_WORKER_H diff --git a/cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.cpp b/cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.cpp index 712f68dc1..38c8f14cb 100644 --- a/cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.cpp +++ b/cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.cpp @@ -22,7 +22,7 @@ PictureLoaderWorkerWork::PictureLoaderWorkerWork(const PictureLoaderWorker *work connect(this, &PictureLoaderWorkerWork::requestImageDownload, worker, &PictureLoaderWorker::queueRequest); connect(this, &PictureLoaderWorkerWork::urlRedirected, worker, &PictureLoaderWorker::cacheRedirect); connect(this, &PictureLoaderWorkerWork::cachedUrlInvalidated, worker, &PictureLoaderWorker::removedCachedUrl); - connect(this, &PictureLoaderWorkerWork::imageLoaded, worker, &PictureLoaderWorker::imageLoadedSuccessfully); + connect(this, &PictureLoaderWorkerWork::imageLoaded, worker, &PictureLoaderWorker::handleImageLoaded); // Hook up signals to settings connect(&SettingsCache::instance(), SIGNAL(picDownloadChanged()), this, SLOT(picDownloadChanged()));