Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cuda registration #822

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

blackzafiro
Copy link

I see there are a few things which can be improved, but this is a functional prototipe. Please let me know what you think.

blackzafiro and others added 6 commits April 4, 2017 16:58
…compile with thrust, since I get an exception due to the use of catch.
CudaRegistration is properly initialized, with depth and color maps.
Apply with cuda, filtering is not ready.

Finished registration with cuda, still untested.

Added LIBFREENECT2_API to CudaDeviceFrame.

CudaRegistration apply is working.
@xlz
Copy link
Member

xlz commented Apr 4, 2017

Will need thrust to create 3D clouds

Why?

CMakeLists.txt Outdated
@@ -125,6 +125,7 @@ SET(SOURCES
include/libfreenect2/packet_pipeline.h
include/internal/libfreenect2/packet_processor.h
include/libfreenect2/registration.h
include/libfreenect2/cuda_registration.h
Copy link
Member

@xlz xlz Apr 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API specific implementation class should be put in registration.h, not cuda_registration.h. See how CUDA depth processor doesn't have its own header instead its definition is in depth_packet_processor.h.

Here was the API design I outlined #744 (comment)

But actually, don't worry about this at this moment.

class LIBFREENECT2_API CudaDeviceFrame: public Frame
{
public:
/** Construct a new frame.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should fix your indentation. This project uses 2 spaces. It's hard to look at code that switches between indentation styles.

* uint8_t g = p[1];
* uint8_t r = p[2];
*/
void getPointXYZRGB(const Frame* undistorted, const Frame* registered, thrust::device_vector<TupleXYZRGB>& cloud_data) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is mostly for testing. I think most of the users would generate the point cloud with http://wiki.ros.org/depth_image_proc.

So don't change its signature. Add another method to do point cloud generation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all your comments. I will address them as soon as I get a chance.

@xlz
Copy link
Member

xlz commented Apr 4, 2017

There are more issues related to API design. But let's test the algorithm's correctness first.

(But you still need to remove thrust references from build first.)

The next step is to create test cases that compare CPU and CUDA registration results, specifically for both use cases:

  • apply(rgb, depth, undistorted, registered, true, bigdepth, color_depth_map)
  • apply(rgb, depth, undistorted, registered, false, bigdepth, color_depth_map)

The output data structures undistorted, registered, bigdepth, color_depth_map should be the same between CPU and CUDA, within floating error.

Please create a branch in your own repo and directly modify Protonect.cpp to facilitate the test cases. I will use that branch to verify the results.

You don't need to worry about API design. I will move them around. There's more to it (I'm thinking changing Frame to allow optionally avoiding passing data back to CPU for registration.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants