Pass back QImage from thread repeatedly, deletes, proper way

[Linwood]

I am going around in circles and hope someone can help. I have a much more complicated routine but have been able to distill it, mostly, so I can post the code.

The core of the problem is I am using poppler-qt to render PDF's, and that provides a QImage as output of a page. I am rendering in many threads at once, and trying to pass the images back to the main thread via signals/slots. This is modeled, somewhat, from the Mandelbrot example, though it immediately loads the QImage into a Pixmap and does not use it again. I need to save many of them.

These are loaded to an array of QImage*, and when I want to replace one (e.g. different scaling) I want to appropriately delete the old. I say appropriately as it is a bit unclear if I need to delete it manually, but if I do not it leaks memory (hugely so, not subtle, as shown in this loop below).

If I delete it, I get a segmentation fault and crash. I have read other threads about this, but without clear solution (most steered to alternative approaches).

It seems specific to the threading; if I do a fake threaded routine instead (i.e. call on the main thread), still using a signal/slot, the delete works. That routine is shown also but the call commented.

The failure is a bit fragile in this distilled version. Some little things, like removing a widget update, can make it work, sometimes, at least for a while. But it is almost completely reproducible for me in this code. Build has c++11 added as a config option.

This is happening with QT 5.5.1 (GCC 5.2.1 20151129) on Ubuntu 16.04 in a HyperV VM Guest, with 16GB memory and 3 cores. It uses the distro version of everything involved, and is up to date as of today.

The program is written to also be a stress test of sorts, and continue requesting pages rapidly on the main thread; it never uses them, it just deletes them when the next is requested.

I've left in a few comments showing some alternatives, e.g. allocating the QImage in the thread with new vs a local copy, that seem to make no difference (other than a need to adjust from pointers to non in the associated code).

As best I can tell, other than the possible shallow copy of the QImage, I do not think the worker thread is accessing anything that the primary thread is using. I have read about the shallow copy feature of QImage, and the double buffered aspect of the signal, but without it really helping me understand if this is a permitted mechanism or not (but then there's that official sample that seems to do the same).

My assumption is the failure is due to deleting something that no longer exists -- but if so, why does it build up memory usage indefinitely if I do not delete it? Is that double buffering perhaps breaking the reference count links?

Is there a more appropriate way to get that QImage back to the main thread? Should I pass in the address of where I want it, and let the thread load it directly? (and protect it with a mutex?)?

If anyone is bored enough to run the code, does it work for you, i.e. fail?

Thanks,

Linwood

mainwindow.h

#ifndef MAINWINDOW_H
#define MAINWINDOW_H
 
#include <QMainWindow>
#include <QLabel>
#include <QImage>
#include <QWidget>
#include <QTimer>
#include <QApplication>
 
 
#include "renderthread.h"
 
class MainWindow : public QMainWindow
{
    Q_OBJECT
 
public:
    MainWindow(QWidget *parent = 0);
    void doStuff();
private:
    QLabel outer;
    QTimer t;
    void requestPage();
    const QImage *pageImages;
    int pagesRequested;
    int pagesRetrieved;
    renderThread *pageThreads;
    bool pageThreadActive;
private slots:
    void updateImage(const QImage &image);
};
#endif // MAINWINDOW_H

#ifndef MAINWINDOW_H
#define MAINWINDOW_H

#include <QMainWindow>
#include <QLabel>
#include <QImage>
#include <QWidget>
#include <QTimer>
#include <QApplication>


#include "renderthread.h"

class MainWindow : public QMainWindow
{
    Q_OBJECT

public:
    MainWindow(QWidget *parent = 0);
    void doStuff();
private:
    QLabel outer;
    QTimer t;
    void requestPage();
    const QImage *pageImages;
    int pagesRequested;
    int pagesRetrieved;
    renderThread *pageThreads;
    bool pageThreadActive;
private slots:
    void updateImage(const QImage &image);
};
#endif // MAINWINDOW_H

To copy to clipboard, switch view to plain text mode 

renderthread.h

#ifndef RENDERTHREAD_H
#define RENDERTHREAD_H
 
#include <QThread>
#include <QMutex>
#include <QImage>
#include <QWaitCondition>
#include <cassert>
 
 
class renderThread : public QThread
{
    Q_OBJECT
 
public:
    renderThread(QObject *parent = 0);
    void render();
    void fake_run();
 
signals:
    void renderedImage(const QImage &image);
 
protected:
    void run() Q_DECL_OVERRIDE;
 
private:
    QMutex mutex;
    QWaitCondition condition;
//    QImage theImage;
    QImage *theImagePtr;
};
 
#endif // RENDERTHREAD_H

#ifndef RENDERTHREAD_H
#define RENDERTHREAD_H

#include <QThread>
#include <QMutex>
#include <QImage>
#include <QWaitCondition>
#include <cassert>


class renderThread : public QThread
{
    Q_OBJECT

public:
    renderThread(QObject *parent = 0);
    void render();
    void fake_run();

signals:
    void renderedImage(const QImage &image);

protected:
    void run() Q_DECL_OVERRIDE;

private:
    QMutex mutex;
    QWaitCondition condition;
//    QImage theImage;
    QImage *theImagePtr;
};

#endif // RENDERTHREAD_H

To copy to clipboard, switch view to plain text mode 

main.cpp

#include <QApplication>
#include "mainwindow.h"
 
 
int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    MainWindow w;
    w.setMinimumSize(600,20);
    w.show();
    return a.exec();
}

