OpenNI2 driver

[hanyazou]

OpenNI2 driver. (#243)

rebase into head of master at 2015/12/31.
(old request was #302)

[xlz]

Generally I will not comment on actual functionalities because first I don't know openni2 well, and second because it is isolated from the rest of the code base, as long as this works it should be acceptable.

I have a few comments:

• Squash redundant commits. If two commits have similar purposes but do things at different places, they should be merged. Merge "fix" commits onto the original commit they are fixing (example: Fix timestamp according to MasWag's feedback and Fix timestamp again. (the frame sequence numbers seem to be drifting) should be merged onto the original timestamp commit Add timestamp on the frames.). The rationale of this is that as a submitter of a pull request, it's your job to fix your own mistakes before submitting instead of exposing all the history of fixing mistakes, such that from the commit history point of view, each individual change is done right in one step. git rebase -i is your friend.
• Put prefix of "openni2: " to all the commit subjects. This driver is quite isolated from the rest of the code base, which should be represented in the commit messages.
• What is the purpose of "move OpenNI2-Freenect2Driver into drivers/ sub directory"? I think leaving OpenNI2-FreenectDriver in the root directory is quite fine, because there is no other similar "driver".
• Some commits from OpenNI2 driver #302 review are lost. At least the commit to add license headers is lost. You should check and add the lost commits.
• Remove D2S.h and S2D.h. Use this formula to generate these two tables.

        uint16_t *s2d = new uint16_t[2048]();
        uint16_t *d2s = new uint16_t[10001]();
        for (int i = 1; i <= 1052; i++)
                s2d[i] = 342205.0/(1086.671 - i);
        for (int i = 315; i <= 10000; i++)
                d2s[i] = 1086.671 - 342205.0/(i + 1);

• BUILD_OPENNI2FREENECT2DRIVER -> BUILD_OPENNI2_DRIVER ?

[xlz]

"Configurating" was a typo. Should use "Configuring". You can fix the typo in the above "Configurating examples" too.

[hanyazou]

@xlz Thank you for you comment.

Squash redundant commits.
Put prefix of "openni2: " to all the commit subjects.
Some commits from #302 review are lost. At least the commit to add license headers is lost. You should check and add the lost commits.
Remove D2S.h and S2D.h. Use this formula to generate these two tables.
BUILD_OPENNI2FREENECT2DRIVER -> BUILD_OPENNI2_DRIVER ?

These above are done.

What is the purpose of "move OpenNI2-Freenect2Driver into drivers/ sub directory"? I think leaving OpenNI2-FreenectDriver in the root directory is quite fine, because there is no other similar "driver".

I feel that the directory name "OpenNI2-Freenect2Driver" looks out of place in the root because of it's capitalization. So I wanted to move the directory into another place.

[xlz]

• Remove D2S.h and S2D.h. Use this formula to generate these two tables.

This is not added yet.

[xlz]

FreenectDriver -> Freenect2Driver

[xlz]

I feel that the directory name "OpenNI2-Freenect2Driver" looks out of place in the root because of it's capitalization. So I wanted to move the directory into another place.

Hiding it under drivers will not make it look better. Check your repo: https://github.com/hanyazou/libfreenect2/tree/pull-request201601

It becomes "drivers/OpenNI2-Freenect2...".

[hanyazou]

@xlz I forgot to exec last 'git push', sorry.

Check your repo:

I knew. But I thought that was better.
I will follow a project policy anyway.

[xlz]

I tried to build this and had multiple compilation errors. I also see several issues with the build system. I'll personally massage this patch series. I expect several changes:

• git mv drivers/OpenNI2-Freenect2Driver src/openni2, and make it a first-class component.
• Add a cross-platform FindOpenNI2.cmake. There is pkgconfig provided in libopenni2-dev on Linux.
• Enable this driver by default, if its dependency is satisfied.
• Move the docs to the main README.md.

[hanyazou]

Add a cross-platform FindOpenNI2.cmake. There is pkgconfig provided in libopenni2-dev on Linux.

I've added FindOpenNI2.cmake and I could build Protonect on Ubuntu 15.10 while I couldn't run the Protonect because the ubuntu was running in virtual machine on Mac.
Could you please try this? I hope this works for you.

[xlz]

There are a few bugs in your FindOpenNI2.cmake. I don't think it works.

Also, this is enough to build this driver:

IF(BUILD_OPENNI2_DRIVER)
  FIND_PACKAGE(OpenNI2)
  IF(OpenNI2_FOUND)
    FILE(GLOB OPENNI2_DRIVER_SOURCES src/openni2/*.cpp)
    ADD_LIBRARY(freenect2-openni2 ${OPENNI2_DRIVER_SOURCES} ${LIBFREENECT2_THREADING_SOURCE})
    TARGET_INCLUDE_DIRECTORIES(freenect2-openni2 PRIVATE ${OpenNI2_INCLUDE_DIRS})
    TARGET_LINK_LIBRARIES(freenect2-openni2 freenect2 ${LIBFREENECT2_THREADING_LIBRARIES})
  ENDIF()
ENDIF()

I also tried to rebase your patch series cleanly, but there are something wrong in your commits that makes that almost impossible. Take a closer look at the two

• openni2: Add registration

     case ONI_PIXEL_FORMAT_RGB888:
 -      reg->colorFrameRGB888(srcFrame, dstFrame);
 +      if (reg->isEnabled()) {
 +        libfreenect2::Frame registered(512, 424, 3);
 +
 +        reg->colorFrameRGB888(srcFrame, &registered);
 +
 +        copyFrame(static_cast<uint8_t*>(registered.data), srcX, srcY, registered.width * registered.bytes_per_pixel, 
 +                  static_cast<uint8_t*>(dstFrame->data), dstX, dstY, dstFrame->stride, 
 +                  width, height, mirroring);
 +      } else {
 +        copyFrame(static_cast<uint8_t*>(srcFrame->data), srcX, srcY, srcFrame->width * srcFrame->bytes_per_pixel, 
 +                  static_cast<uint8_t*>(dstFrame->data), dstX, dstY, dstFrame->stride, 
 +                  width, height, mirroring);
 +      }

• openni2: Change depth video mode from 512x424 to 640x480 to work with…

     case ONI_PIXEL_FORMAT_RGB888:
 -      uint8_t* source = static_cast<uint8_t*>(data);
 -      uint8_t* target = static_cast<uint8_t*>(frame->data);
 -      for (uint8_t* p = source; p < source + frame->dataSize; p+=3) {
 -          *target++ = p[2];
 -          *target++ = p[1];
 -          *target++ = p[0];
 -      }
 +      reg->colorFrameRGB888(srcFrame, dstFrame);

The first commit is ahead of the second in the patch series, but by patch logic the first commit must be after the second commit. And even if the first commit does go after the second one, why is the registration functionality is already added before "Add registration"?

I was trying to merge these commits onto other commits, which should be:

• openni2: Change name and directory, FreenectDriver -> Freenect2Driver
• openni2: Move OpenNI2-Freenect2Driver into drivers/ sub directory
• openni2: Delete unused lines
• openni2: Fix compile errors after last merge
• openni2: Have a member of libfreenect2::Freenect2 in the OpenNI2 driver
• openni2: Delete OpenNI2 header files
• openni2: Remove D2S.h adn S2D.h

[xlz]

I put together a cleaned up patch series https://github.com/xlz/libfreenect2/commits/openni2

Each commit builds correctly. Windows specific things will be later.

[hanyazou]

I put together a cleaned up patch series https://github.com/xlz/libfreenect2/commits/openni2

Each commit builds correctly on my Mac (OS X 10.11) too.

But I had to specify '-DENABLE_OPENGL=NO' for cmake because of the NVIDIA driver problem.
Disabling OpenGL with the option, UserViewer (NiTE), NiViewer (OpenNI2) and pcl_openni2_viewer (PCL) start up correctly to show color and depth images in their window.