12

I have a function for occasionally getting a frame from GigE camera, and want it to return quickly. The standard procedure is like this:

// ...
camera.StartCapture();
Image img=camera.GetNextFrame();
camera.StopCapture(); // <--  takes a few secs
return img;

Return data is ready after GetNextFrame() and StopCapture() is quite slow; therefore, I'd like to return img as soon as possible and spawn a background thread to do StopCapture(). However, in the (unlikely) case that the acquisition is started again, I would like to protect the access by a mutex. There are places where exceptions can be thrown, so I decide to use a RAII-style lock, which will release at scope exit. At the same time, I need to transfer the lock to the background thread. Something like this (pseudocode):

class CamIface{
   std::mutex mutex;
   CameraHw camera;
public:
   Image acquire(){
      std::unique_lock<std::mutex> lock(mutex); // waits for cleanup after the previous call to finish
      camera.StartCapture();
      Image img=camera.GetNextFrame();
      std::thread bg([&]{
         camera.StopCapture(); // takes a long time
         lock.release(); // release the lock here, somehow
       });
       bg.detach();
       return img;
       // do not destroy&release lock here, do it in the bg thread
   };

};

How can I transfer the lock from the caller to the background thread spawned? Or is there some better way to handle this?

EDIT: Sufficient lifetime of CamIface instance is assured, please suppose it exists forever.

eudoxos
  • 18,545
  • 10
  • 61
  • 110
  • I would attach the thread to `CamIface` instead of detach it. – Jarod42 Sep 14 '16 at 07:23
  • http://stackoverflow.com/a/20669290/104774 should answer your question, although I prefer the answer of @PeterT – stefaanv Sep 14 '16 at 07:23
  • 3
    I don't think it's as simple as a move capture. std::mutex::unlock must be called on the same thread that the mutex was locked on: http://en.cppreference.com/w/cpp/thread/mutex/unlock – David Millard Sep 14 '16 at 07:25
  • Tangential - there is a race condition between this thread being scheduled and this object possibly being destroyed. When the lambda begins to execute, the `this` which was captured by reference may point to deallocated memory, and accessing `camera` is UB. If StopCapture is a vital call for the hardware to stop properly, consider using enable_shared_from_this and capturing a std::shared_ptr to keep the object alive. – David Millard Sep 14 '16 at 07:31
  • @DavidMillard: yes, good point, but in this case it is not a problem as the CamIface object lifetime is long. I will edit the question. – eudoxos Sep 14 '16 at 07:53
  • 1
    The fact that this is hard to do correctly should be a sign that your design is oddly asymmetric. Instead put all of the camera interaction in the background thread, with all the mutex operations from that thread. Then just deliver the captured frame across the thread boundary with a std::future or other simple synchronization. You could consider from here making the background thread persistent, and maybe never even stopping capture. – Peter Sep 14 '16 at 13:15
  • @Peter good point about leaving all in the bg thread, that sounds like the best idea so far. Not stopping capture is not possible, there is strobe light attached and it would take too much energy (possibly battery-powered). – eudoxos Sep 14 '16 at 15:17
  • @Peter, could you turn your comment about bg thread into an answer so that I can accept it? Thanks! – eudoxos Sep 25 '16 at 17:24

4 Answers4

7

Updated Answer: @Revolver_Ocelot is right that my answer encourages undefined behavior, which I'd like to avoid.

So let me use the simple Semaphore implementation from this SO Answer

#include <mutex>
#include <thread>
#include <condition_variable>

class Semaphore {
public:
    Semaphore (int count_ = 0)
        : count(count_) {}

    inline void notify()
    {
        std::unique_lock<std::mutex> lock(mtx);
        count++;
        cv.notify_one();
    }

    inline void wait()
    {
        std::unique_lock<std::mutex> lock(mtx);

        while(count == 0){
            cv.wait(lock);
        }
        count--;
    }

private:
    std::mutex mtx;
    std::condition_variable cv;
    int count;
};


class SemGuard
{
    Semaphore* sem;
public:
    SemGuard(Semaphore& semaphore) : sem(&semaphore)
    {
        sem->wait();
    }
    ~SemGuard()
    {
        if (sem)sem->notify();
    }
    SemGuard(const SemGuard& other) = delete;
    SemGuard& operator=(const SemGuard& other) = delete;
    SemGuard(SemGuard&& other) : sem(other.sem)
    {
        other.sem = nullptr;
    }
    SemGuard& operator=(SemGuard&& other)
    {
        if (sem)sem->notify();
        sem = other.sem;
        other.sem = nullptr;
        return *this;
    }
};

class CamIface{
   Semaphore sem;
   CameraHw camera;
public:
   CamIface() : sem(1){}
   Image acquire(){
      SemGuard guard(sem);
      camera.StartCapture();
      Image img=camera.GetNextFrame();
      std::thread bg([&](SemGuard guard){
         camera.StopCapture(); // takes a long time
       }, std::move(guard));
       bg.detach();
       return img;
   };

};

