Threaded Video Player sync

Question

Disclaimer:I asked this question a few days ago on codereview,but got no answer.Here I change the question format from review request to a specific problems.

I am developing a video player with the following design:

The main thread - is GUI thread (Qt SDK).

Second thread - player thread which accepts commands from the GUI thread to play, forward, backward, stop etc. Now,this thread runs in a constant loop and and uses mutexes and wait conditions to live in sync with the main thread commands.

I have 2 problems with this code:

I don't feel my design is completely correct:I am using both mutex locks and atomic variables.I wonder if I can stay only with the atomics and use locks only for setting the wait conditions.

I am experiencing inconsistent bugs(probably due to the condition race when the play command tries to lock mutex which is already locked by the thread while the play loop is working) when I run "play" commands which activates a loop inside the thread loop. So I suppose it blocks the access to the shared variables to the main thread.

I have stripped off the code from unneeded stuff and it generally goes like this:

  void PlayerThread::drawThread()//thread method passed into new boost::thread
{

   //some init goes here....

      while(true)
      {
          boost::unique_lock<boost::mutex> lock(m_mutex);
          m_event.wait(lock); //wait for event

          if(!m_threadRun){
             break; //exit the tread
          }

           ///if we are in playback mode,play in a loop till interrupted:
          if(m_isPlayMode == true){

              while(m_frameIndex < m_totalFrames && m_isPlayMode){

                       //play
                       m_frameIndex ++;

              }

               m_isPlayMode = false;

          }else{//we are in a single frame play mode:

               if(m_cleanMode){ ///just clear the screen with a color

                       //clear the screen from the last frame
                       //wait for the new movie to get loaded:

                       m_event.wait(lock); 


                       //load new movie......

               }else{ //render a single frame:

                       //play single frame....

               }


          }


      }

}

Here are the member functions of the above class which send commands to the thread loop:

void PlayerThread::PlayForwardSlot(){
//   boost::unique_lock<boost::mutex> lock(m_mutex);
    if(m_cleanMode)return;
    m_isPlayMode = false;
    m_frameIndex++;
     m_event.notify_one();
}

 void PlayerThread::PlayBackwardSlot(){
 //  boost::unique_lock<boost::mutex> lock(m_mutex);
  if(m_cleanMode)return;
   m_isPlayMode = false;
   m_frameIndex-- ;
   if(m_frameIndex < 0){
       m_frameIndex = 0;
   }

    m_event.notify_one();

 }


 void PlayerThread::PlaySlot(){
 // boost::unique_lock<boost::mutex> lock(m_mutex);
   if(m_cleanMode)return;
   m_isPlayMode = true;
   m_event.notify_one(); //tell thread to  start playing.

  }

All the flag members like m_cleanMode, m_isPlayMode and m_frameIndex are atomics:

  std::atomic<int32_t>   m_frameIndex;
  std::atomic<bool>      m_isPlayMode; 
  std::atomic<bool>      m_cleanMode;

The questions summary::

1. Do I need mutex locks when using atomics?

2. Do I set waiting in the correct place inside the while loop of the thread?

3. Any suggestion of a better design?

UPDATE:

Though I got an answer which seems to be in the right direction I don't really understand it.Especially the pseudo-code part which is talking about service.It is completely unclear to me how it would work.I would like to get a more elaborated answer.It is also strange that I received only one constructive answer to such a common problem.So I am resetting the bounty.

Answer 1

The biggest issue with your code is that you wait unconditionally. boost::condition::notify_one only wake up a thread which is waiting. Which means Forward Step\Backward Step then Play if fast enough will ignore the play command. I dont get clean mode, but you need at least

if(!m_isPlayMode)
{
     m_event.wait(lock);
}

In your code stop and stepping to a frame are virtually the same thing .You may want to use a tristate PLAY,STEP, STOP to be able to use the recommended way of waiting on a condition variable

while(state == STOP)
{
    m_event.wait(lock);
}

1. Do I need mutex locks when using atomics?

Technically yes. In this specific case I don't think so. Current races conditions (I noticed) :

• playing mode, playforward and playbackward will not result in the same m_frameIndex depending whether or not drawThread is within the while(m_frameIndex < m_totalFrames && m_isPlayMode) loop. Indeed m_frameIndexcould be incremented once or twice (playforward).
• Entering the playing state in PlaySlot can be ignored if drawThread execute m_isPlayMode = false; before receiving the next event. Right now it is a non-issue because it will only happen if m_frameIndex < m_totalFrames is false. If PlaySlot was modifying m_frameIndex then you will have case of pushing play and nothing happen.

