diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_image_formats.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_image_formats.h new file mode 100644 index 000000000..139cdabbf --- /dev/null +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_image_formats.h @@ -0,0 +1,69 @@ +#ifndef COCKATRICE_CARD_PICTURE_LOADER_IMAGE_FORMATS_H +#define COCKATRICE_CARD_PICTURE_LOADER_IMAGE_FORMATS_H + +#include +#include +#include +#include +#include +#include +#include +#include + +namespace CardPictureLoaderImageFormats +{ +inline QByteArray acceptHeader() +{ + // Decoder plugins differ across distributions. Prefer only formats this + // runtime can read, then retain valid wildcards for other image endpoints. + static const QByteArray header = [] { + const auto supported = QImageReader::supportedMimeTypes(); + QByteArrayList types; + for (const QByteArray &preferred : + {QByteArray("image/avif"), QByteArray("image/webp"), QByteArray("image/apng"), QByteArray("image/png"), + QByteArray("image/jpeg")}) { + if (supported.contains(preferred)) { + types.append(preferred); + } + } + for (const QByteArray &type : supported) { + if (type.startsWith("image/") && !types.contains(type)) { + types.append(type); + } + } + types.append("image/*;q=0.5"); + types.append("*/*;q=0.1"); + return types.join(','); + }(); + return header; +} + +struct DecodedImage +{ + QImage image; + QString error; +}; + +inline DecodedImage readImage(QIODevice *device) +{ + static constexpr int riffHeaderSize = 12; // RIFF_HEADER_SIZE from webp/format_constants.h + const auto header = device->peek(riffHeaderSize); + if (header.startsWith("RIFF") && header.endsWith("WEBP")) { + // Preserve the existing first-frame behavior for animated WebP files. + QBuffer buffer; + buffer.setData(device->readAll()); + QMovie movie(&buffer); + movie.start(); + movie.stop(); + return {movie.currentImage(), movie.lastErrorString()}; + } + + QImageReader reader; + reader.setDecideFormatFromContent(true); + reader.setDevice(device); + QImage image = reader.read(); + return {image, image.isNull() ? reader.errorString() : QString()}; +} +} // namespace CardPictureLoaderImageFormats + +#endif // COCKATRICE_CARD_PICTURE_LOADER_IMAGE_FORMATS_H diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 3ab5eb684..96e859906 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -2,11 +2,13 @@ #include "../../client/settings/cache_settings.h" #include "card_picture_loader_cache_method.h" +#include "card_picture_loader_image_formats.h" #include "card_picture_loader_local.h" #include "card_picture_loader_worker_work.h" #include "card_picture_to_load.h" #include "libcockatrice/card/printing/exact_card.h" #include "libcockatrice/utility/server_rate_limiter.h" +#include "redirect_chain.h" #include #include @@ -100,6 +102,22 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() connect(&SettingsCache::instance().downloads(), &DownloadSettings::hostRequestLimitsChanged, this, [this] { hostRequestLimits = SettingsCache::instance().downloads().getHostRequestLimits(); }); + connect(&SettingsCache::instance().downloads(), &DownloadSettings::picDownloadChanged, this, [this] { + picDownload = SettingsCache::instance().downloads().getPicDownload(); + if (!picDownload) { + // Waiting for network quota or 429 recovery cannot help a cache-only lookup. + // Retire cancelled status entries just as the normal dispatcher does. + while (!requestLoadQueue.isEmpty()) { + const auto entry = requestLoadQueue.takeFirst(); + if (entry.second) { + makeRequest(entry.first, entry.second); + } else { + emit imageRequestSucceeded(entry.first); + } + } + updateTimerState(); + } + }); } CardPictureLoaderWorker::~CardPictureLoaderWorker() @@ -136,52 +154,49 @@ QThread *CardPictureLoaderWorker::workerThread() const void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker) { - QUrl cachedRedirect = getCachedRedirect(url); - if (!cachedRedirect.isEmpty()) { - queueRequest(cachedRedirect, worker); - return; - } - if (static_cast( - SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == - CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && - cache->metaData(url).isValid()) { + const QUrl resolvedUrl = resolveCachedRedirect(url); + if (!picDownload || (static_cast( + SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == + CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && + cache->metaData(resolvedUrl).isValid() && !worker->shouldBypassCache(resolvedUrl))) { // A request that will be served from the disk cache never touches the network and therefore // doesn't use up any of the rate limit, so it gets to skip the queue. - makeRequest(url, worker); + // Every reply retires a status, including online cache hits and deferred + // 429 retries. Pair that completion with this request before dispatch. + emit imageRequestQueued(resolvedUrl, worker->cardToDownload.getCard(), worker->cardToDownload.getSetName()); + makeRequest(resolvedUrl, worker); return; } - requestLoadQueue.append(qMakePair(url, worker)); - emit imageRequestQueued(url, worker->cardToDownload.getCard(), worker->cardToDownload.getSetName()); + requestLoadQueue.append(qMakePair(resolvedUrl, QPointer(worker))); + emit imageRequestQueued(resolvedUrl, worker->cardToDownload.getCard(), worker->cardToDownload.getSetName()); processQueuedRequests(); } QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker) { - // Check for cached redirects - QUrl cachedRedirect = getCachedRedirect(url); - if (!cachedRedirect.isEmpty()) { - // The status bar still needs to reclaim this URL's widget even when we hand the request back - // for a deferred retry instead of dispatching it onto the network. + // Redirect mappings can be learned while a request waits in the queue. + // Resolve again without recursion, and honor the target host's backoff. + const QUrl resolvedUrl = resolveCachedRedirect(url); + if (resolvedUrl != url) { emit imageRequestSucceeded(url); - // The redirect target is a different host, which may itself be in 429 backoff; hand the - // entry back to its worker so it waits the backoff out instead of dispatching straight - // onto the backed-off host. - if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(cachedRedirect.host(), - QDateTime::currentDateTime())) { - worker->scheduleDeferredRetry(cachedRedirect.host()); + if (requestTouchesNetwork(resolvedUrl, worker) && CardPictureLoaderWorkerWork::rateLimiter().isRateLimited( + resolvedUrl.host(), QDateTime::currentDateTime())) { + worker->scheduleDeferredRetry(resolvedUrl.host()); return nullptr; } - return makeRequest(cachedRedirect, worker); + // The original status was retired above; track the resolved request + // separately so its eventual completion has a matching queued entry. + emit imageRequestQueued(resolvedUrl, worker->cardToDownload.getCard(), worker->cardToDownload.getSetName()); } - QNetworkRequest req(url); + QNetworkRequest req(resolvedUrl); req.setHeader(QNetworkRequest::UserAgentHeader, QString("Cockatrice %1").arg(VERSION_STRING)); - req.setRawHeader("Accept", "image/avif,image/webp,image/apng,image/,/*;q=0.8"); + req.setRawHeader("Accept", CardPictureLoaderImageFormats::acceptHeader()); // Cached entries are served straight from the disk cache even when picture downloads are // enabled: re-fetching an already-cached image would burn the rate limit for nothing. Only a // genuine cache miss goes to the network, and only when downloads are enabled. - bool useNetworkCache = !requestTouchesNetwork(url); + bool useNetworkCache = !requestTouchesNetwork(resolvedUrl, worker); req.setAttribute(QNetworkRequest::CacheLoadControlAttribute, useNetworkCache ? QNetworkRequest::AlwaysCache : QNetworkRequest::AlwaysNetwork); @@ -190,7 +205,7 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture // Track in-flight replies per host so the unlocked fast path can bound how many requests it // issues at once, instead of creating replies that time out before Qt opens a connection. - const QString host = url.host(); + const QString host = resolvedUrl.host(); hostInFlight.insert(host, hostInFlight.value(host) + 1); // Release the in-flight slot when the reply is destroyed, not when it emits `finished`, and use @@ -201,8 +216,22 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture connect(reply, &QObject::destroyed, this, [this, host] { hostInFlight.insert(host, qMax(0, hostInFlight.value(host) - 1)); }); + // Retire the status even if the work object is destroyed before the reply + // finishes. Connect this before the work callback, which may enqueue a retry. + connect(reply, &QNetworkReply::finished, this, [this, reply] { emit imageRequestSucceeded(reply->url()); }); + // Connect reply handling; the work object is the context so its handler dies with it. - connect(reply, &QNetworkReply::finished, worker, [worker, reply] { worker->handleNetworkReply(reply); }); + const QPointer guardedWorker(worker); + connect(reply, &QNetworkReply::finished, worker, [guardedWorker, reply] { + if (guardedWorker) { + guardedWorker->handleNetworkReply(reply); + } + }); + // A work item may be cancelled before the reply finishes. In that case its + // context-bound handler is disconnected, but the reply must still release + // the host's in-flight slot and any network resources. + connect(worker, &QObject::destroyed, reply, [reply] { reply->abort(); }); + connect(reply, &QNetworkReply::finished, reply, &QObject::deleteLater); return reply; } @@ -269,19 +298,25 @@ void CardPictureLoaderWorker::dispatchQueuedRequest() // connection for them. for (int i = 0; i < requestLoadQueue.size();) { const auto &request = requestLoadQueue.at(i); + if (!request.second) { + const auto cancelled = requestLoadQueue.takeAt(i); + emit imageRequestSucceeded(cancelled.first); + continue; + } // Dispatch decisions must key on the host the request will actually go to, not the URL that // merely redirects to it: a redirect learned after this URL was queued would otherwise // bypass the in-flight cap and drain the whole queue onto the target host unchecked. const QUrl resolvedUrl = resolveCachedRedirect(request.first); const QString host = resolvedUrl.host(); if (isUnlockedHost(host)) { - if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { + if (requestTouchesNetwork(resolvedUrl, request.second) && + CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { ++i; continue; } if (hostInFlight.value(host) < MAX_IN_FLIGHT_PER_HOST) { - makeRequest(resolvedUrl, request.second); - requestLoadQueue.removeAt(i); + const auto entry = requestLoadQueue.takeAt(i); + makeRequest(entry.first, entry.second); dispatched = true; continue; } @@ -347,6 +382,11 @@ bool CardPictureLoaderWorker::processSingleRequest() for (int i = 0; i < requestLoadQueue.size(); ++i) { // Copy the entry: takeAt(i) below erases within the list this reference points into. const auto request = requestLoadQueue.at(i); + if (!request.second) { + requestLoadQueue.removeAt(i--); + emit imageRequestSucceeded(request.first); + continue; + } // Resolve cached redirects so the rate-limit and allowance arithmetic keys on the host the // request will actually hit (see resolveCachedRedirect). const QUrl resolvedUrl = resolveCachedRedirect(request.first); @@ -356,13 +396,14 @@ bool CardPictureLoaderWorker::processSingleRequest() // instead of leaving it parked in the queue with no reply pending. Only applies to // requests that will actually touch the network: one that will be served from the disk // cache costs nothing and shouldn't wait out the 429. - if (requestTouchesNetwork(request.first) && + if (requestTouchesNetwork(resolvedUrl, request.second) && CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { // The queued URL is usually a cached-redirect target whose host differs from // cardToDownload.getCurrentUrl(), so scheduleDeferredRetry() (which waits out the // blocked host's deadline) is used instead of startNextPicDownload() looping on the // original host. auto entry = requestLoadQueue.takeAt(i); + emit imageRequestSucceeded(entry.first); if (host != entry.first.host()) { // A cached redirect target is what is blocked, which the work object would not // discover from its own URL; wait out that specific host (with jitter) instead. @@ -393,15 +434,21 @@ bool CardPictureLoaderWorker::processSingleRequest() if (allowance > 0) { hostQuotaRemaining.insert(host, allowance - 1); auto entry = requestLoadQueue.takeAt(i); - makeRequest(resolvedUrl, entry.second); + makeRequest(entry.first, entry.second); return true; } } return false; } -bool CardPictureLoaderWorker::requestTouchesNetwork(const QUrl &url) const +bool CardPictureLoaderWorker::requestTouchesNetwork(const QUrl &url, const CardPictureLoaderWorkerWork *worker) const { + if (!picDownload) { + return false; + } + if (worker->shouldBypassCache(url)) { + return true; + } bool useNetworkCache = static_cast( SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && @@ -477,33 +524,19 @@ void CardPictureLoaderWorker::cacheRedirect(const QUrl &originalUrl, const QUrl // saveRedirectCache(); } +void CardPictureLoaderWorker::invalidateRedirect(const QUrl &url) +{ + redirectCache.remove(url); +} + void CardPictureLoaderWorker::removedCachedUrl(const QUrl &url) { networkManager->cache()->remove(url); } -QUrl CardPictureLoaderWorker::getCachedRedirect(const QUrl &originalUrl) const +QUrl CardPictureLoaderWorker::resolveCachedRedirect(const QUrl &url) { - if (redirectCache.contains(originalUrl)) { - return redirectCache[originalUrl].first; - } - return {}; -} - -QUrl CardPictureLoaderWorker::resolveCachedRedirect(const QUrl &url) const -{ - // Follow the whole cached-redirect chain so dispatch keys on the host that is really hit. The - // depth bound keeps a corrupt or self-referencing cache entry from spinning us forever. - QUrl resolved = url; - int depth = 0; - while (depth++ < MAX_REDIRECT_CHAIN_DEPTH) { - QUrl target = getCachedRedirect(resolved); - if (target.isEmpty() || target == resolved) { - break; - } - resolved = target; - } - return resolved; + return resolveImageRedirects(url, redirectCache, MAX_REDIRECT_CHAIN_DEPTH); } void CardPictureLoaderWorker::loadRedirectCache() diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index 4b42c6e59..1b3f63e59 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -139,6 +140,9 @@ public slots: /** @brief Caches a redirect mapping between original and redirected URL. */ void cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl); + /** @brief Removes a stale redirect mapping. */ + void invalidateRedirect(const QUrl &url); + /** @brief Removes a URL from the network cache. */ void removedCachedUrl(const QUrl &url); @@ -150,7 +154,7 @@ private: QString cacheFilePath; ///< Path to persistent redirect cache file static constexpr int CacheTTLInDays = 30; ///< Time-to-live for redirect cache entries (days) bool picDownload; ///< Whether downloading images from network is enabled - QQueue> requestLoadQueue; ///< Queue of pending network requests + QQueue>> requestLoadQueue; ///< Queue of pending network requests QTimer requestTimer; ///< Timer to reset the request quota QTimer dispatchTimer; ///< Timer pacing individual network requests @@ -185,12 +189,9 @@ private: */ [[nodiscard]] bool isUnlockedHost(const QString &host) const; - /** @brief Returns cached redirect URL for the given original URL, if available. */ - [[nodiscard]] QUrl getCachedRedirect(const QUrl &originalUrl) const; - /** @brief Whether a request for this URL would actually touch the network, rather than being served from the disk * cache. */ - [[nodiscard]] bool requestTouchesNetwork(const QUrl &url) const; + [[nodiscard]] bool requestTouchesNetwork(const QUrl &url, const CardPictureLoaderWorkerWork *worker) const; /** * @brief Follows the cached-redirect chain to the URL that will actually be requested. @@ -200,7 +201,7 @@ private: * Dispatch decisions (unlocked-host fast path, 429 backoff, in-flight cap) must key on the host * a request really goes to, not the URL that merely redirects to it. */ - [[nodiscard]] QUrl resolveCachedRedirect(const QUrl &url) const; + [[nodiscard]] QUrl resolveCachedRedirect(const QUrl &url); /** @brief Loads redirect cache from disk. */ void loadRedirectCache(); diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp index 92524384a..f0f2dc86a 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp @@ -1,22 +1,20 @@ #include "card_picture_loader_worker_work.h" #include "../../client/settings/cache_settings.h" +#include "card_picture_loader_image_formats.h" #include "card_picture_loader_worker.h" #include "card_picture_to_load.h" #include "libcockatrice/card/card_info.h" #include "libcockatrice/card/printing/exact_card.h" #include "libcockatrice/utility/server_rate_limiter.h" -#include #include #include #include #include -#include #include #include #include -#include #include #include #include @@ -25,6 +23,8 @@ #include #include #include +#include +#include ServerRateLimiter CardPictureLoaderWorkerWork::s_rateLimiter; #include @@ -50,23 +50,32 @@ CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(CardPictureLoaderWorker // Hook up signals to the orchestrator connect(this, &CardPictureLoaderWorkerWork::requestImageDownload, worker, &CardPictureLoaderWorker::queueRequest); connect(this, &CardPictureLoaderWorkerWork::urlRedirected, worker, &CardPictureLoaderWorker::cacheRedirect); + connect(this, &CardPictureLoaderWorkerWork::redirectInvalidated, worker, + &CardPictureLoaderWorker::invalidateRedirect); connect(this, &CardPictureLoaderWorkerWork::cachedUrlInvalidated, worker, &CardPictureLoaderWorker::removedCachedUrl); connect(this, &CardPictureLoaderWorkerWork::imageLoaded, worker, &CardPictureLoaderWorker::handleImageLoaded); - connect(this, &CardPictureLoaderWorkerWork::requestSucceeded, worker, - &CardPictureLoaderWorker::imageRequestSucceeded); connect(this, &CardPictureLoaderWorkerWork::rateLimited, worker, &CardPictureLoaderWorker::onHostRateLimited); // Hook up signals to settings connect(&SettingsCache::instance().downloads(), SIGNAL(picDownloadChanged()), this, SLOT(picDownloadChanged())); + deferredRetry = new QTimer(this); + deferredRetry->setSingleShot(true); + connect(deferredRetry, &QTimer::timeout, this, [this] { + s_rateLimiter.clearExpired(QDateTime::currentDateTime()); + cardToDownload.resetIndices(); + startNextPicDownload(); + }); + startNextPicDownload(); } void CardPictureLoaderWorkerWork::startNextPicDownload() { + redirectVisited.clear(); QDateTime now = QDateTime::currentDateTime(); - while (!cardToDownload.getCurrentUrl().isEmpty() && + while (picDownload && !cardToDownload.getCurrentUrl().isEmpty() && s_rateLimiter.isRateLimited(QUrl(cardToDownload.getCurrentUrl()).host(), now)) { QString host = QUrl(cardToDownload.getCurrentUrl()).host(); if (s_rateLimiter.rounds(host) == 1) { @@ -96,7 +105,11 @@ void CardPictureLoaderWorkerWork::startNextPicDownload() QString picUrl = cardToDownload.getCurrentUrl(); if (picUrl.isEmpty()) { - scheduleDeferredRetry(); + if (picDownload) { + scheduleDeferredRetry(); + } else { + concludeImageLoad(QImage()); + } } else { QUrl url(picUrl); qCDebug(CardPictureLoaderWorkerWorkLog).nospace() @@ -127,27 +140,38 @@ void CardPictureLoaderWorkerWork::picDownloadFailed() void CardPictureLoaderWorkerWork::handleNetworkReply(QNetworkReply *reply) { - QVariant redirectTarget = reply->attribute(QNetworkRequest::RedirectionTargetAttribute); - bool redirectFailure = false; + const QVariant redirectTarget = reply->attribute(QNetworkRequest::RedirectionTargetAttribute); + const int statusCode = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); + const bool isRedirect = + statusCode == 301 || statusCode == 302 || statusCode == 303 || statusCode == 307 || statusCode == 308; + bool redirectFailure = isRedirect && !redirectTarget.isValid(); if (redirectTarget.isValid()) { - QUrl url = reply->request().url(); - QUrl redirectUrl = redirectTarget.toUrl(); - if (redirectUrl.isRelative()) { - redirectUrl = url.resolved(redirectUrl); - } - if (url == redirectUrl) { - qCWarning(CardPictureLoaderWorkerWorkLog) << "recursive redirect detected!"; + const QUrl url = reply->url(); + const QUrl redirectUrl = url.resolved(redirectTarget.toUrl()); + redirectVisited.insert(url); + if (!redirectUrl.isValid() || redirectUrl.isEmpty() || redirectVisited.contains(redirectUrl) || + redirectVisited.size() > 10) { + qCWarning(CardPictureLoaderWorkerWorkLog) << "Invalid, cyclic or overlong redirect from" << url; redirectFailure = true; } else { emit urlRedirected(url, redirectUrl); } } - if (redirectFailure || reply->error()) { + if (redirectFailure) { + for (const QUrl &visited : std::as_const(redirectVisited)) { + emit redirectInvalidated(visited); + } + // Do not retry a cached cyclic redirect as a corrupt image: advance to + // the next configured source, just as for an ordinary failed response. + if (reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool()) { + emit cachedUrlInvalidated(reply->url()); + } + picDownloadFailed(); + } else if (reply->error()) { handleFailedReply(reply); } else { handleSuccessfulReply(reply); - emit requestSucceeded(reply->url()); } reply->deleteLater(); @@ -184,6 +208,15 @@ void CardPictureLoaderWorkerWork::handleFailedReply(const QNetworkReply *reply) QDateTime backoffUntil = s_rateLimiter.on429(host, now, retryAfterMs); emit rateLimited(host); + // Downloads may have been disabled while this request was in flight. + // Record the server's limit for future online requests, but resume the + // cache-only search now instead of scheduling a new network backoff. + if (!picDownload) { + cardToDownload.resetIndices(); + startNextPicDownload(); + return; + } + if (s_rateLimiter.rounds(host) == 1) { qCWarning(CardPictureLoaderWorkerWorkLog).nospace() << "PictureLoader: [card: " << cardToDownload.getCard().getName() @@ -208,7 +241,9 @@ void CardPictureLoaderWorkerWork::handleFailedReply(const QNetworkReply *reply) emit cachedUrlInvalidated(reply->url()); - emit requestImageDownload(reply->url(), this); + if (!retryCachedUrl(reply->url())) { + picDownloadFailed(); + } } else { qCDebug(CardPictureLoaderWorkerWorkLog).nospace() << "PictureLoader: [card: " << cardToDownload.getCard().getName() @@ -226,9 +261,8 @@ void CardPictureLoaderWorkerWork::handleSuccessfulReply(QNetworkReply *reply) // List of status codes from https://doc.qt.io/qt-6/qnetworkreply.html#redirected int statusCode = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); - if (statusCode == 301 || statusCode == 302 || statusCode == 303 || statusCode == 305 || statusCode == 307 || - statusCode == 308) { - QUrl redirectUrl = reply->header(QNetworkRequest::LocationHeader).toUrl(); + if (statusCode == 301 || statusCode == 302 || statusCode == 303 || statusCode == 307 || statusCode == 308) { + QUrl redirectUrl = reply->url().resolved(reply->attribute(QNetworkRequest::RedirectionTargetAttribute).toUrl()); qCDebug(CardPictureLoaderWorkerWorkLog).nospace() << "PictureLoader: [card: " << cardToDownload.getCard().getName() << " set: " << cardToDownload.getSetName() << "]: following " << (isFromCache ? "cached redirect" : "redirect") << " to " @@ -252,15 +286,23 @@ void CardPictureLoaderWorkerWork::handleSuccessfulReply(QNetworkReply *reply) return; } - QImage image = tryLoadImageFromReply(reply); + const auto decoded = CardPictureLoaderImageFormats::readImage(reply); + const QImage &image = decoded.image; if (image.isNull()) { qCDebug(CardPictureLoaderWorkerWorkLog).nospace() << "PictureLoader: [card: " << cardToDownload.getCard().getName() << " set: " << cardToDownload.getSetName() << "]: Possible " << (isFromCache ? "cached" : "downloaded") << " picture at " - << reply->url().toDisplayString() << " could not be loaded: " << reply->errorString(); + << reply->url().toDisplayString() << " could not be loaded: " << decoded.error; - picDownloadFailed(); + // HTTP success can still contain an invalid or unsupported image. Remove + // that cache entry so future loads do not keep selecting the same bytes. + emit cachedUrlInvalidated(reply->url()); + if (!isFromCache || !retryCachedUrl(reply->url())) { + // Fresh bad responses advance to the next source; cached-only mode + // must never turn a decoder failure into a network download. + picDownloadFailed(); + } } else { qCDebug(CardPictureLoaderWorkerWorkLog).nospace() << "PictureLoader: [card: " << cardToDownload.getCard().getName() << " set: " << cardToDownload.getSetName() @@ -271,27 +313,21 @@ void CardPictureLoaderWorkerWork::handleSuccessfulReply(QNetworkReply *reply) } } -QImage CardPictureLoaderWorkerWork::tryLoadImageFromReply(QNetworkReply *reply) +bool CardPictureLoaderWorkerWork::shouldBypassCache(const QUrl &url) const { - static constexpr int riffHeaderSize = 12; // RIFF_HEADER_SIZE from webp/format_constants.h - auto replyHeader = reply->peek(riffHeaderSize); + return picDownload && retriedCacheUrls.contains(url); +} - if (replyHeader.startsWith("RIFF") && replyHeader.endsWith("WEBP")) { - auto imgBuf = QBuffer(this); - imgBuf.setData(reply->readAll()); - - auto movie = QMovie(&imgBuf); - movie.start(); - movie.stop(); - - return movie.currentImage(); +bool CardPictureLoaderWorkerWork::retryCachedUrl(const QUrl &url) +{ + if (!picDownload || retriedCacheUrls.contains(url)) { + return false; } - - QImageReader imgReader; - imgReader.setDecideFormatFromContent(true); - imgReader.setDevice(reply); - - return imgReader.read(); + // Record before emitting: the queue can dispatch synchronously, and removing + // an unreadable cache entry can fail. The retry must bypass that entry. + retriedCacheUrls.insert(url); + emit requestImageDownload(url, this); + return true; } void CardPictureLoaderWorkerWork::scheduleDeferredRetry(const QString &preferredHost) @@ -324,12 +360,7 @@ void CardPictureLoaderWorkerWork::scheduleDeferredRetry(const QString &preferred << " set: " << cardToDownload.getSetName() << "]: All URLs exhausted, scheduling deferred retry in " << waitMs << "ms"; - QTimer::singleShot(waitMs, this, [this] { - s_rateLimiter.clearExpired(QDateTime::currentDateTime()); - - cardToDownload.resetIndices(); - startNextPicDownload(); - }); + deferredRetry->start(static_cast(qMin(waitMs, std::numeric_limits::max()))); } void CardPictureLoaderWorkerWork::concludeImageLoad(const QImage &image) @@ -341,4 +372,9 @@ void CardPictureLoaderWorkerWork::concludeImageLoad(const QImage &image) void CardPictureLoaderWorkerWork::picDownloadChanged() { picDownload = SettingsCache::instance().downloads().getPicDownload(); + if (!picDownload && deferredRetry->isActive()) { + deferredRetry->stop(); + cardToDownload.resetIndices(); + startNextPicDownload(); + } } diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h index c3dbcc6e8..da9420773 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h @@ -6,7 +6,10 @@ #include #include #include +#include #include +#include +#include #include class ExactCard; @@ -47,6 +50,9 @@ public: /** @brief Shared per-server 429 backoff state. */ static const ServerRateLimiter &rateLimiter(); + /** @brief A corrupt cached response gets one network retry per URL and work item. */ + [[nodiscard]] bool shouldBypassCache(const QUrl &url) const; + /** * @brief Starts downloading the next URL for this card. * @@ -74,7 +80,12 @@ public slots: void handleNetworkReply(QNetworkReply *reply); private: - bool picDownload; ///< Whether network downloading is enabled + bool picDownload; ///< Whether network downloading is enabled + QTimer *deferredRetry = nullptr; ///< Owned retry, cancellable when downloads are disabled + QSet retriedCacheUrls; + + bool retryCachedUrl(const QUrl &url); + QSet redirectVisited; ///< Redirect chain for the current source URL static ServerRateLimiter s_rateLimiter; ///< Shared per-server 429 backoff state @@ -87,13 +98,6 @@ private: /** @brief Processes a successful network reply. */ void handleSuccessfulReply(QNetworkReply *reply); - /** - * @brief Attempts to read an image from a network reply. - * @param reply The network reply - * @return The loaded QImage or an empty image if reading failed - */ - QImage tryLoadImageFromReply(QNetworkReply *reply); - /** * @brief Finalizes the image loading process. * @param image The loaded image (empty if failed) @@ -116,15 +120,15 @@ signals: */ void imageLoaded(const ExactCard &card, const QImage &image); - /** @brief Emitted when a network request completes successfully. */ - void requestSucceeded(const QUrl &url); - /** @brief Request that a URL be downloaded. */ void requestImageDownload(const QUrl &url, CardPictureLoaderWorkerWork *instance); /** @brief Emitted when a URL has been redirected. */ void urlRedirected(const QUrl &originalUrl, const QUrl &redirectUrl); + /** @brief A redirect mapping is unsafe and should be removed. */ + void redirectInvalidated(const QUrl &url); + /** @brief Emitted when a server returned HTTP 429. */ void rateLimited(const QString &host); diff --git a/cockatrice/src/interface/card_picture_loader/redirect_chain.h b/cockatrice/src/interface/card_picture_loader/redirect_chain.h new file mode 100644 index 000000000..ffcfe6b10 --- /dev/null +++ b/cockatrice/src/interface/card_picture_loader/redirect_chain.h @@ -0,0 +1,37 @@ +#ifndef COCKATRICE_REDIRECT_CHAIN_H +#define COCKATRICE_REDIRECT_CHAIN_H + +#include +#include +#include +#include +#include +#include + +// Resolve previously accepted redirects without recursion. Invalid chains are +// removed so subsequent requests can rediscover them from the source server. +inline QUrl resolveImageRedirects(const QUrl &url, QHash> &cache, int maxHops) +{ + QUrl resolved = url; + QSet visited; + for (int depth = 0; depth <= maxHops; ++depth) { + if (visited.contains(resolved)) { + break; + } + const auto it = cache.constFind(resolved); + if (it == cache.cend() || it.value().first.isEmpty()) { + return resolved; + } + if (depth == maxHops) { + break; + } + visited.insert(resolved); + resolved = it.value().first; + } + for (const QUrl &entry : std::as_const(visited)) { + cache.remove(entry); + } + return url; +} + +#endif diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 84b9f02bf..fedadfd1a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -171,6 +171,7 @@ endfunction() # --- Unit tests ------------------------------------------------------------------ +cockatrice_add_test(NAME image_redirect_chain_test SOURCES image_redirect_chain_test.cpp) cockatrice_add_test(NAME expression_test SOURCES expression_test.cpp LIBS libcockatrice_utility) cockatrice_add_test(NAME clamped_arithmetic_test SOURCES clamped_arithmetic_test.cpp LIBS libcockatrice_utility) cockatrice_add_test(NAME test_age_formatting SOURCES test_age_formatting.cpp LIBS libcockatrice_utility) diff --git a/tests/image_redirect_chain_test.cpp b/tests/image_redirect_chain_test.cpp new file mode 100644 index 000000000..072512d96 --- /dev/null +++ b/tests/image_redirect_chain_test.cpp @@ -0,0 +1,82 @@ +#include "../cockatrice/src/interface/card_picture_loader/redirect_chain.h" + +#include + +namespace +{ +using RedirectCache = QHash>; + +QUrl url(int index) +{ + return QUrl(QStringLiteral("https://cards.example/%1").arg(index)); +} + +RedirectCache chain(int hops) +{ + RedirectCache cache; + for (int index = 0; index < hops; ++index) { + cache.insert(url(index), {url(index + 1), QDateTime()}); + } + return cache; +} +} // namespace + +TEST(ImageRedirectChain, MissingHopReturnsLastKnownUrlWithoutRemovingMappings) +{ + auto cache = chain(2); + const auto original = cache; + EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(2)); + EXPECT_EQ(cache, original); + EXPECT_EQ(resolveImageRedirects(url(9), cache, 10), url(9)); +} + +TEST(ImageRedirectChain, ExactlyMaxHopsResolvesAndKeepsCache) +{ + auto cache = chain(10); + const auto original = cache; + EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(10)); + EXPECT_EQ(cache, original); +} + +TEST(ImageRedirectChain, CycleReturnsOriginalAndPurgesVisitedMappings) +{ + auto cache = chain(2); + cache.insert(url(2), {url(1), QDateTime()}); + cache.insert(url(9), {url(10), QDateTime()}); + EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(0)); + EXPECT_FALSE(cache.contains(url(0))); + EXPECT_FALSE(cache.contains(url(1))); + EXPECT_FALSE(cache.contains(url(2))); + EXPECT_TRUE(cache.contains(url(9))); +} + +TEST(ImageRedirectChain, OverlongChainReturnsOriginalAndPurgesVisitedMappings) +{ + auto cache = chain(12); + cache.insert(url(20), {url(21), QDateTime()}); + EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(0)); + for (int index = 0; index < 10; ++index) { + EXPECT_FALSE(cache.contains(url(index))); + } + EXPECT_TRUE(cache.contains(url(10))); + EXPECT_TRUE(cache.contains(url(11))); + EXPECT_TRUE(cache.contains(url(20))); +} + +TEST(ImageRedirectChain, EmptyTargetStopsAtItsSource) +{ + auto cache = chain(1); + cache.insert(url(1), {QUrl(), QDateTime()}); + const auto original = cache; + EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(1)); + EXPECT_EQ(cache, original); +} + +TEST(ImageRedirectChain, ZeroHopsDoesNotFollowOrRemoveMappings) +{ + auto cache = chain(1); + EXPECT_EQ(resolveImageRedirects(url(5), cache, 0), url(5)); + EXPECT_TRUE(cache.contains(url(0))); + EXPECT_EQ(resolveImageRedirects(url(0), cache, 0), url(0)); + EXPECT_TRUE(cache.contains(url(0))); +}