From f894befedf669fb864a500b0aa395157ff0fb929 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Tue, 17 Aug 2010 15:38:33 +0200 Subject: QDeclarativeImageProvider: Do not keep the global declarative mutex locked when processing. The point is to be able to process images in a thread. If the mutex is locked, this is useless. Use case is a slow QDeclarativeImageProvider that generates thumbmails from large files. Even with the asynchronous attribute set to true, the gui thread would be blocked by the mutex. By using QSharedPointer, I also fix the leak of the providers (which were not deleted) Reviewed-by: Martin Jones --- src/declarative/qml/qdeclarativeengine.cpp | 15 +-- src/declarative/qml/qdeclarativeengine_p.h | 2 +- src/declarative/qml/qdeclarativeimageprovider.cpp | 4 +- .../tst_qdeclarativeimageprovider.cpp | 105 +++++++++++++++++++-- 4 files changed, 110 insertions(+), 16 deletions(-) diff --git a/src/declarative/qml/qdeclarativeengine.cpp b/src/declarative/qml/qdeclarativeengine.cpp index 513fc65..bc7468f 100644 --- a/src/declarative/qml/qdeclarativeengine.cpp +++ b/src/declarative/qml/qdeclarativeengine.cpp @@ -674,7 +674,7 @@ void QDeclarativeEngine::addImageProvider(const QString &providerId, QDeclarativ { Q_D(QDeclarativeEngine); QMutexLocker locker(&d->mutex); - d->imageProviders.insert(providerId, provider); + d->imageProviders.insert(providerId, QSharedPointer(provider)); } /*! @@ -684,7 +684,7 @@ QDeclarativeImageProvider *QDeclarativeEngine::imageProvider(const QString &prov { Q_D(const QDeclarativeEngine); QMutexLocker locker(&d->mutex); - return d->imageProviders.value(providerId); + return d->imageProviders.value(providerId).data(); } /*! @@ -698,13 +698,14 @@ void QDeclarativeEngine::removeImageProvider(const QString &providerId) { Q_D(QDeclarativeEngine); QMutexLocker locker(&d->mutex); - delete d->imageProviders.take(providerId); + d->imageProviders.take(providerId); } QDeclarativeImageProvider::ImageType QDeclarativeEnginePrivate::getImageProviderType(const QUrl &url) { QMutexLocker locker(&mutex); - QDeclarativeImageProvider *provider = imageProviders.value(url.host()); + QSharedPointer provider = imageProviders.value(url.host()); + locker.unlock(); if (provider) return provider->imageType(); return static_cast(-1); @@ -714,7 +715,8 @@ QImage QDeclarativeEnginePrivate::getImageFromProvider(const QUrl &url, QSize *s { QMutexLocker locker(&mutex); QImage image; - QDeclarativeImageProvider *provider = imageProviders.value(url.host()); + QSharedPointer provider = imageProviders.value(url.host()); + locker.unlock(); if (provider) image = provider->requestImage(url.path().mid(1), size, req_size); return image; @@ -724,7 +726,8 @@ QPixmap QDeclarativeEnginePrivate::getPixmapFromProvider(const QUrl &url, QSize { QMutexLocker locker(&mutex); QPixmap pixmap; - QDeclarativeImageProvider *provider = imageProviders.value(url.host()); + QSharedPointer provider = imageProviders.value(url.host()); + locker.unlock(); if (provider) pixmap = provider->requestPixmap(url.path().mid(1), size, req_size); return pixmap; diff --git a/src/declarative/qml/qdeclarativeengine_p.h b/src/declarative/qml/qdeclarativeengine_p.h index 3b5dd5a..db2db35 100644 --- a/src/declarative/qml/qdeclarativeengine_p.h +++ b/src/declarative/qml/qdeclarativeengine_p.h @@ -232,7 +232,7 @@ public: mutable QNetworkAccessManager *networkAccessManager; mutable QDeclarativeNetworkAccessManagerFactory *networkAccessManagerFactory; - QHash imageProviders; + QHash > imageProviders; QDeclarativeImageProvider::ImageType getImageProviderType(const QUrl &url); QImage getImageFromProvider(const QUrl &url, QSize *size, const QSize& req_size); QPixmap getPixmapFromProvider(const QUrl &url, QSize *size, const QSize& req_size); diff --git a/src/declarative/qml/qdeclarativeimageprovider.cpp b/src/declarative/qml/qdeclarativeimageprovider.cpp index ea68327..ef31be7 100644 --- a/src/declarative/qml/qdeclarativeimageprovider.cpp +++ b/src/declarative/qml/qdeclarativeimageprovider.cpp @@ -161,7 +161,9 @@ QDeclarativeImageProvider::QDeclarativeImageProvider(ImageType type) } /*! - \internal + Destroys the QDeclarativeImageProvider + + \note The destructor of your derived class need to be thread safe. */ QDeclarativeImageProvider::~QDeclarativeImageProvider() { diff --git a/tests/auto/declarative/qdeclarativeimageprovider/tst_qdeclarativeimageprovider.cpp b/tests/auto/declarative/qdeclarativeimageprovider/tst_qdeclarativeimageprovider.cpp index e0b46f0..4a9224e 100644 --- a/tests/auto/declarative/qdeclarativeimageprovider/tst_qdeclarativeimageprovider.cpp +++ b/tests/auto/declarative/qdeclarativeimageprovider/tst_qdeclarativeimageprovider.cpp @@ -44,6 +44,7 @@ #include #include #include +#include #ifdef Q_OS_SYMBIAN // In Symbian OS test data is located in applications private dir @@ -85,6 +86,8 @@ private slots: void removeProvider_data(); void removeProvider(); + void threadTest(); + private: QString newImageFileName() const; void fillRequestTestsData(const QString &id); @@ -95,9 +98,15 @@ private: class TestQImageProvider : public QDeclarativeImageProvider { public: - TestQImageProvider() - : QDeclarativeImageProvider(Image) + TestQImageProvider(bool *deleteWatch = 0) + : QDeclarativeImageProvider(Image), deleteWatch(deleteWatch) + { + } + + ~TestQImageProvider() { + if (deleteWatch) + *deleteWatch = true; } QImage requestImage(const QString &id, QSize *size, const QSize& requestedSize) @@ -114,6 +123,8 @@ public: image = image.scaled(requestedSize); return image; } + + bool *deleteWatch; }; Q_DECLARE_METATYPE(TestQImageProvider*); @@ -121,11 +132,17 @@ Q_DECLARE_METATYPE(TestQImageProvider*); class TestQPixmapProvider : public QDeclarativeImageProvider { public: - TestQPixmapProvider() - : QDeclarativeImageProvider(Pixmap) + TestQPixmapProvider(bool *deleteWatch = 0) + : QDeclarativeImageProvider(Pixmap), deleteWatch(deleteWatch) { } + ~TestQPixmapProvider() + { + if (deleteWatch) + *deleteWatch = true; + } + QPixmap requestPixmap(const QString &id, QSize *size, const QSize& requestedSize) { if (id == QLatin1String("no-such-file.png")) @@ -140,6 +157,8 @@ public: image = image.scaled(requestedSize); return image; } + + bool *deleteWatch; }; Q_DECLARE_METATYPE(TestQPixmapProvider*); @@ -225,7 +244,9 @@ void tst_qdeclarativeimageprovider::requestImage_sync_data() void tst_qdeclarativeimageprovider::requestImage_sync() { - runTest(false, new TestQImageProvider); + bool deleteWatch = false; + runTest(false, new TestQImageProvider(&deleteWatch)); + QVERIFY(deleteWatch); } void tst_qdeclarativeimageprovider::requestImage_async_data() @@ -235,7 +256,9 @@ void tst_qdeclarativeimageprovider::requestImage_async_data() void tst_qdeclarativeimageprovider::requestImage_async() { - runTest(true, new TestQImageProvider); + bool deleteWatch = false; + runTest(true, new TestQImageProvider(&deleteWatch)); + QVERIFY(deleteWatch); } void tst_qdeclarativeimageprovider::requestPixmap_sync_data() @@ -245,13 +268,15 @@ void tst_qdeclarativeimageprovider::requestPixmap_sync_data() void tst_qdeclarativeimageprovider::requestPixmap_sync() { - runTest(false, new TestQPixmapProvider); + bool deleteWatch = false; + runTest(false, new TestQPixmapProvider(&deleteWatch)); + QVERIFY(deleteWatch); } void tst_qdeclarativeimageprovider::requestPixmap_async() { QDeclarativeEngine engine; - QDeclarativeImageProvider *provider = new TestQPixmapProvider; + QDeclarativeImageProvider *provider = new TestQPixmapProvider(); engine.addImageProvider("test", provider); QVERIFY(engine.imageProvider("test") != 0); @@ -305,6 +330,70 @@ void tst_qdeclarativeimageprovider::removeProvider() delete obj; } +class TestThreadProvider : public QDeclarativeImageProvider +{ + public: + TestThreadProvider() : QDeclarativeImageProvider(Image), ok(false) {} + + ~TestThreadProvider() {} + + QImage requestImage(const QString &id, QSize *size, const QSize& requestedSize) + { + mutex.lock(); + if (!ok) + cond.wait(&mutex); + mutex.unlock(); + QVector v; + for (int i = 0; i < 10000; i++) + v.prepend(i); //do some computation + QImage image(50,50, QImage::Format_RGB32); + image.fill(QColor(id).rgb()); + if (size) + *size = image.size(); + if (requestedSize.isValid()) + image = image.scaled(requestedSize); + return image; + } + + QWaitCondition cond; + QMutex mutex; + bool ok; +}; + + +void tst_qdeclarativeimageprovider::threadTest() +{ + QDeclarativeEngine engine; + + TestThreadProvider *provider = new TestThreadProvider; + + engine.addImageProvider("test_thread", provider); + QVERIFY(engine.imageProvider("test_thread") != 0); + + QString componentStr = "import Qt 4.7\nItem { \n" + "Image { source: \"image://test_thread/blue\"; asynchronous: true; }\n" + "Image { source: \"image://test_thread/red\"; asynchronous: true; }\n" + "Image { source: \"image://test_thread/green\"; asynchronous: true; }\n" + "Image { source: \"image://test_thread/yellow\"; asynchronous: true; }\n" + " }"; + QDeclarativeComponent component(&engine); + component.setData(componentStr.toLatin1(), QUrl::fromLocalFile("")); + QObject *obj = component.create(); + //MUST not deadlock + QVERIFY(obj != 0); + QList images = obj->findChildren(); + QCOMPARE(images.count(), 4); + QTest::qWait(100); + foreach(QDeclarativeImage *img, images) { + QCOMPARE(img->status(), QDeclarativeImage::Loading); + } + provider->ok = true; + provider->cond.wakeAll(); + foreach(QDeclarativeImage *img, images) { + TRY_WAIT(img->status() == QDeclarativeImage::Ready); + } +} + QTEST_MAIN(tst_qdeclarativeimageprovider) -- cgit v0.12