Added Multi Camera Grabbing with Threads - #11
Conversation
|
This pull request is the example solution for the issue |
tpanzarella
left a comment
tpanzarella
left a comment
There was a problem hiding this comment.
I made some specific comments in-line below and I do have a few more.
I think we should revise this a bit before publishing it as the idiomatic way to handle multiple cameras across a thread pool. I do think the triggering chain shown is informative but I believe the example should be more fully worked out. For example, how do you cleanly terminate each thread (e.g., while (true) {...})? Also, I think the callbacks that receive the data should do something with the actual image data (it could simply be accessing one of the underlying image arrays) not just print to the console. Again, just to make the example more realistic and applicable for those looking for instruction.
I have not had the time to actually build and run this code, so, I apologize for not having more concrete observations, but, this is my initial feedback from reading the diffs.
| while(true) | ||
| { | ||
| std::this_thread::sleep_for(std::chrono::duration<int, std::milli>(1)); | ||
| ifm3d::ImageBuffer::Ptr image_buffer = std::make_shared<ifm3d::ImageBuffer>(); |
There was a problem hiding this comment.
Why is a new ImageBuffer being allocated on each loop iteration? This is inefficient. The internals of the ifm-supported image containers intentionally use the .create(...) factory functions on the internally wrapped cv::Mat objects so that memory allocations are kept to a minimum in high frequency frame grabbing loops. Writing the code this way will incur expensive heap allocations for every frame acquired from the camera.
| std::this_thread::sleep_for(std::chrono::duration<int, std::milli>(1)); | ||
| ifm3d::ImageBuffer::Ptr image_buffer = std::make_shared<ifm3d::ImageBuffer>(); | ||
| const bool in_time = frame_grabber->WaitForFrame(image_buffer.get(), 10000); | ||
| callback(image_buffer,in_time); |
There was a problem hiding this comment.
In general, it is an anti-pattern to pass a std::shared_ptr to a function that does not participate in the ownership of the underlying pointer. This is mostly due to performance. For each callback you are incurring a lock -> ref count increment -> unlock then at the end of the function lock -> ref count decrement -> unlock. As with my comment above, in high frequency frame grabbing loops this cost will accumulate to something non-trivial. It would be recommended to pass the naked pointer as callback(...) (I assume) does not participate in the ownership of the ImageBuffer.
There was a problem hiding this comment.
So the idea of passing the std::shared_ptr as copy was to have a clean ownership handling. If I do pass the naked pointer the ownership is not defined in a clean way. What happens if the user in the callback enqueue the ImageBuffer in an input queue of another thread for example? The callback goes out of scope the image buffer will be destroyed because the reference count is zero. We will get an dangling pointer. I agree this is not the most performant way but a trade off between performance and a robust example.
There was a problem hiding this comment.
So, I think the signature of callback does in fact express ownership intentions. In this case, I think you are saying that you expect callback to participate in the ownership of the ImageBuffer. As noted in my message above, I assumed it did not participate in the ownership which is why I made my comment.
I think that perhaps more realistically, the user should pass in the ImageBuffer. This alleviates both of my concerns: 1) too many costly heap allocations; 2) clarity on ownership. Of course, this brings up thread synchronization issues as ImageBuffer is not thread safe, so, perhaps also a mutex (or other lock) should be passed in as well -- better yet, an interface which wraps/subclasses (not sure if a subclass would work) the ImageBuffer, perhaps a ThreadSafeImageBuffer or something like that. This approach solves the two issues called out above and thread safety.
There was a problem hiding this comment.
@tpanzarella thanks for pointing out the thread safety issue of the ImageBuffer
2337cd6 to
307ebb5
Compare
|
Resolved the issue clean exit of threads, Imagebuffer is now created outside the grabbing loop. |
|
@graugans @theseankelly I have resolved all the issues can we Merge this PR |
theseankelly
left a comment
theseankelly
left a comment
There was a problem hiding this comment.
This still needs some additional work. In addition to the specific comments in the code review:
- The code doesn't compile for me without changes -- has it been tested??
- Please audit formatting and consistency, specifically:
- 2-space tabs everywhere
- sanitze for trailing whitespace (present on several lines)
- break up long lines (80 character columns)
|
|
||
| //for configuration of required trigger on camera | ||
| void configuration(ifm3d::Camera::Ptr& camera, trigger_mode type) | ||
| { |
There was a problem hiding this comment.
consistency: use 2 space tabs everywhere
307ebb5 to
a8385c1
Compare
a8385c1 to
c2da04f
Compare
Resolve "[Maintenance] OVP8xx dir README" Closes #11 See merge request syntron/support/csr/ifm3d/ifm3d-examples!14
4 participants
example for Grabbing from multiple cameras using thread.