Hi,
|
From e304cf03215e36d000d3e7dc0bd69c3677a2185c Mon Sep 17 00:00:00 2001 From: "Robert C. Helling" <[email protected]> Date: Tue, 15 Mar 2016 21:31:59 +0100 Subject: [PATCH 1/2] When handing off a picture to a worker thread, copy it first To: [email protected]
as otherwise we crash when the picture is freed before the worker thread (to load from the net or to compute hashes) is finished Signed-off-by: Robert C. Helling <[email protected]> --- subsurface-core/dive.c | 14 +++++++++++++- subsurface-core/dive.h | 2 ++ subsurface-core/imagedownloader.cpp | 13 +++++++++---- subsurface-core/imagedownloader.h | 1 + subsurface-core/qthelper.cpp | 6 +++++- 5 files changed, 30 insertions(+), 6 deletions(-) diff --git a/subsurface-core/dive.c b/subsurface-core/dive.c index 47b3e1f..cad2a85 100644 --- a/subsurface-core/dive.c +++ b/subsurface-core/dive.c @@ -3374,7 +3374,7 @@ void dive_set_geodata_from_picture(struct dive *dive, struct picture *picture) } } -static void picture_free(struct picture *picture) +void picture_free(struct picture *picture) { if (!picture) return; @@ -3383,6 +3383,18 @@ static void picture_free(struct picture *picture) free(picture); } +// When handling pictures in different threads, we need to copy them so we don't +// run into problems when the main thread frees the picture. + +struct picture *clone_picture(struct picture *src) +{ + struct picture *dst; + + dst = alloc_picture(); + copy_pl(src, dst); + return dst; +} + void dive_remove_picture(char *filename) { struct picture **picture = ¤t_dive->picture_list; diff --git a/subsurface-core/dive.h b/subsurface-core/dive.h index 74f55ee..484a67f 100644 --- a/subsurface-core/dive.h +++ b/subsurface-core/dive.h @@ -376,6 +376,7 @@ struct picture { for (struct picture *picture = (_divestruct).picture_list; picture; picture = picture->next) extern struct picture *alloc_picture(); +extern struct picture *clone_picture(struct picture *src); extern bool dive_check_picture_time(struct dive *d, int shift_time, timestamp_t timestamp); extern void dive_create_picture(struct dive *d, char *filename, int shift_time, bool match_all); extern void dive_add_picture(struct dive *d, struct picture *newpic); @@ -385,6 +386,7 @@ extern bool picture_check_valid(char *filename, int shift_time); extern void picture_load_exif_data(struct picture *p); extern timestamp_t picture_get_timestamp(char *filename); extern void dive_set_geodata_from_picture(struct dive *d, struct picture *pic); +extern void picture_free(struct picture *picture); extern int explicit_first_cylinder(struct dive *dive, struct divecomputer *dc); extern int get_depth_at_time(struct divecomputer *dc, unsigned int time); diff --git a/subsurface-core/imagedownloader.cpp b/subsurface-core/imagedownloader.cpp index 9451f8a..d28ec95 100644 --- a/subsurface-core/imagedownloader.cpp +++ b/subsurface-core/imagedownloader.cpp @@ -17,6 +17,11 @@ ImageDownloader::ImageDownloader(struct picture *pic) picture = pic; } +ImageDownloader::~ImageDownloader() +{ + picture_free(picture); +} + void ImageDownloader::load(bool fromHash){ QUrl url; if(fromHash) @@ -79,21 +84,21 @@ SHashedImage::SHashedImage(struct picture *picture) : QImage() if (filename.isNull()) { // That didn't produce a local filename. // Try the cloud server - QtConcurrent::run(loadPicture, picture, true); + QtConcurrent::run(loadPicture, clone_picture(picture), true); } else { // Load locally from translated file name load(filename); if (!isNull()) { // Make sure the hash still matches the image file - QtConcurrent::run(updateHash, picture); + QtConcurrent::run(updateHash, clone_picture(picture)); } else { // Interpret filename as URL - QtConcurrent::run(loadPicture, picture, false); + QtConcurrent::run(loadPicture, clone_picture(picture), false); } } } else { // We loaded successfully. Now, make sure hash is up to date. - QtConcurrent::run(hashPicture, picture); + QtConcurrent::run(hashPicture, clone_picture(picture)); } } diff --git a/subsurface-core/imagedownloader.h b/subsurface-core/imagedownloader.h index cd85c95..2ff6834 100644 --- a/subsurface-core/imagedownloader.h +++ b/subsurface-core/imagedownloader.h @@ -14,6 +14,7 @@ class ImageDownloader : public QObject { Q_OBJECT; public: ImageDownloader(struct picture *picture); + ~ImageDownloader(); void load(bool fromHash); private: diff --git a/subsurface-core/qthelper.cpp b/subsurface-core/qthelper.cpp index 7c67e09..b389129 100644 --- a/subsurface-core/qthelper.cpp +++ b/subsurface-core/qthelper.cpp @@ -1108,11 +1108,14 @@ QString fileFromHash(char *hash) return localFilenameOf[QByteArray::fromHex(hash)]; } +// This needs to operate on a copy of picture as it frees it after finishing! void updateHash(struct picture *picture) { QByteArray hash = hashFile(fileFromHash(picture->hash)); learnHash(picture, hash); + picture_free(picture); } +// This needs to operate on a copy of picture as it frees it after finishing! void hashPicture(struct picture *picture) { char *oldHash = copy_string(picture->hash); @@ -1120,13 +1123,14 @@ void hashPicture(struct picture *picture) if (!same_string(picture->hash, "") && !same_string(picture->hash, oldHash)) mark_divelist_changed((true)); free(oldHash); + picture_free(picture); } extern "C" void cache_picture(struct picture *picture) { QString filename = picture->filename; if (!hashOf.contains(filename)) - QtConcurrent::run(hashPicture, picture); + QtConcurrent::run(hashPicture, clone_picture(picture)); } void learnImages(const QDir dir, int max_recursions) -- 2.5.4 (Apple Git-61)
From 6b951ba440357e0d64212564bf0d453ad53e554e Mon Sep 17 00:00:00 2001 From: "Robert C. Helling" <[email protected]> Date: Tue, 15 Mar 2016 22:45:17 +0100 Subject: [PATCH 2/2] Fix loading images from URLs To: [email protected] This was broken when introducing loading images from the server. Now it first tries the server and if that fails tries the actual URL. Still, the image does not show up immediately, since the DivePictureModel is unavailable to the image downloader to be told to update the images. This needs to be fixed but in the mean time, the image is shown when the dive is reselected (possibly on the next run) since it is then loaded from the cache. Signed-off-by: Robert C. Helling <[email protected]> --- subsurface-core/imagedownloader.cpp | 13 +++++++++++-- subsurface-core/imagedownloader.h | 1 + 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/subsurface-core/imagedownloader.cpp b/subsurface-core/imagedownloader.cpp index d28ec95..daa49ea 100644 --- a/subsurface-core/imagedownloader.cpp +++ b/subsurface-core/imagedownloader.cpp @@ -24,6 +24,7 @@ ImageDownloader::~ImageDownloader() void ImageDownloader::load(bool fromHash){ QUrl url; + loadFromHash = fromHash; if(fromHash) url = cloudImageURL(picture->hash); else @@ -45,8 +46,11 @@ void ImageDownloader::saveImage(QNetworkReply *reply) QByteArray imageData = reply->readAll(); QImage image = QImage(); image.loadFromData(imageData); - if (image.isNull()) + if (image.isNull()) { + if (loadFromHash) + load(false); return; + } QCryptographicHash hash(QCryptographicHash::Sha1); hash.addData(imageData); QString path = QStandardPaths::standardLocations(QStandardPaths::CacheLocation).first(); @@ -64,6 +68,11 @@ void ImageDownloader::saveImage(QNetworkReply *reply) } reply->manager()->deleteLater(); reply->deleteLater(); + // This should be called to make the picture actually show. + // Problem is DivePictureModel is not in subsurface-core. + // Nevertheless, the image shows when the dive is selected the next time. + // DivePictureModel::instance()->updateDivePictures(); + } void loadPicture(struct picture *picture, bool fromHash) @@ -74,7 +83,7 @@ void loadPicture(struct picture *picture, bool fromHash) SHashedImage::SHashedImage(struct picture *picture) : QImage() { - QUrl url = QUrl::fromUserInput(QString(picture->filename)); + QUrl url = QUrl::fromUserInput(localFilePath(QString(picture->filename))); if(url.isLocalFile()) load(url.toLocalFile()); if (isNull()) { diff --git a/subsurface-core/imagedownloader.h b/subsurface-core/imagedownloader.h index 2ff6834..f4e3df8 100644 --- a/subsurface-core/imagedownloader.h +++ b/subsurface-core/imagedownloader.h @@ -20,6 +20,7 @@ public: private: struct picture *picture; QNetworkAccessManager manager; + bool loadFromHash; private slots: void saveImage(QNetworkReply *reply); -- 2.5.4 (Apple Git-61)
I think patch 0001 fixes the issue by giving each thread its own copy of the struct picture. Please test! Patch 0002 then addresses another problem with loading images from URLs. Best Robert |
signature.asc
Description: Message signed with OpenPGP using GPGMail
_______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