2. Do I set waiting in the correct place inside the while loop of the thread?

I would suggest to have only one wait in your code, for simplicity. And be explicit about the next thing to do using specific commands :

 PLAY, STOP, LOADMOVIE, STEP

3. Any suggestion of a better design?

Use an explicit event queue. You can use one which is Qt-based (require Qthreads) or boost based. The one based on boost use a boost::asio::io_service and a boost::thread.

You start the event loop using :

boost::asio::io_service service;
//permanent work so io_service::exec doesnt terminate immediately.
boost::asio::io_service::work work(service); 
boost::thread thread(boost::bind(&boost::asio::io_service::exec, boost::ref(service)));

Then you send your commands from the GUI using

MYSTATE state;
service.post(boost::bind(&MyObject::changeState,this, state));

• Your play method should request another play given that the state hasn't changed, rather than looping. It allows a better user preemption.
• Your step method should request a stop before displaying the frame.

Pseudocode:

play()
{
 if(state != PLAYING)
   return;
 drawframe(index);
 index++;
 service.post(boost::bind(&MyObject::play, this));
}

stepforward()
{
 stop();
 index++;
 drawframe(index);
}

stepbackward()
{
 stop();
 index--;
 drawframe(index);
}

Edit: There is only one player thread which is created once and execute only one event loop. Is is equivalent to QThread::start(). The thread will live as long as the loop doesnt return, which is going to be till the work object is destroyed OR when you explicitly stop the service. When you request to stop a service all posted tasks which are still pending are going to be executed first. You can interrupt the thread for fast exit if neccessary.

When there is a call for an action you post in the event loop ran by the player thread.

Note: You will probably need share pointers for the service and the thread. You will also need to put interrupt points in the play method in order to allow stopping the thread cleanly during playback. You don't need as much atomic as before. You don't need a condition variable anymore.

Answer 2

Any suggestion of a better design?

Yes! Since you are using Qt I would heavily suggest to use Qt's eventloop (apart from the UI stuff this is IMO one of the main selling points of that library) and asynchronous signal/slots to do the controlling instead of your homegrown synchronization, which - as you found out - is a very fragile undertaking.

The main change this will bring to your current design is that you will have to do your video logic as part of the Qt event-loop, or, easier, just do a QEventLoop::processEvents. For that you will need a QThread. Then it's very straightforward: You create some class that inherits from QObject let's say PlayerController which should contain signals like play, pause, stop and a class Player which will have slots onPlay, onPause, onStop (or without the on, your preference). Then create a 'controller' object of the PlayerController class in the GUI thread and the Player object in the 'video' thread (or use QObject::moveToThread). This is important, as Qt has the notion of thread affinity to determine in which thread SLOTs are executed. No connect the objects by doing QObject::connect(controller, SIGNAL(play()), player, SLOT(onPlay())). Any call now to PlayerController:play on the 'controller' from the GUI thread will result in the onPlay method of the 'player' being executed in the video thread on the next event loop iteration. That's where you can then change your boolean status variables or do other kind of action without the need for explicit synchronization as your variables are only changes from the video thread now.

So something along those lines:

class PlayerController: public QObject {
Q_OBJECT

signals:
    void play();
    void pause();
    void stop();
}

class Player: public QObject {
Q_OBJECT

public slots:
    void play() { m_isPlayMode = true; }
    void pause() { m_isPlayMode = false; }
    void stop() { m_isStop = true; };

private:
    bool m_isPlayMode;
    bool m_isStop;
}

class VideoThread: public QThread {

public:
    VideoThread (PlayerController* controller) {
        m_controller = controller;
    }

protected:
    /* override the run method, normally not adviced but we want our special eventloop */
    void run() {
        QEventLoop loop;
        Player* player = new Player;

        QObject::connect(m_controller, SIGNAL(play()), player, SLOT(play()));
        QObject::connect(m_controller, SIGNAL(pause()), player, SLOT(pause()));
        QObject::connect(m_controller, SIGNAL(stop()), player, SLOT(stop()));

        m_isStop = false;
        m_isPlayMode = false;
        while(!m_isStop) {
            // DO video related stuff
            loop.processEvents();
        }
    }


private:
    PlayerController* m_controller;
}



// somewhere in main thread
PlayerController* controller = new PlayerController();
VideoThread* videoThread = new VideoThread(controller);
videoThread.start();
controller.play();