#include <QApplication>
#include "mainwindow.h"


int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    MainWindow w;
    w.setMinimumSize(600,20);
    w.show();
    return a.exec();
}

To copy to clipboard, switch view to plain text mode 

mainwindow.cpp

#include "mainwindow.h"
 
MainWindow::MainWindow(QWidget *parent) : QMainWindow(parent)
{
    outer.setParent(this);
    this->setCentralWidget(&outer);
 
    pageImages = NULL;
    pageThreads = new renderThread();
    connect(pageThreads, SIGNAL(renderedImage(const QImage &)), this, SLOT(updateImage(const QImage &)) );
    pageThreadActive=false;
    pagesRequested=0;
    pagesRetrieved=0;
 
    t.setInterval(1);
    connect(&t,&QTimer::timeout, this, [this]{this->doStuff();});
    t.start();
}
void MainWindow::doStuff()
{
    requestPage();
    outer.setText(QString("Requested=%1, Retrieved=%2").arg(pagesRequested).arg(pagesRetrieved));
    outer.update();  // If this is removed the delete works also.
}
 
void MainWindow::requestPage()
{
    if(!pageThreadActive)
    {
        pageThreadActive=true;
        //pageThreads->fake_run();  // this does a retrieve without using the thread, and works with the delete in place
        pageThreads->render();  // << this threaded one works only if delete is not in place below
        pagesRequested++;
    }
}
 
// Slot
void MainWindow::updateImage(const QImage &image)
{
    if(pageImages)
    {
        delete pageImages;  //  <<  If removed no crash but bad memory leak, if in place crash (sometimes)
        pageImages=NULL;
    }
    pageImages= &image;
    pageThreadActive = false;
    pagesRetrieved++;
}

#include "mainwindow.h"

MainWindow::MainWindow(QWidget *parent) : QMainWindow(parent)
{
    outer.setParent(this);
    this->setCentralWidget(&outer);

    pageImages = NULL;
    pageThreads = new renderThread();
    connect(pageThreads, SIGNAL(renderedImage(const QImage &)), this, SLOT(updateImage(const QImage &)) );
    pageThreadActive=false;
    pagesRequested=0;
    pagesRetrieved=0;

    t.setInterval(1);
    connect(&t,&QTimer::timeout, this, [this]{this->doStuff();});
    t.start();
}
void MainWindow::doStuff()
{
    requestPage();
    outer.setText(QString("Requested=%1, Retrieved=%2").arg(pagesRequested).arg(pagesRetrieved));
    outer.update();  // If this is removed the delete works also.
}

void MainWindow::requestPage()
{
    if(!pageThreadActive)
    {
        pageThreadActive=true;
        //pageThreads->fake_run();  // this does a retrieve without using the thread, and works with the delete in place
        pageThreads->render();  // << this threaded one works only if delete is not in place below
        pagesRequested++;
    }
}

