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

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<QDeclarativeImageProvider>(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<QDeclarativeImageProvider> provider = imageProviders.value(url.host());
+    locker.unlock();
     if (provider)
         return provider->imageType();
     return static_cast<QDeclarativeImageProvider::ImageType>(-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<QDeclarativeImageProvider> 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<QDeclarativeImageProvider> 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<QString,QDeclarativeImageProvider*> imageProviders;
+    QHash<QString,QSharedPointer<QDeclarativeImageProvider> > 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 <QtDeclarative/qdeclarativeimageprovider.h>
 #include <private/qdeclarativeimage_p.h>
 #include <QImageReader>
+#include <QWaitCondition>
 
 #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<int> 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<QDeclarativeImage *> images = obj->findChildren<QDeclarativeImage *>();
+    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)