strange proplem using two Threads and Boolean

Question

(I hate having to put a title like this. but I just couldn't find anything better)

I have two classes with two threads. first one detects motion between two frames:

void Detector::run(){

isActive = true;  

// will run forever
while (isActive){

        //code to detect motion for every frame
        //.........................................

        if(isThereMotion)
        {
            if(number_of_sequence>0){

                theRecorder.setRecording(true);                    
                theRecorder.setup();

               // cout << " motion was detected" << endl;

            }
            number_of_sequence++;
        }
        else 
        {
            number_of_sequence = 0;
            theRecorder.setRecording(false);
            // cout << " there was no motion" << endl;
            cvWaitKey (DELAY);

        }
    }
}

second one will record a video when started:

void Recorder::setup(){

    if (!hasStarted){
        this->start();
       }
}

void Recorder::run(){    

     theVideoWriter.open(filename, CV_FOURCC('X','V','I','D'), 20, Size(1980,1080), true);


if (recording){

    while(recording){
    //++++++++++++++++++++++++++++++++++++++++++++++++
    cout <<  recording << endl;
    hasStarted=true;
    webcamRecorder.read(matRecorder); // read a new frame from video
    theVideoWriter.write(matRecorder); //writer the frame into the file

    }

   }
else{
    hasStarted=false;
    cout << "no recording??" << endl;
    changeFilemamePlusOne();
    }
  hasStarted=false;
  cout << "finished recording" << endl;
  theVideoWriter.release();

}

The boolean recording gets changed by the function:

void Recorder::setRecording(bool x){
    recording = x;
}

The goal is to start the recording once motion was detected while preventing the program from starting the recording twice.

The really strange problem, which honestly doesn't make any sense in my head, is that the code will only work if I cout the boolean recording ( marked with the "++++++"). Else recording never changes to false and the code in the else statment never gets called.

Does anyone have an idea on why this is happening. I'm still just begining with c++ but this problem seems really strange to me..

Answer 1

I suppose your variables isThereMotion and recording are simple class members of type bool.

Concurrent access to these members isn't thread safe by default, and you'll face race conditions, and all kinds of weird behaviors.

I'd recommend to declare these member variables like this (as long you can make use of the latest standard):

class Detector {
    // ...
    std::atomic<bool> isThereMotion;
};

class Recorder {
    // ...
    std::atomic<bool> hasStarted;
};

etc.

The reason behind the scenes is, that even reading/writing a simple boolean value splits up into several assembler instructions applied to the CPU, and those may be scheduled off in the middle for a thread execution path change of the process. Using std::atomic<> provides something like a critical section for read/write operations on this variable automatically.

---

In short: Make everything, that is purposed to be accessed concurrently from different threads, an atomic value, or use an appropriate synchronization mechanism like a std::mutex.

If you can't use the latest c++ standard, you can perhaps workaround using boost::thread to keep your code portable.

---

NOTE:
As from your comments, your question seems to be specific for the Qt framework, there's a number of mechanisms you can use for synchronization as e.g. the mentioned QMutex.

---

Why volatile doesn't help in multithreaded environments?

volatile prevents the compiler to optimize away actual read access just by assumptions of values set formerly in a sequential manner. It doesn't prevent threads to be interrupted in actually retrieving or writing values there.

volatile should be used for reading from addresses that can be changed independently of the sequential or threading execution model (e.g. bus addressed peripheral HW registers, where the HW changes values actively, e.g. a FPGA reporting current data throughput at a register inteface).

See more details about this misconception here:
Why is volatile not considered useful in multithreaded C or C++ programming?

Answer 2

You could use a pool of nodes with pointers to frame buffers as part of a linked list fifo messaging system using mutex and semaphore to coordinate the threads. A message for each frame to be recorded would be sent to the recording thread (appended to it's list and a semaphore released), otherwise the node would be returned (appended) back to the main thread's list.

Example code using Windows based synchronization to copy a file. The main thread reads into buffers, the spawned thread writes from buffers it receives. The setup code is lengthy, but the actual messaging functions and the two thread functions are simple and small.

mtcopy.zip

Answer 3

Could be a liveness issue. The compiler could be re-ordering instructions or hoisting isActive out of the loop. Try marking it as volatile.

From MSDN docs:

Objects that are declared as volatile are not used in certain optimizations because their values can change at any time. The system always reads the current value of a volatile object when it is requested, even if a previous instruction asked for a value from the same object. Also, the value of the object is written immediately on assignment.

Simple example:

#include <iostream>
using namespace std;
int main() {
    bool active = true;
    while(active) {
      cout << "Still active" << endl;
    }
}

Assemble it: g++ -S test.cpp -o test1.a

Add volatile to active as in volatile bool active = true

Assemble it again g++ -S test.cpp -o test2.a and look at the difference diff test1.a test2.a

<   testb   $1, -5(%rbp)
---
>   movb    -5(%rbp), %al
>   testb   $1, %al

Notice the first one doesn't even bother to read the value of active before testing it since the loop body never modifies it. The second version does.