QtConcurrent::mapped() and member functions

[h.a.n.d]

Hi,

I'm trying to convert the "imagescaling" example http://doc.qt.io/qt-5/qtconcurrent-imagescaling-example.html to work with a call to a member function instead of the global one.

Here is the modified code:

imagescaling.cpp


#include "imagescaling.h"
#include <qmath.h>

const int imageSize = 100;

QImage* scale(const QString &imageFileName)
{
QImage* image = new QImage(imageFileName);
image->scaled(QSize(imageSize, imageSize), Qt::IgnoreAspectRatio, Qt::SmoothTransformation);
return image;
}

Images::Images(QWidget *parent)
: QWidget(parent)
{
setWindowTitle(tr("Image loading and scaling example"));
resize(800, 600);

imageScaling = new QFutureWatcher<QImage*>(this);
connect(imageScaling, SIGNAL(resultReadyAt(int)), SLOT(showImage(int)));
connect(imageScaling, SIGNAL(finished()), SLOT(finished()));

openButton = new QPushButton(tr("Open Images"));
connect(openButton, SIGNAL(clicked()), SLOT(open()));

cancelButton = new QPushButton(tr("Cancel"));
cancelButton->setEnabled(false);
connect(cancelButton, SIGNAL(clicked()), imageScaling, SLOT(cancel()));

pauseButton = new QPushButton(tr("Pause/Resume"));
pauseButton->setEnabled(false);
connect(pauseButton, SIGNAL(clicked()), imageScaling, SLOT(togglePaused()));

QHBoxLayout *buttonLayout = new QHBoxLayout();
buttonLayout->addWidget(openButton);
buttonLayout->addWidget(cancelButton);
buttonLayout->addWidget(pauseButton);
buttonLayout->addStretch();

imagesLayout = new QGridLayout();

mainLayout = new QVBoxLayout();
mainLayout->addLayout(buttonLayout);
mainLayout->addLayout(imagesLayout);
mainLayout->addStretch();
setLayout(mainLayout);
}

Images::~Images()
{
imageScaling->cancel();
imageScaling->waitForFinished();
}

QImage* Images::scale_(const QString &imageFileName)
{
QImage* image = new QImage(imageFileName);
image->scaled(QSize(imageSize, imageSize), Qt::IgnoreAspectRatio, Qt::SmoothTransformation);
return image;
}

void Images::open()
{
// Cancel and wait if we are already loading images.
if (imageScaling->isRunning()) {
imageScaling->cancel();
imageScaling->waitForFinished();
}

// Show a file open dialog at QStandardPaths::PicturesLocation.
QStringList files = QFileDialog::getOpenFileNames(this, tr("Select Images"),
QStandardPaths::writableLocation(QStandardPaths::P icturesLocation),
"*.jpg *.png");

if (files.count() == 0)
return;

// Do a simple layout.
qDeleteAll(labels);
labels.clear();

int dim = qSqrt(qreal(files.count())) + 1;
for (int i = 0; i < dim; ++i) {
for (int j = 0; j < dim; ++j) {
QLabel *imageLabel = new QLabel;
imageLabel->setFixedSize(imageSize,imageSize);
imagesLayout->addWidget(imageLabel,i,j);
labels.append(imageLabel);
}
}

// Use mapped to run the thread safe scale function on the files.
//imageScaling->setFuture(QtConcurrent::mapped(files, scale));
imageScaling->setFuture(QtConcurrent::mapped(files, [this](const QString &file){this->scale_(file);}));

openButton->setEnabled(false);
cancelButton->setEnabled(true);
pauseButton->setEnabled(true);
}

void Images::showImage(int num)
{
labels[num]->setPixmap(QPixmap::fromImage(*(imageScaling->resultAt(num))));
}

void Images::finished()
{
openButton->setEnabled(true);
cancelButton->setEnabled(false);
pauseButton->setEnabled(false);
}


imagescaling.h



#ifndef IMAGESCALING_H
#define IMAGESCALING_H

#include <QtWidgets>
#include <QtConcurrent>

class Images : public QWidget
{
Q_OBJECT
public:
Images(QWidget *parent = 0);
~Images();
QImage *scale_(const QString &imageFileName);
public Q_SLOTS:
void open();
void showImage(int num);
void finished();
private:
QPushButton *openButton;
QPushButton *cancelButton;
QPushButton *pauseButton;
QVBoxLayout *mainLayout;
QList<QLabel *> labels;
QGridLayout *imagesLayout;
QFutureWatcher<QImage*> *imageScaling;
};

#endif // IMAGESCALING_H



If I map the global function everything works:


imageScaling->setFuture(QtConcurrent::mapped(files, scale));

If I map to the member function it won't compile:



imageScaling->setFuture(QtConcurrent::mapped(files, &Images::scale_));
or
imageScaling->setFuture(QtConcurrent::mapped(files, [this](const QString &file){this->scale_(file);}));


ERROR:



...\imagescaling.cpp:133: Fehler: C2664: "void QFutureWatcher<QImage *>::setFuture(const QFuture<T> &)" : Converting of Argument 1 of "QFuture<void>" in "const QFuture<T> &" not possible
with
[
T=QImage *
]


Is using QtConcurrent::mapped() the wrong approach to bind with member functions?

[anda_skoa]

Any specific reason you want this to be a member function?
Seems to just "leak" internal implementation details to the API of your class.

Anyway:
- Your first approach could work with a static member function.
- The lambda probably didn't work because it has the wrong signature, i.e. it as "void" as its return type, you could try a lambda that returns T, in your case QImage*.

Which leads to the question, why not stay with the type used in the Qt example? I.e. why allocate QImage on the head and get into the hassle of having to manually delete it again?
Also you should probably revisit the example to look on how the scaling is done.
Your functions (both standalone and member) do not returned scaled images while still needing to calculate the scaling but immediately discarding its result.

Cheers,
_

[h.a.n.d]

Thanks for your answer!

I used the imagescale example as a simplyfied demonstration to show what I am trying to achieve in my own implementation of QFutureWatcher, QFuture, QtConcurrent.

In my application I have the MainWindow (QMainWindow) which has a ImageManager member.
The MainWindow handles all the GUI Input/Output, the ImageManager handles the data (i.e. image processing and holds all the references to the processed images and the image data).

i.e.: The MainWindow triggers, via a connection, the slot function of the ImageManager member, which will load the image files and process them.
The ImageManager member function, I want to map to the QFutureWatcher->setFuture(QtConcurrent::mapped(files, &ImageManager::createImage)); does:

1. Loads the image
2. Processes the image
3. Puts a reference (pointer) into a QList<QImage*> list
4. Emittes a signal, which is connected to a SLOT in the MainWindow, to add the image to a QListWidget
5. Returns the reference (pointer)

Member function I want to map and call concurrent:



QImage* ImageManager::createImage(const QString &file)
{
cv::Mat image = cv::imread(file.toLocal8Bit().constData());
...

QImage* qImage = ImageUtils::cvMatToQImage(image);
m_imageList.append(qImage);
emit imageAdded(qImage);

return qImage;
}


Connection / SLOT in MainWindow:


connect(m_imageManager, &ImageManager::imageAdded, this, &MainWindow::addImage);

void MainWindow::addImage(QImage* image)
{
label->setPixmap(QPixmap::fromImage(*(image)));
}


I've changed the lambda to:


imageScaling->setFuture(QtConcurrent::mapped(files, [this](const QString &file)->QImage*{return this->scale_(file);}));

But the compiler message remains the same.

Maybe I'm doing it the wrong way all along and need to approach this problem an other way?

[anda_skoa]

1. Loads the image
2. Processes the image
3. Puts a reference (pointer) into a QList<QImage*> list
4. Emittes a signal, which is connected to a SLOT in the MainWindow, to add the image to a QListWidget
5. Returns the reference (pointer)

That sounds like you want to the the first two steps with QtConcurrent and then, when the future watchers signals availability, just handle the rest in the slot connected to the future watchers.
So a stand-alone function that does 1+2 with QtConcurrent and handling the things that need the image manager instance in the slot of the image manager that handles the future watcher.

Btw, it is very uncommon to use QImage as a pointer, as this only requires manual pointer ownership handling.

Cheers,
_

[h.a.n.d]

I refactored the code to use stand-alone functions for the mapping.

Returning the pointer of an object, created in a separate thread, was my first attempt not to copy the whole data again. But then I coudn't make use of the QObject->parent utility which deletes the object if the parent gets deleted, because the object was created in an other thread as the parent object.
I guess returning it by value would be the better way.

Is this best-practice?

[anda_skoa]

Returning the pointer of an object, created in a separate thread, was my first attempt not to copy the whole data again.

QImage is reference counted, copying it does not copy the data.



But then I coudn't make use of the QObject->parent utility which deletes the object if the parent gets deleted

QImage is not a QObject derived class

Cheers,
_

[yeye_olive]

The only reason I can see for allocating QImage on the heap rather than simply relying on its implicit sharing semantics is a limitation in QtConcurrent. QFuture only allows read-only access to the results; if we are not interested in keeping the QImages around until all of them are computed (for instance if we just make QPixmaps of them and never need them again), then we can save memory by finding a way to mutably access them. This can be done by storing pointers (so that we can delete each QImage as soon as it has been processed), but a better alternative would simply be to wrap QImage in a structure with a mutable field, whose value can be reset to an empty QImage as needed. Or use a more sophisticated concurrency library instead of hacking around QtConcurrent.