Dive pictures: refactor image downloader

Instead of generating one ImageDownloader object per image to be
downloaded and running every image download in a separate worker
thread, use one global ImageDownloader object owned by the UI thread.

The images are downloaded using event based IO (as probably was the
intention of the QNetworkManager class).

User-visible change: after download from the internet, the thumbnail
is shown without having to change dives.

Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

core/imagedownloader.cpp

@@ -21,81 +21,71 @@ static QUrl cloudImageURL(const char *filename)
 	return QUrl::fromUserInput(QString("https://cloud.subsurface-divelog.org/images/").append(hash));
 }
 
-ImageDownloader::ImageDownloader(const QString &filename_in) : filename(filename_in)
+// Note: this is a global instead of a function-local variable on purpose.
+// We don't want this to be generated in a different thread context if
+// ImageDownloader::instance() is called from a worker thread.
+static ImageDownloader imageDownloader;
+ImageDownloader *ImageDownloader::instance()
 {
+	return &imageDownloader;
 }
 
-void ImageDownloader::load(bool fromHash)
+ImageDownloader::ImageDownloader()
 {
-	if (fromHash && loadFromUrl(cloudImageURL(qPrintable(filename))))
+	connect(&manager, &QNetworkAccessManager::finished, this, &ImageDownloader::saveImage);
-		return;
-
-	// If loading from hash failed, try to load from filename
-	loadFromUrl(QUrl::fromUserInput(filename));
 }
 
-bool ImageDownloader::loadFromUrl(const QUrl &url)
+void ImageDownloader::load(QString filename, bool fromHash)
 {
-	bool success = false;
+	QUrl url = fromHash ? cloudImageURL(qPrintable(filename)) : QUrl::fromUserInput(filename);
-	if (url.isValid()) {
+
-		QEventLoop loop;
+	if (!url.isValid())
-		QNetworkAccessManager manager;
+		emit failed(filename);
-		QNetworkRequest request(url);
+
-		connect(&manager, &QNetworkAccessManager::finished, this,
+	QNetworkRequest request(url);
-			[this,&success] (QNetworkReply *reply) { saveImage(reply, success); });
+	request.setAttribute(QNetworkRequest::User, filename);
-		connect(&manager, &QNetworkAccessManager::finished, &loop, &QEventLoop::quit);
+	request.setAttribute(static_cast<QNetworkRequest::Attribute>(QNetworkRequest::User + 1), fromHash);
-		qDebug() << "Downloading image from" << url;
+	manager.get(request);
-		QNetworkReply *reply = manager.get(request);
+}
-		loop.exec();
+
-		delete reply;
+void ImageDownloader::saveImage(QNetworkReply *reply)
+{
+	QString filename = reply->request().attribute(QNetworkRequest::User).toString();
+
+	if (reply->error() != QNetworkReply::NoError) {
+		bool fromHash = reply->request().attribute(static_cast<QNetworkRequest::Attribute>(QNetworkRequest::User + 1)).toBool();
+		if (fromHash)
+			load(filename, false);
+		else
+			emit failed(filename);
+	} else {
+		QByteArray imageData = reply->readAll();
+		QCryptographicHash hash(QCryptographicHash::Sha1);
+		hash.addData(imageData);
+		QString path = QStandardPaths::standardLocations(QStandardPaths::CacheLocation).first();
+		QDir dir(path);
+		if (!dir.exists())
+			dir.mkpath(path);
+		QFile imageFile(path.append("/").append(hash.result().toHex()));
+		if (imageFile.open(QIODevice::WriteOnly)) {
+			qDebug() << "Write image to" << imageFile.fileName();
+			QDataStream stream(&imageFile);
+			stream.writeRawData(imageData.data(), imageData.length());
+			imageFile.waitForBytesWritten(-1);
+			imageFile.close();
+			learnHash(filename, imageFile.fileName(), hash.result());
+		}
+		emit loaded(filename);
 	}
-	return success;
-}
-
-void ImageDownloader::saveImage(QNetworkReply *reply, bool &success)
-{
-	success = false;
-	QByteArray imageData = reply->readAll();
-	QImage image;
-	image.loadFromData(imageData);
-	if (image.isNull())
-		return;
-	success = true;
-	QCryptographicHash hash(QCryptographicHash::Sha1);
-	hash.addData(imageData);
-	QString path = QStandardPaths::standardLocations(QStandardPaths::CacheLocation).first();
-	QDir dir(path);
-	if (!dir.exists())
-		dir.mkpath(path);
-	QFile imageFile(path.append("/").append(hash.result().toHex()));
-	if (imageFile.open(QIODevice::WriteOnly)) {
-		qDebug() << "Write image to" << imageFile.fileName();
-		QDataStream stream(&imageFile);
-		stream.writeRawData(imageData.data(), imageData.length());
-		imageFile.waitForBytesWritten(-1);
-		imageFile.close();
-		learnHash(filename, imageFile.fileName(), hash.result());
-	}
-	// This should be called to make the picture actually show.
-	// Problem is DivePictureModel is not in core.
-	// Nevertheless, the image shows when the dive is selected the next time.
-	// DivePictureModel::instance()->updateDivePictures();
 
+	reply->deleteLater();
 }
 
 static void loadPicture(QString filename, bool fromHash)
 {
-	static QSet<QString> queuedPictures;
+	// This has to be done in UI main thread, because QNetworkManager refuses
-	static QMutex pictureQueueMutex;
+	// to treat requests from other threads.
-
+	QMetaObject::invokeMethod(ImageDownloader::instance(), "load", Qt::AutoConnection, Q_ARG(QString, filename), Q_ARG(bool, fromHash));
-	QMutexLocker locker(&pictureQueueMutex);
-	if (queuedPictures.contains(filename))
-		return;
-	queuedPictures.insert(filename);
-	locker.unlock();
-
-	ImageDownloader download(filename);
-	download.load(fromHash);
 }
 
 // Overwrite QImage::load() so that we can perform better error reporting.
@@ -108,13 +98,15 @@ static QImage loadImage(const QString &fileName, const char *format = nullptr)
 	return res;
 }
 
