Qt - synchronizing threads not working - threads stop but really don't, and sometimes do when they shouldn't

Question

I have four threads. Two play and analyze videos using OpenCV, one saves data and acts as a middleman between videos and GUI, one is GUI.

My idea was this - video thread does things, sends the frame to the middleman, and stops itself. If it's the first frame in the pair sent, middleman does nothing but set a flag - otherFrame. In theory, since video one's stopped, now the other video sends a frame, the flag of the other frame is True, so middleman emits both frames to the onPassFramesGui, where the frames are placed in QLabels, and then both threads are resumed. And then it loops.

That's the theory and I have no doubt it's wrong. In reality, the video with lower resolution will sooner than later overtake the other one. Like it's never paused.

In video threads, while they do slightly different things, this is the common part of the code:

for(nrKlatkiOne=1; nrKlatkiOne<maxFramesOne; nrKlatkiOne++) {
(....)
    emit sendFrameOne(imageOne, nrKlatkiOne);
    pauseThread();
    pause.lock(); //mutex
    if(threadPausedOne==true){
        pausedCond.wait(&pause); //QWaitCondition
    }
    pause.unlock();
}

and

void VideoOne::pauseThread() { //in VideoTwo "One"s are replaced with "Two"s, so the variables are exclusive to their threads
    if(threadPausedOne==false){
        pause.lock();
        threadPausedOne=true;
        pause.unlock();
     }
}

void VideoOne::resumeThread() {
    if(threadPausedOne==true){
        pause.lock();
        threadPausedOne=false;
        pause.unlock();
        pausedCond.wakeAll();
    }
}

in the middleman thread, this is the slot that receives the signal (there's a similar signal for VideoTwo):

void Analiza::onSendFrameOne(QImage frameOneImage, int frameNoOne) {  

//videoOne->pauseThread();        
one = frameOneImage;    
frOne = frameNoOne;  

if(otherFrameTwo == true && videoTwoLoaded == true){  
    emit passFramesGui(one, two, frOne, frTwo);  
    pauseMutex.lock();     
    otherFrameOne = false;    
    otherFrameTwo = false;
    pauseMutex.unlock();
} else if(videoTwoLoaded == false) {   
    emit passFramesGui(one, two, frOne, frTwo);   //to GUI
}
if(otherFrameTwo == false){     
    pauseMutex.lock();
    otherFrameOne = true;                  
    pauseMutex.unlock();
    qDebug() << "otherFrameOne = true";
}
}

and the last, but not least, please bear with me, this function sets the images in the GUI

void MainWindow::onPassFramesGui(QImage one, QImage two, int frameNoOne, int frameNoTwo){ 

if(videoOneLoaded==true) {          
    <converting image>
    <keeping aspect ratio>
    (setting stuff)
}

if(videoTwoLoaded==true) { 
    <same stuff here>
}

if(videoOneLoaded == true) { 
    videoOne->resumeThread();
}

if(videoTwoLoaded == true) { 
    videoTwo->resumeThread();  
}
}

I've put QDebug messages to get a rough idea what's going in. Sooner or later this happens:

(....)
otherFrameOne == true && videoOneLoaded == true //frames emitted from onSendFramesTwo
otherFrameTwo = true //onSendFramesTwo received the frame, waits
otherFrameTwo == true && videoTwoLoaded == true //onSendrameOne sent the frames
otherFrameOne = true //onSendFrameOne received a frame
otherFrameOne = true //onSendFrameOne received another frame? But it's stopped...

I'm pretty sure stopping threads work. If I remove resumeThread() from either, the program will just stop. It will send frame 1, 2 -> gui, then 1 and it'll just stop.

Another case is when there's just one video loaded, instead of two. The video thread will just stop stop at random at not resume. I have a signal that qDebugs a message, and after the video stops, it won't do anything (can be resumed when I change tabs - which causes the threadPausedOne/Two to be set to true and then to false.

The code compiles without any errors or warnings.

What is the correct way to do this? It's my first time doing this kind of thing.

Oh, a bonus question. Is it better to have two separate slightly different objects, or one with 60% more code and just create multiple instances?

Answer 1

You have overcomplicated it, introducing a lot of flags and synchronization objects. I suggest that you use two QSemaphore per video thread. I will name them allowWork and hasFrame.

The video thread will work as follows:

// Wait until middleman allows work (when it has handled a pair of frames)
allowWork.acquire(1);

// Do frame processing here
// ...

// Tell middleman one frame is ready
hasFrame.release(1);

The middleman thread will work as follows:

// Wait until both video threads have a frame
video1.hasFrame.acquire(1);
video2.hasFrame.acquire(1);

// Handle a pair of frames here
// ...

// Tell video threads to make another pair of frames
video1.allowWork.release(1);
video2.allowWork.release(1);

In startup code, you will initialize allowWork to 1, and hasFrame to 0.

With this approach, it will also be easy to add a buffer for frames from video1 and video2 (for example both video threads work until they have 16 frames ready each, while middleman still handles once a pair is available), this way you're likely to get better thread utilization.

Answer 2

You must lock your mutex whenever you use the shared variables, for writting (as you do), but also for reading

Example:

void VideoOne::pauseThread() { //in VideoTwo "One"s are replaced with "Two"s, so the variables are exclusive to their threads
    pause.lock(); // lock here
    if(threadPausedOne==false){
        // pause.lock(); // not here!
        threadPausedOne=true;
        // pause.unlock(); // not here!
    }
    pause.lock(); // unlock here
}

void VideoOne::resumeThread() {
    pause.lock(); // lock here
    if(threadPausedOne==true){
        // pause.lock(); // not here!
        threadPausedOne=false;
        // pause.unlock(); // not here!
        pausedCond.wakeAll();
    }
    pause.lock(); // unlock here
}

For resumeThread(), if wakeAll must be called when pause is not locked, just do this. That's a trick you may have to use in other places to prevent dead-locks...

void VideoOne::resumeThread() {
    bool needWakeAll = false;
    pause.lock(); // lock here
    if(threadPausedOne==true){
        threadPausedOne=false;
        needWakeAll=true;
    }
    pause.lock(); // unlock here
    if ( needWakeAll )
        pausedCond.wakeAll();
}

onSendFrameOne most likely has to be reworked the same way...

Answer 3

You think you locked the mutex and no other is accessible to it, but both threads look they change the boolean value and try lock the mutex. Isn't it?

The boolean value is set, but it's accessed while the mutex not locked in a moment.

Look over Wikipedia article Double-checked locking. It will help.