Best way to send data from a GUI thread to a processing thread?

[dmginc]

As the topic suggests, I'm looking for the best way to send data from the main window GUI thread to a processing thread.

At the moment, the run() function in the ProcessingThread looks something like this:

void ProcessingThread::run()
{
while(1)
{
frame=grabFrame();
if(flag1)
doProcessingOnFrame1(frame,setting1,setting2);
if(flag2)
doProcessingOnFrame2(frame,setting3,setting4);
if(flag3)
doProcessingOnFrame3(frame,setting5,setting6);

emit newFrame(frame);
}
}

MainWindows.cpp has:

// Create queued connection between processing thread (emitter) and GUI thread (receiver/listener)
connect(controller->processingThread,SIGNAL(newFrame(QImage)),this,SLO T(updateFrame(QImage)),Qt::QueuedConnection);

and also:

void MainWindow::updateFrame(const QImage &frame)
{
// Update public "flags" and "settings" members in ProcessingThread
updateData();
// Display frame in main window
frameLabel->setPixmap(QPixmap::fromImage(frame));
} // updateFrame()

Eveything works perfectly :)

However, at the moment I am updating/changing the "flags" and "setting" variables contained within the ProcessingThread by making them public and then altering them within updateFrame() in MainWindow.cpp (see above). This is merely my attempt update them "synchronously" after each loop of run() - so that they don't change in the middle of the execution of run() and thus erroneousness turn on more image processing than that actually set by the user.

However, it is my understanding that run() does not block when emitting the newFrame signal in the case of a queued-connection - thus the data update is not truly synchronous.

There has to be a better way to do change these variables in response to GUI events in MainWindow.

Here is a method I was thinking of:

Create a binary semaphore (initialized to 1) and use it like so in run():

GLOBAL: sem = new QSemaphore(1);


void ProcessingThread::run()
{
while(1)
{

frame=grabFrame();

sem->acquire();
if(flag1)
doProcessingOnFrame1(frame,setting1,setting2);
if(flag2)
doProcessingOnFrame1(frame,setting3,setting4);
if(flag3)
doProcessingOnFrame1(frame,setting5,setting6);
sem->release();

emit newFrame(frame);
}
}

And then merely use this global semaphore in the updateFrame() slot in MainWindow:

void MainWindow::updateFrame(const QImage &frame)
{
// Update public "flags" and "settings" members in ProcessingThread
sem->acquire();
updateData();
sem->release();
// Display frame in main window
frameLabel->setPixmap(QPixmap::fromImage(frame));
} // updateFrame()

However, this method again relies on making the "flags" and "settings" variables PUBLIC in ProcessingThread...

Is there a better way? :confused:

I'm open to and would really appreciate any suggestions :)
(If I have omitted any crucial information, please let me know.)

Thanks in advance to anyone who could help me with this!

[high_flyer]

However, this method again relies on making the "flags" and "settings" variables PUBLIC in ProcessingThread...
Is there a better way?
Make them protected/private and give the setters/getters?
And you should mutex them!

[dmginc]

Thanks for the reply! The only problem with that idea is that set/get functions for each variable could get quite tedious when the number of settings and flags are numerous.

I was also thinking about creating a signal/slot connection from the GUI thread to the processing thread which passes a structure containing all the flags and settings to the thread on every setting change GUI event.

Something like this (in MainWindow.cpp):

connect(this,SIGNAL(newData(struct data)),this,SLOT(updateData(struct data)),Qt::QueuedConnection);

Slot in ProcessingThread:

void updateData(struct data)
{
sem->acquire();
// save structure contents to ProcessingThread private members:
flag1=data.flag1;
flag2=data.flag2;

//etc.
sem->release();
}

And also use sem in MainWindow followed by emit newData(data) when GUI events change the values contained in the structure "data". For example (slot connected to gui event):


void onButtonPush()
{
sem->acquire();
data.flag1=true;
sem->release();
emit newData(data);
}


Is this also a valid/safe solution?
I remember reading that slots in a thread are a bad idea?
Also, can "emit" be used in a SLOT?

Thanks!

[high_flyer]

Thanks for the reply! The only problem with that idea is that set/get functions for each variable could get quite tedious when the number of settings and flags are numerous.
You could group them in groups that make sense in your applications logic (structs?).

