1
\$\begingroup\$

I have made a little example of a code that stores frames of a camera in a buffer that runs in a separate thread and another thread can consume it. Is this model efficient?

//isEmpty_buffer is an atomic<bool>
//Mat is a opencv Matrix of pixels
//Max buffersize is 1000
//Camera mutex is a shared_timed_mutex


Mat Camera::get_last()
{
    while (isEmpty_buffer.load()) {

    }

    shared_lock<shared_timed_mutex> lock(camera_mutex);

    Mat ret = buffer.back().clone();
    buffer.front().release();
    buffer.pop();

    if (buffer.empty()) {
        isEmpty_buffer.store(true);
    }

    return ret;
}

void Camera::add_frame(Mat &frame)
{
    lock_guard<shared_timed_mutex> lock(camera_mutex);

    if(buffer.size() < MAX_BUFFER_SIZE){
        buffer.push(frame.clone());
        isEmpty_buffer.store(false);
    } else{
        cout << "Buffer at limit!" << endl;
    }

}
\$\endgroup\$
5
  • \$\begingroup\$ there is a little mistake that i fixed related to using the back insted of the front, as a queue should be \$\endgroup\$
    – Ollegn
    May 3, 2018 at 13:23
  • \$\begingroup\$ What type is buffer? \$\endgroup\$
    – Josiah
    May 3, 2018 at 18:50
  • \$\begingroup\$ Buffer is a Mat queue \$\endgroup\$
    – Ollegn
    May 4, 2018 at 15:45
  • \$\begingroup\$ MAX_BUFFER_SIZE is acutally a #define preprocessor macro \$\endgroup\$
    – Ollegn
    May 4, 2018 at 15:51
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Malachi
    May 4, 2018 at 16:09

1 Answer 1

2
\$\begingroup\$

I'll look first at some general programming principles as would be applicable to a single threaded queue, and then I'll look at the concurrency concerns.

First, if something can fail, it must be able to indicate to calling code that it has failed. In some cases, if failure means that something has gone really quite badly wrong, the way to indicate that would be with exceptions. In other cases some cases it would be by returning some sort of success code. In other cases you should rewrite the code so that it cannot fail, perhaps in this case by waiting for the buffer to get some space. However, your add_frame doesn't handle this. If the buffer is full then it prints to standard output (which in most programs is not even being monitored) but as far as the producer is concerned its image has been sent into the system and will be processed in due course. Instead, it's just gone nowhere.


As a matter of style, it is generally not advised to use using namespace, and even more so doing it twice in the same file. It's better to be explicit: use std::cout and cv::Mat. In this case in particular, there is an annoying amount of overlap between OpenCV function and object names and the standard library ones. OpenCV also supplies such things as abs and Vector (although not vector) and confusion about which you're using is like a trail of honey for bugs to find your code!


Also on style,

MAX_BUFFER_SIZE

Do not use ALL_CAPS for constants just because constants used to be macros.

Thus sayeth the style guide.

Unless MAX_BUFFER_SIZE is actually a preprocessor macro, don't use capitals. If MAX_BUFFER_SIZE is a preprocessor macro, change it to be a constexpr.


In terms of program architecture, a thread safe queue is quite a general utility method, and one that is conceptually distinct from a camera. It would probably be better to move all this code (together with the buffer, mutexes, etc) to its own class and its own file.

Because it is such a general utility, it is perhaps worthwhile templating it so that you could use a similar queue for datatypes other than cv::Mat. The only thing that currently interferes with that is the clone() call.


Because cv::Mat is an inherently costly data type, it is worth thinking quite hard about whether each clone() call is needed. By default I would not expect a queue to be deep copying its input. In a producer/consumer context, I would expect that the producer pushes to the queue once it is done creating the object, and has no further use. Therefore I would suggest that the clone() can probably be removed. It is definitely unnecessary to have a clone taken on entering and on leaving the queue! (If in fact the producer does need to hold onto the image, and further needs to ensure that the consumer doesn't write to or otherwise mess with that image, it would be more explicit to do the clone before it gets into add_frame.)


Mat ret = buffer.back().clone();
buffer.front().release();
buffer.pop();

This just looks wrong to me. If the image at the back of the buffer is the one that you're returning, why is it the image at the front of the buffer that you're releasing? Further, why are you releasing it at all? The cv::Mat destructor calls release(), so as soon as the reference counter decides you're done with it. Combined with not wanting to clone if possible, I'd expect something more like

cv::Mat ret = buffer.front();
buffer.pop;

Edit: I have noticed that the second and third lines of this got caught up in your edit to switch from back to front, and the first was missed. So it's probably just an editing bug rather than a conceptual one, and a very easy fix. I would still question the clone() and release() though.


Now for concurrency. Please note that I don't have enough context to run this code, much less extensively test it. Concurrency of course needs very extensive testing, because the bugs it produces tend to be intermittent.


I am inherently suspicious of this atomic isEmpty_buffer flag. If you can't directly check buffer.empty() because it may not be updated perfectly in sync with buffer, you also cannot rely on this external atomic variable which will also not be updated in sync with buffer.


I do not believe that a shared_timed_mutex is suitable for this use case. It is generally used when some jobs are guaranteed not to mutate the data structure. If threads aren't going to change anything, you can have as many of them swarming over it at once as you want, and hence shared becomes relevant. The mutex only needs to make sure that threads which do change things get exclusive access, so that a thread which is only reading doesn't read half the state from before an update and half after. (And also so that two updates don't clash) However this is not what's happening here: both the producer and the consumer change the data structure by pushing and popping.

It might in fact work out okay if you guarantee that there is never more than one consumer, in which case you're not really using the shared aspect of the mutex at all. If this is the case you should be very explicit about your assumptions.


Presuming that you selected instead a more conventional mutex, your

while (isEmpty_buffer.load()) {

}

is still not enough to prevent things from breaking if there are multiple consumers. It is outside the lock, which means that when the queue has one element in it, two consumers could both get past that check and queue up in front of the mutex. The first consumer would go through, pop the element, and unlock the mutex on its way out. Then the second consumer would go through, pop against the empty buffer, and crash and burn.

\$\endgroup\$
6
  • \$\begingroup\$ isn't using constant expressions just unnecessary memory access as the values could be just put directly in the code with a preprocessor? \$\endgroup\$
    – Ollegn
    May 4, 2018 at 15:55
  • \$\begingroup\$ The .clone() was put there because the default behavior of the mat is a shallow copy, i was afraid that the data would be unallocated after the scope ended. \$\endgroup\$
    – Ollegn
    May 4, 2018 at 16:00
  • \$\begingroup\$ the .back() that that should be a .front() was a mistake that i fixed in the code but forgot to fix here, im sorry \$\endgroup\$
    – Ollegn
    May 4, 2018 at 16:03
  • 1
    \$\begingroup\$ If you have even basic compiler optimisations on, constexpr literals will be put straight into the code at compile time, just as much as if you used a preprocessor or even the number explicitly. \$\endgroup\$
    – Josiah
    May 4, 2018 at 16:43
  • 1
    \$\begingroup\$ As to Mat, it is true that they are shallow copied but the copy includes a reference counting smart pointer. If openCV allocated the memory in the first place, it is usually safe. You might need one clone if you allocated the memory buffer yourself and then wrapped a Mat around it. I am pretty sure that you don't need two. \$\endgroup\$
    – Josiah
    May 4, 2018 at 17:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.