Add Intel LibRealSense grabber and viewer #1633

lebronzhang · 2016-06-15T09:02:48Z

This pull request adds a new grabber for Intel RealSense devices (e.g. Intel® RealSense™ F200, SR300 and R200 cameras) that supports Intel librealsense. I think it is ready to be merged into the PCL.

If there is anything needed to be done, please contact me immediately. Thanks.

lebronzhang · 2016-06-15T09:11:53Z

This grabber could also close #1215

taketwo · 2016-06-15T09:18:31Z

PCL definitely needs support for librealsense, but I strongly dislike an idea of having both RealSenseGrabber and LibRealSenseGrabber. In my opinion, this should be a single grabber which can choose between backend drivers. From the end-user perspective, there should be no difference if SDK of librealsense is used! Also, in it's current form, LibRealSenseGrabber is largely a copy-paste of the RealSenseGrabber, which suggests that most of the code could and should be shared. Finally, the proposed grabber exposes no control for the frame rate and resolution, which is a major missing feature.

lebronzhang · 2016-06-16T08:45:44Z

@taketwo Thank you for your replay, I am happy to know that PCL needs librealsense. Your points make sense, we should extract just one interface for the RealSense camera users and the control of frame rate and resolution should be added. However, I am not sure how to design the interface for this moment, I will discuss with my colleagues on how to design the grabber and if there is any update, I will post it in this thread. You are always welcomed to give me some suggestions, thank you again.

Good day!

lebronzhang · 2016-06-23T06:11:32Z

@taketwo Hi, sorry for the late update. I have discussed with my colleague and take your words as a reference, we will change our grabber name as RealSenseGrabber too, and let PCL decide which one will be compiled based on the running operating system and existing library. Then we will extract the common part of two grabbers as pcl::io::real_sense::common, and both grabbers hold the object. Therefore, we could share the common code and deal with SDK related part in our own grabber. The attached files are the former RealSenseGrabber class diagram and the latest RealSenseGrabber class diagram respectively, please check it out. If you have any suggestions, please reach me at once, thank you.
The former RealSenseGrabber class diagram:

The latest RealSenseGrabber class diagram:

Good day!

taketwo · 2016-06-29T09:28:45Z

Since we decide which driver back-end to use during the configuration of PCL, we don't need any "real" polymorphism. We just need to separate functions into common and backend-specific at the level of source code files.

That means, there is one and only "real_sense_grabber.h", it has no backend-specific code. And in the implementation file "real_sense_grabber.cpp" we keep only common functions, and put SDK and librealsense-dependent stuff into separate files (e.g. "real_sense/sdk/real_sense_grabber.cpp" and "real_sense/librealsense/real_sense_grabber.cpp"). And CMake decides which one to compile. The same for the device manager code.

What do you think?

lebronzhang · 2016-06-30T02:38:37Z

@taketwo Thank you for your suggestion! I love your solution, actually I am doing it mostly like your suggestion since I found it is not a good way to extract a common class, and there is a little bit difference between what I have done and your suggestion. But I think your solution is better than mine, I will use your solution and test it when I have finished.
You are always welcomed to give me another suggestion :)
Thank you again!

Good day!

huningxin · 2016-07-11T03:20:14Z

@lebronzhang , please rebase to master. Thanks!

lebronzhang · 2016-07-11T04:51:31Z

@taketwo please take a look. Thanks!

huningxin · 2016-07-11T05:02:05Z

@lebronzhang , the buildbot failed, please take a look.

lebronzhang · 2016-07-11T09:27:24Z

@huningxin Thank you for your reminder, all checks have passed now:)
@taketwo please take a look, if there is anything that needed to be done, please let me know, thanks!

taketwo · 2016-07-11T14:11:53Z

In general looks fine, pretty much how I expected it.

One thing that I'm not satisfied with though is the CMake-related bits. Now we have two options: WITH_RSSDK and WITH_LIBREALSENSE. What happens if both are enabled? Not clear for an end-user. (Actual answer is: SDK will be used.) I think we should have one switch to enable Real Sense support, and another one to choose between backends. But this is something I can look into later after this pull request is merged.

Would be nice to have some people confirming that this works with both backends on Windows and with librealsense on Linux.

taketwo · 2016-07-11T14:12:38Z

io/src/real_sense/librealsense/real_sense_grabber.cpp

+  {
+    PCL_WARN ("[pcl::RealSenseGrabber::setConfidenceThreshold] Attempted to set threshold outside valid range (0-15)");
+  }
+  else if (!device_->getDevice ()->supports_option (rs::option::f200_confidence_threshold))


