Results 1 to 6 of 6

Thread: Pass back QImage from thread repeatedly, deletes, proper way

  1. #1
    Join Date
    Nov 2016
    Location
    Florida, US
    Posts
    26
    Thanks
    7
    Qt products
    Qt5
    Platforms
    Unix/X11

    Default Pass back QImage from thread repeatedly, deletes, proper way

    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

    Qt Code:
    1. #ifndef MAINWINDOW_H
    2. #define MAINWINDOW_H
    3.  
    4. #include <QMainWindow>
    5. #include <QLabel>
    6. #include <QImage>
    7. #include <QWidget>
    8. #include <QTimer>
    9. #include <QApplication>
    10.  
    11.  
    12. #include "renderthread.h"
    13.  
    14. class MainWindow : public QMainWindow
    15. {
    16. Q_OBJECT
    17.  
    18. public:
    19. MainWindow(QWidget *parent = 0);
    20. void doStuff();
    21. private:
    22. QLabel outer;
    23. QTimer t;
    24. void requestPage();
    25. const QImage *pageImages;
    26. int pagesRequested;
    27. int pagesRetrieved;
    28. renderThread *pageThreads;
    29. bool pageThreadActive;
    30. private slots:
    31. void updateImage(const QImage &image);
    32. };
    33. #endif // MAINWINDOW_H
    To copy to clipboard, switch view to plain text mode 

    renderthread.h

    Qt Code:
    1. #ifndef RENDERTHREAD_H
    2. #define RENDERTHREAD_H
    3.  
    4. #include <QThread>
    5. #include <QMutex>
    6. #include <QImage>
    7. #include <QWaitCondition>
    8. #include <cassert>
    9.  
    10.  
    11. class renderThread : public QThread
    12. {
    13. Q_OBJECT
    14.  
    15. public:
    16. renderThread(QObject *parent = 0);
    17. void render();
    18. void fake_run();
    19.  
    20. signals:
    21. void renderedImage(const QImage &image);
    22.  
    23. protected:
    24. void run() Q_DECL_OVERRIDE;
    25.  
    26. private:
    27. QMutex mutex;
    28. QWaitCondition condition;
    29. // QImage theImage;
    30. QImage *theImagePtr;
    31. };
    32.  
    33. #endif // RENDERTHREAD_H
    To copy to clipboard, switch view to plain text mode 

    main.cpp

    Qt Code:
    1. #include <QApplication>
    2. #include "mainwindow.h"
    3.  
    4.  
    5. int main(int argc, char *argv[])
    6. {
    7. QApplication a(argc, argv);
    8. MainWindow w;
    9. w.setMinimumSize(600,20);
    10. w.show();
    11. return a.exec();
    12. }
    To copy to clipboard, switch view to plain text mode 

    mainwindow.cpp

    Qt Code:
    1. #include "mainwindow.h"
    2.  
    3. MainWindow::MainWindow(QWidget *parent) : QMainWindow(parent)
    4. {
    5. outer.setParent(this);
    6. this->setCentralWidget(&outer);
    7.  
    8. pageImages = NULL;
    9. pageThreads = new renderThread();
    10. connect(pageThreads, SIGNAL(renderedImage(const QImage &)), this, SLOT(updateImage(const QImage &)) );
    11. pageThreadActive=false;
    12. pagesRequested=0;
    13. pagesRetrieved=0;
    14.  
    15. t.setInterval(1);
    16. connect(&t,&QTimer::timeout, this, [this]{this->doStuff();});
    17. t.start();
    18. }
    19. void MainWindow::doStuff()
    20. {
    21. requestPage();
    22. outer.setText(QString("Requested=%1, Retrieved=%2").arg(pagesRequested).arg(pagesRetrieved));
    23. outer.update(); // If this is removed the delete works also.
    24. }
    25.  
    26. void MainWindow::requestPage()
    27. {
    28. if(!pageThreadActive)
    29. {
    30. pageThreadActive=true;
    31. //pageThreads->fake_run(); // this does a retrieve without using the thread, and works with the delete in place
    32. pageThreads->render(); // << this threaded one works only if delete is not in place below
    33. pagesRequested++;
    34. }
    35. }
    36.  
    37. // Slot
    38. void MainWindow::updateImage(const QImage &image)
    39. {
    40. if(pageImages)
    41. {
    42. delete pageImages; // << If removed no crash but bad memory leak, if in place crash (sometimes)
    43. pageImages=NULL;
    44. }
    45. pageImages= &image;
    46. pageThreadActive = false;
    47. pagesRetrieved++;
    48. }
    To copy to clipboard, switch view to plain text mode 

    renderthread.cpp

    Qt Code:
    1. #include "renderthread.h"
    2.  
    3. renderThread::renderThread(QObject *parent): QThread(parent)
    4. {
    5. }
    6.  
    7. void renderThread::render()
    8. {
    9. mutex.lock(); // shouldn't be necessary but want to make sure the emit completes before we issue wakeOne
    10. if(!isRunning()) start(LowPriority);
    11. else condition.wakeOne();
    12. mutex.unlock();
    13. return;
    14. }
    15.  
    16. void renderThread::run()
    17. {
    18. forever
    19. {
    20. // theImage = QImage(1000,1000,QImage::Format_ARGB32_Premultiplied); // same thing happens if declaration is not a pointer and appropriate other changes made
    21. theImagePtr = new QImage(1000,1000,QImage::Format_ARGB32_Premultiplied);
    22. mutex.lock();
    23. emit renderedImage(*theImagePtr);
    24. condition.wait(&mutex);
    25. mutex.unlock();
    26. }
    27. }
    28.  
    29. void renderThread::fake_run()
    30. {
    31. theImagePtr = new QImage(1000,1000,QImage::Format_ARGB32_Premultiplied);
    32. emit renderedImage(*theImagePtr);
    33. }
    To copy to clipboard, switch view to plain text mode 

  2. #2
    Join Date
    Nov 2016
    Location
    Florida, US
    Posts
    26
    Thanks
    7
    Qt products
    Qt5
    Platforms
    Unix/X11

    Default Re: Pass back QImage from thread repeatedly, deletes, proper way

    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.

  3. #3
    Join Date
    Jan 2008
    Location
    Alameda, CA, USA
    Posts
    4,111
    Thanks
    235
    Thanked 654 Times in 644 Posts
    Qt products
    Qt5
    Platforms
    Windows Android

    Default Re: Pass back QImage from thread repeatedly, deletes, proper way

    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= &image;
    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.
    <=== The Great Pumpkin says ===>
    Please use CODE tags when posting source code so it is more readable. Click "Go Advanced" and then the "#" icon to insert the tags. Paste your code between them.

  4. #4
    Join Date
    Nov 2016
    Location
    Florida, US
    Posts
    26
    Thanks
    7
    Qt products
    Qt5
    Platforms
    Unix/X11

    Default Re: Pass back QImage from thread repeatedly, deletes, proper way

    Quote Originally Posted by d_stranz View Post
    - 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:

    Qt Code:
    1. QImage image(resultSize, QImage::Format_RGB32);
    To copy to clipboard, switch view to plain text mode 
    inside the run(), and then

    Qt Code:
    1. 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:

    Qt Code:
    1. pixmap = QPixmap::fromImage(image);
    To copy to clipboard, switch view to plain text mode 
    Quote Originally Posted by d_stranz View Post
    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?

  5. #5
    Join Date
    Jan 2008
    Location
    Alameda, CA, USA
    Posts
    4,111
    Thanks
    235
    Thanked 654 Times in 644 Posts
    Qt products
    Qt5
    Platforms
    Windows Android

    Default Re: Pass back QImage from thread repeatedly, deletes, proper way

    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.
    Last edited by d_stranz; 10th November 2016 at 17:16.
    <=== The Great Pumpkin says ===>
    Please use CODE tags when posting source code so it is more readable. Click "Go Advanced" and then the "#" icon to insert the tags. Paste your code between them.

  6. #6
    Join Date
    Nov 2016
    Location
    Florida, US
    Posts
    26
    Thanks
    7
    Qt products
    Qt5
    Platforms
    Unix/X11

    Default Re: Pass back QImage from thread repeatedly, deletes, proper way

    Quote Originally Posted by d_stranz View Post
    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

Similar Threads

  1. QImage deconstructor or delete / proper usage
    By chrisbay90 in forum Qt Programming
    Replies: 2
    Last Post: 14th July 2015, 19:01
  2. Replies: 1
    Last Post: 28th March 2012, 18:58
  3. QGraphicsPixmapItem to QPixmap to QImage and back again!
    By bruceariggs in forum Qt Programming
    Replies: 2
    Last Post: 10th September 2011, 14:08
  4. Replies: 7
    Last Post: 12th August 2006, 15:11
  5. Replies: 1
    Last Post: 14th June 2006, 08:12

Tags for this Thread

Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  
Digia, Qt and their respective logos are trademarks of Digia Plc in Finland and/or other countries worldwide.