UVC camera support - #492
Conversation
…ue to races between the camera and processing threads.
|
|
||
| if(NOT WIN32) | ||
| find_package(libuvc REQUIRED) | ||
| find_package(PkgConfig) |
There was a problem hiding this comment.
We should have a find module for libusb1 already, shouldn't need to use just PkgConfig.
There was a problem hiding this comment.
OK, will attack this next.
| //Check if there are any frames available | ||
| if (frames_.empty()) | ||
| //Wait for some frames to become available | ||
| frames_available_.wait(lock); |
There was a problem hiding this comment.
can you put curly braces around the "then" part here, and also run clang-format?
Also, I think this would be subject to spurious wakeups - might want to use the other form of .wait that lets you pass in a predicate.
There was a problem hiding this comment.
I only use one notify statement, which happens after the frame is added, but I agree, a predicated version would be neater. I've pushed a patch for this.
|
|
||
| current_frame = frames_.front(); | ||
| frames_.pop(); | ||
| }; |
There was a problem hiding this comment.
think you don't need the semicolon
There was a problem hiding this comment.
OK, I've just fixed this and run clang-format.
| // Convert the image to at cv::Mat | ||
| color = cv::Mat(rgb->height, rgb->width, CV_8UC3, rgb->data).clone(); | ||
|
|
||
| uvc_free_frame(current_frame); |
There was a problem hiding this comment.
could this perhaps be handled by a RAII "finally" from <osvr/Util/Finally.h>, so we don't accidentally leak frames?
(Or, tbh, I know it's @godbyk 's initial code, but could really go for using unique_ptr with custom deleter - yes, those exist! just need to be in the type - on those UVC frames and a little helper make_uvc_frame_buffer function that returns a buffer allocated with libuvc and managed by such a unique_ptr. That would be the ideal way to go.)
There was a problem hiding this comment.
Sure, I can try to tidy this up with unique_ptrs, I guess something similar is needed for the frames_ queue?
There was a problem hiding this comment.
@toastedcrumpets I think you could use something like this (untested):
#include <memory> // for std::unique_ptr
#include <functional> // for std::function
using UvcFramePtr = std::unique_ptr<uvc_frame_t, std::function<void(uvc_frame_t*)>>;
inline UvcFramePtr make_unique_uvc_frame(size_t data_bytes)
{
return UvcFramePtr{ uvc_allocate_frame(data_bytes),
[](uvc_frame_t* f) { uvc_free_frame(f); }};
}|
Its 10pm UK time, so I'll have to work on this tomorrow.
|
|
oh no problem, thanks for doing this in the first place! |
|
@rpavlik Correct me if I'm wrong, but I think I have now addressed all of your comments. I've moved to smart pointers for all the libuvc raw pointers and changed the CMakelists.txt to use your FindLibusb1.cmake module. I know that you're currently working on a new tracker branch (is that what blobs-undo-bad is?), but If this pull request and #482 are merged into master, then at least ubuntu users (and probably a lot more) can get a reasonable out-the-box experience with the positional tracker code. Finally, it would be interesting to see how well this works on Windows! |
|
Also, thanks to @godbyk for getting this started! |
|
Quick "port" to the blobs-undo-bad branch: NOT including the last two commits "Move to smart pointers for handling libuvc ptr types" and "Change to use FindLibusb1.cmake rather than pkgconfig". Config file for the HDK2: https://gist.github.com/ChristophHaag/5d8f217ae58408ee8cd60b33c914e21f |
|
It's quite bad though. Dropping frames like crazy and very high cpu usage. @toastedcrumpets You should add a newline after the message that frames are dropped. |
I'd hold off on this just yet, as its likely this pull request will "mature" some more as @rpavlik reviews it.
This was done in the smart pointers commit.
On my system, I get 60% cpu utilisation for osvr_server (69% when debug is on), with no dropped frames. Can you provide more details on your system for comparison? I noticed performance dropped a little when I moved to smart pointers, this is probably due to the overhead of moving the smart pointers in an out of the queue. I'll take a look if I can do that a little better, but it is difficult given the use of a deleter. |
|
I meant performance with the libuvc on top of the blobs-undo-bad is bad: https://github.com/ChristophHaag/OSVR-Core/commits/uvc-camera-blobs-undo-bad. The tracking also jumps around randomly. Perhaps it's just not working right yet? |
|
Ah OK. On 20 Oct 2016 10:40 a.m., "Christoph Haag" notifications@github.com
|
|
New tracker branch is not ready for prime time atm if you have the IMU data turned on. |
rpavlik
left a comment
There was a problem hiding this comment.
Took a quick look - unique_ptr shouldn't add any overhead, but saw why it was (you were using stateful deleters instead of stateless deleter functors). A handful of other suggestions as well from just a read-thru.
| uvc_context_t *uvcContext_; | ||
| uvc_device_t *camera_; | ||
| uvc_device_handle_t *cameraHandle_; | ||
| typedef std::unique_ptr<uvc_frame_t, decltype(&uvc_free_frame)> |
There was a problem hiding this comment.
So this is a "stateful deleter", so it's larger/slower than a single pointer. Something akin to the following (not tested, just coded in the comment box, but hopefully it's close) would let you skip the second constructor parameter to unique_ptr and would remove the state (and thus overhead) from the smart pointer.
struct UVCFrameDeleter {
void operator()(uvc_frame_t * ptr) const { uvc_free_frame(ptr); }
};
using FramePtr = std::unique_ptr<uvc_frame_t, UVCFrameDeleter>;There was a problem hiding this comment.
OK, have already fixed this and just pushed! Hopefully you don't mind the slight bit of template programming... This should be a very common pattern, so I wouldn't be surprised if something already exists?
| } | ||
| "disrupt tracking." | ||
| << std::endl; | ||
| frames_ = std::queue<Frame_ptr>(); //< clear the queue |
There was a problem hiding this comment.
Is there a member .clear() function that can be called instead?
There was a problem hiding this comment.
Nope! I'm as shocked as you are.
There was a problem hiding this comment.
@toastedcrumpets You could use std::vector instead. It has push_back() and pop_back() methods as well as clear().
There was a problem hiding this comment.
Thanks for the input godbyk, but I need a push_back() and pop_front() as I need a FIFO. If you implemented your own pop_front() on a std::vector, you'd have to reshuffle all the vector contents, which is additional overhead.
If I was going for speed, I'd actually use a pool of pre-allocated frame pointers which have the linked list structure built in (i.e., like boost's intrusive containers). This would avoid the malloc/free I'm performing each frame, and give me all the container operations I need (clear is just removing the front reference).
However, this is a function which is called 100 times a second AT MOST, and performs next to no computation. I think the current code is more readable, and "premature optimisation is the root of all evil!", so lets see if any performance tests show this as a bottle neck (my bet is that the image processing is the bottleneck here).
| current_frame = frames_.front(); | ||
| frames_.pop(); | ||
| // Grab a frame from the queue, but don't keep the queue locked! | ||
| std::unique_lock<std::mutex> lock(mutex_); |
There was a problem hiding this comment.
I'd suggest just opening a scope here for the lock and avoiding the manual unlock.
There was a problem hiding this comment.
With my previous Frame_ptr implementation I needed to RAII it, which meant I couldn't close the scope as I created the Frame_ptr in the same scope, but moving to the stateless implementation will fix this.
|
and yes, @ChristophHaag I'd suggest hanging on a bit until we get this totally hammered out here, then we can port it ahead to the new tracker. |
|
Before completing this pull request, I want to try moving the RGB conversion into the uvc thread callback, as this eliminates one copy of the frame and shouldn't be much more work than the copy already done by the callback. I'll commit the patch in a few hours. |
|
OK, I think I'm done again. Please review and let me know if you need more changes. I could squeeze more performance out of the code by using a pool of preallocated frames to avoid any malloc/free during running (see discussion above with @godbyk) but I think this is "premature optimisation" and think the current code is readable/fast enough (its only run 100 times per second). |
|
@rpavlik I'm keen to get this pull request accepted and #339 closed out (I'll try to collect the bounty, although it should be shared with @godbyk). Can you let me know how this should be progressed? I note your CI travis build system is failing to compile the branch as it does not have libuvc installed (thus the FindLibuvc.cmake file is missing), can this be added? I'd appreciate having a second pair of eyes/compiler checking the build on a range of systems. Also, with regards to porting to blobs-undo-bad (the new tracker). I'd be happy to complete the port to that branch and use the atomic queue structure there if that's what it takes to get it pulled in to the new code. Let me know if this would be useful. |
|
@toastedcrumpets You can ignore AppVeyor for now as I haven't finished getting it set up yet. For Travis CI, you can edit the |
|
@godbyk Thanks for the heads-up on travis.yml. Haven't seen that before, its amazing you can get free CI tests, even for OSX builds! Sure beats CDash for ease of use, I spent ages setting that up for my own projects... I've just pushed a couple of commits which build and install libuvc, now this pull request compiles on the linux systems. The OSX systems seem to have issues with OpenCV, but I guess that should be handled outside this pull request. |
…t time I used OSVR)
|
I've realised that this commit needs more work but I need some clarifications from @rpavlik or any other developer.
I've realised that above is incorrect, libuvc/usb does not allow that kind of control, the current approach is best.
Second, I note mention of a high-gain 50hz mode in the code, is this something you want enabled? libuvc has controls for fps and gain if they are needed still. |
|
To put these patches into your local repository, run |
|
I have not had a chance to get to this, sorry - I anticipate doing so after I get the new tracker branch finished. The 50Hz is actually not framerate or gain, it's the anti-flicker setting (the one that's either "none", 50Hz, 60Hz), that in these cameras for some reason 50Hz is the high-gain mode used. |
|
@toastedcrumpets re the api rework around the uvc support: the new tracker branch does have a reworked video input pipeline, might be worth looking at, I'd appreciate your comments on it if it needs improvements, etc. It's certainly cleaner, and provides better data (can propagate the timestamp from the capture driver, instead of tacking one on later, which turns out to be a non-negligible difference especially since the new tracker "back-dates" images based on camera and capture stack latency so that it fuses the data at the right point in time) |
|
@rpavlik I can try to write a patch against the new tracker branch if you want and give it a review at the same time? Anything to try to get this mainlined and close #339. Can you just confirm the new API branch is blobs-undo-bad? BTW, getting honest frame timestamps from usb cameras might be difficult. The onboard camera itself has no timing information, and USB is a master driven bus, so there will always be some uncertainty; however, I will do my best to get as accurate a stamp as possible! |
|
I did that here, just to try it out: #493 Not super well done, so you can rewrite it if you want, or just finish it (still needs the travis stuff and the timestamp likely needs to be fixed, otherwise it works), |
|
Thanks @ChristophHaag, I'd still like it if @rpavlik can confirm that's what he wants the patch based off. I'd like to use a reasonably functioning commit to be able to perform some testing. |
fixes compilation after libuvc/libuvc#58
add libusb.h include
|
Yeah, blobs-undo-bad is the latest tracker branch, and the unifiedvideobasedinertial plugin is the one. Sorry for the slow response: GitHub email is too high volume ATM for them to be really useful, so I end up taking more time than I'd like to respond to things directly addressed to me. |
|
oh my gosh, I apologized for slow response a year ago. This is quite shameful. But, @ChristophHaag reminded me, and I will try to get this merged this week, since something's better than nothing, etc. (and I'm booting full-time into Linux on my main machine ATM so it's easier for me to test now) This "old" tracker plugin only really works for the HDK 1.x since the LEDs got moved (and some removed) on the HDK2, but I know @ChristophHaag has ported this over to the new tracker plugin, etc, so this is presumably long-since ready to use. |
|
Thank you so much for your contribution. Sorry it languished here for so long. |
|
No problem, thanks for merging it! I had given up on it after a while. When I tried @ChristophHaag pull on the badblobs branch it wasn't working (this was way back when though). If you tell me where you want the new version (confirm the current development branch) I might try to get it working there; however, I still only have a OSVR HDK1 so I can't test on the HDK 2. |
|
The bounty still sitting there was what was bugging me the most, so that's the idea. You did all the hard work, you should get it. The other pull request is almost an exact port of your commits to the new tracker, only minor modifications were necessary, and every time I've tried running it, it worked fine. It's probably still a bit messy because I was mostly concerned in getting something working to test and not much about the looks and style. |
|
All the stuff for the new tracker plugin is in the master branch now. Yeah, you can use parts of the HDK2Upgraded config to get a 1.x config (if there isn't one? oops) - since an hdk2 produce via the upgrade kit still has the HDK 1 LED pattern. No problem using that with the new plugin, it supports both. BTW - if anybody feels like figuring out how to make Travis CI quiet down (if nothing else, just make the libuvc stuff optional and just disable the plugin if it's not found), 👍 :) Yeah, we intended for you to get the bounty, didn't realize there was another step to do that. |
This is a patch which implements libuvc-based tracking camera support as default on non-windows systems and addresses #339.
This has been tested on Ubuntu 16.04.