Why f200_confidence_threshold? Don't other cameras support it?

According to https://github.com/IntelRealSense/librealsense/blob/5831907ed32a52d9ff4cf7170c0580de8c283ff5/include/librealsense/rs.hpp#L61, there is only f200_confidence_threshold defined.

SergioRAgostinho · 2016-07-11T15:15:27Z

I'm not sure who has hardware for testing this, but we can always request some help on the dev's mailing list.

Although working with grabbers tends to be more of the same, it would also be cool if you guys could provide a tutorial, especially detailing how to select your backend.

taketwo · 2016-07-11T15:27:17Z

Hardware I have, if only someone gives me some time... :D
I'll give it a try later, but don't know when.

SergioRAgostinho · 2016-07-11T15:50:40Z

Well, I think this will be integrated into 1.9.0 and it would be cool to release a 1.8.1 before, so I would say there's no rush.

huningxin · 2016-07-12T00:53:48Z

I'm not sure who has hardware for testing this, but we can always request some help on the dev's mailing list.

@lebronzhang did test the RSSDK backend on Windows and librealsense backend on Linux. They are the most common use cases IMO.

@lebronzhang , could you please share the test results here?

Although working with grabbers tends to be more of the same, it would also be cool if you guys could provide a tutorial, especially detailing how to select your backend.

Great idea. @taketwo , did you have the RSSDK grabber tutorial? It would be good to have a tutorial of RealSense cameras and include backend selection steps. What do you think?

lebronzhang · 2016-07-12T04:43:15Z

Hi, guys, I have tested the RSSDK backend on Windows and librealsense backend on Linux. Here is the details:
RSSDK backend on Windows
Machine:
OS version: 10.0
CPU: Intel i5-5200U @ 2.20GHz
GPU: NVIDIA GeForce 840M
RSSDK version: 8.0.24.6528
IDE: Visual Studio Professional 2015
The new RealSenseGrabber has same performance as the old one, but there are two problems with my test:

the framerate is low (around 11) with pcl_real_sense_viewer_debug.exe.
When I closed the viewer's window, an exception occurs.

I found the same problems in the PCL master branch, so do you ever come across these problems? What do you think the problem might be? @taketwo

Librealsense backend on Linux
Machine:
OS version: ubuntu 14.04 LTS
CPU: Intel i7-5960X @ 3.00GHz × 16
GPU: NVIDIA GeForce GTX 960
librealsesne version: Compiled with the source
Everything works fine with new RealSenseGrabber, but one thing should be noticed:
The librealsense is written in standards-conforming C++11.
So the CMAKE_CXX_FLAGS at least should be "-std=c++11" to compile PCL with librealsense, maybe you want to mention this in the tutorial of RealSense cameras. @taketwo

SergioRAgostinho · 2016-07-12T12:23:02Z

When I closed the viewer's window, an exception occurs.

Backtrace it if possible.

The librealsense is written in standards-conforming C++11.

Well now this definitely pushes it to 1.9.0. ^^

UnaNancyOwen · 2016-07-12T13:00:36Z

io/src/real_sense/librealsense/real_sense_grabber.cpp