// Slot
void MainWindow::updateImage(const QImage &image)
{
    if(pageImages)
    {
        delete pageImages;  //  <<  If removed no crash but bad memory leak, if in place crash (sometimes)
        pageImages=NULL;
    }
    pageImages= &image;
    pageThreadActive = false;
    pagesRetrieved++;
}

To copy to clipboard, switch view to plain text mode 

renderthread.cpp

#include "renderthread.h"
 
renderThread::renderThread(QObject *parent): QThread(parent)
{
}
 
void renderThread::render()
{
    mutex.lock(); // shouldn't be necessary but want to make sure the emit completes before we issue wakeOne
    if(!isRunning())  start(LowPriority);
    else condition.wakeOne();
    mutex.unlock();
    return;
}
 
void renderThread::run()
{
    forever
    {
//      theImage = QImage(1000,1000,QImage::Format_ARGB32_Premultiplied);  // same thing happens if declaration is not a pointer and appropriate other changes made
        theImagePtr = new QImage(1000,1000,QImage::Format_ARGB32_Premultiplied);
        mutex.lock();
        emit renderedImage(*theImagePtr);
        condition.wait(&mutex);
        mutex.unlock();
    }
}
 
void renderThread::fake_run()
{
    theImagePtr = new QImage(1000,1000,QImage::Format_ARGB32_Premultiplied);
    emit renderedImage(*theImagePtr);
}

#include "renderthread.h"

renderThread::renderThread(QObject *parent): QThread(parent)
{
}

void renderThread::render()
{
    mutex.lock(); // shouldn't be necessary but want to make sure the emit completes before we issue wakeOne
    if(!isRunning())  start(LowPriority);
    else condition.wakeOne();
    mutex.unlock();
    return;
}

void renderThread::run()
{
    forever
    {
//      theImage = QImage(1000,1000,QImage::Format_ARGB32_Premultiplied);  // same thing happens if declaration is not a pointer and appropriate other changes made
        theImagePtr = new QImage(1000,1000,QImage::Format_ARGB32_Premultiplied);
        mutex.lock();
        emit renderedImage(*theImagePtr);
        condition.wait(&mutex);
        mutex.unlock();
    }
}

void renderThread::fake_run()
{
    theImagePtr = new QImage(1000,1000,QImage::Format_ARGB32_Premultiplied);
    emit renderedImage(*theImagePtr);
}

To copy to clipboard, switch view to plain text mode 

[Linwood]

In the FWIW department I reconstructed this so that the address of the pointer to the main thread's copy of the QImage is passed into the thread, and it updates the pointer directly.

Doing it that way, I am able to delete against this pointer without crashing, and many thousands of iterations does not show any memory growth.

Maybe that's the proper way, though that doesn't explain the official (?) example doing it this way, unless I did not copy it correctly.

[d_stranz]

theImagePtr = new QImage(1000,1000,QImage::Format_ARGB32_Premultipli ed);

This is your memory leak. Your signal passes a reference to the QImage back to the main thread, not a pointer, and this new allocation never gets deleted.

pageImages= ℑ

And this line is just crazy. You're trying to assign the address of a temporary variable created in another thread to a pointer variable held by the main thread. You aren't making a copy of the image, you're making a copy of the address. When the image at that address goes out of scope (as it does in the version with line 20 commented out in renderthread.cpp) pageImages will point to deleted memory.

The proper way to do this is as they do it in the Mandelbrot example:

- create the QImage as a stack variable (not a pointer variable) in the thread class
- pass that variable (as a const reference: const QImage &) in the signal
- in your main thread slot, copy that QImage into another QImage variable (not a pointer).

And keep in mind if you have multiple render threads, they will almost certainly signal your main thread out of order, so you'll need to include a page number as part of the signal, I think.

[Linwood]

- create the QImage as a stack variable (not a pointer variable) in the thread class