Old Answer: Just like PanicSheep said, move the mutex into the thread. For example like this:

std::mutex mut;

void func()
{
    std::unique_lock<std::mutex> lock(mut);
    std::thread bg([&](std::unique_lock<std::mutex> lock)
    {
         camera.StopCapture(); // takes a long time
    },std::move(lock));
    bg.detach();
}

Also, just to remark, don't do this:

std::thread bg([&]()
{
     std::unique_lock<std::mutex> local_lock = std::move(lock);
     camera.StopCapture(); // takes a long time
     local_lock.release(); // release the lock here, somehow
});

Because you're racing the thread startup and the function scope ending.

Community
  • 1
  • 1
PeterT
  • 7,981
  • 1
  • 26
  • 34
  • 3
    Mutex ownership is a property of thread. You **must** unlock mutex in same thread you aquired it. [Otherwise you have UB](http://stackoverflow.com/a/38442900/3410396) – Revolver_Ocelot Sep 14 '16 at 07:23
  • 1
    true, I guess a semaphore or something else, would be appropriate – PeterT Sep 14 '16 at 07:24
  • @Revolver_Ocelot thanks again for the comment, I added a version using a simple semaphore. (edit: but I forgot about the exception requirement, I'm going to see about that) – PeterT Sep 14 '16 at 07:53
  • How will the seamphore behave if there are exceptions thrown? If `GetNextFrame` throws, notify will never be called. – eudoxos Sep 14 '16 at 07:56
  • @Revolver_Ocelot: is the one-thread rule also valid for shared mutexes? I could ask for exclusive lock at the beginning of acquire, then downgrade to shared (between the thread returning and the cleanup thread); boost::thread had this (unlock_and_lock_shared), not sure about std::thread. – eudoxos Sep 14 '16 at 08:06
  • @eudoxos yeah, I forgot about the exception stuff. Added a movable guard for the semaphore. – PeterT Sep 14 '16 at 08:20
  • @eudoxos the rule is that you must release mutex in same thread you aquired it, or it will be considered still holding it. It is how mutexes and locks work. Lock is just a fancy class which calls `unlock` on mutex. And you cannot make other thread relinquish its lock on mutex. It must be done by same thread which locked it. Leaking locks is a problem in multithread programming, which often leads to deadlocks. – Revolver_Ocelot Sep 14 '16 at 10:11
3

Move the std::unique_lock to the background thread.

Dominic Hofer
  • 5,751
  • 4
  • 18
  • 23
3

You can use both mutex and condition_variable to do the synchronization. Also it's dangerous to detach the background thread, since the thread might still running while the CamIface object has been destructed.

class CamIface {
public:
    CamIface() {
        background_thread = std::thread(&CamIface::stop, this);
    }
    ~CamIface() {
        if (background_thread.joinable()) {
            exit = true;
            cv.notify_all();
            background_thread.join();
        }
    }
    Image acquire() {
        std::unique_lock<std::mutex> lock(mtx);
        cv.wait(lock, [this]() { return !this->stopping; });
        // acquire your image here...
        stopping = true;
        cv.notify_all();
        return img;
    }
private:
    void stop() {
        while (true) {
            std::unique_lock<std::mutex> lock(mtx);
            cv.wait(lock, [this]() { return this->stopping || this->exit; });

            if (exit) return;   // exit if needed.

            camera.StopCapture();
            stopping = false;
            cv.notify_one();
        }
    }

    std::mutex mtx;
    std::condition_variable cv;
    atomic<bool> stopping = {false};
    atomic<bool> exit = {false};
    CameraHw camera;
    std::thread background_thread;
};
for_stack
  • 21,012
  • 4
  • 35
  • 48
  • CamIface lifetime is not an issue, I added that to the question. What you suggest is to run a thread doing the cleanup all the time, wait, and kick in whenever stopping is true? Nice idea, thanks! – eudoxos Sep 14 '16 at 07:59
  • @eudoxos Yes, there's no need to create a new thread again and again. Creating thread also takes its price. – for_stack Sep 14 '16 at 08:06
  • I guess I have to put some kind of loop into `stop()`, otherwise it will be called just once. And then check whether the dtor is called using some other variable, to know when to return? – eudoxos Sep 14 '16 at 08:12
  • @eudoxos Yes, that's a bug. You should have a loop, and also have some mechanism to make the stop thread awaken when CamIface is destroying. I'll update the answer. Thank you! – for_stack Sep 14 '16 at 08:36
1

The fact that this is hard to do correctly should indicate that your design is oddly asymmetric. Instead, put all of the camera interaction in the background thread, with all the mutex operations from that thread. Think of the camera thread as owning the camera resource and the corresponding mutex.

Then deliver the captured frame(s) across the thread boundary with a std::future or other synchronization like a concurrent queue. You could consider from here making the background thread persistent. Note that this doesn't mean that the capture has to run all the time, it might just make the thread management easier: if the camera object owns the thread, the destructor can signal it to exit, then join() it.

Peter
  • 14,559
  • 35
  • 55