+      }
+      for (int dy = 0; dy < depth_height; ++dy)
+      {
+        uint i = dy * depth_width - 1;


Why do you use uint instead of uint32_t? (real_sense_grabber.cpp #L215, #L280)

Thank you for your reminder, this is a mistake, I will modify it at once:)

nouei · 2016-12-05T21:05:24Z

realsense produces pcl::PointXYZRGBA but what we should do if we want to use pcl::PointXYZRGB ?

Aphoh · 2017-07-21T08:35:03Z

Any update on this?

SergioRAgostinho · 2017-07-21T08:49:35Z

We've started reviewing PRs in LIFO order. This won't be merged before the next release. If you need it already just apply the PR on your local fork.

cjue · 2017-08-24T14:03:28Z

For those interested in using this on Linux:

Merging this PR into current master works without a hitch.
Tested on Ubuntu 16.04.2 with a RealSense R200.
pcl_real_sense_viewer worked like a charm.

Setup

add -std=c++11 to CMAKE_CXX_FLAGS
get librealsense and set LIBREALSENSE_DIR. Example:

 sudo apt-get install ros-kinetic-librealsense
 export LIBREALSENSE_DIR=/opt/ros/kinetic/

run cmake and build
run pcl_real_sense_viewer

SergioRAgostinho · 2017-09-03T11:02:55Z

cmake/Modules/Findlibrealsense.cmake

+    endif ()
+  else (WIN32)
+    set(LIBREALSENSE_RELEASE_NAME librealsense.so)
+    set(LIBREALSENSE_DEBUG_NAME librealsense.a)


This doesn't make much sense, in contrast with the windows branch. The first one is the shared lib and the second the static one. But both can be either release or debug.

SergioRAgostinho · 2017-09-03T11:05:46Z

cmake/Modules/Findlibrealsense.cmake

+include(FindPackageHandleStandardArgs)
+find_package_handle_standard_args(LIBREALSENSE LIBREALSENSE_LIBRARIES LIBREALSENSE_INCLUDE_DIRS)
+
+endif (LIBREALSENSE_LIBRARIES AND LIBREALSENSE_INCLUDE_DIRS)


Lack of new line at the end of the file.

SergioRAgostinho · 2017-09-03T11:14:56Z

io/include/pcl/io/real_sense/librealsense/real_sense_device_manager.h

+
+      class RealSenseDevice;
+
+      class PCL_EXPORTS RealSenseDeviceManager : boost::noncopyable


The moment you define your custom destructor the class is stripped of default copy operators, so this is not so usefull anymore. But then again, your destructor is not doing anything so it should just be deleted.

SergioRAgostinho · 2017-09-03T11:22:31Z

io/include/pcl/io/real_sense/librealsense/real_sense_device_manager.h

+
+          typedef boost::shared_ptr<RealSenseDeviceManager> Ptr;
+
+          static Ptr&


Shouldn't this be a const reference? Your class is implemented as a lazy initialized singleton, so I assume you don't want to allow it to be reset and a new manager allocated later on.

SergioRAgostinho · 2017-09-03T11:29:17Z

io/include/pcl/io/real_sense/librealsense/real_sense_device_manager.h

+  } // namespace io
+} // namespace pcl
+
+#endif /* PCL_IO_REAL_SENSE_DEVICE_MANAGER_H */


New line at the end of the file.

SergioRAgostinho · 2017-09-03T11:39:14Z

io/src/real_sense/librealsense/real_sense_device_manager.cpp

+{
+  // This is called from public captureDevice() functions and should already be
+  // under scoped lock
+  RealSenseDevice::Ptr device (new RealSenseDevice (device_info.serial));


Use make_shared instead.

SergioRAgostinho · 2017-09-03T11:39:32Z

io/src/real_sense/librealsense/real_sense_device_manager.cpp

+  device->device_ = context_.get_device (device_info.index);
+  device_info.isCaptured = true;
+  return device;
+}


No new line at the end of the file.

SergioRAgostinho · 2017-09-03T12:21:40Z

io/include/pcl/io/real_sense/librealsense/real_sense_device_manager.h

+          friend class RealSenseDeviceManager;
+
+          std::string device_id_;
+          rs::device* device_;


I don't like this very much. You're passing a pointer to the user which is managed by the realsense. If the user wants he can delete the pointer. Instead I propose to hold a reference to the rs::device. The user can do whatever he wants with the reference without being able to delete the resource.

SergioRAgostinho · 2017-09-03T12:35:02Z

io/src/real_sense/librealsense/real_sense_grabber.cpp

+    }
+  }
+  device->stop ();
+}


Needs a new line at the end of the file.

SergioRAgostinho · 2017-09-03T12:42:22Z

io/src/real_sense/librealsense/real_sense_device_manager.cpp

+
+pcl::io::real_sense::RealSenseDeviceManager::~RealSenseDeviceManager ()
+{
+}


To be deleted.

stale · 2017-12-14T14:34:09Z

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

nawalgupta · 2018-01-19T19:48:14Z

Does this grabber supports Intel® RealSense™ SDK 2.0 (Librealsense) 2 for D400, D415, D430 Camera?

kne1p · 2018-01-20T01:53:42Z

I'd like to see this merged on master.

If I apply the requested changes, should I create my own pull request (with attribution to lebronzhang), or can I attach it to this one in some way?

SergioRAgostinho · 2018-01-22T11:33:25Z

Hey. This one requires C++11 support which we currently don't officially support but it's a milestone for the first quarter/half of this year. So merging to master (even after applying whatever change might be necessary) is not really possible at this point.

I would apply it on your personal fork and keep it public for the time being.

UnaNancyOwen · 2018-01-22T14:26:02Z