(My C++ programming days date to about 1999, so there's a whole lot of refreshing memory going on, and not the virtual kind, so this may be digging my hole deeper still...).

OK, but...

The Mandelbrot example is not allocating on the stack either. It does:

QImage image(resultSize, QImage::Format_RGB32);

QImage image(resultSize, QImage::Format_RGB32);

To copy to clipboard, switch view to plain text mode 

inside the run(), and then

emit renderedImage(image, scaleFactor);

emit renderedImage(image, scaleFactor);

To copy to clipboard, switch view to plain text mode 

(With the renderedImage() declared as passing the QImage as &image)

and then in the main thread uses it directly:

pixmap = QPixmap::fromImage(image);

pixmap = QPixmap::fromImage(image);

To copy to clipboard, switch view to plain text mode 

This is your memory leak. Your signal passes a reference to the QImage back to the main thread, not a pointer, and this new allocation never gets deleted.

I get why that would leak, but not why I can't delete it. If I'm passing a reference to the QImage, then deleting against that - why does it crash (and why only if it is passed up through the tread and not by a direct call)? Isn't the reference to the QImage I passed out a reference that preserves it from automatic deletion, and shouldn't it be available for a manual delete?

As I mentioned in the second posting I found a way to do it more efficiently by just letting it (suitably protected by mutex) update the main thread's data structure directly from the thread, and just emit a signal to say it's ready. But I remain a bit confused why the program above doesn't allow the delete without crashing?

[d_stranz]

QImage image(resultSize, QImage::Format_RGB32);

Sorry, but this is an allocation on the stack. Dig into your C++ archive a bit deeper. That line of code constructs a QImage within the scope of the function where it is defined. That instance is created on the stack. When the function exits, the stack unwinds, the instance goes out of scope, and is deleted.

emit renderedImage(image, scaleFactor);

This is just Qt "syntactic sugar". A Qt signal is nothing more than a C++ method call. It is synchronous and is handled by your slot (which should make a copy of the image content, not the image address) prior to it going out of scope.

If I'm passing a reference to the QImage, then deleting against that - why does it crash

You may be passing a reference to the QImage (via *image), but then in your main thread, you are assigning the address of that reference (i.e. a pointer to the QImage created in and *owned by* your thread) to your pointer variable in the main thread. So long as the instance owned by the render thread is alive and in scope, you can dereference that pointer in your main thread (via operator->()), maybe safely, maybe not - it depends what the thread is doing to that instance. operator->() isn't protected by a mutex.

But you can't call operator delete() on it in the main thread because the memory it points to isn't owned by your main thread. The only place you can delete it is in the render thread, and if you do that, then the address you have copied into the pointer variable in your main thread will no longer be valid and the main thread will crash as soon as it tries to dereference the pointer.

As I mentioned in the second posting I found a way to do it more efficiently by just letting it (suitably protected by mutex) update the main thread's data structure directly from the thread

Which may work, so long as your main thread isn't doing anything to that QImage instance. As I said, the safest thing is to follow the Mandelbrot example exactly and pass a reference to a stack-based QImage owned by the thread, then make a copy of that QImage into a QImage variable in your main thread.

If you need to keep more than one QImage instance around in your main thread, you can put them into a QVector< QImage > (in which the QImage instances are stack based), or allocate new QImage instances on the heap (using QImage * pImage = new QImage( imageReferenceFromSignal )) and pushing those onto a QVector< QImage * >. In the latter case, you will need to manually delete these QImage instances yourself when you remove them from the QVector; in the former case, QVector::erase(), clear(), and other manipulators will result in the stack-based instance going out of scope and automatically being destroyed.

[Linwood]

Sorry, but this is an allocation on the stack. Dig into your C++ archive a bit deeper.
...

Indeed. Too many years in between, some pollution from C# added to the mix. I need to read through this again and do a bit of research into the "syntactical sugar" in particular. I'm still puzzled a bit by how the actual threading played a role, I do want to find that discussion again about queued vs direct connections and how objects are passed. At best I'm confused by it (and how it affects holding a reference to an object and whether that allows it to delete an object out from under me differently when threaded).

Thank you for taking the time to provide some pointers (no pun intended).

I think I have it working with a suitably synchronized thread just updating a cache in the main thread with images when needed, primarily as I think it may be more efficient (if nothing else the copies are made in parallel). And it ran fairly nicely on the Pi in some brief testing, and high volume testing on a faster system showed no leaks.

Thanks again.

Linwood