From 5ee658b482ad634706c21d20d4f8fd15828fc1d8 Mon Sep 17 00:00:00 2001 From: Roger Siddons Date: Mon, 12 Oct 2015 14:54:36 +0100 Subject: [PATCH 11/13] MythUiImage: Don't block UI when exiting screens When deleting MythUIImages the UI blocks until all image loading threads complete. This patch removes the need for that by introducing an image proxy for the load threads that prohibits access to a deleted image. Note the ImageLoadEvent is unchanged but has been moved to resolve dependancies. It still contains a (potentially dangerous) pointer to the deleted MythUIImage, which IMO is redundant and should be removed. diff --git a/mythtv/libs/libmythui/mythuiimage.cpp b/mythtv/libs/libmythui/mythuiimage.cpp index fc067f7..a937358 100644 --- a/mythtv/libs/libmythui/mythuiimage.cpp +++ src/mythtv/libs/libmythui/mythuiimage.cpp @@ -121,25 +121,104 @@ void ImageProperties::SetMaskImage(MythImage *image) } /*! + * \class ImageLoadEvent + */ +class ImageLoadEvent : public QEvent +{ + public: + ImageLoadEvent(const MythUIImage *parent, MythImage *image, + const QString &basefile, const QString &filename, + int number, bool aborted) + : QEvent(kEventType), + m_parent(parent), m_image(image), m_basefile(basefile), + m_filename(filename), m_number(number), + m_images(NULL), m_aborted(aborted) { } + + ImageLoadEvent(const MythUIImage *parent, AnimationFrames *frames, + const QString &basefile, + const QString &filename, bool aborted) + : QEvent(kEventType), + m_parent(parent), m_image(NULL), m_basefile(basefile), + m_filename(filename), m_number(0), + m_images(frames), m_aborted(aborted) { } + + const MythUIImage *GetParent() const { return m_parent; } + MythImage *GetImage() const { return m_image; } + const QString GetBasefile() const { return m_basefile; } + const QString GetFilename() const { return m_filename; } + int GetNumber() const { return m_number; } + AnimationFrames *GetAnimationFrames() const { return m_images; } + bool GetAbortState() const { return m_aborted; } + + static Type kEventType; + + private: + const MythUIImage *m_parent; + MythImage *m_image; + QString m_basefile; + QString m_filename; + int m_number; + + // Animated Images + AnimationFrames *m_images; + + // Image Load + bool m_aborted; +}; + +QEvent::Type ImageLoadEvent::kEventType = + (QEvent::Type) QEvent::registerEventType(); + +/*! + \brief Guards access to the image by Load threads. If the image is deleted + by the UI this proxy persists until all its load threads complete. + */ +class ImageProxy +{ +public: + ImageProxy(MythUIImage *image) : m_mutex(), m_image(image) {} + + MythUIImage *GetImage() { QMutexLocker lock(&m_mutex); return m_image; } + void Orphan() { QMutexLocker lock(&m_mutex); m_image = NULL; } + void PostEvent(ImageLoadEvent *event) + { + QMutexLocker lock(&m_mutex); + if (m_image) + QCoreApplication::postEvent(m_image, event); + else + LOG(VB_GUI | VB_FILE, LOG_DEBUG, + QString("ImageLoadThread: Discarding load of %1") + .arg(event->GetFilename())); + } + +private: + QMutex m_mutex; + MythUIImage *m_image; +}; + +//! The proxy is shared by the image and all its load threads. +typedef QSharedPointer ProxyPtr; + +/*! * \class ImageLoader */ class ImageLoader { public: - ImageLoader() { }; - ~ImageLoader() { }; + ImageLoader() { } + ~ImageLoader() { } - static QHash m_loadingImages; - static QMutex m_loadingImagesLock; - static QWaitCondition m_loadingImagesCond; + static QHash m_loadingImages; + static QMutex m_loadingImagesLock; + static QWaitCondition m_loadingImagesCond; - static bool PreLoad(const QString &cacheKey, const MythUIImage *uitype) + static bool PreLoad(const QString &cacheKey, const ProxyPtr &proxy) { m_loadingImagesLock.lock(); // Check to see if the image is being loaded by us in another thread if ((m_loadingImages.contains(cacheKey)) && - (m_loadingImages[cacheKey] == uitype)) + (m_loadingImages[cacheKey] == proxy.data())) { LOG(VB_GUI | VB_FILE, LOG_DEBUG, QString("ImageLoader::PreLoad(%1), this " @@ -153,7 +232,7 @@ class ImageLoader while (m_loadingImages.contains(cacheKey)) m_loadingImagesCond.wait(&m_loadingImagesLock); - m_loadingImages[cacheKey] = uitype; + m_loadingImages[cacheKey] = proxy.data(); m_loadingImagesLock.unlock(); return true; @@ -229,15 +308,12 @@ class ImageLoader // Must be a copy for thread safety ImageProperties imProps, ImageCacheMode cacheMode, - // Included only to check address, could be - // replaced by generating a unique value for - // each MythUIImage object? - const MythUIImage *parent, + const ProxyPtr &proxy, bool &aborted, MythImageReader *imageReader = NULL) { QString cacheKey = GenImageLabel(imProps); - if (!PreLoad(cacheKey, parent)) + if (!PreLoad(cacheKey, proxy)) { aborted = true; return NULL; @@ -390,10 +466,7 @@ class ImageLoader // Must be a copy for thread safety ImageProperties imProps, ImageCacheMode cacheMode, - // Included only to check address, could be - // replaced by generating a unique value for - // each MythUIImage object? - const MythUIImage *parent, + const ProxyPtr &proxy, bool &aborted) { QString filename = QString("frame-%1-") + imProps.filename; @@ -411,7 +484,7 @@ class ImageLoader ImageProperties frameProps = imProps; frameProps.filename = frameFilename; - MythImage *im = LoadImage(painter, frameProps, cacheMode, parent, + MythImage *im = LoadImage(painter, frameProps, cacheMode, proxy, aborted, imageReader); if (!im) @@ -428,58 +501,9 @@ class ImageLoader }; -QHash ImageLoader::m_loadingImages; -QMutex ImageLoader::m_loadingImagesLock; -QWaitCondition ImageLoader::m_loadingImagesCond; - -/*! - * \class ImageLoadEvent - */ -class ImageLoadEvent : public QEvent -{ - public: - ImageLoadEvent(const MythUIImage *parent, MythImage *image, - const QString &basefile, const QString &filename, - int number, bool aborted) - : QEvent(kEventType), - m_parent(parent), m_image(image), m_basefile(basefile), - m_filename(filename), m_number(number), - m_images(NULL), m_aborted(aborted) { } - - ImageLoadEvent(const MythUIImage *parent, AnimationFrames *frames, - const QString &basefile, - const QString &filename, bool aborted) - : QEvent(kEventType), - m_parent(parent), m_image(NULL), m_basefile(basefile), - m_filename(filename), m_number(0), - m_images(frames), m_aborted(aborted) { } - - const MythUIImage *GetParent() const { return m_parent; } - MythImage *GetImage() const { return m_image; } - const QString GetBasefile() const { return m_basefile; } - const QString GetFilename() const { return m_filename; } - int GetNumber() const { return m_number; } - AnimationFrames *GetAnimationFrames() const { return m_images; } - bool GetAbortState() const { return m_aborted; } - - static Type kEventType; - - private: - const MythUIImage *m_parent; - MythImage *m_image; - QString m_basefile; - QString m_filename; - int m_number; - - // Animated Images - AnimationFrames *m_images; - - // Image Load - bool m_aborted; -}; - -QEvent::Type ImageLoadEvent::kEventType = - (QEvent::Type) QEvent::registerEventType(); +QHash ImageLoader::m_loadingImages; +QMutex ImageLoader::m_loadingImagesLock; +QWaitCondition ImageLoader::m_loadingImagesCond; /*! * \class ImageLoadThread @@ -487,10 +511,10 @@ QEvent::Type ImageLoadEvent::kEventType = class ImageLoadThread : public QRunnable { public: - ImageLoadThread(const MythUIImage *parent, MythPainter *painter, + ImageLoadThread(const ProxyPtr &proxy, MythPainter *painter, const ImageProperties &imProps, const QString &basefile, int number, ImageCacheMode mode) : - m_parent(parent), m_painter(painter), m_imageProperties(imProps), + m_proxy(proxy), m_painter(painter), m_imageProperties(imProps), m_basefile(basefile), m_number(number), m_cacheMode(mode) { } @@ -500,6 +524,14 @@ class ImageLoadThread : public QRunnable bool aborted = false; QString filename = m_imageProperties.filename; + // Don't even bother if parent image no longer exists + if (!m_proxy || !m_proxy->GetImage()) + { + LOG(VB_GUI | VB_FILE, LOG_DEBUG, + QString("ImageLoadThread: Ignoring load of %1").arg(filename)); + return; + } + // NOTE Do NOT use MythImageReader::supportsAnimation here, it defeats // the point of caching remote images if (ImageLoader::SupportsAnimation(filename)) @@ -508,17 +540,16 @@ class ImageLoadThread : public QRunnable frames = ImageLoader::LoadAnimatedImage(m_painter, m_imageProperties, - m_cacheMode, m_parent, + m_cacheMode, m_proxy, aborted); if (frames && frames->count() > 1) { - ImageLoadEvent *le = new ImageLoadEvent(m_parent, frames, + ImageLoadEvent *le = new ImageLoadEvent(m_proxy->GetImage(), frames, m_basefile, m_imageProperties.filename, aborted); - QCoreApplication::postEvent(const_cast(m_parent), le); - + m_proxy->PostEvent(le); return; } delete frames; @@ -526,18 +557,19 @@ class ImageLoadThread : public QRunnable MythImage *image = ImageLoader::LoadImage(m_painter, m_imageProperties, - m_cacheMode, m_parent, + m_cacheMode, m_proxy, aborted); - ImageLoadEvent *le = new ImageLoadEvent(m_parent, image, m_basefile, + ImageLoadEvent *le = new ImageLoadEvent(m_proxy->GetImage(), image, + m_basefile, m_imageProperties.filename, m_number, aborted); - QCoreApplication::postEvent(const_cast(m_parent), le); + m_proxy->PostEvent(le); } private: - const MythUIImage *m_parent; - MythPainter *m_painter; + const ProxyPtr m_proxy; + MythPainter *m_painter; ImageProperties m_imageProperties; QString m_basefile; int m_number; @@ -549,13 +581,14 @@ class MythUIImagePrivate { public: explicit MythUIImagePrivate(MythUIImage *p) - : m_parent(p), m_UpdateLock(QReadWriteLock::Recursive) - { }; - ~MythUIImagePrivate() {}; - - MythUIImage *m_parent; + : m_parent(p), m_UpdateLock(QReadWriteLock::Recursive), + m_proxy(ProxyPtr(new ImageProxy(p))) + { } + ~MythUIImagePrivate() {} + MythUIImage *m_parent; QReadWriteLock m_UpdateLock; + ProxyPtr m_proxy; }; ///////////////////////////////////////////////////////////////// @@ -609,13 +642,8 @@ MythUIImage::MythUIImage(MythUIType *parent, const QString &name) MythUIImage::~MythUIImage() { - // Wait until all image loading threads are complete or bad things - // may happen if this MythUIImage disappears when a queued thread - // needs it. - if (m_runningThreads > 0) - { - GetMythUI()->GetImageThreadPool()->waitForDone(); - } + // Prevent any updates from running loads + d->m_proxy->Orphan(); Clear(); @@ -695,8 +723,6 @@ void MythUIImage::Init(void) m_animationReverse = false; m_animatedImage = false; - m_runningThreads = 0; - m_showingRandomImage = false; } @@ -1081,9 +1107,8 @@ bool MythUIImage::Load(bool allowLoadInBackground, bool forceStat) LOG(VB_GUI | VB_FILE, LOG_DEBUG, LOC + QString("Load(), spawning thread to load '%1'").arg(filename)); - m_runningThreads++; ImageLoadThread *bImgThread; - bImgThread = new ImageLoadThread(this, GetPainter(), + bImgThread = new ImageLoadThread(d->m_proxy, GetPainter(), imProps, bFilename, i, static_cast(cacheMode2)); @@ -1102,7 +1127,7 @@ bool MythUIImage::Load(bool allowLoadInBackground, bool forceStat) myFrames = ImageLoader::LoadAnimatedImage(GetPainter(), imProps, static_cast(cacheMode2), - this, aborted); + d->m_proxy, aborted); // TODO We might want to handle an abort here more gracefully if (aborted) @@ -1121,7 +1146,7 @@ bool MythUIImage::Load(bool allowLoadInBackground, bool forceStat) image = ImageLoader::LoadImage(GetPainter(), imProps, static_cast(cacheMode2), - this, aborted); + d->m_proxy, aborted); // TODO We might want to handle an abort here more gracefully if (aborted) @@ -1604,8 +1629,6 @@ void MythUIImage::customEvent(QEvent *event) animationFrames = le->GetAnimationFrames(); aborted = le->GetAbortState(); - m_runningThreads--; - d->m_UpdateLock.lockForRead(); QString propFilename = m_imageProperties.filename; d->m_UpdateLock.unlock(); diff --git a/mythtv/libs/libmythui/mythuiimage.h b/mythtv/libs/libmythui/mythuiimage.h index 7847c4a..e5390a1 100644 --- a/mythtv/libs/libmythui/mythuiimage.h +++ src/mythtv/libs/libmythui/mythuiimage.h @@ -174,8 +174,6 @@ class MUI_PUBLIC MythUIImage : public MythUIType ImageProperties m_imageProperties; - int m_runningThreads; - bool m_showingRandomImage; QString m_imageDirectory; -- 2.1.4