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.
buffer
? \$\endgroup\$