The problem with signal and slots among threads is not that they are not safe, they are, and is probably the best way to transfer data between threads in Qt, however they are queued - so you don't know when that data is arrived and set, which in your case I think is important (per frame, for a specific frame).
So in your case you have to set() get() them.
Public members is VERY bad idea, and its very dangerous.
And don't forget to mutex them!

[dmginc]

You could group them in groups that make sense in your applications logic (structs?).

The problem with signal and slots among threads is not that they are not safe, they are, and is probably the best way to transfer data between threads in Qt, however they are queued - so you don't know when that data is arrived and set, which in your case I think is important (per frame, for a specific frame).
So in your case you have to set() get() them.
Public members is VERY bad idea, and its very dangerous.
And don't forget to mutex them!

The most important thing is that all the flags and settings get updated at the same time before or after processing is finished. In other words, I don't want the flags/settings being updated straight after for example "if(flag1) ..." (below in run()) as this will momentarily turn on the wrong combination of processing.

This is what I have chosen to do: (and it seems to be working)

ProcessingThread.cpp

void ProcessingThread::run()
{
while(1)
{
frame=grabFrame();
mutex.lock();
if(flag1)
doProcessingOnFrame1(frame,setting1,setting2);
if(flag2)
doProcessingOnFrame2(frame,setting3,setting4);
if(flag3)
doProcessingOnFrame3(frame,setting5,setting6);
mutex.unlock();
emit newFrame(frame);
}
}

Slot in ProcessingThread.cpp

void ProcessingThread::updateProcessingFlags(struct ProcessingFlags p_flags)
{
QMutexLocker locker(&mutex);
// Update private members of ProcessingThread
this->flag1=p_flags.flag1;
this->flag2=p_flags.flag2;

// etc
} // updateProcessingFlags()

In the main window GUI slots:

emit newProcessingFlags(processingFlags);

And of course again in MainWindow.cpp:


qRegisterMetaType<struct ProcessingFlags>("ProcessingFlags");
connect(this,SIGNAL(newProcessingFlags(struct ProcessingFlags)),controller->processingThread,SLOT(updateProcessingFlags(struct ProcessingFlags)),Qt::QueuedConnection);


(Note: The updating of the settings is also done exactly the same way as above)

Does this seem like a reasonable solution to anyone? I'd appreciate any comments at all.

Thanks

[high_flyer]

Does this seem like a reasonable solution to anyone? I'd appreciate any comments at all.
Reasonable - sure.
But its hard to tell exactly without getting deeper in to the logic of your application.

The only non related comment I have is that you are locking the mutex basically the hole run() duration, which is not good - it is legal, but it cause your application or parts of the logic to wait longer they they should/could.
You should only mutex where data is accessed, and try to be as atomic as possible.

[dmginc]

Reasonable - sure.
But its hard to tell exactly without getting deeper in to the logic of your application.

The only non related comment I have is that you are locking the mutex basically the hole run() duration, which is not good - it is legal, but it cause your application or parts of the logic to wait longer they they should/could.
You should only mutex where data is accessed, and try to be as atomic as possible.

Thanks - I must point out that the slots (connected to signals triggered by the main window GUI) in the ProcessingThread are quite short and only copy a small amount of data. As I said before, my goal in using the mutexes is merely to make sure the update of the flags/settings is predictable and does not occur in the middle of processing. So it looks like the maximum penalty I would pay is a one iteration delay of run() in updating these flags/settings I guess...
Would you agree?

On a related note: When using mutexes to protect different data in the same thread, should I declare seperate mutexes?

eg:

void ProcessingThread::run()
{
while(1)
{
frame=grabFrame();
mutex.lock();
if(flag1)
doProcessingOnFrame1(frame,setting1,setting2);
if(flag2)
doProcessingOnFrame2(frame,setting3,setting4);
if(flag3)
doProcessingOnFrame3(frame,setting5,setting6);
mutex.unlock();
emit newFrame(frame);
mutex2.lock();
updateFPSvalue(); // saves current FPS value in a private member (this member is accessed by the GUI thread using a "get" function containing a QMutexLocker)
mutex2.unlock();
}
}

Thanks again.

[high_flyer]

On a related note: When using mutexes to protect different data in the same thread, should I declare seperate mutexes?
It depends.
If it is allowed for various variables to be accessed from out side at the same time, you will need a mutex per such location.
If only one access per time to any of the data in thread is allowed, then you can use one mutex (member).
In your example one mutex is enough, since they are all in the same place.
But if you had other methods that should allow parallel access to data, you might need more mutexes.

Usually however, you only need one mutex, if you design correctly.