-QImage getHashedImage(const QString &file)
+// Returns: thumbnail, still loading
+static std::pair<QImage,bool> getHashedImage(const QString &file)
 {
-	QImage res;
+	QImage thumb;
+	bool stillLoading = false;
 	QUrl url = QUrl::fromUserInput(localFilePath(file));
 	if (url.isLocalFile())
-		res = loadImage(url.toLocalFile());
+		thumb = loadImage(url.toLocalFile());
-	if (res.isNull()) {
+	if (thumb.isNull()) {
 		// This did not load anything. Let's try to get the image from other sources
 		// Let's try to load it locally via its hash
 		QString filenameLocal = localFilePath(qPrintable(file));
@@ -124,22 +116,24 @@ QImage getHashedImage(const QString &file)
 			// Try the cloud server
 			// TODO: This is dead code at the moment.
 			loadPicture(file, true);
+			stillLoading = true;
 		} else {
 			// Load locally from translated file name
-			res = loadImage(filenameLocal);
+			thumb = loadImage(filenameLocal);
-			if (!res.isNull()) {
+			if (!thumb.isNull()) {
 				// Make sure the hash still matches the image file
 				hashPicture(filenameLocal);
 			} else {
 				// Interpret filename as URL
 				loadPicture(filenameLocal, false);
+				stillLoading = true;
 			}
 		}
 	} else {
 		// We loaded successfully. Now, make sure hash is up to date.
 		hashPicture(file);
 	}
-	return res;
+	return { thumb, stillLoading };
 }
 
 static QImage renderIcon(const char *id, int size)
@@ -157,6 +151,8 @@ Thumbnailer::Thumbnailer() : failImage(renderIcon(":filter-close", maxThumbnailS
 	// Currently, we only process one image at a time. Stefan Fuchs reported problems when
 	// calculating multiple thumbnails at once and this hopefully helps.
 	pool.setMaxThreadCount(1);
+	connect(ImageDownloader::instance(), &ImageDownloader::loaded, this, &Thumbnailer::imageDownloaded);
+	connect(ImageDownloader::instance(), &ImageDownloader::failed, this, &Thumbnailer::imageDownloadFailed);
 }
 
 Thumbnailer *Thumbnailer::instance()
@@ -217,7 +213,11 @@ void Thumbnailer::processItem(QString filename)
 	QImage thumbnail = getThumbnailFromCache(filename);
 
 	if (thumbnail.isNull()) {
-		thumbnail = getHashedImage(filename);
+		auto res = getHashedImage(filename);
+		if (res.second)
+			return;
+		thumbnail = res.first;
+
 		if (thumbnail.isNull()) {
 			thumbnail = failImage;
 		} else {
@@ -232,6 +232,21 @@ void Thumbnailer::processItem(QString filename)
 	workingOn.remove(filename);
 }
 
+void Thumbnailer::imageDownloaded(QString filename)
+{
+	// Image was downloaded and the filename connected with a hash.
+	// Try thumbnailing again.
+	QMutexLocker l(&lock);
+	workingOn[filename] = QtConcurrent::run(&pool, [this, filename]() { processItem(filename); });
+}
+
+void Thumbnailer::imageDownloadFailed(QString filename)
+{
+	emit thumbnailChanged(filename, failImage);
+	QMutexLocker l(&lock);
+	workingOn.remove(filename);
+}
+
 QImage Thumbnailer::fetchThumbnail(PictureEntry &entry)
 {
 	QMutexLocker l(&lock);

core/imagedownloader.h

@@ -10,13 +10,17 @@
 class ImageDownloader : public QObject {
 	Q_OBJECT
 public:
-	ImageDownloader(const QString &filename);
+	static ImageDownloader *instance();
-	void load(bool fromHash);
+	ImageDownloader();
-
+public slots:
+	void load(QString filename, bool fromHash);
+signals:
+	void loaded(QString filename);
+	void failed(QString filename);
 private:
-	bool loadFromUrl(const QUrl &);	// return true on success
+	QNetworkAccessManager manager;
-	void saveImage(QNetworkReply *reply, bool &success);
+	void loadFromUrl(const QString &filename, const QUrl &);
-	QString filename;
+	void saveImage(QNetworkReply *reply);
 };
 
 class PictureEntry;
@@ -35,6 +39,9 @@ public:
 	static int maxThumbnailSize();
 	static int defaultThumbnailSize();
 	static int thumbnailSize(double zoomLevel);
+public slots:
+	void imageDownloaded(QString filename);
+	void imageDownloadFailed(QString filename);
 signals:
 	void thumbnailChanged(QString filename, QImage thumbnail);
 private:
@@ -49,6 +56,4 @@ private:
 	QMap<QString,QFuture<void>> workingOn;
 };
 
-QImage getHashedImage(const QString &filename);
-
 #endif // IMAGEDOWNLOADER_H