@nawalgupta No, This grabber based on librealsense 1.x.
I think RealSense D400 series is one candidate of sensor to replace Kinect series.
In the future, I plan to implement the grabber based on librealsense 2.x.

stale · 2018-03-23T14:38:08Z

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

stale · 2020-02-21T20:43:39Z

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

kunaltyagi · 2020-05-21T19:32:03Z

@SergioRAgostinho We don't really need libRealSense1.0 grabber now that we have libRealSense2.0 grabber? If not, please close this as well as #1215

huningxin mentioned this pull request Jul 11, 2016

Combine librealsense and RSSDK as one single grabber lebronzhang/pcl#14

Merged

lebronzhang force-pushed the master branch from 7d9a628 to e9c60f6 Compare July 11, 2016 04:46

lebronzhang force-pushed the master branch 3 times, most recently from 6fc567b to 9128e24 Compare July 11, 2016 07:33

taketwo reviewed Jul 11, 2016
View reviewed changes

SergioRAgostinho added the needs: testing Specify why not closed/merged yet label Jul 11, 2016

SergioRAgostinho added this to the pcl-1.9.0 milestone Jul 11, 2016

lebronzhang force-pushed the master branch from 9128e24 to cec650a Compare July 12, 2016 01:52

UnaNancyOwen reviewed Jul 12, 2016
View reviewed changes

Add librealsense support to RealSenseGrabber

d20fa4e

SergioRAgostinho requested changes Sep 3, 2017

View reviewed changes

SergioRAgostinho added the changelog: new feature Meta-information for changelog generation label Nov 27, 2017

stale bot added the status: stale label Dec 14, 2017

SergioRAgostinho removed this from the pcl-1.9.0 milestone Dec 19, 2017

stale bot removed the status: stale label Dec 19, 2017

SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed needs: more work Specify why not closed/merged yet labels Dec 19, 2017

daniel-packard mentioned this pull request Feb 13, 2018

Add RSSDK 2.0 (librealsense2) grabber #2214

Merged

SergioRAgostinho added the c++11 label Feb 13, 2018

stale bot added the status: stale label Mar 23, 2018

kunaltyagi removed the modernization label Feb 22, 2020

stale bot removed the status: stale label Feb 22, 2020

kunaltyagi removed the stale label Feb 27, 2020

taketwo added the status: stale label Mar 3, 2020

stale bot removed the status: stale label May 21, 2020

SergioRAgostinho closed this May 22, 2020


		class RealSenseDevice;

		class PCL_EXPORTS RealSenseDeviceManager : boost::noncopyable


		typedef boost::shared_ptr<RealSenseDeviceManager> Ptr;

		static Ptr&

Add Intel LibRealSense grabber and viewer #1633

Add Intel LibRealSense grabber and viewer #1633

Conversation

lebronzhang commented Jun 15, 2016

lebronzhang commented Jun 15, 2016 • edited Loading

taketwo commented Jun 15, 2016

lebronzhang commented Jun 16, 2016

lebronzhang commented Jun 23, 2016

taketwo commented Jun 29, 2016

lebronzhang commented Jun 30, 2016 • edited Loading

huningxin commented Jul 11, 2016

lebronzhang commented Jul 11, 2016

huningxin commented Jul 11, 2016

lebronzhang commented Jul 11, 2016

taketwo commented Jul 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioRAgostinho commented Jul 11, 2016

taketwo commented Jul 11, 2016

SergioRAgostinho commented Jul 11, 2016

huningxin commented Jul 12, 2016

lebronzhang commented Jul 12, 2016

SergioRAgostinho commented Jul 12, 2016

UnaNancyOwen Jul 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nouei commented Dec 5, 2016

Aphoh commented Jul 21, 2017

SergioRAgostinho commented Jul 21, 2017 via email

cjue commented Aug 24, 2017

Setup

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Dec 14, 2017

nawalgupta commented Jan 19, 2018

kne1p commented Jan 20, 2018

SergioRAgostinho commented Jan 22, 2018

UnaNancyOwen commented Jan 22, 2018 • edited Loading

stale bot commented Mar 23, 2018

stale bot commented Feb 21, 2020

kunaltyagi commented May 21, 2020

lebronzhang commented Jun 15, 2016 •

edited

Loading

lebronzhang commented Jun 30, 2016 •

edited

Loading

UnaNancyOwen Jul 12, 2016 •

edited

Loading

UnaNancyOwen commented Jan 22, 2018 •

edited

Loading